-
Notifications
You must be signed in to change notification settings - Fork 3k
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 #5410
Manylinux2010 #5410
Conversation
This reflects the change in the tag name in PEP 571.
There was a problem hiding this 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, just one technical suggestion to reduce risk, and a request to make one comment a bit more detailed
src/pip/_internal/pep425tags.py
Outdated
@@ -263,7 +280,8 @@ def get_supported(versions=None, noarch=False, platform=None, | |||
|
|||
if not noarch: | |||
arch = platform or get_platform() | |||
if arch.startswith('macosx'): | |||
arch_prefix, arch_sep, arch_suffix = arch.partition('_') | |||
if arch_prefix == 'macosx': |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how the macosx
platform tags are actually structured, so it would be safer to keep the startswith
for this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I've reverted this line to use startswith
.
src/pip/_internal/pep425tags.py
Outdated
elif platform is None and is_manylinux1_compatible(): | ||
arches = [arch.replace('linux', 'manylinux1'), arch] | ||
elif arch_prefix == 'manylinux2010': | ||
# manylinux1 wheels run on manylinux2010 systems. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the discussion on the previous PR, we should add a link to the relevant PEP section in this comment, and qualify it as "Most" wheels being compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the comment here. Anything else you'd like to see added? Do you have a preferred method for including log urls?
This LGTM. @dstufft Could you take a look? |
Can this be merged? |
This looks like it's ready to be merged. Is there something more that needs to be done? |
@ehashman recently merged the change to let |
Thanks @pfmoore! |
Hmm, looks like this broke the build :-( See https://travis-ci.org/pypa/pip/builds/458357307 - can someone please submit a fix for this? It looks like a trivial lint failure - how come the CI didn't pick this up on the PR? (Sorry, I can't do it right now) |
@pfmoore It could be a new check was added since the PR was last updated - if I'd thought about it, I would have suggested closing and reopening to force a fresh CI run first. |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This is an update of @markrwilliams merge request #5008. Updates the tag from manylinux2 to manylinux2010 to reflect the changes in PEP 571. Also addresses @dstufft's suggestion to not use
startswith
to detect the arch. Thanks to @ncoghlan for the help!