classification
Title: subprocess.getstatusoutput changed behavior in 3.4 (maybe 3.3.4?)
Type: behavior Stage: resolved
Components: Documentation Versions: Python 3.7, Python 3.6, Python 3.5, Python 3.4
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: Arfrever, acassaigne, benjamin.peterson, christian.heimes, docs@python, gregory.p.smith, haypo, josh.r, ncoghlan, r.david.murray, tim.golden
Priority: normal Keywords: patch

Created on 2014-10-14 21:09 by josh.r, last changed 2017-09-07 23:45 by gregory.p.smith. This issue is now closed.

Files
File name Uploaded Description Edit
issue22635.diff acassaigne, 2016-10-13 11:46 review
issue22635-2.diff acassaigne, 2016-10-13 12:50 review
issue22635-3.diff acassaigne, 2016-10-13 13:13 review
issue22635-4.diff acassaigne, 2016-10-13 15:06 review
Pull Requests
URL Status Linked Edit
PR 3398 merged gregory.p.smith, 2017-09-06 23:07
PR 3411 merged python-dev, 2017-09-07 00:39
PR 3435 merged gregory.p.smith, 2017-09-07 22:37
PR 3439 merged python-dev, 2017-09-07 23:11
Messages (27)
msg229354 - (view) Author: Josh Rosenberg (josh.r) * Date: 2014-10-14 21:09
(U) The examples for the function still show the return code in the form os.popen would produce (a program exiting with status 1 would return 256 as the status), but the new code from #10197 makes the status 1, not 256.

(U) This is a breaking change for code relying on what was already a legacy interface. Either the docs should call out the change, or the code needs to restore the previous behavior.

(U) Ultra simple repro:

>>> subprocess.getstatusoutput('python -c "exit(1)"')
Expected:
(256, '')
Actual:
(1, '')
msg229355 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-10-14 21:14
It probably comes from this change:
---
changeset:   86879:c34e163c0086
branch:      3.3
parent:      86870:dbff708e393f
user:        Tim Golden <mail@timgolden.me.uk>
date:        Sun Nov 03 12:53:17 2013 +0000
files:       Lib/subprocess.py Lib/test/test_subprocess.py Misc/NEWS
description:
Issue #10197 Rework subprocess.get[status]output to use subprocess functionality and thus to work on Windows. Patch by Nick Coghlan.
---

> (U) This is a breaking change for code relying on what was already a legacy interface.

CommandTests.test_getoutput() only checks that an error returns a non-zero status :-/ It doens't check for the exact status. We should use a small Python program using a specific exit code (ex: sys.exit(5)) to check the status.
msg229356 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-10-14 21:18
Oh, I now understand why I feel guilty, I proposed a patch rewriting getstatusoutput() in the issue #10197. My patch ends with:

+    if os.name != 'nt':
+        # convert status to be interpreted according to the wait() rules
+        sts = sts << 8

This fix is that simple, but it means that depending on the Python version, you will get a different status...

Python 3.3 doesn't accept bugfixes anymore and so cannot be changed.
msg229357 - (view) Author: Josh Rosenberg (josh.r) * Date: 2014-10-14 21:19
Ah blech. Can someone with privileges edit my original message to remove the junk at the beginning of each paragraph? Habit from an old job. Wish I could just edit the message.
msg229358 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-10-14 21:21
> Can someone with privileges edit my original message to remove the junk at the beginning of each paragraph?

It's not possible to edit a message, only to remove it. I don't like removing the initial message of an issue.

Don't worry, (U) looks a bullet, it can read it ;-)
msg229359 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-10-14 21:23
You are right, it did change in 3.3.4 (see issue 10197).  That change should not have been applied to 3.3, and obviously there was a missing test concerning the return code format.

At this point I think we are stuck with changing the documentation.  The new return code is both more convenient and more consistent with the rest of the subprocess module.  It is very unfortunate that it did not follow our normal backward compatibility rules, but we are stuck with the mistake now.
msg229360 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-10-14 21:31
> You are right, it did change in 3.3.4 (see issue 10197).  That change should not have been applied to 3.3, ...

