amitt001 / delegator.py

Subprocesses for Humans 2.0.
MIT License
1.7k stars 92 forks source link

Always use a sequence in args to subprocess.Popen() #52

Open fridex opened 6 years ago

fridex commented 6 years ago

According to docs, it is recommended to pass args as a sequence.

fridex commented 6 years ago

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

Unless otherwise stated, it is recommended to pass args as a sequence. On POSIX, if args is a string, the string is interpreted as the name or path of the program to execute. However, this can only be done if not passing arguments to the program.

fridex commented 6 years ago

ping @kennethreitz, any change to get this reviewed/in? thanks!

timofurrer commented 5 years ago

@fridex do you mind resolving the merge conflicts to get a step further to know if this can/should be merged?

fridex commented 5 years ago

@timofurrer done, thanks for refresh.

timofurrer commented 5 years ago

Awesome, @fridex - thanks. Can you have a look at the feedback from @akshaykumar90.

fridex commented 5 years ago

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', ...])

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