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.

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

Add support for Python packages with epoch version #4928

Merged
merged 13 commits into from
Apr 4, 2022
2 changes: 1 addition & 1 deletion python/lib/dependabot/python/requirement_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ class RequirementParser
NAME = /[a-zA-Z0-9](?:[a-zA-Z0-9\-_\.]*[a-zA-Z0-9])?/.freeze
EXTRA = /[a-zA-Z0-9\-_\.]+/.freeze
COMPARISON = /===|==|>=|<=|<|>|~=|!=/.freeze
VERSION = /[0-9]+[a-zA-Z0-9\-_\.*]*(\+[0-9a-zA-Z]+(\.[0-9a-zA-Z]+)*)?/.
VERSION = /([1-9][0-9]*!)?[0-9]+[a-zA-Z0-9\-_.*]*(\+[0-9a-zA-Z]+(\.[0-9a-zA-Z]+)*)?/.
Copy link
Member

Choose a reason for hiding this comment

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

Can't a major version still start with 0 for something like a beta release? v0.0.1

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and that is still handled, there's a test for 0 in the major position: https://github.com/dependabot/dependabot-core/pull/4928/files#diff-b08ebf8cb5c173b20e4eb0bb7028796f7f05c0e6dae30da7744c274ec21422ecR80

The epoch comes before the major though, so that's the part of the regex I changed, unless I am misunderstanding?

We talked a bit about whether to follow the canonical regex above a bit: https://github.com/dependabot/dependabot-core/pull/4928/files/58abde4347fc6ec5bde994425c819455beea8eea..5aed05b994f6af65b5c8b3e914a1c019fecf11e8#diff-0b41ac2b39a98424bc506c956697e44d02a541bdfbe1ad3c548d318d38855ace

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see I wasn't the only one who noticed this. Thanks for the explanation, makes sense why you chose the regex match you did.

Just curious, what happens if there is an epoch that starts with 0! (because someone decided not to read the PEP 😛) Hopefully we raise an error?

