-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
platform.libc_ver() returns incorrect version number #70731
Comments
platform.libc_ver() is trivially broken as it uses string comparison internally to determine the maximum libc version number (which is obviously broken as "2.9" > "2.10"). |
True. At the time the code was written, this was not an issue :-) Is the libc version information documented somewhere ? If so, we could probably add a better parser for it. |
Adding other Python versions as well, since this is a bug. |
We just ran into this in pip -- pypa/pip#3836 I'd really recommend dropping the current "grovel through the binary doing a regex search" strategy -- it's incredibly error prone, and AFAICT doesn't really give any value. This code OTOH reliably lets you detect glibc and gives the exact version number with no fuss: What about non-glibc systems? Unfortunately the current libc_ver() turns out not to work well for those either. Attached is a CSV file showing the return value of ~1.2 billion calls to platform.libc_ver() by the last 6 months of pip users. You can see that the current code basically never returns anything useful for non-glibc platforms. (The one exception is that it seems to be able to detect uclibc 0.9.32 and label it as "libc 0.9.32".) Don't get me wrong: it'd be really really useful if there were some way to detect and distinguish between the common non-glibc libcs like musl/bionic/uclibc/..., but I'm not sure how to do that -- and unfortunately the current code definitely doesn't do the job :-(. |
At the time the code was written, libc and glibc were in wide spread use, so it's not surprising that it doesn't work well for other C libs. Note that the routine returns the highest libc version number used and required by the executable (usually the Python interpreter). This does not necessarily correspond to the version installed on the system. The purpose of the function was to determine the minimum libc compatibility requirements of the executable. The routine you quote uses ctypes and only works for glibc, so parsing needs to be kept around as fallback solution. It also returns the libc version that is currently used on the system; not necessarily the minimum version required, so semantics are different. |
For what it's worth, I didn't know this, the pip authors obviously didn't know this, and after rereading the docs just now I still can't quite tell that this was intended. I'm not sure why one would want these semantics either, but at a minimum it would help to document the intended semantics much more clearly.
The point of the data I attached was that AFAICT there don't exist any currently-in-use, non-glibc systems where "parsing" returns a meaningful result. |
I agree that the strategy used for platform.libc_ver() is not perfect. But the implementation has bugs that make it useless. The following PR fixes two bugs in the implementation:
|
Can we close this issue? It seems like the bug has been fixed, no? |
please don't close this yet. the patch introduces a dependency on the distutils package. I don't see any reason to compare the libc version as a python egg version, so please avoid that. if we need to have a module to compare arbitrary version strings, then we should think about adding one in the standard library, but I think that's what the packaging module is for. |
a tuple comparison should be good enough |
no, it's not fixed at all. Setting as a release blocker for 3.6 and 3.7. |
proposed patch attached |
platform-no-distutils.diff restores the original bug: "2.9" > "2.10". PR 8356 removes the dependency from distutils and use a sophisticated function for parsing versions. |
fine! what prevents merging and backporting this issue? |
I'll merge it if it looks good to you. |
I added a comment to the PR, but other than that I think it's good to go. |
+1 |
Would it be possible to add a few tests on platform._comparable_version()? It would make me more confortable for backports. |
Thanks for adding tests ;-) It now looks better to me ;-) |
Thanks everybody! The issue should now be fixed in all supported branches ;-) |
Thank you for merging PRs Victor, but seems you have merged wrong PR for 2.7. |
This is now only open and a release blocker for 2.7, correct? |
"Coarse benchmark on Fedora 29: 1.6 sec => 0.1 sec." Oops, my benchmark in the commit message was wrong, it included the startup time. Correct benchmark says 44x faster, it's way better! Python 2.7: [old_py2] 1.51 sec +- 0.03 sec -> [if_in] 34.6 ms +- 0.4 ms: 43.61x faster (-98%) |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: