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

Manylinux2010 #92

Merged
merged 12 commits into from
Nov 18, 2018
Merged

Manylinux2010 #92

merged 12 commits into from
Nov 18, 2018

Conversation

wtolson
Copy link
Contributor

@wtolson wtolson commented May 15, 2018

This is an update of @markrwilliams merge request #88. Updates the tag from manylinux2 to manylinux2010 to reflect the changes in PEP 571. Teaches auditwheel about manylinux2010. See pypa/manylinux#152 for the corresponding Docker image.

markrwilliams and others added 6 commits January 29, 2018 16:55
This script is derived from the explanation in ec6171f's commit
message.  Thanks to @ehashman for describing the source of these
symbols!
- Remove libpanel and libncurses from the lib_whitelist (pypa#94)
- Generate symbol_versions
- Bump priority to 200.
test_manylinux uses an interim manylinux2 image stored on Docker Hub,
and a fork of pip that understands the manylinux2 ABI tag.
@ehashman
Copy link
Member

@wtolson, @markrwilliams, thanks for putting this together. At a glance this all looks great to me (love the new symbol generation script), but I do want a chance to review this more closely before I hit merge. I dunno if any of the other maintainers would like to take a look.

@ehashman ehashman mentioned this pull request May 24, 2018
@mayeut
Copy link
Member

mayeut commented Jun 5, 2018

Tests are now showing that wheels get manylinux2010 when built on manylinux1 image.
Shall priority be reversed between the 2 policies ?

Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

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

This looks good to me. I'm going to sync up with @njsmith before merging.

@ofek
Copy link
Contributor

ofek commented Aug 27, 2018

Can this be merged?

@ehashman
Copy link
Member

@ofek: @njsmith pointed out to me (as does @mayeut's comment above) that manylinux2010 labels are being applied to manylinux1 compliant wheels. The order should be reversed. If a wheel passes both the manylinux1 and manylinux2010 policy, it should be labeled with the more restrictive policy (in that case, manylinux1).

So we'll need to get that fixed before this is ready to merge, otherwise this will break backwards compatibility and relabel manylinux1 wheels as manylinux2010, which we don't want.

@@ -25,5 +25,26 @@
"libX11.so.6", "libXext.so.6", "libXrender.so.1", "libICE.so.6",
"libSM.so.6", "libGL.so.1", "libgobject-2.0.so.0",
"libgthread-2.0.so.0", "libglib-2.0.so.0", "libresolv.so.2"
]},
{"name": "manylinux2010",
"priority": 200,
Copy link
Member

Choose a reason for hiding this comment

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

Following up on my most recent comment, I believe this priority should be lowered such that it is between manylinux1 (most restrictive) and "linux" (least restrictive). Current manylinux1 binaries should also be manylinux2010 compliant, but not vice versa.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should have done the priorities in increasing rather than decreasing order... feel free to pick an arbitrary number between 0 and 100 for now (90?) and if we need to renumber later because the range was too small we can do that eventually.

@safijari
Copy link

What needs to be done to get this resolved? Is there some way I can help?

@ehashman
Copy link
Member

@safijari the policy precedence needs to be adjusted and the tests updated, per my last comment (and others)

@safijari
Copy link

safijari commented Oct 17, 2018

@ehashman Okay, I'm a little new to contributing to opensource projects, and I'm trying to see if I can help push this forward. Is it possible for me to push onto this PR? or should I open a new one?

Edit: I opened #104 as I do not know of a better way.

@ehashman
Copy link
Member

I'm committing to getting this merged this upcoming weekend. I'll make the necessary changes. Send me angry emails/GH messages if this isn't merged by Monday the 19th 😄

Copy link
Member

@njsmith njsmith left a comment

Choose a reason for hiding this comment

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

I didn't do a super-detailed review since I'm not actually that familiar with auditwheel internals, but I did read it over and it looked fine to me!

Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

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

Finished my own review of the changes and added one more test update for pure wheels. Once that passes Travis this is GTM.

@ehashman ehashman merged commit d99cc1d into pypa:master Nov 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants