Merge lp:~salgado/linaro-image-tools/improve-cmd_runner into lp:linaro-image-tools/11.11

[Guilherme Salgado]

Create a subprocess.Popen subclass which raises an error when the process
exits with a non-zero return code. This is useful in cases when cmd_runner
can't be used but we still want to raise an error on non-zero exit.

[James Westby (james-w)]

Looks good thanks.

This is assuming that other things than wait() aren't used (e.g. communicate()),
but I think that's ok.

Plus communicate() would be trickier to do like this anyway.

Thanks,

James

review: Approve

[Guilherme Salgado (salgado)]

The nice thing is that it works with communicate() just fine as communicate() wait()s:

>>> p = Popen('false', stdout=PIPE, stdin=PIPE, stderr=PIPE)
>>> p.communicate('')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.6/subprocess.py", line 691, in communicate
    return self._communicate(input)
  File "/usr/lib/python2.6/subprocess.py", line 1258, in _communicate
    self.wait()
  File "media_create/cmd_runner.py", line 38, in wait
    raise SubcommandNonZeroReturnValue(self._my_args, returncode)
media_create.cmd_runner.SubcommandNonZeroReturnValue: Sub process "false" returned a non-zero value: 1

[James Westby (james-w)]

On Thu, 09 Dec 2010 19:39:52 -0000, Guilherme Salgado <email address hidden> wrote:
> The nice thing is that it works with communicate() just fine as communicate() wait()s:
>
> >>> p = Popen('false', stdout=PIPE, stdin=PIPE, stderr=PIPE)
> >>> p.communicate('')
> Traceback (most recent call last):
> File "<stdin>", line 1, in <module>
> File "/usr/lib/python2.6/subprocess.py", line 691, in communicate
> return self._communicate(input)
> File "/usr/lib/python2.6/subprocess.py", line 1258, in _communicate
> self.wait()
> File "media_create/cmd_runner.py", line 38, in wait
> raise SubcommandNonZeroReturnValue(self._my_args, returncode)
> media_create.cmd_runner.SubcommandNonZeroReturnValue: Sub process "false" returned a non-zero value: 1

Ah, ok.

Is it still possible to get the output when using communicate() with
pipes?

I'm not sure we do that anyway right now, but there are cases when you
have to use communicate, with stdout/stderr pipes, and you want to
report the output if the command fails.

Thanks,

James

[Guilherme Salgado (salgado)]

Yes, communicate is meant to be used only with pipes and it always
returns (stdout, stderr), but when the subprocess returns non-zero
communicate() will raise (via wait()) before it returns, so I'll have to
think of a way to make stdout/stderr available in that case. Maybe just
including them in the exception is good enough?

[James Westby (james-w)]

On Thu, 09 Dec 2010 20:56:45 -0000, Guilherme Salgado <email address hidden> wrote:
> Yes, communicate is meant to be used only with pipes and it always
> returns (stdout, stderr), but when the subprocess returns non-zero
> communicate() will raise (via wait()) before it returns, so I'll have to
> think of a way to make stdout/stderr available in that case. Maybe just
> including them in the exception is good enough?

That would work.

It can probably wait until we need it though.

Thanks,

James

=== modified file 'media_create/cmd_runner.py'
--- media_create/cmd_runner.py	2010-12-03 18:55:10 +0000
+++ media_create/cmd_runner.py	2010-12-09 19:21:50 +0000
@@ -16,15 +16,27 @@
     if as_root:
         args = args[:]
         args.insert(0, 'sudo')
-    return_value = do_run(args, stdout=stdout)
-    if return_value != 0:
-        raise SubcommandNonZeroReturnValue(args, return_value)
-    return return_value
-
-
-def do_run(args, stdout=None):
-    """A wrapper around subprocess.call() to make testing easier."""
-    return subprocess.call(args, stdout=stdout)
+    proc = Popen(args, stdout=stdout)
+    proc.wait()
+    return proc.returncode
+
+
+class Popen(subprocess.Popen):
+    """A version of Popen which raises an error on non-zero returncode.
+
+    Once the subprocess completes we check its returncode and raise
+    SubcommandNonZeroReturnValue if it's non-zero.
+    """
+
+    def __init__(self, args, **kwargs):
+        self._my_args = args
+        super(Popen, self).__init__(args, **kwargs)
+
+    def wait(self):
+        returncode = super(Popen, self).wait()
+        if returncode != 0:
+            raise SubcommandNonZeroReturnValue(self._my_args, returncode)
+        return returncode
 
 
 class SubcommandNonZeroReturnValue(Exception):

=== modified file 'media_create/tests/fixtures.py'
--- media_create/tests/fixtures.py	2010-12-07 09:15:09 +0000
+++ media_create/tests/fixtures.py	2010-12-09 19:21:50 +0000
@@ -70,7 +70,17 @@
         return 0
 
 
-class MockDoRunFixture(MockSomethingFixture):
+class MockCmdRunnerPopen(object):
+    def __call__(self, args, **kwargs):
+        self.args = args
+        self.returncode = 0
+        return self
+
+    def wait(self):
+        return self.returncode
+
+
+class MockCmdRunnerPopenFixture(MockSomethingFixture):
     """A test fixture which mocks cmd_runner.do_run with the given mock.
 
     If no mock is given, MockDoRun is used.
@@ -78,8 +88,9 @@
 
     def __init__(self, mock=None):
         if mock is None:
-            mock = MockDoRun()
-        super(MockDoRunFixture, self).__init__(cmd_runner, 'do_run', mock)
+            mock = MockCmdRunnerPopen()
+        super(MockCmdRunnerPopenFixture, self).__init__(
+            cmd_runner, 'Popen', mock)
 
 
 class ChangeCurrentWorkingDirFixture(object):

=== modified file 'media_create/tests/test_media_create.py'
--- media_create/tests/test_media_create.py	2010-12-08 13:09:27 +0000
+++ media_create/tests/test_media_create.py	2010-12-09 19:21:50 +0000
@@ -29,7 +29,7 @@
     ChangeCurrentWorkingDirFixture,
     CreateTempDirFixture,
     CreateTarballFixture,
-    MockDoRunFixture,
+    MockCmdRunnerPopenFixture,
     MockSomethingFixture,
     )
 
@@ -127,14 +127,14 @@
 class TestCmdRunner(TestCaseWithFixtures):
 
     def test_run(self):
-        fixture = MockDoRunFixture()
+        fixture = MockCmdRunnerPopenFixture()
         self.useFixture(fixture)
         return_code = cmd_runner.run(['foo', 'bar', 'baz'])
         self.assertEqual(0, return_code)
         self.assertEqual(['foo', 'bar', 'baz'], fixture.mock.args)
 
     def test_run_as_root(self):
-        fixture = MockDoRunFixture()
+        fixture = MockCmdRunnerPopenFixture()
         self.useFixture(fixture)
         cmd_runner.run(['foo', 'bar'], as_root=True)
         self.assertEqual(['sudo', 'foo', 'bar'], fixture.mock.args)
@@ -151,9 +151,10 @@
     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)
+    def test_Popen(self):
+        proc = cmd_runner.Popen('true')
+        returncode = proc.wait()
+        self.assertEqual(0, returncode)
 
 
 class TestPopulateBoot(TestCaseWithFixtures):
@@ -163,14 +164,14 @@
             populate_boot, '_get_file_matching',
             lambda regex: regex))
 
-    def _mock_do_run(self):
-        fixture = MockDoRunFixture()
+    def _mock_Popen(self):
+        fixture = MockCmdRunnerPopenFixture()
         self.useFixture(fixture)
         return fixture
 
     def test_make_uImage(self):
         self._mock_get_file_matching()
-        fixture = self._mock_do_run()
+        fixture = self._mock_Popen()
         make_uImage('load_addr', 'parts_dir', 'sub_arch', 'boot_disk')
         expected = [
             'sudo', 'mkimage', '-A', 'arm', '-O', 'linux', '-T', 'kernel',
@@ -180,7 +181,7 @@
 
     def test_make_uInitrd(self):
         self._mock_get_file_matching()
-        fixture = self._mock_do_run()
+        fixture = self._mock_Popen()
         make_uInitrd('parts_dir', 'sub_arch', 'boot_disk')
         expected = [
             'sudo', 'mkimage', '-A', 'arm', '-O', 'linux', '-T', 'ramdisk',
@@ -190,7 +191,7 @@
 
     def test_make_boot_script(self):
         self._mock_get_file_matching()
-        fixture = self._mock_do_run()
+        fixture = self._mock_Popen()
         make_boot_script('boot_script', 'tmp_dir')
         expected = [
             'sudo', 'mkimage', '-A', 'arm', '-O', 'linux', '-T', 'script',