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

Modernize Lib/platform.py code #79527

Closed
vstinner opened this issue Nov 29, 2018 · 15 comments
Closed

Modernize Lib/platform.py code #79527

vstinner opened this issue Nov 29, 2018 · 15 comments
Labels
3.8 only security fixes stdlib Python modules in the Lib dir

Comments

@vstinner
Copy link
Member

BPO 35346
Nosy @malemburg, @brettcannon, @vstinner, @serhiy-storchaka
PRs
  • bpo-35346, platform: replace os.popen() with subprocess #10786
  • bpo-35346, platform: import subprocess in _syscmd_file() #10892
  • bpo-35346: Drop Mac OS 9 support from platform #10959
  • bpo-35346: Cleanup platform.architecture() #11130
  • 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-12-14.12:15:57.567>
    created_at = <Date 2018-11-29.01:42:38.864>
    labels = ['3.8', 'library']
    title = 'Modernize Lib/platform.py code'
    updated_at = <Date 2018-12-14.12:15:57.565>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2018-12-14.12:15:57.565>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-12-14.12:15:57.567>
    closer = 'vstinner'
    components = ['Library (Lib)']
    creation = <Date 2018-11-29.01:42:38.864>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 35346
    keywords = ['patch']
    message_count = 15.0
    messages = ['330657', '330718', '330727', '330729', '330731', '330733', '330735', '330737', '330943', '331061', '331285', '331709', '331710', '331813', '331815']
    nosy_count = 4.0
    nosy_names = ['lemburg', 'brett.cannon', 'vstinner', 'serhiy.storchaka']
    pr_nums = ['10786', '10892', '10959', '11130']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue35346'
    versions = ['Python 3.8']

    @vstinner
    Copy link
    Member Author

    Lib/platform.py still contains code to support Python 2.3. It's maybe time to modernize it.

    platform imports subprocess since commit fc990e9. The two calls to os.popen() can now be replaced with subprocess.Popen.

    @vstinner vstinner added 3.8 only security fixes stdlib Python modules in the Lib dir labels Nov 29, 2018
    @brettcannon
    Copy link
    Member

    It's because the module was originally kept compatible with Python 1.5.2: https://www.python.org/dev/peps/pep-0291/#backward-compatible-packages-modules-and-tools

    @serhiy-storchaka
    Copy link
    Member

    While we are here, look at other outdated code:

    • Top-imported warnings is not used.
    • struct.calcsize('P') is always success.
    • plistlib is always available.

    Should we keep compatibility with 2.7?

    @vstinner
    Copy link
    Member Author

    Should we keep compatibility with 2.7?

    I don't think that the master branch (Python 3.8) has to be compatible with Python 2.7.

    @malemburg
    Copy link
    Member

    Please keep Python 2.7 compatibility. It should be possible to copy the module back into Python 2.7 and use it there. This is not hard to do and allows it to fulfill its purpose as platform detection module even while part of the stdlib.

    @vstinner
    Copy link
    Member Author

    Please keep Python 2.7 compatibility. It should be possible to copy the module back into Python 2.7 and use it there.

    I don't understand why someone would like to copy Lib/platform.py from the master branch to Python 2.7? Python 2.7 already provides the platform module, it's part of its standard library.

    platform.linux_distribution() function has been removed from master, so you cannot simply copy platform.py from master and use it in Python 2.7: it has a different API.

    @malemburg
    Copy link
    Member

    Ok, let me add some history here:

    When I created the platform module it was clear that this would be
    a module which will frequently need updates, since platforms evolve
    faster than Python does.

    I had developed this with a larger number of contributors outside
    the stdlib for a while and then there was a request to add it to the
    stdlib.

    Now in order to keep the module more or less up-to-date, it still
    required regular updates, so the plan was to have it updated in the
    current versions of Python, but allow it to be used in older Python
    versions as well. That was the compromise to have it in the stdlib
    and not external. Otherwise, I would have not added it to the stdlib.

    This is why it has a special status and keep backwards compatibility
    much longer than other code in the stdlib.

    This worked quite well, but for some systems such as the Linux
    distros, it was impossible to keep up with the development in that
    mode. Well, actually, there were multiple reasons why this part
    failed: 1. Linux distros didn't not have a standard when I added
    the code, 2. Then some distros started two or three different ones,
    3. Distros started to use multiple standards with conflicting data,
    4. New distros became popular more often than we could update the
    code.

    That's why I was fine with removing the code again and leaving this
    part to a PyPI package.

    Does it make more sense now ?

    @vstinner
    Copy link
    Member Author

    Now in order to keep the module more or less up-to-date, it still
    required regular updates, so the plan was to have it updated in the
    current versions of Python, but allow it to be used in older Python
    versions as well. That was the compromise to have it in the stdlib
    and not external. Otherwise, I would have not added it to the stdlib.

    I don't understand if you are describing the current status or the history here. Do you know users who don't use platform form the stdlib, but use a more recent copy?

    These users cannot get a more recent version of Python?

    When you talk about copying Lib/platform.py from master to Python 2.7, are you talking about the ability to copy platform.py from master into an application to get a more recent version on Python 2.7? If yes, I don't understand how you plan to add back the linux_distribution() feature?

    Sorry, I'm still lost :-(

    @vstinner
    Copy link
    Member Author

    vstinner commented Dec 3, 2018

    Another issue with Python 2 compatibility (I addition to platform.dist() removal) of Lib/platform.py from master: it uses re.ASCII which doesn't exist in Python 2.7.

    @vstinner
    Copy link
    Member Author

    vstinner commented Dec 4, 2018

    New changeset b8e689a by Victor Stinner in branch 'master':
    bpo-35346, platform: import subprocess in _syscmd_file() (GH-10892)
    b8e689a

    @vstinner
    Copy link
    Member Author

    vstinner commented Dec 7, 2018

    New changeset 3a521f0 by Victor Stinner in branch 'master':
    bpo-35346, platform: replace os.popen() with subprocess (GH-10786)
    3a521f0

    @vstinner
    Copy link
    Member Author

    New changeset b0e0877 by Victor Stinner in branch 'master':
    bpo-35346: Drop Mac OS 9 support from platform (GH-10959)
    b0e0877

    @vstinner
    Copy link
    Member Author

    Top-imported warnings is not used.

    Fixed by commit b8e689a.

    struct.calcsize('P') is always success.

    I wrote PR 11130 for that.

    Note: struct.calcsize('P') always works on Python 2.7 as well.

    plistlib is always available.

    I don't know this module. This module imports many other modules like xml.parsers.expat. I don't think that the try/except ImportError hurts. Feel free to propose a PR if you want.

    @vstinner
    Copy link
    Member Author

    New changeset 4aa917c by Victor Stinner in branch 'master':
    bpo-35346: Cleanup platform.architecture() (GH-11130)
    4aa917c

    @vstinner
    Copy link
    Member Author

    I close the issue. I made that changes that I wanted :-)

    Serhiy: reopen the issue or open a new one if you want to remove the try/except ImportError around "import plistlib".

    @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.8 only security fixes stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants