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

subprocess.py doesn't correctly detect Windows machines #52357

Closed
midnightdf mannequin opened this issue Mar 10, 2010 · 28 comments
Closed

subprocess.py doesn't correctly detect Windows machines #52357

midnightdf mannequin opened this issue Mar 10, 2010 · 28 comments
Labels
3.8 only security fixes OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@midnightdf
Copy link
Mannequin

midnightdf mannequin commented Mar 10, 2010

BPO 8110
Nosy @gpshead, @amauryfa, @pitrou, @vstinner, @giampaolo, @DinoV, @bitdancer, @JimJJewett, @zware
PRs
  • bpo-8110: Refactor platform detection in subprocess #9053
  • Files
  • issue8110_subprocess_mswindows.diff: Patch, apply to 2.x
  • issue8110.diff
  • 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-10.23:28:08.714>
    created_at = <Date 2010-03-10.15:57:11.997>
    labels = ['3.8', 'type-bug', 'library', 'OS-windows']
    title = "subprocess.py doesn't correctly detect Windows machines"
    updated_at = <Date 2018-09-10.23:29:20.933>
    user = 'https://bugs.python.org/midnightdf'

    bugs.python.org fields:

    activity = <Date 2018-09-10.23:29:20.933>
    actor = 'zach.ware'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-09-10.23:28:08.714>
    closer = 'gregory.p.smith'
    components = ['Library (Lib)', 'Windows']
    creation = <Date 2010-03-10.15:57:11.997>
    creator = 'midnightdf'
    dependencies = []
    files = ['16518', '36063']
    hgrepos = []
    issue_num = 8110
    keywords = ['patch']
    message_count = 28.0
    messages = ['100788', '100789', '100790', '100791', '100793', '100794', '100795', '100796', '100798', '100957', '100978', '122717', '220803', '220831', '220833', '220835', '220836', '221105', '221107', '223850', '228392', '324614', '324641', '324644', '324645', '324956', '324957', '324958']
    nosy_count = 10.0
    nosy_names = ['gregory.p.smith', 'amaury.forgeotdarc', 'pitrou', 'vstinner', 'giampaolo.rodola', 'dino.viehland', 'r.david.murray', 'midnightdf', 'Jim.Jewett', 'zach.ware']
    pr_nums = ['9053']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'commit review'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue8110'
    versions = ['Python 3.8']

    @midnightdf
    Copy link
    Mannequin Author

    midnightdf mannequin commented Mar 10, 2010

    subprocess.py contains the following line (380 in CPython 2.6.3):
    mswindows = (sys.platform == "win32")

    which only correctly detects CPython on Windows. This line should be changed to:
    mswindows = (sys.platform == "win32" or sys.platform == "cli" or sys.platform == "silverlight")

    so subprocess can be utilized from IronPython as well.

    This bug should be trivial to fix.

    @midnightdf midnightdf mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Mar 10, 2010
    @bitdancer
    Copy link
    Member

    Probably it should use platform.system() == 'Windows' instead.

    @bitdancer bitdancer added the easy label Mar 10, 2010
    @amauryfa
    Copy link
    Member

    but, does it work even with your fix?
    it seems to me that "from _subprocess import *" will fail miserably.

    @briancurtin
    Copy link
    Member

    Also, using platform.system() == 'Windows' would exclude IronPython running on Mono.

    @midnightdf
    Copy link
    Mannequin Author

    midnightdf mannequin commented Mar 10, 2010

    platform.system()=="Windows" won't work unless you change platform as well:
    IronPython 2.6.1 DEBUG (2.6.10920.0) on .NET 2.0.50727.4927
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import platform
    >>> platform.system()
    'cli'

    @amauryfa
    Copy link
    Member

    it's not about platform detection; does IronPython have the equivalent of msvcrt.open_osfhandle()?

    @midnightdf
    Copy link
    Mannequin Author

    midnightdf mannequin commented Mar 10, 2010

    Is there any reason _subprocess couldn't be implemented in C#? If not, this is the first of many steps needed to get subprocess working under IronPython.

    @florentx
    Copy link
    Mannequin

    florentx mannequin commented Mar 10, 2010

    Proposed patch, with duck subprocessing. :D

    @briancurtin
    Copy link
    Member

    Amaury is right. On Windows, _subprocess is a built-in, so the platform stuff just currently enables further failure.

    Dave: My C# skills aren't the greatest, but looking at PC/_subprocess.c there are a few Win32 functions you might need to P/Invoke, but I believe a lot of the functionality is supported in .NET.

    @briancurtin briancurtin removed the easy label Mar 10, 2010
    @pitrou
    Copy link
    Member

    pitrou commented Mar 12, 2010

    Florent's patch makes sense to me.

    @amauryfa
    Copy link
    Member

    What does the patch achieve?

    @briancurtin
    Copy link
    Member

    Jeff Hardy just made this change for IronPython 2.7: http://bitbucket.org/ironpython/ironlanguages/changeset/b6bb2a9a7bc5

    Any opposition to us matching that so they don't need to patch Lib/subprocess.py?

    @vstinner
    Copy link
    Member

    sys.platform == "cli" or sys.platform == "silverlight"

    It cannot happen in CPython, so I think that it's fine to leave CPython stdlib unchanged. I consider that the issue is fixed because the bug report was for IronPython and the bug was fixed there.

    @bitdancer
    Copy link
    Member

    It doesn't matter that it can't happen in CPython. The idea is that IronPython should be able to copy as much of the stdlib as possible without having to change it.

    That said, I'm not going to object if people decide this is in some sense a step too far in that direction. So I'm +0.5, Brain is +1, and Victor is -1. Any other votes? (Obviously IronPython hasn't wanted it enough to push it, so that could be considered as weighing on the negative side.)

    @bitdancer bitdancer reopened this Jun 17, 2014
    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Jun 17, 2014

    The important line from the link in msg122717 is :-

    mswindows = (sys.platform == "win32" or (sys.platform == "cli" and os.name == 'nt'))

    so +1 from me unless someone can show one or more problems with it.

    @vstinner
    Copy link
    Member

    I'm -0, I don't really care :-) I just that the issue is old and
    didn't get much attention.

    @vstinner
    Copy link
    Member

    Oh oh, new try in english:

    I vote -0, I don't really care :-) It's just that the issue is old and didn't get much attention.

    @jimjjewett
    Copy link
    Mannequin

    jimjjewett mannequin commented Jun 20, 2014

    It would be good to have the library work unchanged on both implementations.

    If subprocess only failed later, it would be less good, as the stdlib would then set an example that doesn't actually succeed.

    Note that the attached patch (by flox) does NOT implement the discussed "or" tests on sysm.platform; it instead checks whether _subprocess it importable. Is the assumption of an accelerator module itself too implementation-specific? I'm also not sure that the test in the patch isn't too broad -- is windows the only platform with _subprocess? Because if not, then the new test would mess up logic later in the file, such as that at line 635 in Popen.__init__

    @jimjjewett
    Copy link
    Mannequin

    jimjjewett mannequin commented Jun 20, 2014

    (The above concerns -- other than whether it is sufficient to work -- do not apply to the change at https://bitbucket.org/ironpython/ironlanguages/commits/b6bb2a9a7bc5/ )

    @zware
    Copy link
    Member

    zware commented Jul 24, 2014

    How about this? Should apply equally to 3.4 and default, 2.7 is different but can use the same concept (with s/_winapi/_subprocess/ among other changes).

    @BreamoreBoy
    Copy link
    Mannequin

    BreamoreBoy mannequin commented Oct 3, 2014

    Just to note that this is referenced from bpo-19453 pydoc and ironpython.

    @zware zware added 3.7 (EOL) end of life 3.8 only security fixes labels Sep 4, 2018
    @giampaolo
    Copy link
    Contributor

    If os.name == 'nt' is True on IronPython then why not simply do:

        mswindows = sys.platform == "win32" or os.name == "nt"

    For the record, both variants are used in different places in cPython source code. It would nice to figure out a golden standard and apply it everywhere. Maybe even add it as a new WINDOWS constant in the platform module.

    @zware
    Copy link
    Member

    zware commented Sep 5, 2018

    The real question isn't "are we on Windows?" but rather "should we use msvcrt/_winapi or _posixsubprocess/select?", which is what my PR is designed to better fit. I could see how we wouldn't want to backport that patch to maintenance branches, though; it's a significant rearrangement. If we do want to backport a fix, I agree that simply adding or os.name == 'nt' should be sufficiently low-impact.

    @vstinner
    Copy link
    Member

    vstinner commented Sep 5, 2018

    I don't think that we should backport this change to 3.7 and older.

    I understand that the change is about supporting Cygwin. My policy to support a new platform is always the same: first fix *all* issues in master, get a working CI, find a core developer responsible to fix all issues specific to this platform, and only after that we can start discussing about backporting specific changes for that platform.

    https://pythondev.readthedocs.io/cpython.html#i-want-cpython-to-support-my-platform

    @zware
    Copy link
    Member

    zware commented Sep 5, 2018

    This one has nothing to do with Cygwin, this is about minimizing patching that IronPython (or other implementations) have to do to standard library modules.

    @zware
    Copy link
    Member

    zware commented Sep 10, 2018

    New changeset 880d42a by Zachary Ware in branch 'master':
    bpo-8110: Refactor platform detection in subprocess (GH-9053)
    880d42a

    @zware
    Copy link
    Member

    zware commented Sep 10, 2018

    Do we want to bother backporting anything, or call it fixed in 3.8 and move on?

    @gpshead
    Copy link
    Member

    gpshead commented Sep 10, 2018

    lets call this fixed. if and only if another Python implementation pipes up as happier if this exists in an older than 3.8 version we can consider backporting the same refactoring to that code.

    @gpshead gpshead closed this as completed Sep 10, 2018
    @zware zware removed the 3.7 (EOL) end of life label Sep 10, 2018
    @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 OS-windows stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants