Code review comment for lp:~salgado/linaro-image-tools/cmd-runner

On Tue, 2010-11-30 at 20:52 +0000, Peter Maydell wrote:
> 12 + :param shell: Should the given command be run in a shell?
>
> I don't think we should have this, we should never be running things via the shell. subprocess.Popen() should give you everything you need.
>
> 15 + if isinstance(command, (list, tuple)):
> 16 + command = " ".join(command)
>
> This is going to do the wrong thing if Shell=false -- in that case you must pass Popen a list of program and arguments, you can't pass it a string. And as James says there are quoting issues.
>

I'm happy to drop the shell argument. I don't know why I added it in
the first place as none of the existing callsites for subprocess.Popen()
use it anyway.

> 24 + # XXX: Should we raise an error when the return code is not 0, so that it
> 25 + # behaves like the original shell script which was run with 'set -e'?
> 26 + return do_run(command, shell=shell)
>
> "set -e" is better than "blithely ignore errors and continue" which is the only other behaviour that's straightforward to obtain in shell scripts. However it means that the error reporting is very poor, and it tends to fail silently or incomprehensibly rather than with some useful error. One of the advantages of moving to python is that we can do better than this. So run() should report failures in whatever the sensible pythonic fashion is, and all its callers should handle them. The obvious choice is just to follow the same semantics as subprocess.Popen(), ie return codes are returned but things like "couldn't find that command binary" throw exceptions.
>

Right, but my point here is that callsites may forget to check the
return-value, silently ignoring errors in sub-processes. If we agree
it's fair to assume that any sub-process that returns non-zero should
cause the script to abort, then in that case raising an exception on
non-zero return codes is the best way to ensure it always happens.

Callsites can (and should) catch these exceptions and report meaningful
errors but I don't like the idea of relying solely on callsites for
that.

> Are we going to want to do more advanced things than just "run this command", like "run this command and capture the output", or "run command A and pipe the output to B"? If so then perhaps we should just have a wrapper around Popen() which provides exactly the same semantics with extra logging.

That's possible, but I'd prefer if we add these new bits as they become
necessary.

>
> 29 +def do_run(command, shell):
> 30 + proc = subprocess.Popen(command, shell=shell)
> 31 + proc.wait()
> 32 + return proc.returncode
>
> Am I missing something, or is this function just equivalent to subprocess.call() ?

Looks like it is; I'll use it here instead of the above.