classification
Title: subprocess._execute_child doesn't accept a single PathLike argument for args
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: Phaqui, Roy Williams, eryksun, gregory.p.smith, ned.deily, njs, r.david.murray, serhiy.storchaka
Priority: Keywords: patch

Created on 2017-11-06 17:27 by Roy Williams, last changed 2018-12-04 16:24 by serhiy.storchaka.

Pull Requests
URL Status Linked Edit
PR 4329 merged python-dev, 2017-11-08 03:45
PR 5912 merged serhiy.storchaka, 2018-02-26 21:27
PR 5914 open serhiy.storchaka, 2018-02-26 22:42
PR 5931 merged miss-islington, 2018-02-27 23:04
Messages (32)
msg305663 - (view) Author: Roy Williams (Roy Williams) * Date: 2017-11-06 17:27
Repro:

```python
from pathlib import Path
import subprocess

subprocess.run([Path('/bin/ls')])  # Works Fine
subprocess.run(Path('/bin/ls'))  # Fails
```

The problem seems to originate from here:
https://github.com/python/cpython/blob/master/Lib/subprocess.py#L1237-L1240

This file auspiciously avoids importing pathlib, which I'm guessing is somewhat intentional?  Would the correct fix be to check for the existence of a `__fspath__` attribute as per https://www.python.org/dev/peps/pep-0519/ ?
msg305669 - (view) Author: Roy Williams (Roy Williams) * Date: 2017-11-06 18:31
Ignore my comment re: pathlib, it looks like PathLike is defined in `os` and not `pathlib`.
msg305710 - (view) Author: Anders Lorentsen (Phaqui) * Date: 2017-11-07 03:52
I was able to make a test that reproduces your code, and expectedly fails. Also implemented a fix for it. See a temporary diff here: https://pastebin.com/C9JWkg0i

However, there is also a specific MS Windows version of _execute_child() (a phrase only computer-folks can say out loud without feeling very...wrong...). It uses a different method to extract and parse arguments, and it should be corrected and tested under windows, before I submit a PR for this.

Also, my test messes up the formatting. Instead of saying "....ok", they both say "... total 0\nok". I have no idea why.
msg305802 - (view) Author: Anders Lorentsen (Phaqui) * Date: 2017-11-08 03:49
While researching this, I discovered that on MS Windows

>>> subprocess.run([pathlike_object, additional_arguments])

did not run like it did on Posix. My PR includes this problem and it's fix.
msg311247 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-01-30 07:27
New changeset dd42cb71f2cb02f3a32f016137b12a146bc0d0e2 by Gregory P. Smith (Anders Lorentsen) in branch 'master':
bpo-31961: subprocess now accepts path-like args (GH-4329)
https://github.com/python/cpython/commit/dd42cb71f2cb02f3a32f016137b12a146bc0d0e2
msg311250 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-01-30 07:36
Thanks for the PR!  I accepted it as this makes sense as a feature.

<meme> pathlib all the things! </meme>

I think the code be polished up a bit to not rely on catching TypeError but this gets the feature in before the 3.7 feature freeze (just).  If further changes are needed or bugs are found, they can be addressed during the 3.7 betas. :)
msg311604 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-04 16:02
This made tests failing on Windows. See issue32764.
msg311611 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-04 17:45
Actually this feature looks wrong to me. The args argument is either a sequence containing a program name and arguments, or a command line string. In the first case supporting path-like objects makes sense, and this was supported. But the command line is not a path-like object. It is a concatenation of quoted program name and arguments. Neither operation for path-like objects makes sense for a command line.

I think this change should be reverted.
msg311639 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-02-05 03:21
Don't revert something just because you found a bug, we can fix it.  fwiw, the PR passed appveyor's Windows run: https://ci.appveyor.com/project/python/cpython/build/3.7build11551

So if there's a bug, we're missing some kind of test coverage.
msg311645 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-05 06:11
The bug itself can be easily fixed. But I think this PR shouldn't be merged at first place. Not all functions should accept path-like objects for arbitrary arguments. Only if the argument semantically is a path, a path-like object should be accepted. Several similar propositions already were rejected for this reason.
msg311675 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-02-05 16:56
got any pointers to those?  I want to familiarize myself with existing arguments for/against such a feature in subprocess to decide.
msg311683 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-05 19:36
I don't remember all details, here is what I have found.

