classification
Title: patch to subprocess docs to better explain Popen's 'args' argument
Type: enhancement Stage: resolved
Components: Documentation Versions: Python 3.1, Python 3.2, Python 2.7, Python 2.6
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: georg.brandl Nosy List: benjamin.peterson, brian.curtin, christoph.neuroth, cvrebert, eric.smith, georg.brandl, ncoghlan, pitrou, r.david.murray, terry.reedy
Priority: normal Keywords: needs review, patch

Created on 2009-08-22 10:40 by cvrebert, last changed 2010-02-18 14:28 by christoph.neuroth. This issue is now closed.

Files
File name Uploaded Description Edit
subprocess-doc.patch r.david.murray, 2010-02-02 14:36
subprocess-doc.patch r.david.murray, 2010-02-02 15:24
subprocess.rst.patch cvrebert, 2010-02-04 10:47 tweak shell transcript
Messages (33)
msg91859 - (view) Author: Chris Rebert (cvrebert) * Date: 2009-08-22 10:40
From what I've seen on several c.l.p threads, some people have a tough
time figuring the correct 'args' argument to subprocess.Popen's
constructor. In an effort to cut down on such discussions in the future,
I've written the attached docs patch to better explain the subject.

I'm not an rst/Sphinx guru and thus was unable to actually test the
patch, so there might be some (hopefully minor) formatting errors.
msg92688 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2009-09-16 14:54
We are trying to cut down on the number of "warning" directives in the
docs; I think a "note" directive would be appropriate for yours.
msg93519 - (view) Author: Chris Rebert (cvrebert) * Date: 2009-10-04 06:41
Ok, changed to "note" directives instead of "warning"s. Anything else
that keeps this from being applied?
msg98658 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-02-01 11:47
This is a particularly verbose patch for the information it is trying to convey.
msg98683 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2010-02-01 20:00
The note starts out "Do not (do x) unless you're not (doing y)". This is confusing. I believe this means "If you are (doing y), you may (do x)". If so, please write it so. If not, please write what you do mean.

After thinking about it, "Only (do x) if you are (doing y)" might be a better rewrite. Or "You may (do x) only if you are (doing y)"

Change "be mindful of escaping" to the more direct "escape". Perhaps give a list of possible special characters.

I agree that this seems a bit verbose for reference documentation. Perhaps there should be a separate PyWiki entry or HowTo doc. That could, for instance, list the special characters for various shells if they are not all the same.
msg98684 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2010-02-01 20:03
PS. I only looked at the style of the patch. Once that is clearer, someone who knows more than me needs to review it for content correctness.
msg98707 - (view) Author: Chris Rebert (cvrebert) * Date: 2010-02-02 02:46
Working on a more concise new draft...
msg98711 - (view) Author: Chris Rebert (cvrebert) * Date: 2010-02-02 04:04
Okay; new, hopefully better, draft. Feedback?
msg98713 - (view) Author: Brian Curtin (brian.curtin) * (Python committer) Date: 2010-02-02 04:17
The raw_input() doesn't provide anything. I'd just drop that and pass the string directly to shlex.split.

"Do not put an argument-taking option together with its argument as a single item in the *args* list"
-- Something like "Argument-taking options should not be grouped with their arguments in the *args* list." may be a cleaner read, and avoids the big scary "do not".

"Because that is incorrect"
-- Maybe something like "shlex.split would suggest the following" instead?
msg98719 - (view) Author: Chris Rebert (cvrebert) * Date: 2010-02-02 05:53
Gonna have to disagree about the raw_input(), because the escaping involved would complicate the example and could be distracting/confusing.
Rest of Brian's suggestions taken into account.
msg98729 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-02-02 13:03
I still think this is much too verbose. The second note looks redundant with the first one and the third one.
Remember, the notes you are adding may be useful, but they'll also make reading the whole page more tedious.
msg98731 - (view) Author: Chris Rebert (cvrebert) * Date: 2010-02-02 13:18
Well, the same basic example is used for cohesiveness, but the issue/pitfall being highlighted in each note is distinct. But you have a point about shlex being pointed out twice, so here's a version with that redundancy excised.
msg98732 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-02-02 13:30
I like the idea of pointing out that shlex can be used to determine exactly what to pass to subprocess, but I agree that the proposed patch is too wordy (and still too much in a negative voice).

Here is an alternate simpler patch.

Note that while I have also clarified the last sentence in the shell=True case, I think that this is in fact a bug in the Popen API.  As far as I can tell the ability to pass arguments to the shell after the -c is useless, and I think Popen should instead raise a ValueError if passed a sequence when shell is True.  But that is a different bug report....
msg98733 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-02-02 13:33
Hmm.  Somehow the patch got lost.  Let's try again.
msg98734 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-02-02 13:38
Woops, spotted a word I left out.  Fixed.
msg98735 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-02-02 13:56
From David's patch:

   Note in particular that options and their arguments go in separate list
   elements, while arguments that need quoting when used in the shell
   (such as filenames containing spaces or the python command shown
   above) are single list elements.

