Add support for Python packages with epoch version #4928
Conversation
Dependabot currently considers a Python package version with an epoch to be illformed. See definition here https://peps.python.org/pep-0440/#version-epochs. This change adds support for parsing them and using epochs in version comparisons. Where one is not specified, the epoch is treated as 0.
…n-packages Add support for Python packages with epoch version
| "1.0.0-beta.11", | ||
| "1.0.0-rc.1", | ||
| "1", | ||
| # "1.0.0.post", TODO fails comparing to 1 |
There was a problem hiding this comment.
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.
| "2!0.1.0", | ||
| "10!0.1.0" | ||
| ] | ||
| sorted_versions.combination(2).each do |lhs, rhs| |
There was a problem hiding this comment.
I'm jealous. I've never had an occasion to use combination.
# Conflicts: # python/lib/dependabot/python/version.rb
Co-authored-by: Rob <[email protected]>
|
Thanks to @kohder for the initial fix! |
| @@ -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]+)*)?/. | |||
There was a problem hiding this comment.
Can't a major version still start with 0 for something like a beta release? v0.0.1
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
@kohder thanks for PR #4891!
While I was reviewing that PR I noticed we would also need to add epoch support to the
requirement_parser, so I went ahead and did that in this branch on top of your changes. I also made a few simplifications to the code and brought in more thorough tests like those in the nuget spec.Let me know what you think, and I will also get another @dependabot/reviewers to take a look at these changes as well!
Fixes #3165