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

platform.libc_ver() returns incorrect version number #70731

Closed
thomaswaldmann mannequin opened this issue Mar 12, 2016 · 36 comments
Closed

platform.libc_ver() returns incorrect version number #70731

thomaswaldmann mannequin opened this issue Mar 12, 2016 · 36 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes release-blocker stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@thomaswaldmann
Copy link
Mannequin

thomaswaldmann mannequin commented Mar 12, 2016

BPO 26544
Nosy @malemburg, @doko42, @vstinner, @benjaminp, @ned-deily, @njsmith, @serhiy-storchaka, @ThomasWaldmann
PRs
  • bpo-26544: Fixed implementation of platform.libc_ver(). #7684
  • [3.7] bpo-26544: Fixed implementation of platform.libc_ver(). (GH-7684). #8193
  • [3.6] bpo-26544: Fixed implementation of platform.libc_ver(). (GH-7684) #8195
  • [2.7] bpo-26544: Fixed implementation of platform.libc_ver(). (GH-7684) #8196
  • bpo-26544: Get rid of dependence from distutils in platform. #8356
  • [2.7] bpo-26544: Get rid of dependence from distutils in platform. (GH-8356) #8952
  • [3.7] bpo-26544: Get rid of dependence from distutils in platform. (GH-8356). #8970
  • [3.6] bpo-26544: Get rid of dependence from distutils in platform. (GH-8356). #8971
  • bpo-26544: Add test for platform._comparable_version(). #8973
  • [3.6] [3.7] bpo-26544: Get rid of dependence from distutils in platform. (GH-8356). (GH-8970) #9061
  • [2.7] bpo-26544: Get rid of dependence from distutils in platform. (GH-8356). (GH-8970) #9062
  • [2.7] bpo-26544: Make platform.libc_ver() less slow #10868
  • Files
  • results-20160709-224820.csv
  • platform-no-distutils.diff: platform-not-using-distutils
  • 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:

    assignee = None
    closed_at = <Date 2018-09-17.10:25:38.814>
    created_at = <Date 2016-03-12.00:36:03.132>
    labels = ['3.7', '3.8', 'type-bug', 'library', 'release-blocker']
    title = 'platform.libc_ver() returns incorrect version number'
    updated_at = <Date 2018-12-05.14:23:05.563>
    user = 'https://github.com/ThomasWaldmann'

    bugs.python.org fields:

    activity = <Date 2018-12-05.14:23:05.563>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-09-17.10:25:38.814>
    closer = 'serhiy.storchaka'
    components = ['Library (Lib)']
    creation = <Date 2016-03-12.00:36:03.132>
    creator = 'Thomas.Waldmann'
    dependencies = []
    files = ['43672', '47755']
    hgrepos = []
    issue_num = 26544
    keywords = ['patch', '3.6regression', '3.7regression']
    message_count = 36.0
    messages = ['261624', '261648', '261649', '270073', '270080', '270089', '319456', '319479', '321301', '321308', '321310', '321311', '321462', '322021', '322023', '323795', '323796', '323802', '323805', '323816', '323823', '323825', '324164', '324231', '324581', '324582', '324588', '324637', '324638', '324639', '324784', '324794', '325158', '325520', '330951', '331116']
    nosy_count = 8.0
    nosy_names = ['lemburg', 'doko', 'vstinner', 'benjamin.peterson', 'ned.deily', 'njs', 'serhiy.storchaka', 'Thomas.Waldmann']
    pr_nums = ['7684', '8193', '8195', '8196', '8356', '8952', '8970', '8971', '8973', '9061', '9062', '10868']
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue26544'
    versions = ['Python 2.7', 'Python 3.6', 'Python 3.7', 'Python 3.8']

    @thomaswaldmann
    Copy link
    Mannequin Author

    thomaswaldmann mannequin commented Mar 12, 2016

    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").

    @thomaswaldmann thomaswaldmann mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Mar 12, 2016
    @malemburg
    Copy link
    Member

    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.

    @malemburg
    Copy link
    Member

    Adding other Python versions as well, since this is a bug.

    @njsmith
    Copy link
    Contributor

    njsmith commented Jul 10, 2016

    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:
    https://github.com/pypa/pip/blob/master/pip/utils/glibc.py#L9

    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 :-(.

    @malemburg
    Copy link
    Member

    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.

    @njsmith
    Copy link
    Contributor

    njsmith commented Jul 10, 2016

    The purpose of the function was to determine the minimum libc compatibility requirements of the executable.

    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.

    parsing needs to be kept around as fallback solution

    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.

    @serhiy-storchaka
    Copy link
    Member

    I need the glibc version for skipping a test if run with glibc containing a bug (see bpo-31630 and bpo-33630). platform.libc_ver() is not usable, it always returns '2.9' (the version that has bugs), while the actual version on my computer is '2.25' (the version that doesn't have these bugs).

    @serhiy-storchaka
    Copy link
    Member

    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:

    1. Version numbers compared as strings.
    2. Versions that are located on the border of 16 KiB blocks were not recognized or were recognized incorrectly.

    @serhiy-storchaka serhiy-storchaka added 3.7 (EOL) end of life 3.8 only security fixes labels Jun 13, 2018
    @serhiy-storchaka
    Copy link
    Member

    New changeset 2a9b8ba by Serhiy Storchaka in branch 'master':
    bpo-26544: Fixed implementation of platform.libc_ver(). (GH-7684)
    2a9b8ba

    @serhiy-storchaka
    Copy link
    Member

    New changeset 7c43b80 by Serhiy Storchaka in branch '3.7':
    [3.7] bpo-26544: Fixed implementation of platform.libc_ver(). (GH-7684). (GH-8193)
    7c43b80

    @serhiy-storchaka
    Copy link
    Member

    New changeset d73497b by Serhiy Storchaka (Miss Islington (bot)) in branch '3.6':
    bpo-26544: Fixed implementation of platform.libc_ver(). (GH-7684). (GH-8193) (GH-8195)
    d73497b

    @serhiy-storchaka
    Copy link
    Member

    New changeset b1e6e56 by Serhiy Storchaka in branch '2.7':
    bpo-26544: Fixed implementation of platform.libc_ver(). (GH-7684). (GH-8193) (GH-8196)
    b1e6e56

    @vstinner
    Copy link
    Member

    Can we close this issue? It seems like the bug has been fixed, no?

    @doko42
    Copy link
    Member

    doko42 commented Jul 20, 2018

    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.

    @doko42
    Copy link
    Member

    doko42 commented Jul 20, 2018

    a tuple comparison should be good enough

    @doko42
    Copy link
    Member

    doko42 commented Aug 20, 2018

    no, it's not fixed at all. Setting as a release blocker for 3.6 and 3.7.

    @doko42
    Copy link
    Member

    doko42 commented Aug 20, 2018

    proposed patch attached

    @serhiy-storchaka
    Copy link
    Member

    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.

    @doko42
    Copy link
    Member

    doko42 commented Aug 20, 2018

    fine! what prevents merging and backporting this issue?

    @serhiy-storchaka
    Copy link
    Member

    I'll merge it if it looks good to you.

    @malemburg
    Copy link
    Member

    I added a comment to the PR, but other than that I think it's good to go.

    @doko42
    Copy link
    Member

    doko42 commented Aug 21, 2018

    +1

    @serhiy-storchaka
    Copy link
    Member

    New changeset 7d81e8f by Serhiy Storchaka in branch 'master':
    bpo-26544: Get rid of dependence from distutils in platform. (GH-8356)
    7d81e8f

    @vstinner
    Copy link
    Member

    Would it be possible to add a few tests on platform._comparable_version()? It would make me more confortable for backports.

    @serhiy-storchaka
    Copy link
    Member

    New changeset 7917aad by Serhiy Storchaka in branch 'master':
    bpo-26544: Add test for platform._comparable_version(). (GH-8973)
    7917aad

    @vstinner
    Copy link
    Member

    vstinner commented Sep 4, 2018

    Thanks for adding tests ;-) It now looks better to me ;-)

    @serhiy-storchaka
    Copy link
    Member

    New changeset 20a8392 by Serhiy Storchaka in branch '3.7':
    [3.7] bpo-26544: Get rid of dependence from distutils in platform. (GH-8356). (GH-8970)
    20a8392

    @vstinner
    Copy link
    Member

    vstinner commented Sep 5, 2018

    New changeset 1a3eb12 by Victor Stinner (Miss Islington (bot)) in branch '3.6':
    [3.7] bpo-26544: Get rid of dependence from distutils in platform. (GH-8356) (GH-8970) (GH-9061)
    1a3eb12

    @vstinner
    Copy link
    Member

    vstinner commented Sep 5, 2018

    New changeset 9734024 by Victor Stinner (Miss Islington (bot)) in branch '2.7':
    bpo-26544: Get rid of dependence from distutils in platform. (GH-8356) (GH-8952)
    9734024

    @vstinner
    Copy link
    Member

    vstinner commented Sep 5, 2018

    Thanks everybody! The issue should now be fixed in all supported branches ;-)

    @vstinner vstinner closed this as completed Sep 5, 2018
    @serhiy-storchaka
    Copy link
    Member

    Thank you for merging PRs Victor, but seems you have merged wrong PR for 2.7.

    @vstinner
    Copy link
    Member

    vstinner commented Sep 7, 2018

    Thank you for merging PRs Victor, but seems you have merged wrong PR for 2.7.

    Oh. I didn't notice that there were two PRs for 2.7. Do you want to rebase your PR 9062 on 2.7, or revert the commit that I merged? It's up to you ;-)

    I reopen the issue.

    It seems like PR 8971 is also open.

    @vstinner vstinner reopened this Sep 7, 2018
    @ned-deily
    Copy link
    Member

    This is now only open and a release blocker for 2.7, correct?

    @serhiy-storchaka
    Copy link
    Member

    Seems the only difference between PR 8952 and PR 9062 is additional tests. This is not important.

    @vstinner
    Copy link
    Member

    vstinner commented Dec 3, 2018

    New changeset 8687bd8 by Victor Stinner in branch '2.7':
    bpo-26544: Make platform.libc_ver() less slow (GH-10868)
    8687bd8

    @vstinner
    Copy link
    Member

    vstinner commented Dec 5, 2018

    bpo-26544: Make platform.libc_ver() less slow (GH-10868)
    8687bd8

    "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%)

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes release-blocker stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants