Always use a sequence in args to subprocess.Popen() #52
Conversation
According to docs, it is recommended to pass args as a sequence.
|
Without this fix I'm experiencing the following issue: In [1]: import delegator
In [2]: delegator.run(['echo', 'hello']).out
Out[2]: '\n'
In [3]: According to docs (see args section):
|
|
ping @kennethreitz, any change to get this reviewed/in? thanks! |
There was a problem hiding this comment.
The reason your code example above doesn't work is the shell argument being set to True in _default_popen_kwargs. Further below in the docs:
The shell argument (which defaults to False) specifies whether to use the shell as the program to execute. If shell is True, it is recommended to pass args as a string rather than as a sequence.
This is because
If args is a sequence, the first item specifies the command string, and any additional items will be treated as additional arguments to the shell itself.
This translates to the following code in your example, and a bare echo is run which correctly returns '\n'.
Popen(['/bin/sh', '-c', 'echo', 'hello', ...])| @@ -182,7 +182,11 @@ def run(self, block=True, binary=False, cwd=None, env=None): | |||
| popen_kwargs['cwd'] = cwd | |||
| if env: | |||
| popen_kwargs['env'].update(env) | |||
| s = subprocess.Popen(self._popen_args, **popen_kwargs) | |||
| args = self._popen_args | |||
| if not isinstance(args, str): | |||
There was a problem hiding this comment.
Need to account for unicode string type in PY2.
There was a problem hiding this comment.
If python2 is still relevant, are you fine with introducing six as a new dependency and use six.string_types?
| args = self._popen_args | ||
| if not isinstance(args, str): | ||
| # It is recommended to pass a sequence based on docs. | ||
| args = subprocess.list2cmdline(args) |
There was a problem hiding this comment.
subprocess.list2cmdline is used mostly (only?) on Windows, so using it here might have unexpected results on *nix systems.
There was a problem hiding this comment.
An alternative could be to use shlex here, but required routines are only Python3.3+?
There was a problem hiding this comment.
Maybe it's better to set shell kwargs depending on what the given args are.
Using shlex and/or subprocess.list2cmdline doesn't work in all cases, I guess.
More over shell=True should only be used if the cmd line can't be represented with a sequence.
| s = subprocess.Popen(self._popen_args, **popen_kwargs) | ||
| args = self._popen_args | ||
| if not isinstance(args, str): | ||
| # It is recommended to pass a sequence based on docs. |
There was a problem hiding this comment.
This comment, although correct, is confusing in this context since we're converting from sequence to string on the next line.
|
@fridex do you mind resolving the merge conflicts to get a step further to know if this can/should be merged? |
|
@timofurrer done, thanks for refresh. |
|
Awesome, @fridex - thanks. |
That's is true - but this is based on the internal behavior relying on how delegator.py is implemented. Following my example: In [1]: import delegator
In [2]: delegator.run(['echo', 'hello']).out
Out[2]: '\n'I would expect "hello" to be printed relying solely on delegator's API (as a consumer of delegator library). |
| args = self._popen_args | ||
| if not isinstance(args, str): | ||
| # It is recommended to pass a sequence based on docs. | ||
| args = subprocess.list2cmdline(args) |
There was a problem hiding this comment.
Maybe it's better to set shell kwargs depending on what the given args are.
Using shlex and/or subprocess.list2cmdline doesn't work in all cases, I guess.
More over shell=True should only be used if the cmd line can't be represented with a sequence.
According to docs, it is recommended to pass args as a sequence.