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) * |
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) * |
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) * |
Date: 2018-02-04 16:02 |
This made tests failing on Windows. See issue32764.
|
msg311611 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2018-12-04 16:24 |
Ping.
|
msg342778 - (view) |
Author: Cheryl Sabella (cheryl.sabella) * |
Date: 2019-05-18 00:11 |
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!
|
msg343814 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2019-05-28 19:49 |
New changeset 9e3c4526394856d6376eed4968d27d53e1d69b7d by Serhiy Storchaka in branch 'master':
bpo-31961: Fix support of path-like executables in subprocess. (GH-5914)
https://github.com/python/cpython/commit/9e3c4526394856d6376eed4968d27d53e1d69b7d
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:54 | admin | set | github: 76142 |
2019-08-02 22:39:24 | steve.dower | set | status: open -> closed resolution: fixed stage: patch review -> resolved |
2019-05-28 19:49:45 | serhiy.storchaka | set | messages:
+ msg343814 |
2019-05-18 00:11:38 | cheryl.sabella | set | nosy:
+ cheryl.sabella messages:
+ msg342778
|
2018-12-04 16:24:08 | serhiy.storchaka | set | messages:
+ msg331062 |
2018-11-27 07:40:06 | serhiy.storchaka | set | messages:
+ msg330496 |
2018-09-15 21:12:15 | serhiy.storchaka | link | issue34699 superseder |
2018-09-15 21:12:06 | serhiy.storchaka | link | issue33617 superseder |
2018-05-23 14:16:53 | vstinner | unlink | issue33617 superseder |
2018-05-23 14:16:13 | serhiy.storchaka | link | issue33617 superseder |
2018-02-27 23:38:27 | ned.deily | set | priority: deferred blocker ->
messages:
+ msg313038 versions:
- Python 3.7 |
2018-02-27 23:30:36 | ned.deily | set | messages:
+ msg313035 |
2018-02-27 23:04:14 | miss-islington | set | pull_requests:
+ pull_request5703 |
2018-02-27 23:03:49 | ned.deily | set | messages:
+ msg313033 |
2018-02-26 22:42:24 | serhiy.storchaka | set | pull_requests:
+ pull_request5685 |
2018-02-26 22:24:14 | serhiy.storchaka | set | messages:
+ msg312966 |
2018-02-26 21:44:16 | ned.deily | set | messages:
+ msg312962 |
2018-02-26 21:31:43 | gregory.p.smith | set | messages:
+ msg312960 |
2018-02-26 21:27:33 | serhiy.storchaka | set | stage: needs patch -> patch review pull_requests:
+ pull_request5683 |
2018-02-26 21:26:31 | gregory.p.smith | set | messages:
+ msg312958 |
2018-02-25 20:55:06 | ned.deily | set | messages:
+ msg312859 |
2018-02-10 11:12:16 | serhiy.storchaka | set | messages:
+ msg311948 |
2018-02-07 08:21:27 | eryksun | set | nosy:
+ eryksun messages:
+ msg311775
|
2018-02-06 18:16:41 | r.david.murray | set | nosy:
+ r.david.murray messages:
+ msg311737
|
2018-02-06 17:38:31 | serhiy.storchaka | set | messages:
+ msg311734 |
2018-02-06 16:52:23 | gregory.p.smith | set | priority: release blocker -> deferred blocker
messages:
+ msg311733 |
2018-02-06 16:10:54 | Phaqui | set | messages:
+ msg311730 |
2018-02-06 07:56:07 | serhiy.storchaka | set | messages:
+ msg311715 |
2018-02-05 23:40:49 | Phaqui | set | messages:
+ msg311698 |
2018-02-05 23:06:42 | gregory.p.smith | set | priority: normal -> release blocker nosy:
+ ned.deily
versions:
+ Python 3.8 |
2018-02-05 23:06:29 | gregory.p.smith | set | resolution: fixed -> (no value) messages:
+ msg311697 stage: resolved -> needs patch |
2018-02-05 22:33:14 | njs | set | nosy:
+ njs messages:
+ msg311695
|
2018-02-05 19:36:51 | serhiy.storchaka | set | messages:
+ msg311683 |
2018-02-05 16:56:50 | gregory.p.smith | set | messages:
+ msg311675 |
2018-02-05 06:11:45 | serhiy.storchaka | set | messages:
+ msg311645 |
2018-02-05 03:21:21 | gregory.p.smith | set | messages:
+ msg311639 |
2018-02-04 17:45:07 | serhiy.storchaka | set | status: closed -> open
messages:
+ msg311611 |
2018-02-04 16:07:24 | serhiy.storchaka | set | status: open -> closed stage: needs patch -> resolved |
2018-02-04 16:02:14 | serhiy.storchaka | set | status: closed -> open
nosy:
+ serhiy.storchaka messages:
+ msg311604
stage: commit review -> needs patch |
2018-01-30 07:36:58 | gregory.p.smith | set | status: open -> closed type: enhancement messages:
+ msg311250
resolution: fixed stage: patch review -> commit review |
2018-01-30 07:32:27 | gregory.p.smith | set | assignee: gregory.p.smith versions:
- Python 3.6, Python 3.8 |
2018-01-30 07:27:31 | gregory.p.smith | set | nosy:
+ gregory.p.smith messages:
+ msg311247
|
2017-11-08 03:49:14 | Phaqui | set | messages:
+ msg305802 |
2017-11-08 03:45:59 | python-dev | set | keywords:
+ patch stage: patch review pull_requests:
+ pull_request4285 |
2017-11-07 03:52:42 | Phaqui | set | nosy:
+ Phaqui messages:
+ msg305710
|
2017-11-06 18:31:29 | Roy Williams | set | messages:
+ msg305669 |
2017-11-06 17:27:25 | Roy Williams | create | |