It's not always true that options and their arguments should be in separate elements. For example:
['/bin/ls', '-l',  '--block-size=1024']
I understand the sentiment, but if we're trying to give guidance that people can follow without understanding the underlying issue, then they should be correct all the time.

Also, the code snippet includes subprocess without using it.
msg98736 - (view) Author: Chris Rebert (cvrebert) * Date: 2010-02-02 14:01
Counterpatch incorporating R. David Murray's succinctness improvements while retaining correct positioning of the first note, managing to incorporate the 3rd note not present in Mr. Murray's, and including more precise wording to address the problem Eric Smith pointed out.
msg98738 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-02-02 14:36
My placement of the note was carefully considered.  It is discussing the shell=False case and IMO belongs after that paragraph.  I understand now your concern about the note I omitted...and again I think this is a bug in the Popen API.  If shell=False, I think Popen should generate an error if args is a string.  Absent that fix, I've reworded the existing sentence along the lines you suggest, but using positive language rather than negative.  I've incorporated your fix for Eric's bug report, but I haven't added in the explicit references to the strings in the example.  IMO those are obvious and don't need repeating, but if others think it is clearer to add them, I'm fine with that.

As for subprocess, IMO there's no need to show the subprocess call.  The shlex result shows the list that would go into the subprocess call, and I think that's sufficient.  What do others think?

I've updated my version of the patch.
msg98739 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-02-02 14:40
By the way, I've been wanting the Popen docs improved for a long time but never got around to it, so thanks for pushing for this.
msg98742 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-02-02 14:56
The following sentences look like a distraction to me:

« Additional quoting may be required because the entire string is a Python string.  It may be useful to use a Python raw string in complex cases. »

It is true of every string literal and is not specific to the subprocess module.

Also, I don't understand what the reference to "-c" is about. subprocess isn't only for executing Python interpreters. Or perhaps you are talking about "sh -c", but it is quite cryptic.
msg98745 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-02-02 15:24
I'm happy to delete the two sentences about quoting.

As for -c, you are so right that it is cryptic.  In the new version of the patch I've changed the sentence to be as precise as possible, but I'm not at all convinced that it is less confusing.  This is why I think the API should be changed.
msg98746 - (view) Author: Chris Rebert (cvrebert) * Date: 2010-02-02 15:35
This version takes Murray's most recent draft, applies some minor tweaks from my prior patch, and has the "python -c" etc. changed to "echo '$MONEY'" so the sh -c comment is completely unambiguous (and it's a simpler example generally).

Whether the subprocess.Popen() call should be demonstrated in the code snippet remains an open issue.
msg98747 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-02-02 15:56
The change to echo is an excellent improvement.  You forgot to change 'python' to 'echo' in the following paragraph, though. You are also correct about /bin/sh vs sh, my bad.  And I was even looking at the source code when I wrote that...
msg98748 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-02-02 16:26
Now you need to put the import of subprocess back in!

Otherwise it looks good to me.
msg98826 - (view) Author: Chris Rebert (cvrebert) * Date: 2010-02-04 10:47
Okay, now if this could just get dev review...
msg98827 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-02-04 11:31
I think this is an improvement to the existing docs, and should be committed.
msg98832 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2010-02-04 12:45
Committed for 2.7 as r77959.

Still needs to be merged to the other branches.
msg98836 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2010-02-04 13:15
When merging to py3k, don't forget to modify the print statement to be a function.
msg98840 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-02-04 16:49
Merged as part of r77961 (2.6), r77962 (py3k), and r77963 (3.1).  Print fixed for py3.
msg98841 - (view) Author: Chris Rebert (cvrebert) * Date: 2010-02-04 16:52
Thanks to all for the copious feedback & suggestions, and R. David Murray for his superior docs writing skills!
msg98880 - (view) Author: Chris Rebert (cvrebert) * Date: 2010-02-05 16:12
One problem with the 3.x versions: the raw_input() should be input().
msg98881 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-02-05 16:28
Thanks for catching that.  Fixed r77987 r77988.
msg99508 - (view) Author: Christoph Neuroth (christoph.neuroth) Date: 2010-02-18 14:28
As recommended by eric.smith on #7950, I'd like to suggest further extending the documentation to include a security warning about (quite easily) possible code injection bugs when using the shell=True parameter (similar to other places in the documentation).
History
Date User Action Args
2010-07-22 00:26:06r.david.murrayunlinkissue7950 superseder
2010-02-18 14:28:48christoph.neurothsetnosy: + christoph.neuroth
messages: + msg99508
2010-02-17 11:55:23eric.smithlinkissue7950 superseder
2010-02-16 14:44:59r.david.murraylinkissue7939 superseder
2010-02-05 16:28:04r.david.murraysetmessages: + msg98881
2010-02-05 16:12:34cvrebertsetmessages: + msg98880
2010-02-04 16:52:28cvrebertsetmessages: + msg98841
2010-02-04 16:49:42r.david.murraysetstatus: open -> closed

