classification
Title: subprocess.check_output should allow specifying stdin as a string
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: BreamoreBoy, python-dev, r.david.murray, serhiy.storchaka, zwol
Priority: normal Keywords: patch

Created on 2012-12-05 22:54 by zwol, last changed 2013-04-22 17:23 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
issue16624-v34a.diff zwol, 2013-04-18 17:37 Patch vs 3.4 (updated)
Messages (22)
msg177014 - (view) Author: Zack Weinberg (zwol) * Date: 2012-12-05 22:54
subprocess.check_output calls Popen.communicate but does not allow you to specify its argument (i.e. data to send to the child process's stdin).  It would be nice if it were enhanced to allow this.  Proposed patch attached (to the 2.7 subprocess.py; should apply with trivial changes in 3.x).
msg177036 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-12-06 10:32
2.7 is in bugfix only mode. Please update your patch to 3.4.
msg177120 - (view) Author: Zack Weinberg (zwol) * Date: 2012-12-07 19:57
OK, here is a patch against the latest development version.  Now also with tests and documentation updates.
msg177125 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-12-07 20:21
This is a beautiful patch. LGTM.

However it should be tested on Windows. I'm not sure that reading not closed file in different process works on Windows.

Zack, can you please submit a contributor form?

http://python.org/psf/contrib/contrib-form/
http://python.org/psf/contrib/
msg177130 - (view) Author: Zack Weinberg (zwol) * Date: 2012-12-07 20:53
I don't have the ability to test on Windows, but the construct you are concerned about was copied from other tests in the same file which were not marked as Unix-only.

I have faxed in a contributor agreement.
msg179692 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-11 16:17
Zack, can you please resend your agreement by email?
msg180871 - (view) Author: Zack Weinberg (zwol) * Date: 2013-01-28 18:26
Contributor agreement resent by email.  Sorry for the delay.
msg187254 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2013-04-18 14:32
The patch doesn't apply cleanly on Windows.  Trying to sort this with TortoiseHg has left me completely flummoxed so can I leave this with the OP to provide a new patch?
msg187272 - (view) Author: Zack Weinberg (zwol) * Date: 2013-04-18 17:37
Here is a new patch vs latest trunk.
msg187277 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-18 18:19
I think that it will be better not introduce a new argument, but reuse stdin. Just allow io.BytesIO (or perhaps even any file object) be specified as stdin.

The change will be straightforward:

if isinstance(stdin, io.BytesIO):
    inputdata = stdin.read()
    stdin = PIPE

Or more general:

if not(stdin is None or stdin in (PIPE, DEVNULL) or isinstance(stdin, int)):
    try:
        stdin.fileno()
    except (AttributeError, UnsupportedOperation, OSError):
        inputdata = stdin.read()
        stdin = PIPE
msg187278 - (view) Author: Zack Weinberg (zwol) * Date: 2013-04-18 18:32
> I think that it will be better not introduce a new argument, but reuse stdin. Just allow io.BytesIO (or perhaps even any file object) be specified as stdin.

If we were designing from scratch I might agree, but we aren't.  Principle of least astonishment says that the API here should match `subprocess.communicate`, and that has separate `stdin=FILE` and `input=STRING` arguments.
msg187279 - (view) Author: Zack Weinberg (zwol) * Date: 2013-04-18 18:39
Note also that allowing `stdin=<any filelike>` in a clean fashion would require rather more surgery than you suggest, because a filelike can produce an infinite stream of data, and people would expect that to work when the subprocess only reads a finite prefix; making it *actually* work would involve teaching communicate() to take a filelike and copy blocks into the pipe.  I have no *objection* to that change but I think it is too much mission creep for this proposal.

With the present design, where stdin= has to be something for which fileno() is defined, and input= has to be a string (hence of finite length), no one is going to expect something to work that won't.
msg187285 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-18 19:48
I partially agree with you. We must copy blocks in communicate().

Your patch is great, but I doubt that there is a best feature. If we teach communicate() to work with file-like objects, this feature will exceed your suggestion and 'input' parameter of check_output() will be redundant. Are you want to implement a more powerful feature? If not, I will commit your patch. But I hope that before the 3.4 release we will replace it with a more powerful feature.

Mark, can you please run subprocesses tests on Windows?
msg187289 - (view) Author: Zack Weinberg (zwol) * Date: 2013-04-18 20:23
My position is:

* input= should be supported in check_output(), for consistency with communicate().

* I like the idea of making stdin= support file-like objects which don't have a fileno, in both communicate() and everything that calls it, but that does not belong in this issue, I do not have time to code it myself, and, again, it should be *in addition to*, not *instead of*, supporting input= in check_output().
msg187290 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-04-18 20:38
Since it is a new feature either way, we can add stdin support and deprecate the input= argument of communicate.  But we can also go with the input= for check_output now, and see if anyone steps up to do the bigger patch before 3.4 hits beta.  Which is what I hear Serhiy suggesting.
msg187291 - (view) Author: Zack Weinberg (zwol) * Date: 2013-04-18 20:40
??? communicate() has always had input= AFAIK.
msg187292 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-04-18 20:55
Yes.  IIUC are talking about possibly replacing it with a more powerful feature.  We wouldn't actually remove it, but we would recommend using the new feature instead, thus making the fact that check_output doesn't have it irrelevant.
msg187294 - (view) Author: Zack Weinberg (zwol) * Date: 2013-04-18 21:08
OK, I get that, but what I'm saying is I think input= is still desirable even if stdin= becomes more powerful.
msg187299 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2013-04-18 21:52
Tests rerun on Windows and were fine.
msg187317 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-04-19 00:16
Why?  What is the use case that it serves better?  I'm not saying there isn't one, but one isn't occurring to me.
msg187573 - (view) Author: Roundup Robot (python-dev) Date: 2013-04-22 17:21
New changeset 5c45d0ca984f by Serhiy Storchaka in branch 'default':
Issue #16624: `subprocess.check_output` now accepts an `input` argument,
http://hg.python.org/cpython/rev/5c45d0ca984f
msg187574 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-22 17:23
Thanks Zack for you patch. Thanks Mark for testing.
History
Date User Action Args
2013-04-22 17:23:51serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg187574

stage: patch review -> resolved
2013-04-22 17:21:52python-devsetnosy: + python-dev
messages: + msg187573
2013-04-19 00:16:38r.david.murraysetmessages: + msg187317
2013-04-18 21:52:59BreamoreBoysetmessages: + msg187299
2013-04-18 21:08:36zwolsetmessages: + msg187294
2013-04-18 20:55:16r.david.murraysetmessages: + msg187292
2013-04-18 20:40:11zwolsetmessages: + msg187291
2013-04-18 20:38:16r.david.murraysetnosy: + r.david.murray
messages: + msg187290
2013-04-18 20:23:12zwolsetmessages: + msg187289
2013-04-18 19:48:37serhiy.storchakasetmessages: + msg187285
2013-04-18 18:39:33zwolsetmessages: + msg187279
2013-04-18 18:32:42zwolsetmessages: + msg187278
2013-04-18 18:19:57serhiy.storchakasetmessages: + msg187277
2013-04-18 17:37:35zwolsetfiles: - issue16624-v34.diff
2013-04-18 17:37:23zwolsetfiles: + issue16624-v34a.diff

messages: + msg187272
2013-04-18 14:32:54BreamoreBoysetnosy: + BreamoreBoy
messages: + msg187254
2013-01-28 18:26:45zwolsetmessages: + msg180871
2013-01-11 16:17:44serhiy.storchakasetmessages: + msg179692
2012-12-29 22:01:23serhiy.storchakasetassignee: serhiy.storchaka
2012-12-07 20:53:26zwolsetmessages: + msg177130
2012-12-07 20:21:18serhiy.storchakasetmessages: + msg177125
stage: needs patch -> patch review
2012-12-07 19:58:18zwolsetfiles: - subprocess-check-output-allow-input.diff
2012-12-07 19:57:52zwolsetfiles: + issue16624-v34.diff

messages: + msg177120
2012-12-06 10:32:52serhiy.storchakasetversions: + Python 3.4
nosy: + serhiy.storchaka

messages: + msg177036

stage: needs patch
2012-12-05 22:54:16zwolcreate