Issue28230 and issue28231 -- the support of path-like object was added only for external names (paths of archives, paths of added files, paths for extraction), but not for internal names.

Issue29447 -- the initial idea was rejected, and it seems to me that the tempfile module doesn't need any changes. Path-like objects already are accepted for the dir argument, but they shouldn't be accepted for arguments like pre or suf which are not paths.

Issue29448 is only tangentially related. It's rejection is based on issue29447.
msg311695 - (view) Author: Nathaniel Smith (njs) * (Python committer) Date: 2018-02-05 22:33
I think I agree with Serhiy here... the whole difference between run([a]) and run(a) is that in the first case 'a' is treated as a path, and in the second it isn't. This distinction long predates pathlib.

Say I have an oddly named binary with an embedded space in its name:

bin = Path("/bin/ls -l")

# Check that this weird file actually exists:
assert bin.exists()

# runs this weird file
subprocess.run([bin])

# Currently an error; if this is implemented, would run
# /bin/ls, and pass it the -l argument. Refers to something
# completely different than our .exists() call above.
subprocess.run(bin)
msg311697 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-02-05 23:06
I agree with both of you at this point.

TODO: Revert the PR and backport that to the 3.7 branch (i'll take care of it)
msg311698 - (view) Author: Anders Lorentsen (Phaqui) * Date: 2018-02-05 23:40
> # runs this weird file
> subprocess.run([bin])

> # Currently an error; if this is implemented, would run
> # /bin/ls, and pass it the -l argument. Refers to something
> # completely different than our .exists() call above.

I do not understand where it is stated that this is the behavior. My understanding was that if run() is passed a single argument, it will interpret the string "/bin/ls -l" to mean "run the `/bin/ls` program, and pass it one argument `-l`, whereas if instead run() is given a list, it will literally treat the first element as the program to run, regardless of how many spaces or whatever else it contains ... And as such, if `bin` is a Path-like object, this issue tries to resolve that
run([bin]) works as excepted, but run(bin) fails horribly.

Sure, I can understand that for strings it may not feel natural how these two things are interpreted differently, but if `bin` is a Path-like object, it at least feels very natural to me that then it should be run as the program name itself (as "paths" does not have arguments).
msg311715 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-06 07:56
Anders, the problem is that running subprocess.run(executable) instead of subprocess.run([executable]) (where executable is a path to a program) is a bug. For example it doesn't work if the path contains spaces. And PR 4329 encourages making this bug.
msg311730 - (view) Author: Anders Lorentsen (Phaqui) * Date: 2018-02-06 16:10
What do you mean "is a bug", and "the PR would encourage this"? Can't it be fixed?

Are you saying that just because it is a bug now, we should be discouraged from making it work in the way you'd expect it to work?

If `exe` is a path, these two lines of code should do exactly the same, right? It seems unintuitive to me if they produce different results.

run(exe)
run([exe])
msg311733 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-02-06 16:52
Nathaniel's specific description of a problem is wrong.  A Path with embedded spaces is treated no different than one without by default.

Where things change, and what needs fixing, is when shell=True is passed. In that case a PathLike object should not be allowed as it will be turned into a flat string passed to the shell for parsing.

# I've created an executable bash script called "x/mybin -h" for this that just does: echo "hello from mybin $*"

>>> run("x/mybin -h", shell=True)
/bin/sh: 1: x/mybin: not found
CompletedProcess(args='x/mybin -h', returncode=127)
>>> run("x/mybin -h")
hello from mybin 
CompletedProcess(args='x/mybin -h', returncode=0)
>>> run(Path("x/mybin -h"), shell=True)
/bin/sh: 1: x/mybin: not found
CompletedProcess(args=PosixPath('x/mybin -h'), returncode=127)
>>> run([Path("x/mybin -h")], shell=True)
/bin/sh: 1: x/mybin: not found
CompletedProcess(args=[PosixPath('x/mybin -h')], returncode=127)
>>> run(Path("x/mybin -h"))
hello from mybin 
CompletedProcess(args=PosixPath('x/mybin -h'), returncode=0)
>>> run([Path("x/mybin -h")])
hello from mybin
msg311734 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-06 17:38
Oh, my mistake. I thought the case when args is a string and shell=False is deprecated.
msg311737 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2018-02-06 18:16
"I thought the case when args is a string and shell=False is deprecated."

IMO it ought to be :)  See issue 7839 for background.
msg311775 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2018-02-07 08:21
If a PathLike args value is supported in Windows, then it must be processed through list2cmdline, in case it needs to be quoted. Also, the preferred usage is to pass args as a list when shell is false. This common case shouldn't be penalized as a TypeError. Also, passing `executable` as PathLike should be supported, as is already the case in the POSIX implementation. For example.

