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

Match wheel tags against Requires-Python major-minor #5289

Merged
merged 1 commit into from
Jul 22, 2024

Conversation

charliermarsh
Copy link
Member

Summary

Given Requires-Python: ">=3.12.3", we were rejecting wheels like dearpygui-1.11.1-cp312-cp312-win_amd64.whl, since 3.12.0 is not included in >=3.12.3. We instead need to test against the major-minor version of Requires-Python.

The easiest way to do this, I think, is the use the RequiresPython struct, which has a single bound that we can truncate the major-minor. This also means that we now allow dearpygui-1.11.1-cp312-cp312-win_amd64.whl for specifiers like Requires-Python: "==3.10.*". This is incorrect on the surface, but it does match our semantics for Requires-Python elsewhere: we treat it as a lower bound.

Closes #5287.

@charliermarsh charliermarsh added the bug Something isn't working label Jul 22, 2024
// Ex) `>3.10.1` -> `>=3.10`
// This is unintuitive, but `>3.10.1` does indicate that _some_ version of Python 3.10
// is supported.
Bound::Excluded(version) => RequiresPythonBound(Bound::Included(Version::new(
Copy link
Member Author

Choose a reason for hiding this comment

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

This is strange but I think correct...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah >3.10 is a footgun because it does actually include 3.10 version, this is basically PYI006

@konstin
Copy link
Member

konstin commented Jul 22, 2024

This also means that we now allow dearpygui-1.11.1-cp312-cp312-win_amd64.whl for specifiers like Requires-Python: "==3.10.*"

For warehouse with ==3.11.*, this increases the lockfile from 4161 lines to 4466 lines (+7%).

// Ex) `>3.10.1` -> `>=3.10`
// This is unintuitive, but `>3.10.1` does indicate that _some_ version of Python 3.10
// is supported.
Bound::Excluded(version) => RequiresPythonBound(Bound::Included(Version::new(
Copy link
Member

Choose a reason for hiding this comment

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

Yeah >3.10 is a footgun because it does actually include 3.10 version, this is basically PYI006

@charliermarsh
Copy link
Member Author

For warehouse with ==3.11.*, this increases the lockfile from 4161 lines to 4466 lines (+7%).

Yeah, unfortunately I think we kinda have to do this, unless we want to change our Requires-Python semantics. But we should do that holistically if so.

@charliermarsh charliermarsh enabled auto-merge (squash) July 22, 2024 14:25
@konstin
Copy link
Member

konstin commented Jul 22, 2024

The most correct solution is probably computing an upper bound the same way we compute the lower bound, incl. widening to the next full major-minor version/discarding the patch version, and then only applying the lower bound for matching dependencies' requires-python, but applying the lower and the upper bound for selecting their wheels. (Giving us requires-python semantic #3, i think).

Improving this isn't schema-breaking or badly breaking backwards compatibility (the wheels can never be selected anyway), so we can move this post-release-ready.

@charliermarsh charliermarsh merged commit b6f4704 into main Jul 22, 2024
54 checks passed
@charliermarsh charliermarsh deleted the charlie/req-python-wheels branch July 22, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wheel tag-Python version matching has false negatives for minor versions
2 participants