Skip to content

fix(pypi): finish PEP508/PEP440 impl for version matching #2856

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
Merged
merged 58 commits into from
May 11, 2025
Merged

fix(pypi): finish PEP508/PEP440 impl for version matching #2856

merged 58 commits into from
May 11, 2025
+641 −257

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented May 3, 2025 •
edited
Loading

This reuses the previous work by @vonschultz who implemented
a PEP440 version normalizer. We extend it and use it in the
PEP508 marker evaluation.

Summary:

  • Extend the normalization parser to output individual parts
    of the versions to the parsing context.
  • Re-implement all of the version comparison calls to use the
    parsed version.
  • Add extra validation for .* usage in the environment markers
  • Fallback to non-version matching in the environment markers
    if one of the sides is not a version.
  • Rename the original normalizer file to version.bzl because
    as far as Python is concerned this is the only version that
    there can be. We could in theory probably reuse this in other
    code where we are parsing the Python interpreter version many
    times, but this is left for the future.

Fixes #2826
Work towards #2821

rickeylev
rickeylev reviewed May 3, 2025
View reviewed changes
Copy link
Collaborator

@rickeylev rickeylev left a comment

Choose a reason for hiding this comment

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

I factored a check_evaluation helper out.

Also added a test to just loop over (expr, want, env) cases. I figure we can copy/paste some real expressions. Also makes it easy to add some arbitrary expression to test.

@rickeylev
Copy link
Collaborator

I tried fixing a bug with prefix matching exposed by the extra tests. I'm not entirely sure my revision is entirely correct. For fun, I ran the question through Gemini, it took like minutes (:astonished:) of thinking, and ultimately came back with something resembling:

if left and right are prefix: ...
elif left is prefix: ...
elif right is prefix: ...
else: ...

Which satisfies my confirmation bias, but I didn't look closely or think it through, so yeah, grain of salt.

Also, for version equality, the local part should be ignored, so I commented out that part of _version_eq

https://packaging.python.org/en/latest/specifications/version-specifiers/#id5

Except where specifically noted below, local version identifiers MUST NOT be permitted in version specifiers, and local version labels MUST be ignored entirely when checking if candidate versions match a given version specifier.

I couldn't find an "except where noted" later, so I'm pretty sure the '+blabla` is supposed to be ignored.

@aignas
Copy link
Collaborator Author

aignas commented May 4, 2025

Oh nice, thank you, I will check it my tomorrow. I knew that the prefix matching is not fully done yet, but ran out of time yesterday.

@aignas
Copy link
Collaborator Author

aignas commented May 5, 2025

I realized that the prefix matching could be a simple string prefix match so I went that way, the code became easier to reason about and I think it needs to be optimized a little, because I am normalizing the version twice, but I think this direction is promising. Ran out of time today, so I'll leave it for the next time.

@aignas
Copy link
Collaborator Author

aignas commented May 6, 2025

It seems that this is still not done. I am not sure I can fully grok the spec about what we should do with the version ordering.

The exclusive ordered comparison
https://peps.python.org/pep-0440/#exclusive-ordered-comparison

is talking about pre-release versions, but I am not sure if that applies to the dev and local versions. That said, that part of the pep is about version specifiers and not necessarily comparison of two versions.

Maybe the ordering is answered in
https://peps.python.org/pep-0440/#exclusive-ordered-comparison

@aignas aignas changed the title wip: finish PEP508/PEP440 impl for version matching fix(pypi): finish PEP508/PEP440 impl for version matching May 10, 2025
@aignas aignas requested a review from rickeylev May 10, 2025 01:28
@aignas aignas marked this pull request as ready for review May 10, 2025 01:37
@aignas aignas requested a review from groodt as a code owner May 10, 2025 01:37
rickeylev
rickeylev approved these changes May 10, 2025
View reviewed changes
python/private/version.bzl
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please put a suffix/prefix on this file name. We have like 4 files named "version.bzl" or "versions.bzl" and I find it confusing. parser_version? version_parser? pypa_version_parser?

@rickeylev
Copy link
Collaborator

While I don't like the "version.bzl" file name, I odn't think it needs to block merging; enqueing.

@rickeylev rickeylev enabled auto-merge May 10, 2025 21:49
@rickeylev rickeylev added this pull request to the merge queue May 11, 2025
Merged via the queue into main with commit efc7589 May 11, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vonschultz vonschultz vonschultz left review comments

@rickeylev rickeylev rickeylev approved these changes

@groodt groodt Awaiting requested review from groodt groodt is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Full pep 508 / pypa dependency specifier support in starlark
3 participants
@aignas @rickeylev @vonschultz