_execute_child:

    if executable is not None and not isinstance(executable, str):
        executable = os.fsdecode(executable)

    if not isinstance(args, str):
        try:
            args = list2cmdline(args)
        except TypeError:
            if isinstance(args, bytes):
                args = os.fsdecode(args)
            elif isinstance(args, os.PathLike):
                args = list2cmdline([args])
            else:
                raise
        
list2cmdline should support PathLike arguments via os.fsdecode. This removes an existing inconsistency since the POSIX implementation converts args via PyUnicode_FSConverter in Modules/_posixsubprocess.c. For example:

list2cmdline:

    for arg in seq:
        if not isinstance(arg, str):
            arg = os.fsdecode(arg)

Finally, passing args as a string when shell is false can never be deprecated on Windows. list2cmdline works in most cases, but Windows CreateProcess takes a command line, and applications are free to parse the command line however they want. IMHO, the case that should be deprecated is passing args as a list/sequence when shell is true.
msg311948 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-10 11:12
This is a complex issue for doing it right. Related issues -- supporting bytes paths on Windows, supporting path-like executable, deprecating some corner cases. I suggest to revert this change now and try to do it right in 3.8.
msg312859 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-02-25 20:55
What's the status of this issue?  We need to make a decision soon about what to do for 3.7.0.
msg312958 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-02-26 21:26
I'm fine with undoing this for 3.7 in light of the many things we don't do "right" all over the place with path like objects.

