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

Preliminary manylinux2 support. #5008

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@markrwilliams

markrwilliams commented Jan 31, 2018

Teaches pip how to install manylinux2 wheels. See pypa/manylinux#152, pypa/auditwheel#88.

@markrwilliams markrwilliams referenced this pull request Jan 31, 2018

Merged

PEP 571: Manylinux2 #565

@pradyunsg

This comment has been minimized.

Member

pradyunsg commented Apr 4, 2018

This'll have to wait till at least manylinux makes the merge.

@markrwilliams markrwilliams referenced this pull request Apr 14, 2018

Open

Tracking issue for manylinux2010 rollout #179

3 of 14 tasks complete
@ncoghlan

This comment has been minimized.

Member

ncoghlan commented Apr 15, 2018

@pradyunsg I think pip can avoid having to wait for manylinux2010 build support by using a test wheel that's actually just a renamed manylinux1 wheel. As long as the test wheel used doesn't depend on the ncurses 5 ABI, that's a valid thing to do.

elif platform is None and is_manylinux1_compatible():
arches = [arch.replace('linux', 'manylinux1'), arch]
elif arch.startswith('manylinux2'):
# manylinux1 wheels run on manylinux2 systems.

This comment has been minimized.

@ncoghlan

ncoghlan Apr 15, 2018

Member

Given the removal of ncurses 5 from the ABI requirement, this implication isn't necessarily true: it's possible for a platform to be compatible with manylinux2010 without being compatible with manylinux1.

This comment has been minimized.

@njsmith

njsmith Apr 15, 2018

Member

In practice pip's heuristics for deciding whether a platform is compatible with manylinux1 and manylinux2010 don't actually do any checking of ncurses. I suppose it's possible for someone to provide a _manylinux module that disables manylinux1 but enables manylinux2010 (though this would be very unusual).

The branch below that just calls is_manylinux2_compatible and is_manylinux1_compatible seems like the simplest approach, and it's obviously correct. In fact I don't even understand how this branch could get hit in the first place...

This comment has been minimized.

@ncoghlan

ncoghlan Apr 15, 2018

Member

@njsmith The _manylinux.is_manylinux1_compatible == False override is exactly the case I had in mind. While that's unusual now, and while formally incompatible distros may choose not to set it on the basis of "ncurses dependencies are rare, while manylinux1 wheels are common", I think we want to start out with the position that both variants of Linux platform compatibility should be checked independently.

As for how this branch can be reached, it's only when manylinux2010 is passed in specifically as the platform argument (presumably because the system checking the tags isn't actually a Linux system, or else isn't manylinux compatible).

So I think the best way to start out will be with a strict interpretation of the manylinux2010 compatibility tag, and then treat "Opt-in to accepting manylinux1 wheels when explicitly specifying a manylinux2010 target" as a follow-on feature request.

This comment has been minimized.

@ncoghlan

ncoghlan May 15, 2018

Member

@wtolson pointed out that the PEP explicitly calls for manylinux1 wheels to be treated as manylinux2010 compatible, even with the ncurses dependency issue: https://www.python.org/dev/peps/pep-0571/#backwards-compatibility-with-manylinux1-wheels

I don't think it's worth going back and amending the PEP at this point, so let's just keep the branch, and add a reference to that part of the PEP in a comment.

@pradyunsg

This comment has been minimized.

Member

pradyunsg commented Apr 15, 2018

@ncoghlan I'm cool with that. I'm not completely familiar with the details here so, I'll defer for the merge to someone else. :)

PS: Thanks for the tracking issue. I was gonna ask "what's the POA" and noticed that you'd created one. ^.^

@ncoghlan

Just putting a blocking "needs updating to reflect the accepted version of PEP 571" change request on here :)

That should mostly just be the name change to manylinux2010, but there's also the inline comment about the fact that a platform can be manylinux2010 compatible without being manylinux1 compatible, so the two tags should be checked separately rather than assuming one implies the other.

@ncoghlan

This comment has been minimized.

Member

ncoghlan commented Apr 15, 2018

Ah, I forgot I don't actually have commit access on the pip repo - @pradyunsg or @pfmoore, would you mind adding a "requires changes" review to prevent accidental merges?

@pradyunsg

As @ncoghlan requested.

@@ -276,8 +293,16 @@ def get_supported(versions=None, noarch=False, platform=None,
else:
# arch pattern didn't match (?!)
arches = [arch]
elif platform is None and is_manylinux1_compatible():
arches = [arch.replace('linux', 'manylinux1'), arch]
elif arch.startswith('manylinux2'):

This comment has been minimized.

@dstufft

dstufft Apr 15, 2018

Member

FWIW I feel like this is probably a bit of footgun. It feels like the kind of thing that's asking to be a bug once we have a manylinux2018 or something.

@wtolson wtolson referenced this pull request May 15, 2018

Open

Manylinux2010 #5410

@pradyunsg

This comment has been minimized.

Member

pradyunsg commented Aug 27, 2018

Closing in favour of #5410. @markrwilliams if you do want to continue with this, feel free to ask to reopen this PR. :)

@pradyunsg pradyunsg closed this Aug 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment