-
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
Detect x32 userspace ABI on 64-bit kernel (fixes #4962) #5391
Conversation
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 way out of my water here. It's gonna be someone else who has to review this.
if machine == "x86_64": | ||
result = "linux_x32" | ||
else: | ||
result = "linux_i686" # and machine == "i686" |
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.
Maybe add a conditional with warning here, if machine != "i686"
?
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.
Is there a standard way within pip that I should warn?
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.
logger.warning
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 don't think a warning is necessary here - falling back to source installs when there's no defined wheel tag is perfectly normal (e.g. it always happens on non-macOS BSD systems, Solaris, etc), and there's nothing stopping anyone setting up their own registry with linux_x32
wheels if they want to (even if PyPI
doesn't allow them).
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.
Sounds fair to me. :)
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.
The warning would be to highlight that the platform cannot be detected correctly; eg. some ficticious future x42 ABI that pip doesn't yet understand.
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.
Another possibility: if we set result = "linux_" + machine
, would that be compatible with all platforms when using setarch? I guess I'd be worried about whether that formatting is right for all arches. (Since this patch will now make pip require setarch for non-x32 ABIs per #5391 (comment))
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 way out of my water here. It's gonna be someone else who has to review this.
Paging @rmcgibbo to take a look, I think you wrote the initial 32-bit userspace on 64-bit kernel logic here :) |
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 like a nice targeted fix to me. It could also be turned into a patch against setuptools to adjust the answer given by distutils.util.get_platform()
(although assessing the compatibility implications of making the change at that level could be trickier).
Either way, this is a good way to handle the problem at the pip level.
if machine == "x86_64": | ||
result = "linux_x32" | ||
else: | ||
result = "linux_i686" # and machine == "i686" |
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 don't think a warning is necessary here - falling back to source installs when there's no defined wheel tag is perfectly normal (e.g. it always happens on non-macOS BSD systems, Solaris, etc), and there's nothing stopping anyone setting up their own registry with linux_x32
wheels if they want to (even if PyPI
doesn't allow them).
This PR does cause a subtle behaviour change; currently if you run an i686 Python on an x86_64 kernel without changing personality using Note that a similar problem already arises with other arches eg. a mipsel Python on a mips kernel, or an armel Python on an armhf kernel will misdetect the architecture if you do not use (There is no |
^ I tested this and was able to confirm this finding, but I'm not sure if I have a good answer for how to address it. FWIW, I didn't notice the breakage initially because schroot uses setarch by default. |
@dstufft Could you take a look at this? |
This isn't finished because it breaks i686 detection without setarch. The "correct" way to fix this as far as I can tell is to parse the running Python ELF binary to see which interpreter it's using. Do you know if I will need to implement the logic myself or can I take a dependency on parseelf? (or whatever that library is called) |
Taking up parseelf should be fine, unless it has a dependency on a C library. |
Given #6908, do we want to put try to get this into pypa/packaging instead? |
cross-reference #7102, seems it could be similar to armhf detection. |
A little bit more complicated since it will need to handle 64-bit which changes some field sizes. I think for this use case we can assume it's LE? If not then we'll need to account for that too. |
Thanks @ehashman for your work on this! Some ELF parsing logic is being added in pypa/packaging#221. Once that change is in, this should be straightforward to address over there. I will close this and we can track that followup in pypa/packaging#217. |
Closes #4962
I didn't see any unit tests for this particular logic, and the tests ran very slow on my old laptop, so I'm going to let Travis take a look at this for me.
See #4962 for more context and my reproduction of the bug/how to test the fix.
I chose this approach because it is almost the same as the current logic, with an exception for the x32 ABI. There are other possible approaches I could have taken (including throwing an exception for a case we haven't accounted for in order to avoid hitting this bug in the future), but I wanted to be conservative here. I'm open to other suggestions.