Merge lp:~salgado/linaro-image-tools/cmd-runner into lp:linaro-image-tools/11.11

[Guilherme Salgado]

In several places we will need to use subprocess.Popen to run external
commands, but we don't really want to run these commands when testing, so this
new function will make it easy for us to test these things.

[lp:~salgado/linaro-image-tools/cmd-runner]

176. By Guilherme Salgado on 2010-11-30

Add an XXX for a question to the reviewer

[James Westby (james-w)]

Hi,

15 + if isinstance(command, (list, tuple)):
16 + command = " ".join(command)

Do we want to do this and risk bugs with shell quoting?

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

I think so.

Do we need shell=True? I assume we do, but using it as little as possible
would be good.

Thanks,

James

[Peter Maydell (pmaydell)]

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.

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.

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.

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() ?

[Peter Maydell (pmaydell)]

> with extra logging

...er, or stubbing out for running in test mode.

[James Westby (james-w)]

On Tue, 30 Nov 2010 20:27:28 -0000, James Westby <email address hidden> wrote:
> Do we need shell=True? I assume we do, but using it as little as possible
> would be good.

Sorry, seems I wasn't really paying attention to that part of the code.

Thanks,

James

[Paul Larson (pwlars)]

Do you think this might ever be used to run something that takes a while to complete? If so, you are not going to see any of the output until the whole command completes. Unfortunately, the only solutions I've seen to that are a little ugly and choke with sudo unless you handle it specifically.

[James Westby (james-w)]

On Wed, 01 Dec 2010 05:31:55 -0000, Paul Larson <email address hidden> wrote:
> Do you think this might ever be used to run something that takes a
> while to complete? If so, you are not going to see any of the output
> until the whole command completes.

Really? That's not my experience. Am I missing something in this code
that will cause that?

Thanks,

James

[Guilherme Salgado (salgado)]

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.

[James Westby (james-w)]

On Wed, 01 Dec 2010 17:43:52 -0000, Guilherme Salgado <email address hidden> wrote:
> 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.

+1

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

+1

Thanks,

James

[lp:~salgado/linaro-image-tools/cmd-runner]

177. By Guilherme Salgado on 2010-12-01

Refactor a bunch of things taking into account Peter's and James' comments

[Guilherme Salgado (salgado)]

This looks much better now. Do you guys think it's good enough to land?

[James Westby (james-w)]

Yes, I do.

I'm not sure there is much point in returning the return code now, but
it's not a problem to do so.

Thanks,

James

review: Approve

=== added file 'media_create/cmd_runner.py'
--- media_create/cmd_runner.py	1970-01-01 00:00:00 +0000
+++ media_create/cmd_runner.py	2010-12-01 19:11:36 +0000
@@ -0,0 +1,37 @@
+import subprocess
+
+
+def run(args, as_root=False):
+    """Run the given command as a sub process.
+
+    :param command: A list or tuple containing the command to run and the
+                    arguments that should be passed to it.
+    :param as_root: Should the given command be run as root (with sudo)?
+    """
+    assert isinstance(args, (list, tuple)), (
+        "The command to run must be a list or tuple, found: %s" % type(args))
+    # TODO: We might want to always use 'sudo -E' here to avoid problems like
+    # https://launchpad.net/bugs/673570
+    if as_root:
+        args = args[:]
+        args.insert(0, 'sudo')
+    return_value = do_run(args)
+    if return_value != 0:
+        raise SubcommandNonZeroReturnValue(args, return_value)
+    return return_value
+
+
+def do_run(args):
+    """A wrapper around subprocess.call() to make testing easier."""
+    return subprocess.call(args)
+
+
+class SubcommandNonZeroReturnValue(Exception):
+
+    def __init__(self, command, return_value):
+        self.command = command
+        self.retval = return_value
+
+    def __str__(self):
+        return 'Sub process "%s" returned a non-zero value: %d' % (
+            self.command, self.retval)

=== modified file 'media_create/tests/test_media_create.py'
--- media_create/tests/test_media_create.py	2010-11-30 12:35:27 +0000
+++ media_create/tests/test_media_create.py	2010-12-01 19:11:36 +0000
@@ -8,6 +8,7 @@
 from hwpack.testing import TestCaseWithFixtures
 
 from media_create.boot_cmd import create_boot_cmd
+from media_create import cmd_runner
 from media_create import ensure_command
 
 from media_create.remove_binary_dir import remove_binary_dir
@@ -75,7 +76,7 @@
         super(TestRemoveBinaryDir, self).setUp()
         self.temp_dir_fixture = CreateTempDirFixture()
         self.useFixture(self.temp_dir_fixture)
-    
+
     def test_remove_binary_dir(self):
         rc = remove_binary_dir(
             binary_dir=self.temp_dir_fixture.get_temp_dir(),
@@ -89,15 +90,63 @@
 
     def setUp(self):
         super(TestUnpackBinaryTarball, self).setUp()
-        
+
         self.temp_dir_fixture = CreateTempDirFixture()
         self.useFixture(self.temp_dir_fixture)
-        
+
         self.tarball_fixture = CreateTarballFixture(
             self.temp_dir_fixture.get_temp_dir())
         self.useFixture(self.tarball_fixture)
-    
+
     def test_unpack_binary_tarball(self):
         rc = unpack_binary_tarball(self.tarball_fixture.get_tarball(),
             as_root=False)
         self.assertEqual(rc, 0)
+
+
+@contextmanager
+def do_run_mocked(mock):
+    orig_func = cmd_runner.do_run
+    cmd_runner.do_run = mock
+    yield
+    cmd_runner.do_run = orig_func
+
+
+class MockDoRun(object):
+    """A mock for do_run() which just stores the args given to it."""
+    args = None
+    def __call__(self, args):
+        self.args = args
+        return 0
+
+
+class TestCmdRunner(TestCase):
+
+    def test_run(self):
+        mock = MockDoRun()
+        with do_run_mocked(mock):
+            return_code = cmd_runner.run(['foo', 'bar', 'baz'])
+        self.assertEqual(0, return_code)
+        self.assertEqual(['foo', 'bar', 'baz'], mock.args)
+
+    def test_run_as_root(self):
+        mock = MockDoRun()
+        with do_run_mocked(mock):
+            cmd_runner.run(['foo', 'bar'], as_root=True)
+        self.assertEqual(['sudo', 'foo', 'bar'], mock.args)
+
+    def test_run_succeeds_on_zero_return_code(self):
+        return_code = cmd_runner.run(['true'])
+        self.assertEqual(0, return_code)
+
+    def test_run_raises_exception_on_non_zero_return_code(self):
+        self.assertRaises(
+            cmd_runner.SubcommandNonZeroReturnValue,
+            cmd_runner.run, ['false'])
+
+    def test_run_must_be_given_list_as_args(self):
+        self.assertRaises(AssertionError, cmd_runner.run, 'true')
+
+    def test_do_run(self):
+        return_code = cmd_runner.do_run('true')
+        self.assertEqual(0, return_code)