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

Fix priority of platform tags for manylinux #2483

Merged
merged 1 commit into from
Mar 16, 2024
Merged

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Mar 15, 2024

@zanieb zanieb added the bug Something isn't working label Mar 15, 2024
@zanieb zanieb changed the title Fix platform tag ordering Fix priority of platform tags for manylinux Mar 16, 2024
@zanieb
Copy link
Member Author

zanieb commented Mar 16, 2024

I fixed the ordering of the abi tags, but I'm not sure where it's documented that linux_x86_64 should take priority over manylinux_<abi>_x86_64. If someone has a link for that part of the specification I'd appreciate it — I can add it to the code.

Looks correct per https://github.com/pypa/packaging/blob/fd4f11139d1c884a637be8aa26bb60a31fbc9411/packaging/tags.py#L434-L444

@charliermarsh
Copy link
Member

I think it might be the other way around, I think higher priority tags come first in that code, so linux_x86_64 would be last? That's at least my read of the code, but I didn't test it.

@charliermarsh
Copy link
Member

You can test by comparing to:

from packaging import tags

for tag in tags.sys_tags(): print(tag)

@zanieb
Copy link
Member Author

zanieb commented Mar 16, 2024

Apologies for all the back and forth apparently this needed a more rigorous look :)

I've addressed the last round of comments. I might look into a better way to test we match Python's behavior.

@zanieb
Copy link
Member Author

zanieb commented Mar 16, 2024

It looks like our ordering of ABI tags is wrong, i.e. Python does

cp311-cp311-manylinux_2_28_x86_64
...
cp311-cp311-manylinux_2_17_x86_64
cp311-cp311-manylinux2014_x86_64
cp311-cp311-manylinux_2_16_x86_64
...
cp311-cp311-manylinux_2_12_x86_64
cp311-cp311-manylinux2010_x86_64
cp311-cp311-manylinux_2_11_x86_64
...
cp311-cp311-manylinux_2_5_x86_64
cp311-cp311-manylinux1_x86_64
cp311-cp311-linux_x86_64
cp311-abi3-manylinux_2_28_x86_64
...
cp311-abi3-manylinux_2_17_x86_64
cp311-abi3-manylinux2014_x86_64
cp311-abi3-manylinux_2_16_x86_64
...
cp311-abi3-manylinux_2_12_x86_64
cp311-abi3-manylinux2010_x86_64
cp311-abi3-manylinux_2_11_x86_64
...
cp311-abi3-manylinux_2_5_x86_64
cp311-abi3-manylinux1_x86_64
cp311-abi3-linux_x86_64
cp311-none-manylinux_2_28_x86_64
...
cp311-none-manylinux_2_17_x86_64
cp311-none-manylinux2014_x86_64
cp311-none-manylinux_2_16_x86_64
...
cp311-none-manylinux_2_12_x86_64
cp311-none-manylinux2010_x86_64
cp311-none-manylinux_2_11_x86_64
...
cp311-none-manylinux_2_5_x86_64
cp311-none-manylinux1_x86_64
cp311-none-linux_x86_64
cp310-abi3-manylinux_2_28_x86_64
...

but we do

cp311_cp311_manylinux_2_28_x86_64
cp311_none_manylinux_2_28_x86_64
...
cp311_cp311_manylinux_2_17_x86_64
cp311_none_manylinux_2_17_x86_64
cp311_cp311_manylinux2014_x86_64
cp311_none_manylinux2014_x86_64
cp311_cp311_manylinux_2_16_x86_64
cp311_none_manylinux_2_16_x86_64
...
cp311_cp311_manylinux_2_12_x86_64
cp311_none_manylinux_2_12_x86_64
cp311_cp311_manylinux2010_x86_64
cp311_none_manylinux2010_x86_64
cp311_cp311_manylinux_2_11_x86_64
cp311_none_manylinux_2_11_x86_64
...
cp311_cp311_manylinux_2_5_x86_64
cp311_none_manylinux_2_5_x86_64
cp311_cp311_manylinux1_x86_64
cp311_none_manylinux1_x86_64
cp311_cp311_linux_x86_64
cp311_none_linux_x86_64
cp311_abi3_manylinux_2_28_x86_64
...
cp311_abi3_manylinux_2_17_x86_64
cp311_abi3_manylinux2014_x86_64
cp311_abi3_manylinux_2_16_x86_64
...
cp311_abi3_manylinux_2_12_x86_64
cp311_abi3_manylinux2010_x86_64
cp311_abi3_manylinux_2_11_x86_64
...
cp311_abi3_manylinux_2_5_x86_64
cp311_abi3_manylinux1_x86_64
cp311_abi3_linux_x86_64
cp310_abi3_manylinux_2_28_x86_64
...

This is a different part of the code though, I'll address in a second pull request.

@charliermarsh
Copy link
Member

Oh interesting… they might be mutually exclusive such that it hasn’t mattered? Hard to reason about from my phone :D Thank you for digging into this!

@zanieb zanieb force-pushed the zb/platform-tags-fix branch 3 times, most recently from ec7d8cc to b325acd Compare March 16, 2024 17:34
@zanieb
Copy link
Member Author

zanieb commented Mar 16, 2024

I'm no expert, just mindlessly matching Python's behavior at this point. See #2489 for the ABI changes.

I'll wait for @konstin to review both of these.

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

Great!

Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

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

I’m comfortable signing off on this if you want to merge.

Copy link

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Thanks, this looks right to me too.

Base automatically changed from zb/platform-tags to main March 16, 2024 22:20
zanieb added a commit that referenced this pull request Mar 16, 2024
Adding ordering test coverage as a preface to fixing
#2477 (see #2483)
# Conflicts:
#	crates/platform-tags/src/tags.rs

# Conflicts:
#	crates/platform-tags/src/tags.rs

# Conflicts:
#	crates/platform-tags/src/tags.rs
@zanieb zanieb enabled auto-merge (squash) March 16, 2024 22:21
@zanieb zanieb merged commit 653327b into main Mar 16, 2024
30 checks passed
@zanieb zanieb deleted the zb/platform-tags-fix branch March 16, 2024 22: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.

3 participants