The purpose of the issue #10197 was to fix a bug on Windows: getoutput() didn't work.
msg229361 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-10-14 21:34
But it was also a feature addition: getoutput had not been *intended* to work on Windows.  I understand why the mistake was made (the argument that it was a bug has weight), but the fact that a versionchanged was needed mentioning 3.3.4 indicates it wasn't really a bug fix.
msg278562 - (view) Author: Cassaigne (acassaigne) * Date: 2016-10-13 11:46
I correct the doc concerning subprocess.getstatusoutput(cmd) function.
More specifically the part on examples and I add a quick explanation about how are manage the return code.
msg278570 - (view) Author: Cassaigne (acassaigne) * Date: 2016-10-13 12:50
I improve the patch a little bit, following the recommendations of the Stinner's review. 
I replace "status" mention by "exit code".
msg278573 - (view) Author: Cassaigne (acassaigne) * Date: 2016-10-13 13:13
According to the Victor's review, I remove "Pay attention" words and change exit code by returncode.
msg278574 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-10-13 13:15
I disagree with Victor.  The name of the function is "getstatusoutput".  I think the docs should continue to use (status, output) as the names for the return values.  The clarification is that 'status' is now the raw return code, not the shifted return code that it formerly returned.

Also, the patch should include a new test that checks the actual return code value.
msg278575 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2016-10-13 13:21
issue22635-3.diff: LGTM.


R. David Murray: "I disagree with Victor.  The name of the function is "getstatusoutput".  I think the docs should continue to use (status, output) as the names for the return values."

In Python, we use "exitcode" or "returncode" names for the exit code, but "status" for the thing that should be parsed with os.WEXITSTATUS(status).

Just to contradict me, the manual page of the exit() function uses the "status" term: "void _exit(int status);", not "code".

"The clarification is that 'status' is now the raw return code, not the shifted return code that it formerly returned."

Sorry, I'm confused by this sentence :-) getstatusoutput() returns an exit code, the parameter of exit(), no more the annoying "status" thing that should be passed to os.WEXITSTATUS(status) to get a regular exit code.

"Also, the patch should include a new test that checks the actual return code value."

FYI acassaigne is a newcomer currently in a sprint and this issue is tagged as Documentation. I suggest to first push a doc change and then add an unit test.
msg278576 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2016-10-13 13:32
Ah, I see.  So the function's name is wrong now, too :(.

OK, I guess using exitcode is more accurate then.  Surprising that the original docs did not link to os.WEXITSTATUS.  Maybe we could make a crosslink in the versionchanged entry to os.WEXITSTATUS in the way of explanation of what the difference between 'status' and 'exitcode' is?  Otherwise the versionchanged sentence doesn't seem to tell the reader anything.
msg278579 - (view) Author: Cassaigne (acassaigne) * Date: 2016-10-13 15:06
I add a crosslink to WEXITSTATUS function. According David Murray advices.
msg299925 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-08-08 14:23
Bug mentionned on a Twitter discussion:
https://twitter.com/zhenech/status/894444040992829441
msg299965 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-08-09 03:28
I've added Greg to the nosy list and filed an issue with subprocess32 about this: https://github.com/google/python-subprocess32/issues/30

I did that mainly to make sure they don't inadvertently backport this regression, but also to ask if subprocess32 might potentially provide a way to get the old intended-but-not-actually-tested behaviour back in 3.x.
msg299969 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-08-09 04:57
I intend subprocess32 to Python 2 only.  Python 3 users should use the standard library subprocess module.

subprocess32 does not include getstatusoutput() or getoutput() functions as those are available in the Python 2 stdlib commands module.
msg299970 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-08-09 05:05
If you want something API compatible that can be used in a Python 2 and 3 application that wants a getstatusoutput() API I think an appropriate place to put it would be to add a commands modules to https://pypi.python.org/pypi/past/ with a consistent getstatusoutput() API.  Either there or something in https://pypi.python.org/pypi/six/.

The inadvertent API difference between 2.7 commands.getstatusoutput() and 3.3.4 subprocess.getstatusoutput() is unfortunate but - I agree - too late to change.  Fixing the documentation to mention the discrepancy is the least worst option.
msg299979 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-08-09 09:21
IMHO it's too late to change *again* subprocess.getstatusoutput() behaviour. Otherwise, it would mean that a complex test like "if not((3, 4) <= sys.version_info < (3, 7))" would be needed to workaround the bug... Whereas right now, basically we only have to check if we are running on Python 3 or not. (Python 3.0-3.3 is almost not used in the wild.)

Instead we should just *document* the behaviour change using ".. versionchanged:: 3.4" in Doc/library/subprocess.rst.

Any volunteer to do that?
msg300032 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-08-09 20:11
I'll get to it.  acassaigne's patches are already a good attempt at that, i'll probably find wording tweaks to make as I apply them.
msg300038 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-08-10 03:18
Issue filed with six about this: https://github.com/benjaminp/six/issues/207

