classification
Title: subprocess.py doesn't correctly detect Windows machines
Type: behavior Stage: commit review
Components: Library (Lib), Windows Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Jim.Jewett, amaury.forgeotdarc, dino.viehland, giampaolo.rodola, gregory.p.smith, midnightdf, pitrou, r.david.murray, vstinner, zach.ware
Priority: normal Keywords: patch

Created on 2010-03-10 15:57 by midnightdf, last changed 2018-09-10 23:29 by zach.ware. This issue is now closed.

Files
File name Uploaded Description Edit
issue8110_subprocess_mswindows.diff flox, 2010-03-10 16:28 Patch, apply to 2.x review
issue8110.diff zach.ware, 2014-07-24 15:35 review
Pull Requests
URL Status Linked Edit
PR 9053 merged zach.ware, 2018-09-04 05:02
Messages (28)
msg100788 - (view) Author: Dave Fugate (midnightdf) Date: 2010-03-10 15:57
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.
msg100789 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-03-10 16:13
Probably it should use platform.system() == 'Windows' instead.
msg100790 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-03-10 16:16
but, does it work even with your fix?
it seems to me that "from _subprocess import *" will fail miserably.
msg100791 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-03-10 16:19
Also, using platform.system() == 'Windows' would exclude IronPython running on Mono.
msg100793 - (view) Author: Dave Fugate (midnightdf) Date: 2010-03-10 16:22
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'
msg100794 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-03-10 16:26
it's not about platform detection; does IronPython have the equivalent of msvcrt.open_osfhandle()?
msg100795 - (view) Author: Dave Fugate (midnightdf) Date: 2010-03-10 16:27
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.
msg100796 - (view) Author: Florent Xicluna (flox) * (Python committer) Date: 2010-03-10 16:28
Proposed patch, with duck subprocessing. :D
msg100798 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-03-10 16:39
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.
msg100957 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-03-12 19:03
Florent's patch makes sense to me.
msg100978 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-03-12 22:24
What does the patch achieve?
msg122717 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-11-28 20:14
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?
msg220803 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-06-17 07:17
> 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.
msg220831 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-06-17 14:15
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.)
msg220833 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-06-17 14:40
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.
msg220835 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-06-17 15:15
I'm -0, I don't really care :-) I just that the issue is old and
didn't get much attention.
msg220836 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-06-17 15:20
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.
msg221105 - (view) Author: Jim Jewett (Jim.Jewett) (Python triager) Date: 2014-06-20 18:22
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__
msg221107 - (view) Author: Jim Jewett (Jim.Jewett) (Python triager) Date: 2014-06-20 18:25
(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/  )
msg223850 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2014-07-24 15:35
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).
msg228392 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2014-10-03 22:12
Just to note that this is referenced from #19453 pydoc and ironpython.
msg324614 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2018-09-05 09:06
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.
msg324641 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2018-09-05 14:53
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.
msg324644 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-09-05 15:23
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
msg324645 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2018-09-05 15:26
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.
msg324956 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2018-09-10 23:16
New changeset 880d42a3b247306f67837aa95e23f7c3471a30a3 by Zachary Ware in branch 'master':
bpo-8110: Refactor platform detection in subprocess (GH-9053)
https://github.com/python/cpython/commit/880d42a3b247306f67837aa95e23f7c3471a30a3
msg324957 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2018-09-10 23:16
Do we want to bother backporting anything, or call it fixed in 3.8 and move on?
msg324958 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-09-10 23:28
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.
History
Date User Action Args
2018-09-10 23:29:20zach.waresetversions: - Python 2.7, Python 3.6, Python 3.7
2018-09-10 23:28:08gregory.p.smithsetstatus: open -> closed
resolution: fixed
messages: + msg324958

stage: patch review -> commit review
2018-09-10 23:16:57zach.waresetmessages: + msg324957
2018-09-10 23:16:12zach.waresetmessages: + msg324956
2018-09-05 15:26:18zach.waresetmessages: + msg324645
2018-09-05 15:23:36vstinnersetmessages: + msg324644
2018-09-05 14:53:03zach.waresetmessages: + msg324641
2018-09-05 09:06:25giampaolo.rodolasetmessages: + msg324614
2018-09-04 18:58:42zach.waresetnosy: + gregory.p.smith, giampaolo.rodola

versions: + Python 3.6, Python 3.7, Python 3.8, - Python 3.4, Python 3.5
2018-09-04 10:59:22BreamoreBoysetnosy: - BreamoreBoy
2018-09-04 05:02:42zach.waresetpull_requests: + pull_request8514
2014-10-06 14:11:52brian.curtinsetnosy: - brian.curtin
2014-10-03 22:12:13BreamoreBoysetmessages: + msg228392
2014-07-24 15:35:40zach.waresetfiles: + issue8110.diff
versions: + Python 3.4, Python 3.5, - Python 2.6, Python 3.1, Python 3.2
nosy: + zach.ware

messages: + msg223850

stage: test needed -> patch review
2014-06-20 18:25:01Jim.Jewettsetmessages: + msg221107
2014-06-20 18:22:39Jim.Jewettsetnosy: + Jim.Jewett
messages: + msg221105
2014-06-17 15:20:55vstinnersetmessages: + msg220836
2014-06-17 15:15:13vstinnersetmessages: + msg220835
2014-06-17 14:40:17BreamoreBoysetnosy: + BreamoreBoy
messages: + msg220833
2014-06-17 14:15:44r.david.murraysetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg220831
2014-06-17 07:17:36vstinnersetstatus: open -> closed

nosy: + vstinner
messages: + msg220803

resolution: fixed
2010-11-28 20:14:54brian.curtinsetmessages: + msg122717
2010-08-08 21:00:25floxsetnosy: - flox
2010-03-12 22:24:18amaury.forgeotdarcsetmessages: + msg100978
2010-03-12 19:05:39michael.foordsetnosy: - michael.foord
2010-03-12 19:03:46pitrousetnosy: + pitrou
messages: + msg100957
2010-03-10 16:39:40brian.curtinsetkeywords: - easy

messages: + msg100798
2010-03-10 16:28:21floxsetfiles: + issue8110_subprocess_mswindows.diff
keywords: + patch
messages: + msg100796
2010-03-10 16:27:57midnightdfsetmessages: + msg100795
2010-03-10 16:26:39amaury.forgeotdarcsetmessages: + msg100794
2010-03-10 16:22:10midnightdfsetmessages: + msg100793
2010-03-10 16:19:28brian.curtinsetnosy: + brian.curtin
messages: + msg100791
2010-03-10 16:16:22amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg100790
2010-03-10 16:13:26r.david.murraysetkeywords: + easy
nosy: + dino.viehland, r.david.murray, michael.foord
messages: + msg100789

2010-03-10 16:12:08floxsetnosy: + flox
2010-03-10 16:04:31brian.curtinsetpriority: normal
versions: - Python 3.3
components: + Windows
stage: test needed
2010-03-10 15:57:12midnightdfcreate