freeze
REQUIREMENT =
/(?<comparison>#{COMPARISON})\s*\\?\s*(?<version>#{VERSION})/.freeze
Expand Down
37 changes: 23 additions & 14 deletions python/lib/dependabot/python/version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
module Dependabot
module Python
class Version < Gem::Version
attr_reader :epoch
attr_reader :local_version
attr_reader :post_release_version

VERSION_PATTERN = 'v?[0-9]+[0-9a-zA-Z]*(?>\.[0-9a-zA-Z]+)*' \
# See https://peps.python.org/pep-0440/#appendix-b-parsing-version-strings-with-regular-expressions
VERSION_PATTERN = 'v?([1-9][0-9]*!)?[0-9]+[0-9a-zA-Z]*(?>\.[0-9a-zA-Z]+)*' \
jakecoffman marked this conversation as resolved.
Show resolved Hide resolved
'(-[0-9A-Za-z-]+(\.[0-9a-zA-Z-]+)*)?' \
'(\+[0-9a-zA-Z]+(\.[0-9a-zA-Z]+)*)?'
ANCHORED_VERSION_PATTERN = /\A\s*(#{VERSION_PATTERN})?\s*\z/.freeze
Expand All @@ -29,6 +31,11 @@ def initialize(version)
version, @local_version = version.split("+")
version ||= ""
version = version.gsub(/^v/, "")
if version.include?("!")
@epoch, version = version.split("!")
else
@epoch = "0"
end
version = normalise_prerelease(version)
version, @post_release_version = version.split(/\.r(?=\d)/)
version ||= ""
Expand All @@ -45,33 +52,37 @@ def inspect # :nodoc:
end

def <=>(other)
other = Version.new(other.to_s) unless other.is_a?(Python::Version)

epoch_comparison = epoch_comparison(other)
return epoch_comparison unless epoch_comparison.zero?

version_comparison = super(other)
return version_comparison unless version_comparison.zero?

return post_version_comparison(other) unless post_version_comparison(other).zero?
post_version_comparison = post_version_comparison(other)
return post_version_comparison unless post_version_comparison.zero?

local_version_comparison(other)
end

private

def epoch_comparison(other)
epoch.to_i <=> other.epoch.to_i
end

def post_version_comparison(other)
unless other.is_a?(Python::Version) && other.post_release_version
unless other.post_release_version
return post_release_version.nil? ? 0 : 1
end

return -1 if post_release_version.nil?

# Post release versions should only ever be a single number, so we can
# just string-comparison them.
return 0 if post_release_version.to_i == other.post_release_version.to_i

post_release_version.to_i > other.post_release_version.to_i ? 1 : -1
post_release_version.to_i <=> other.post_release_version.to_i
end

def local_version_comparison(other)
unless other.is_a?(Python::Version)
return local_version.nil? ? 0 : 1
end

# Local version comparison works differently in Python: `1.0.beta`
# compares as greater than `1.0`. To accommodate, we make the
# strings the same length before comparing.
Expand All @@ -89,8 +100,6 @@ def local_version_comparison(other)
lhsegments.count <=> rhsegments.count
end

private

def normalise_prerelease(version)
# Python has reserved words for release states, which are treated
# as equal (e.g., preview, pre and rc).
Expand Down
7 changes: 7 additions & 0 deletions python/spec/dependabot/python/requirement_parser_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,13 @@ def parse(line)
it { is_expected.to be_nil }
end

context "with an epoch specification" do
let(:line) { "luigi==1!1.1.0" }
its([:requirements]) do
is_expected.to eq [{ comparison: "==", version: "1!1.1.0" }]
end
end

context "with a simple specification" do
let(:line) { "luigi == 0.1.0" }
its([:requirements]) do
Expand Down
155 changes: 40 additions & 115 deletions python/spec/dependabot/python/version_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@
let(:version_string) { "1.0.0" }
it { is_expected.to eq(true) }

context "that includes a non-zero epoch" do
let(:version_string) { "1!1.0.0" }
it { is_expected.to eq(true) }
end

context "that includes a local version" do
let(:version_string) { "1.0.0+abc.1" }
it { is_expected.to eq(true) }
Expand Down Expand Up @@ -70,126 +75,46 @@
end

describe "#<=>" do
subject { version <=> other_version }

context "compared to a Gem::Version" do
context "that is lower" do
let(:other_version) { Gem::Version.new("0.9.0") }
it { is_expected.to eq(1) }
end

context "that is equal" do
let(:other_version) { Gem::Version.new("1.0.0") }
it { is_expected.to eq(0) }

context "but our version has a local version" do
let(:version_string) { "1.0.0+gc.1" }
it { is_expected.to eq(1) }
end

context "but our version has a v-prefix" do
let(:version_string) { "v1.0.0" }
it { is_expected.to eq(0) }
end
end

context "that is greater" do
let(:other_version) { Gem::Version.new("1.1.0") }
it { is_expected.to eq(-1) }
sorted_versions = [
"",
"0.9",
"1.0.0-alpha",
"1.0.0-a.1",
"1.0.0-beta",
"1.0.0-b.2",
"1.0.0-beta.11",
"1.0.0-rc.1",
"1",
# "1.0.0.post", TODO fails comparing to 1
Copy link
Member Author

Choose a reason for hiding this comment

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

The more thorough testing is showing here that dependabot thinks 1 > 1.0.0.post, but yet 1 < 1.0.0.post1 so I think there's a bug here.

According to PEP 440, post release identifiers are for non-material changes like fixing release notes. So I assume if we fix that then dependabot will be bumping a lot of major versions to post release versions.

But I want to look into this more after we fix epoch support so I'll make a followup issue.

"1.0.0+gc1",
"1.post2",
"1.post2+gc1",
"1.post2+gc1.2",
"1.post2+gc1.11",
"1.0.0.post11",
"1.0.2",
"1.0.11",
"2016.1",
"1!0.1.0",
"2!0.1.0",
"10!0.1.0"
]
sorted_versions.combination(2).each do |lhs, rhs|
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm jealous. I've never had an occasion to use combination.

it "'#{lhs}' < '#{rhs}'" do
expect(described_class.new(lhs)).to be < rhs
expect(described_class.new(rhs)).to be > lhs
end
end

context "compared to a Python::Version" do
context "that is lower" do
let(:other_version) { described_class.new("0.9.0") }
it { is_expected.to eq(1) }
end

context "that is equal" do
let(:other_version) { described_class.new("1.0.0") }
it { is_expected.to eq(0) }

context "but our version has a local version" do
let(:version_string) { "1.0.0+gc.1" }
it { is_expected.to eq(1) }
end

context "with a prerelease specifier that needs normalising" do
let(:version_string) { "1.0.0c1" }
let(:other_version) { described_class.new("1.0.0rc1") }
it { is_expected.to eq(0) }

context "with a dash that needs sanitizing" do
let(:version_string) { "0.5.4-alpha" }
let(:other_version) { described_class.new("0.5.4a0") }
it { is_expected.to eq(0) }
end
end

context "with a post-release" do
let(:version_string) { "1.0.0-post1" }
it { is_expected.to eq(1) }

context "that needs normalising" do
let(:other_version) { described_class.new("1.0.0-post1") }
let(:version_string) { "1.0.0rev1" }
it { is_expected.to eq(0) }
end

context "that is being compared to another post release" do
let(:other_version) { described_class.new("1.0.0.post1") }
let(:version_string) { "1.0.0.post2" }
it { is_expected.to eq(1) }
end

context "when the other version has a post release" do
let(:other_version) { described_class.new("1.0.0.post1") }
let(:version_string) { "1.0.0" }
it { is_expected.to eq(-1) }
end
end

context "but the other version has a local version" do
let(:other_version) { described_class.new("1.0.0+gc.1") }
it { is_expected.to eq(-1) }
end

context "and both sides have a local version" do
let(:other_version) { described_class.new("1.0.0+gc.1") }

context "that is equal" do
let(:version_string) { "1.0.0+gc.1" }
it { is_expected.to eq(0) }
end

context "when our side is greater" do
let(:version_string) { "1.0.0+gc.2" }
it { is_expected.to eq(1) }
end

context "when our side is lower" do
let(:version_string) { "1.0.0+gc" }
it { is_expected.to eq(-1) }
end

context "when our side is longer" do
let(:version_string) { "1.0.0+gc.1.1" }
it { is_expected.to eq(1) }
end
end
end

context "that is greater" do
let(:other_version) { described_class.new("1.1.0") }
it { is_expected.to eq(-1) }

context "with a dash that needs sanitizing" do
let(:version_string) { "0.5.4-alpha" }
let(:other_version) { described_class.new("0.5.4a1") }
it { is_expected.to eq(-1) }
end
sorted_versions.each do |v|
it "should equal itself #{v}" do
expect(described_class.new(v)).to eq v
end
end
it "should handle missing version segments" do
expect(described_class.new("1")).to eq "v1.0"
expect(described_class.new("1")).to eq "v1.0.0"
end
end

describe "#prerelease?" do
Expand Down