subprocess still presents as a mix of high and low level APIs so if we accept a path like object in a subset of places it seems like that'll add complexity to the APIs rather than being consistent.
msg312960 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-02-26 21:31
Ned: when removing a feature added in beta1, should we remove the original NEWS entry created for it?  Or add a new NEWS entry that states that the previous feature has been reverted?
msg312962 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-02-26 21:44
We should either remove the entry in Misc/NEWS/3.7.0b1.rst or, perhaps better, add a line to it noting that it was removed in 3.7.0b2.
msg312966 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-26 22:24
There are two alternate PRs. PR 5912 removes this feature. PR 5914 fixes it.
msg313033 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-02-27 23:03
New changeset be50a7b627d0aa37e08fa8e2d5568891f19903ce by Ned Deily (Serhiy Storchaka) in branch 'master':
Revert "bpo-31961: subprocess now accepts path-like args (GH-4329)" (#5912)
https://github.com/python/cpython/commit/be50a7b627d0aa37e08fa8e2d5568891f19903ce
msg313035 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-02-27 23:30
New changeset b7dcae358e9d5a3ac9dafec9f6b64e9a66843ad8 by Ned Deily (Miss Islington (bot)) in branch '3.7':
Revert "bpo-31961: subprocess now accepts path-like args (GH-4329)" (GH-5912) (GH-5931)
https://github.com/python/cpython/commit/b7dcae358e9d5a3ac9dafec9f6b64e9a66843ad8
msg313038 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-02-27 23:38
Thanks for everyone's input and thanks for the PRs!  Since there are still outstanding review comments, I decided to revert this from both master and 3.7 for 3.7.0b2. I would suggest getting a polished version stabilized in master for 3.8.0.  We could then discuss the possibility of a 3.7 backport.
msg330496 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-11-27 07:40
Gregory, could you please make a look at PR 5914?

Differences from PR 4329:

* Any item of args can be an iterable of bytes and path-like objects on Windows (not just a first item as in PR 4329).
* Accepts bytes and path-like objects as executable on Windows.
* Accepts bytes as cwd on Windows.
* Raises a TypeError when shell is true and args is a path-like object (or bytes on Windows).
* Does not use raising and catching a TypeError when args is an iterable on Windows or a path-like object on POSIX (this makes the code clearer and a tiny bit more efficient).
msg331062 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-12-04 16:24
Ping.
History
Date User Action Args
2018-12-04 16:24:08serhiy.storchakasetmessages: + msg331062
2018-11-27 07:40:06serhiy.storchakasetmessages: + msg330496
2018-09-15 21:12:15serhiy.storchakalinkissue34699 superseder
2018-09-15 21:12:06serhiy.storchakalinkissue33617 superseder
2018-05-23 14:16:53vstinnerunlinkissue33617 superseder
2018-05-23 14:16:13serhiy.storchakalinkissue33617 superseder
2018-02-27 23:38:27ned.deilysetpriority: deferred blocker ->

messages: + msg313038
versions: - Python 3.7
2018-02-27 23:30:36ned.deilysetmessages: + msg313035
2018-02-27 23:04:14miss-islingtonsetpull_requests: + pull_request5703
2018-02-27 23:03:49ned.deilysetmessages: + msg313033
2018-02-26 22:42:24serhiy.storchakasetpull_requests: + pull_request5685
2018-02-26 22:24:14serhiy.storchakasetmessages: + msg312966
2018-02-26 21:44:16ned.deilysetmessages: + msg312962
2018-02-26 21:31:43gregory.p.smithsetmessages: + msg312960
2018-02-26 21:27:33serhiy.storchakasetstage: needs patch -> patch review
pull_requests: + pull_request5683
2018-02-26 21:26:31gregory.p.smithsetmessages: + msg312958
2018-02-25 20:55:06ned.deilysetmessages: + msg312859
2018-02-10 11:12:16serhiy.storchakasetmessages: + msg311948
2018-02-07 08:21:27eryksunsetnosy: + eryksun
messages: + msg311775
2018-02-06 18:16:41r.david.murraysetnosy: + r.david.murray
messages: + msg311737
2018-02-06 17:38:31serhiy.storchakasetmessages: + msg311734
2018-02-06 16:52:23gregory.p.smithsetpriority: release blocker -> deferred blocker

messages: + msg311733
2018-02-06 16:10:54Phaquisetmessages: + msg311730
2018-02-06 07:56:07serhiy.storchakasetmessages: + msg311715
2018-02-05 23:40:49Phaquisetmessages: + msg311698
2018-02-05 23:06:42gregory.p.smithsetpriority: normal -> release blocker
nosy: + ned.deily

versions: + Python 3.8
2018-02-05 23:06:29gregory.p.smithsetresolution: fixed ->
messages: + msg311697
stage: resolved -> needs patch
2018-02-05 22:33:14njssetnosy: + njs
messages: + msg311695
2018-02-05 19:36:51serhiy.storchakasetmessages: + msg311683
2018-02-05 16:56:50gregory.p.smithsetmessages: + msg311675
2018-02-05 06:11:45serhiy.storchakasetmessages: + msg311645
2018-02-05 03:21:21gregory.p.smithsetmessages: + msg311639
2018-02-04 17:45:07serhiy.storchakasetstatus: closed -> open

messages: + msg311611
2018-02-04 16:07:24serhiy.storchakasetstatus: open -> closed
stage: needs patch -> resolved
2018-02-04 16:02:14serhiy.storchakasetstatus: closed -> open

nosy: + serhiy.storchaka
messages: + msg311604

stage: commit review -> needs patch
2018-01-30 07:36:58gregory.p.smithsetstatus: open -> closed
type: enhancement
messages: + msg311250

resolution: fixed
stage: patch review -> commit review
2018-01-30 07:32:27gregory.p.smithsetassignee: gregory.p.smith
versions: - Python 3.6, Python 3.8
2018-01-30 07:27:31gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg311247
2017-11-08 03:49:14Phaquisetmessages: + msg305802
2017-11-08 03:45:59python-devsetkeywords: + patch
stage: patch review
pull_requests: + pull_request4285
2017-11-07 03:52:42Phaquisetnosy: + Phaqui
messages: + msg305710
2017-11-06 18:31:29Roy Williamssetmessages: + msg305669
2017-11-06 17:27:25Roy Williamscreate