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

Update manylinux detection to be robust to incompatible ABIs #221

Merged
merged 1 commit into from
Jan 2, 2020

Conversation

mayeut
Copy link
Member

@mayeut mayeut commented Oct 17, 2019

armv7l machine overlaps multiple ABI (armhf, armel). The same goes for i686 when running on x86_64 kernel (i686, x32).
This commit checks that ABI is compatible with the ones defined in PEP 513/571/599.
Partially addresses #217

@mayeut mayeut force-pushed the manylinux branch 5 times, most recently from 1240cb2 to 1d6f755 Compare October 19, 2019 10:51
@mayeut mayeut marked this pull request as ready for review October 19, 2019 10:53
@chrahunt
Copy link
Member

This sets some groundwork for but doesn't fully address #217, since it does not add the corresponding platform tags. Cf pypa/pip#5391.

@brettcannon
Copy link
Member

I don't know anything about the ELF format so I'm not in a good position to review this.

@mayeut
Copy link
Member Author

mayeut commented Oct 21, 2019

This sets some groundwork for but doesn't fully address #217, since it does not add the corresponding platform tags.

I'm reluctant to add a new platform tag for x32 since it could break the current ecosystem (where I guess all tools relies on the platform tag being linux_i686). IMHO, the major issue is that, on x32 systems, i686 manylinux wheels are installed instead of using local cache (c.f. #160 also) or building from sources. This PR only prevents downloading i686 manylinux wheels on those systems.

That being said, if @pypa/packaging-committers want me to add a new platform tag, I will do so.

@brettcannon
Copy link
Member

@mayeut if i686 wheels won't work on x32 then listing a more appropriate CPU architecture in the platform tag makes sense to me.

@chrahunt
Copy link
Member

@mayeut if you just change the original comment to say "partially addresses #217" it's fine by me. :)

@mayeut
Copy link
Member Author

mayeut commented Oct 23, 2019

@chrahunt, done.

Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

Several minor comments. In general this approach looks good and is mostly in-line with the changes that were made in pypa/pip#7102.

packaging/tags.py Outdated Show resolved Hide resolved
packaging/tags.py Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
tests/test_tags.py Outdated Show resolved Hide resolved
packaging/tags.py Show resolved Hide resolved
packaging/tags.py Outdated Show resolved Hide resolved
@mayeut
Copy link
Member Author

mayeut commented Nov 2, 2019

@chrahunt, sorry it took so long for me to be able to work on this. The new commit should answer all your remarks. I can squash the commit if need be.

@mayeut mayeut requested a review from chrahunt November 2, 2019 13:33
packaging/tags.py Outdated Show resolved Hide resolved
tests/test_tags.py Outdated Show resolved Hide resolved
Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

Great! This is much easier to follow and will be easier to extend later if needed. Just one small comment, otherwise this looks good to me. Personally I would squash, but that's a decision that can also be made by whoever merges.

@mayeut
Copy link
Member Author

mayeut commented Nov 3, 2019

I squashed all commits and added some more tests.

tests/test_tags.py Outdated Show resolved Hide resolved
Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@mayeut
Copy link
Member Author

mayeut commented Dec 15, 2019

CI failures are not related to the changes in this PR.

@aixtools
Copy link

CI failures are not related to the changes in this PR.

Glad someone else had a push do the same thing my PR did - and if you do not mind, I'll steal your comment.

Still do not understand how #243 ever passed.

@brettcannon
Copy link
Member

@mayeut I think the failures are probably due to coverage 5.0 being released and tweaking its coverage results. I've opend #246 to track the need to fix this.

@mayeut mayeut force-pushed the manylinux branch 3 times, most recently from 78ba370 to 3e88626 Compare December 18, 2019 20:57
`armv7l` machine overlaps multiple ABI (`armhf`, `armel`). The same goes for `i686` when running on `x86_64` kernel (`i686`, `x32`).
This commit checks that ABI is compatible with the ones defined in PEP 513/571/599
@di
Copy link
Member

di commented Jan 2, 2020

This looks ready to merge. @brettcannon Would you like a chance to review/approve before merging?

@brettcannon
Copy link
Member

@brettcannon don't hold this PR up for my review; currently buried under other stuff.

@di di merged commit 6a320ab into pypa:master Jan 2, 2020
@mayeut mayeut deleted the manylinux branch January 10, 2020 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants