Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update lower boundary in Python pip pyproject when bumping versions #6631 #10060

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Update lower boundary in Python pip pyproject when bumping versions #6631 #10060

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a740c39
Added fix for lower pyproject low boundry issue for bump version (inc…
kbukum1 Jun 26, 2024
bb91a7e
added specs for unfixable requirements
kbukum1 Jun 27, 2024
3936a85
lower version check is fixed.
kbukum1 Jun 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions python/lib/dependabot/python/requirement.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ class Requirement < Dependabot::Requirement
"===" => ->(v, r) { v.to_s == r.to_s }
)

# Override the lower bound logic for bump versions strategy.
BUMP_VERSIONS_OPS = OPS.merge(
">=" => ->(v, r) { v.to_s == r.to_s }
)

quoted = OPS.keys.sort_by(&:length).reverse
.map { |k| Regexp.quote(k) }.join("|")
version_pattern = Python::Version::VERSION_PATTERN
Expand Down Expand Up @@ -78,10 +83,10 @@ def initialize(*requirements)
super(requirements)
end

def satisfied_by?(version)
def satisfied_by?(version, ops = OPS)
version = Python::Version.new(version.to_s)

requirements.all? { |op, rv| (OPS[op] || OPS["="]).call(version, rv) }
requirements.all? { |op, rv| (ops[op] || ops["="]).call(version, rv) }
end

def exact?
Expand Down
10 changes: 5 additions & 5 deletions python/lib/dependabot/python/update_checker/requirements_updater.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,14 +278,14 @@ def update_requirements_range(requirement_strings)
requirement_strings.map { |r| requirement_class.new(r) }

updated_requirement_strings = ruby_requirements.flat_map do |r|
next r.to_s if r.satisfied_by?(latest_resolvable_version)
next r.to_s if r.satisfied_by?(latest_resolvable_version, Requirement::BUMP_VERSIONS_OPS)

case op = r.requirements.first.first
when "<"
"<" + update_greatest_version(r.requirements.first.last, latest_resolvable_version)
when "<="
"<=" + latest_resolvable_version.to_s
when "!=", ">", ">="
"#{op}#{update_greatest_version(r.requirements.first.last, latest_resolvable_version)}"
when "<=", ">="
"#{op}#{latest_resolvable_version}"
when "!=", ">"
raise UnfixableRequirement
honeyankit marked this conversation as resolved.
Show resolved Hide resolved
else
raise "Unexpected op for unsatisfied requirement: #{op}"
Expand Down
50 changes: 48 additions & 2 deletions python/spec/dependabot/python/requirement_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
its(:to_s) { is_expected.to eq(Gem::Requirement.new("~> 1.2.0").to_s) }
end

context "when dealing with one digits" do
context "when dealing with one digit" do
let(:requirement_string) { "~1" }

its(:to_s) { is_expected.to eq(Gem::Requirement.new("~> 1.0").to_s) }
Expand Down Expand Up @@ -185,7 +185,7 @@
it { is_expected.to eq([Gem::Requirement.new("1.2.1")]) }
end

context "with a illformed parentheses" do
context "with illformed parentheses" do
let(:requirement_string) { "(== 1.2).1" }

it "raises a helpful error" do
Expand Down Expand Up @@ -319,4 +319,50 @@
end
end
end

describe "#satisfied_by? with BUMP_VERSIONS_OPS" do
subject(:requirement_satisfied_by) { requirement.satisfied_by?(version, described_class::BUMP_VERSIONS_OPS) }

let(:requirement_string) { ">=1.0.0" }

context "with a Python::Version" do
let(:version) { version_class.new(version_string) }

context "when dealing with the exact version" do
let(:version_string) { "1.0.0" }

it { is_expected.to be(true) }
end

context "when dealing with a different version" do
let(:version_string) { "2.0.0" }

it { is_expected.to be(false) }
end

context "when dealing with the latest resolvable version" do
let(:version_string) { "1.0.0" }

it { is_expected.to be(true) }

context "when the requirement includes a local version" do
let(:requirement_string) { ">=1.0.0+gc.1" }

it { is_expected.to be(false) }
end

context "when the version includes a local version" do
let(:version_string) { "1.0.0+gc.1" }

it { is_expected.to be(false) }
end
end

context "when dealing with an out-of-range version" do
let(:version_string) { "0.9.0" }

it { is_expected.to be(false) }
end
end
end
end
100 changes: 89 additions & 11 deletions python/spec/dependabot/python/update_checker/requirements_updater_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,24 @@
end
end

context "when a > req was specified" do
let(:requirement_txt_req_string) { "> 1.3.0" }

it { is_expected.to eq(requirement_txt_req) }

context "when dealing with exactly the version being updated to" do
let(:requirement_txt_req_string) { "> 1.5.0" }

its([:requirement]) { is_expected.to eq(:unfixable) }
end

context "when dealing with the version lower than the lower bound" do
let(:requirement_txt_req_string) { "> 1.6.0" }

its([:requirement]) { is_expected.to eq(:unfixable) }
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above block of specs makes sense to me 👍.


context "when a range requirement was specified" do
let(:requirement_txt_req_string) { ">=1.3.0" }

Expand All @@ -129,7 +147,7 @@
context "when requirement version is too high" do
let(:requirement_txt_req_string) { ">=2.0.0" }

its([:requirement]) { is_expected.to eq(:unfixable) }
its([:requirement]) { is_expected.to eq(">=1.5.0") }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the previous outcome better here. If the latest resolvable version is 1.5.0, and user has specified >=2.0.0 in their manifest, I think either:

  • User setup/manifest is wrong.
  • Dependabot run into a bug when figuring out the latest resolvable version.

Either way, it seems best to leave this kind of ill defined case alone?

Also, the new solution is actually widening the requirement, not increasing it, which seems completely against the spirit of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new solution actually it is not widening it. It is setting the lower bound as the latest version and the upper bound is still following the last version by increasing. As it is mentioned in the document tried to follow the typical scenario.

Always increase the minimum version requirement to match the new version. If a range already exists, typically this only increases the lower bound.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm understanding this test case correctly, the previous requirement is >= 2.0.0, correct? If Dependabot sets the new range to be >= 1.5.0, it's actually decreasing the lower bound and thus widening the requirement.

This seems like an ill-defined case to me. Why would the existing range be >= 2.0.0 if the latest version is 1.5.0? That's why it seemed reasonable to not mess with this case and call it "unfixable".

That said, I just came up with a potential case when this could happen in real life: If latest version 2.0.0 is yanked, then that could make 1.5.0 the "new" latest version and make a case like this possible.

Previously Dependabot would avoid messing up with this case, and now it's downgrading the version to 1.5.0. I guess it's not as bad after all!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am strongly of the opinion that dependabot should never lower a >= constraint. When such a constraint is used, I believe it means one of two things:

  1. We're tracking the latest version: the user should pick the increase strategy.
  2. We're declaring the range of all compatible versions and we know that lower versions are not compatible (e.g. we're using a feature that was introduced in 2.0): user should pick widen strategy and dependabot should not touch lower bound.

Copy link

@sanderr sanderr Jul 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize I got the context wrong: we're talking about increase here. I still think a user intervention would be more appropriate here, but I am not as strongly invested as for the widen case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I do think there could be all sort of expectations here. I guess it could also happen that someone using widen has changed their code to support 2.0 version of some library and bumped their lower bound requirement to >=2.0, then 2.0 is abruptly removed and user may appreciated that Dependabot "rolls back" their requirement change and let them know through a PR they need to revert code changes too to get back on track.

Also, strictly speaking, for the widen strategy, Dependabot is also doing exactly what's advertised right? Namely, "widen the range to accommodate for the latest version". Just that in this case, the latest version is not newer than the upper bound, but older than the lower bound.

In any case, I think this is exactly why I advocate for "leave this kind of case alone", because different users will have different expectations, or even the same user may want different things depending on the library.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do make a good point, this is a case where it's difficult to find a universal approach to fit everyone's use case. Your suggestion to leave it alone sounds sensible to me (biased though I am because that is what I've been advocating regardless).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I implemented this, my thoughts were as follows:

In real-world scenarios, this occurs when no range was specified before, when a higher version is yanked and necessitates a downgrade to a previous release, or when the strategy wasn’t set to increase when a lower bound is added. In such cases, Dependabot can adjust the lower bound accordingly.

For the increase strategy, if a lower constraint is defined to ensure compatibility, the lower bound should not be higher than the latest version since the latest version is functional for the app or library. Otherwise, it risks excluding versions that work correctly, potentially breaking the app.

I would appreciate your thoughts on this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you're saying, but I still feel like it's a risky assumption to make for the widen strategy. For example, consider the case where a library detects a breaking bug in their latest release, so they yank it and then release a patched one soon after. If they do it in that order, dependabot might run in the window where the broken one was yanked but the new one not yet released. It would then change the lower bound, while it wouldn't have taken action at all if it had run a few minutes later.

If the library does not release a patched version (which is quite rare as far as I know), then action will indeed be required. But since this is such a rare occurrence, and lowering the constraint might not always be desired, I feel like it's safer to leave this as a problem for the user to solve.

That said, I'm just a bystander here, so I won't try to push my view any further than this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @sanderr here. I think Dependabot should create update PRs that work for everyone, or when that's not the case, at least allow the behavior to be configurable. This seems such an edge case that feels best to not try be too smart and leave it for the user to fix manually.

end

context "when requirement had a local version" do
Expand All @@ -146,13 +164,13 @@
context "when needing an update" do
let(:requirement_txt_req_string) { ">=1.3.0, <1.5" }

its([:requirement]) { is_expected.to eq(">=1.3.0,<1.6") }
its([:requirement]) { is_expected.to eq(">=1.5.0,<1.6") }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should Dependabot increase the upper bound simmetrically to the lower bound i.e., leave a 3 major version range in place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking it shouldn't because you will never know how upcoming versions are handled. Also here we are handling only lower bound the upper bound is working as how it was working before. Do you think we need to reconsider the upper bound in this case?


context "when requirement version has more digits than the new version" do
let(:requirement_txt_req_string) { "<=1.9.2,>=1.9" }
let(:latest_resolvable_version) { "1.10" }

its([:requirement]) { is_expected.to eq(">=1.9,<=1.10") }
its([:requirement]) { is_expected.to eq("<=1.10,>=1.10") }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, there's no clear solution, but <=1.10,>=1.10 feels not right? Either one of:

  • <=1.11,>=1.10.
  • <=1.10.2,>=1.10
  • :unfixable

I'd probably leave this one alone as :unfixable since it's unclear what the user wants in the case.

end
end
end
Expand Down Expand Up @@ -268,7 +286,7 @@
context "when the requirement version is too high" do
let(:setup_py_req_string) { ">=2.0.0" }

its([:requirement]) { is_expected.to eq(:unfixable) }
its([:requirement]) { is_expected.to eq(">=1.5.0") }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the previous comment on the similar case on requirement.txt files.

end

context "with an upper bound" do
Expand All @@ -279,7 +297,7 @@
context "when needing an update" do
let(:setup_py_req_string) { ">=1.3.0, <1.5" }

its([:requirement]) { is_expected.to eq(">=1.3.0,<1.6") }
its([:requirement]) { is_expected.to eq(">=1.5.0,<1.6") }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the previous comment on the similar case on requirement.txt files.

end
end
end
Expand Down Expand Up @@ -370,7 +388,7 @@
context "when the requirement version is too high" do
let(:setup_cfg_req_string) { ">=2.0.0" }

its([:requirement]) { is_expected.to eq(:unfixable) }
its([:requirement]) { is_expected.to eq(">=1.5.0") }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the previous outcome better here. If the latest resolvable version is 1.5.0, and user has specified >=2.0.0 in their manifest, I think either:

  • User setup/manifest is wrong.
  • Dependabot run into a bug when figuring out the latest resolvable version.

Either way, it seems best to leave this kind of ill defined case alone?

Also, the new solution is actually widening the requirement, not increasing it, which seems completely against the spirit of this PR?

end

context "with an upper bound" do
Expand All @@ -381,7 +399,7 @@
context "when needing an update" do
let(:setup_cfg_req_string) { ">=1.3.0, <1.5" }

its([:requirement]) { is_expected.to eq(">=1.3.0,<1.6") }
its([:requirement]) { is_expected.to eq(">=1.5.0,<1.6") }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the previous comment on the similar case on requirement.txt files.

end
end
end
Expand Down Expand Up @@ -496,6 +514,36 @@
it { is_expected.to eq(pyproject_req) }
end

context "when a != req was specified" do
let(:pyproject_req_string) { "!= 1.3.0" }

it { is_expected.to eq(pyproject_req) }

context "when dealing with exactly the version being updated to" do
let(:pyproject_req_string) { "!=1.5.0" }

its([:requirement]) { is_expected.to eq(:unfixable) }
end
end

context "when a > req was specified" do
let(:pyproject_req_string) { "> 1.3.0" }

it { is_expected.to eq(pyproject_req) }

context "when dealing with exactly the version being updated to" do
let(:pyproject_req_string) { "> 1.5.0" }

its([:requirement]) { is_expected.to eq(:unfixable) }
end

context "when dealing with the version lower than the lower bound" do
let(:pyproject_req_string) { "> 1.6.0" }

its([:requirement]) { is_expected.to eq(:unfixable) }
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above block of specs makes sense to me 👍.


context "when a range requirement was specified" do
let(:pyproject_req_string) { ">=1.3.0" }

Expand All @@ -504,7 +552,7 @@
context "when the requirement version is too high" do
let(:pyproject_req_string) { ">=2.0.0" }

its([:requirement]) { is_expected.to eq(:unfixable) }
its([:requirement]) { is_expected.to eq(">=1.5.0") }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the previous comment on the similar case on requirement.txt files.

end

context "when the requirement had a local version" do
Expand All @@ -521,7 +569,7 @@
context "when needing an update" do
let(:pyproject_req_string) { ">=1.3.0, <1.5" }

its([:requirement]) { is_expected.to eq(">=1.3.0,<1.6") }
its([:requirement]) { is_expected.to eq(">=1.5.0,<1.6") }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the previous comment on the similar case on requirement.txt files.

end
end
end
Expand Down Expand Up @@ -636,6 +684,36 @@
it { is_expected.to eq(pyproject_req) }
end

context "when a != req was specified" do
let(:pyproject_req_string) { "!= 1.3.0" }

it { is_expected.to eq(pyproject_req) }

context "when dealing with exactly the version being updated to" do
let(:pyproject_req_string) { "!=1.5.0" }

its([:requirement]) { is_expected.to eq(:unfixable) }
end
end

context "when a > req was specified" do
let(:pyproject_req_string) { "> 1.3.0" }

it { is_expected.to eq(pyproject_req) }

context "when dealing with exactly the version being updated to" do
let(:pyproject_req_string) { "> 1.5.0" }

its([:requirement]) { is_expected.to eq(:unfixable) }
end

context "when dealing with the version lower than the lower bound" do
let(:pyproject_req_string) { "> 1.6.0" }

its([:requirement]) { is_expected.to eq(:unfixable) }
end
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above block of specs makes sense to me 👍.


context "when a range requirement was specified" do
let(:pyproject_req_string) { ">=1.3.0" }

Expand All @@ -650,7 +728,7 @@
context "when the requirement is too high" do
let(:pyproject_req_string) { ">=2.0.0" }

its([:requirement]) { is_expected.to eq(:unfixable) }
its([:requirement]) { is_expected.to eq(">=1.5.0") }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the previous comment on the similar case on requirement.txt files.

end

context "with an upper bound" do
Expand All @@ -661,7 +739,7 @@
context "when needing an update" do
let(:pyproject_req_string) { ">=1.3.0, <1.5" }

its([:requirement]) { is_expected.to eq(">=1.3.0,<1.6") }
its([:requirement]) { is_expected.to eq(">=1.5.0,<1.6") }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the previous comment on the similar case on requirement.txt files.

end
end
end
Expand Down
Loading