subprocess.py doesn't correctly detect Windows machines

[midnightdf]

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]

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.

[bitdancer]

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

[amauryfa]

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

[briancurtin]

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

[midnightdf]

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]

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

[midnightdf]

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]

Proposed patch, with duck subprocessing. :D

[briancurtin]

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.

[pitrou]

Florent's patch makes sense to me.

[amauryfa]

What does the patch achieve?

[briancurtin]

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]

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]

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

[BreamoreBoy]

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]

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

[vstinner]

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]

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]

(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]

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]

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

[giampaolo]

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]

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]

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]

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]

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

[zware]

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

[gpshead]

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.