It turns out to be somewhat timely on that side as well, as it turns out that six has a not-yet-released change adding "six.moves.commands", which doesn't currently account for this inadvertent incompatibility in the way exit codes are reported.
msg301558 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-09-07 00:39
New changeset 738b7d9766e1a794aaaabfba0d515a467ba833ca by Gregory P. Smith in branch 'master':
bpo-22635: subprocess.getstatusoutput doc update. (#3398)
https://github.com/python/cpython/commit/738b7d9766e1a794aaaabfba0d515a467ba833ca
msg301564 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-09-07 02:57
New changeset dee54f6010ee2df60322c350a5f1dc8bfcf367d6 by Christian Heimes (Miss Islington (bot)) in branch '3.6':
[3.6] bpo-22635: subprocess.getstatusoutput doc update. (GH-3398) (#3411)
https://github.com/python/cpython/commit/dee54f6010ee2df60322c350a5f1dc8bfcf367d6
msg301567 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-09-07 04:34
marking "closed/resolved" but also "won't fix" as the behavior change remains, but we've done the safest thing at this point: documented the fact.
msg301647 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-09-07 23:11
New changeset 2eb0cb4787d02d995a9bb6dc075983792c12835c by Gregory P. Smith in branch 'master':
bpo-22635: Update the getstatusoutput docstring. (#3435)
https://github.com/python/cpython/commit/2eb0cb4787d02d995a9bb6dc075983792c12835c
msg301648 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-09-07 23:45
New changeset fb4c28c032e26b3cdbe67eae3769d45207ac3507 by Gregory P. Smith (Miss Islington (bot)) in branch '3.6':
[3.6] bpo-22635: Update the getstatusoutput docstring. (GH-3435) (#3439)
https://github.com/python/cpython/commit/fb4c28c032e26b3cdbe67eae3769d45207ac3507
History
Date User Action Args
2017-09-14 22:01:32gregory.p.smithlinkissue23508 superseder
2017-09-07 23:45:01gregory.p.smithsetmessages: + msg301648
2017-09-07 23:11:13python-devsetpull_requests: + pull_request3436
2017-09-07 23:11:06gregory.p.smithsetmessages: + msg301647
2017-09-07 22:37:19gregory.p.smithsetpull_requests: + pull_request3432
2017-09-07 04:34:01gregory.p.smithsetstatus: open -> closed
resolution: wont fix
messages: + msg301567

stage: resolved
2017-09-07 02:57:54christian.heimessetnosy: + christian.heimes
messages: + msg301564
2017-09-07 00:39:36python-devsetpull_requests: + pull_request3411
2017-09-07 00:39:25gregory.p.smithsetmessages: + msg301558
2017-09-06 23:07:40gregory.p.smithsetpull_requests: + pull_request3404
2017-08-10 03:18:07ncoghlansetnosy: + benjamin.peterson
messages: + msg300038
2017-08-09 20:11:35gregory.p.smithsetassignee: docs@python -> gregory.p.smith
messages: + msg300032
2017-08-09 09:21:04hayposetmessages: + msg299979
2017-08-09 05:05:04gregory.p.smithsetmessages: + msg299970
2017-08-09 04:57:10gregory.p.smithsetmessages: + msg299969
2017-08-09 03:28:18ncoghlansetnosy: + gregory.p.smith
messages: + msg299965
2017-08-08 14:23:19hayposetmessages: + msg299925
2016-10-13 15:06:29acassaignesetfiles: + issue22635-4.diff

messages: + msg278579
2016-10-13 13:32:20r.david.murraysetmessages: + msg278576
2016-10-13 13:21:51hayposetmessages: + msg278575
2016-10-13 13:15:09r.david.murraysetmessages: + msg278574
2016-10-13 13:13:03acassaignesetfiles: + issue22635-3.diff

messages: + msg278573
2016-10-13 12:50:21acassaignesetfiles: + issue22635-2.diff

messages: + msg278570
2016-10-13 11:46:05acassaignesetfiles: + issue22635.diff
versions: + Python 3.6, Python 3.7
nosy: + acassaigne

messages: + msg278562

keywords: + patch
2014-10-14 23:36:11Arfreversetnosy: + Arfrever
2014-10-14 21:34:02r.david.murraysetmessages: + msg229361
2014-10-14 21:31:45hayposetmessages: + msg229360
2014-10-14 21:23:52r.david.murraysetnosy: + r.david.murray, docs@python
messages: + msg229359

assignee: docs@python
components: + Documentation
2014-10-14 21:21:17hayposetmessages: + msg229358
2014-10-14 21:19:55josh.rsetmessages: + msg229357
2014-10-14 21:18:08hayposetmessages: + msg229356
versions: - Python 3.3
2014-10-14 21:14:25hayposetnosy: + tim.golden, haypo, ncoghlan
messages: + msg229355
2014-10-14 21:09:18josh.rcreate