messages: + msg98840
stage: patch review -> resolved
2010-02-04 13:15:26eric.smithsetstatus: pending -> open

messages: + msg98836
2010-02-04 12:45:20ncoghlansetstatus: open -> pending
resolution: accepted
messages: + msg98832
2010-02-04 12:15:59ncoghlansetfiles: - subprocess.rst.patch
2010-02-04 11:31:10eric.smithsetmessages: + msg98827
2010-02-04 10:48:05cvrebertsetfiles: - subprocess.rst.patch
2010-02-04 10:47:53cvrebertsetfiles: + subprocess.rst.patch

messages: + msg98826
2010-02-02 16:26:31eric.smithsetmessages: + msg98748
2010-02-02 15:59:17cvrebertsetfiles: - subprocess.rst.patch
2010-02-02 15:59:03cvrebertsetfiles: + subprocess.rst.patch
2010-02-02 15:56:16r.david.murraysetmessages: + msg98747
2010-02-02 15:35:42cvrebertsetfiles: - subprocess.rst.patch
2010-02-02 15:35:26cvrebertsetfiles: + subprocess.rst.patch

messages: + msg98746
2010-02-02 15:24:05r.david.murraysetfiles: + subprocess-doc.patch

messages: + msg98745
2010-02-02 14:56:17pitrousetmessages: + msg98742
2010-02-02 14:40:41r.david.murraysetmessages: + msg98739
2010-02-02 14:37:53r.david.murraysetfiles: - subprocess-doc.patch
2010-02-02 14:36:26r.david.murraysetfiles: + subprocess-doc.patch

messages: + msg98738
versions: + Python 3.1, Python 2.7
2010-02-02 14:01:49cvrebertsetfiles: - subprocess.rst.patch
2010-02-02 14:01:37cvrebertsetfiles: - subprocess.rst.patch
2010-02-02 14:01:28cvrebertsetfiles: + subprocess.rst.patch

messages: + msg98736
2010-02-02 13:56:05eric.smithsetnosy: + eric.smith
messages: + msg98735
2010-02-02 13:38:15r.david.murraysetfiles: + subprocess-doc.patch

messages: + msg98734
2010-02-02 13:37:31r.david.murraysetfiles: - subprocess-doc.patch
2010-02-02 13:33:57r.david.murraysetfiles: + subprocess-doc.patch

messages: + msg98733
2010-02-02 13:30:33r.david.murraysetpriority: normal
nosy: + r.david.murray
messages: + msg98732

2010-02-02 13:18:21cvrebertsetfiles: + subprocess.rst.patch

messages: + msg98731
2010-02-02 13:03:23pitrousetmessages: + msg98729
2010-02-02 05:54:26cvrebertsetfiles: - subprocess.rst.patch
2010-02-02 05:53:49cvrebertsetfiles: - subprocess.rst.patch
2010-02-02 05:53:17cvrebertsetfiles: + subprocess.rst.patch

messages: + msg98719
2010-02-02 04:17:39brian.curtinsetnosy: + brian.curtin
messages: + msg98713

keywords: + needs review
stage: patch review
2010-02-02 04:04:45cvrebertsetfiles: + subprocess.rst.patch

messages: + msg98711
versions: - Python 3.1
2010-02-02 02:46:30cvrebertsetmessages: + msg98707
2010-02-01 20:03:29terry.reedysetmessages: + msg98684
2010-02-01 20:00:43terry.reedysetnosy: + terry.reedy
messages: + msg98683
2010-02-01 11:47:24pitrousetnosy: + pitrou
messages: + msg98658
2010-02-01 06:08:56ncoghlansetnosy: + ncoghlan
2009-10-04 06:48:18cvrebertsetfiles: - subprocess.rst.patch
2009-10-04 06:41:46cvrebertsetfiles: + subprocess.rst.patch

messages: + msg93519
2009-09-16 14:54:01benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg92688
2009-09-01 23:31:29cvrebertsetfiles: + subprocess.rst.patch
versions: + Python 3.1, Python 3.2
2009-08-22 11:04:55cvrebertsettype: enhancement
2009-08-22 10:40:31cvrebertcreate