subprocess._execute_child doesn't accept a single PathLike argument for args

[rowillia]

BPO	31961
Nosy	@gpshead, @ned-deily, @bitdancer, @njsmith, @serhiy-storchaka, @eryksun, @Phaqui, @rowillia, @csabella
PRs	bpo-31961: subprocess.run() now accepts path-like args #4329 Revert "bpo-31961: subprocess now accepts path-like args (GH-4329)" #5912 bpo-31961: Fix support of path-like executables in subprocess. #5914 [3.7] Revert "bpo-31961: subprocess now accepts path-like args" (GH-5912) #5931

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/gpshead'
closed_at = <Date 2019-08-02.22:39:24.649>
created_at = <Date 2017-11-06.17:27:25.843>
labels = ['3.8', 'type-feature', 'library']
title = "subprocess._execute_child doesn't accept a single PathLike argument for args"
updated_at = <Date 2019-08-02.22:39:24.649>
user = 'https://github.com/rowillia'

bugs.python.org fields:

activity = <Date 2019-08-02.22:39:24.649>
actor = 'steve.dower'
assignee = 'gregory.p.smith'
closed = True
closed_date = <Date 2019-08-02.22:39:24.649>
closer = 'steve.dower'
components = ['Library (Lib)']
creation = <Date 2017-11-06.17:27:25.843>
creator = 'Roy Williams'
dependencies = []
files = []
hgrepos = []
issue_num = 31961
keywords = ['patch']
message_count = 34.0
messages = ['305663', '305669', '305710', '305802', '311247', '311250', '311604', '311611', '311639', '311645', '311675', '311683', '311695', '311697', '311698', '311715', '311730', '311733', '311734', '311737', '311775', '311948', '312859', '312958', '312960', '312962', '312966', '313033', '313035', '313038', '330496', '331062', '342778', '343814']
nosy_count = 9.0
nosy_names = ['gregory.p.smith', 'ned.deily', 'r.david.murray', 'njs', 'serhiy.storchaka', 'eryksun', 'Phaqui', 'Roy Williams', 'cheryl.sabella']
pr_nums = ['4329', '5912', '5914', '5931']
priority = None
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue31961'
versions = ['Python 3.8']

[rowillia]

Repro:

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/ ?

[rowillia]

Ignore my comment re: pathlib, it looks like PathLike is defined in os and not pathlib.

[phaqui]

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.

[phaqui]

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.

[gpshead]

New changeset dd42cb7 by Gregory P. Smith (Anders Lorentsen) in branch 'master':
bpo-31961: subprocess now accepts path-like args (GH-4329)
dd42cb7

[gpshead]

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. :)

[serhiy-storchaka]

This made tests failing on Windows. See bpo-32764.

[serhiy-storchaka]

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.

[gpshead]

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.

[serhiy-storchaka]

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.

[gpshead]

got any pointers to those? I want to familiarize myself with existing arguments for/against such a feature in subprocess to decide.

[serhiy-storchaka]

I don't remember all details, here is what I have found.

bpo-28230 and bpo-28231 -- 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.

bpo-29447 -- 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.

bpo-29448 is only tangentially related. It's rejection is based on bpo-29447.

[njsmith]

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)

[gpshead]

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)

[phaqui]

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).

[serhiy-storchaka]

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.

[phaqui]

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])

[gpshead]

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

[serhiy-storchaka]

Oh, my mistake. I thought the case when args is a string and shell=False is deprecated.

[bitdancer]

"I thought the case when args is a string and shell=False is deprecated."

IMO it ought to be :) See bpo-7839 for background.

[eryksun]

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.

[serhiy-storchaka]

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.

[ned-deily]

What's the status of this issue? We need to make a decision soon about what to do for 3.7.0.

[gpshead]

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.

[gpshead]

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?

[ned-deily]

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.

[serhiy-storchaka]

There are two alternate PRs. PR 5912 removes this feature. PR 5914 fixes it.

[ned-deily]

New changeset be50a7b by Ned Deily (Serhiy Storchaka) in branch 'master':
Revert "bpo-31961: subprocess now accepts path-like args (GH-4329)" (bpo-5912)
be50a7b

[ned-deily]

New changeset b7dcae3 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)
b7dcae3

[ned-deily]

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.

[serhiy-storchaka]

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).

[serhiy-storchaka]

Ping.

[csabella]

gregory.p.smith, I believe Serhiy was looking for a review on PR5914. Maybe it's not too late for this to get into 3.8? Thanks!

[serhiy-storchaka]

New changeset 9e3c452 by Serhiy Storchaka in branch 'master':
bpo-31961: Fix support of path-like executables in subprocess. (GH-5914)
9e3c452