releasetools: Set default stdout and stderr in common.Run().

stdout and stderr will default to subprocess.PIPE and subprocess.STDOUT
respectively (which is the expected behavior from most of the existing
callers), unless caller specifies any of them.

Test: `m dist`
Test: python -m unittest \
          test_common \
          test_add_img_to_target_files \
          test_ota_from_target_files \
          test_validate_target_files
Change-Id: I43b3f08edfa8a9bcfe54baf9848dc705c048e327

tools/releasetools/add_img_to_target_files.py

@@ -49,7 +49,6 @@ import datetime
 import os
 import shlex
 import shutil
-import subprocess
 import sys
 import uuid
 import zipfile
@@ -259,10 +258,11 @@ def AddDtbo(output_zip):
     args = OPTIONS.info_dict.get("avb_dtbo_add_hash_footer_args")
     if args and args.strip():
       cmd.extend(shlex.split(args))
-    p = common.Run(cmd, stdout=subprocess.PIPE)
+    proc = common.Run(cmd)
-    p.communicate()
+    output, _ = proc.communicate()
-    assert p.returncode == 0, \
+    assert proc.returncode == 0, \
-        "avbtool add_hash_footer of %s failed" % (img.name,)
+        "Failed to call 'avbtool add_hash_footer' for {}:\n{}".format(
+            img.name, output)
 
   img.Write()
   return img.name
@@ -451,9 +451,9 @@ def AddVBMeta(output_zip, partitions, name, needed_partitions):
         assert found, 'Failed to find {}'.format(image_path)
     cmd.extend(split_args)
 
-  p = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+  proc = common.Run(cmd)
-  stdoutdata, _ = p.communicate()
+  stdoutdata, _ = proc.communicate()
-  assert p.returncode == 0, \
+  assert proc.returncode == 0, \
       "avbtool make_vbmeta_image failed:\n{}".format(stdoutdata)
   img.Write()
 
@@ -481,9 +481,9 @@ def AddPartitionTable(output_zip):
   if args:
     cmd.extend(shlex.split(args))
 
-  p = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+  proc = common.Run(cmd)
-  stdoutdata, _ = p.communicate()
+  stdoutdata, _ = proc.communicate()
-  assert p.returncode == 0, \
+  assert proc.returncode == 0, \
       "bpttool make_table failed:\n{}".format(stdoutdata)
 
   img.Write()
@@ -600,12 +600,10 @@ def AddCareMapForAbOta(output_zip, ab_partitions, image_paths):
 
   temp_care_map = common.MakeTempFile(prefix="caremap-", suffix=".pb")
   care_map_gen_cmd = ["care_map_generator", temp_care_map_text, temp_care_map]
-  p = common.Run(care_map_gen_cmd, stdout=subprocess.PIPE,
+  proc = common.Run(care_map_gen_cmd)
-                 stderr=subprocess.STDOUT)
+  output, _ = proc.communicate()
-  output, _ = p.communicate()
+  assert proc.returncode == 0, \
-  if OPTIONS.verbose:
+      "Failed to generate the care_map proto message:\n{}".format(output)
-    print(output.rstrip())
-  assert p.returncode == 0, "Failed to generate the care_map proto message."
 
   care_map_path = "META/care_map.pb"
   if output_zip and care_map_path not in output_zip.namelist():
@@ -656,9 +654,9 @@ def AddSuperEmpty(output_zip):
   cmd += shlex.split(OPTIONS.info_dict.get('lpmake_args').strip())
   cmd += ['--output', img.name]
 
-  p = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+  proc = common.Run(cmd)
-  stdoutdata, _ = p.communicate()
+  stdoutdata, _ = proc.communicate()
-  assert p.returncode == 0, \
+  assert proc.returncode == 0, \
       "lpmake tool failed:\n{}".format(stdoutdata)
 
   img.Write()

tools/releasetools/blockimgdiff.py

@@ -23,7 +23,6 @@ import multiprocessing
 import os
 import os.path
 import re
-import subprocess
 import sys
 import threading
 from collections import deque, OrderedDict
@@ -43,11 +42,10 @@ def compute_patch(srcfile, tgtfile, imgdiff=False):
 
   # Don't dump the bsdiff/imgdiff commands, which are not useful for the case
   # here, since they contain temp filenames only.
-  p = common.Run(cmd, verbose=False, stdout=subprocess.PIPE,
+  proc = common.Run(cmd, verbose=False)
-                 stderr=subprocess.STDOUT)
+  output, _ = proc.communicate()
-  output, _ = p.communicate()
 
-  if p.returncode != 0:
+  if proc.returncode != 0:
     raise ValueError(output)
 
   with open(patchfile, 'rb') as f:
@@ -1494,9 +1492,9 @@ class BlockImageDiff(object):
                "--block-limit={}".format(max_blocks_per_transfer),
                "--split-info=" + patch_info_file,
                src_file, tgt_file, patch_file]
-        p = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+        proc = common.Run(cmd)
-        imgdiff_output, _ = p.communicate()
+        imgdiff_output, _ = proc.communicate()
-        assert p.returncode == 0, \
+        assert proc.returncode == 0, \
             "Failed to create imgdiff patch between {} and {}:\n{}".format(
                 src_name, tgt_name, imgdiff_output)
 

tools/releasetools/check_ota_package_signature.py

@@ -24,7 +24,6 @@ import argparse
 import re
 import subprocess
 import sys
-import tempfile
 import zipfile
 
 from hashlib import sha1
@@ -165,11 +164,11 @@ def VerifyAbOtaPayload(cert, package):
   cmd = ['delta_generator',
          '--in_file=' + payload_file,
          '--public_key=' + pubkey]
-  proc = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+  proc = common.Run(cmd)
   stdoutdata, _ = proc.communicate()
   assert proc.returncode == 0, \
-      'Failed to verify payload with delta_generator: %s\n%s' % (package,
+      'Failed to verify payload with delta_generator: {}\n{}'.format(
-                                                                 stdoutdata)
+          package, stdoutdata)
   common.ZipClose(package_zip)
 
   # Verified successfully upon reaching here.

tools/releasetools/common.py

@@ -121,15 +121,26 @@ class ExternalError(RuntimeError):
 
 
 def Run(args, verbose=None, **kwargs):
-  """Create and return a subprocess.Popen object.
+  """Creates and returns a subprocess.Popen object.
 
-  Caller can specify if the command line should be printed. The global
+  Args:
-  OPTIONS.verbose will be used if not specified.
+    args: The command represented as a list of strings.
+    verbose: Whether the commands should be shown (default to OPTIONS.verbose
+        if unspecified).
+    kwargs: Any additional args to be passed to subprocess.Popen(), such as env,
+        stdin, etc. stdout and stderr will default to subprocess.PIPE and
+        subprocess.STDOUT respectively unless caller specifies any of them.
+
+  Returns:
+    A subprocess.Popen object.
   """
   if verbose is None:
     verbose = OPTIONS.verbose
+  if 'stdout' not in kwargs and 'stderr' not in kwargs:
+    kwargs['stdout'] = subprocess.PIPE
+    kwargs['stderr'] = subprocess.STDOUT
   if verbose:
-    print("  running: ", " ".join(args))
+    print("  Running: \"{}\"".format(" ".join(args)))
   return subprocess.Popen(args, **kwargs)
 
 
@@ -443,8 +454,7 @@ def GetAvbChainedPartitionArg(partition, info_dict, key=None):
   avbtool = os.getenv('AVBTOOL') or info_dict["avb_avbtool"]
   pubkey_path = MakeTempFile(prefix="avb-", suffix=".pubkey")
   proc = Run(
-      [avbtool, "extract_public_key", "--key", key, "--output", pubkey_path],
+      [avbtool, "extract_public_key", "--key", key, "--output", pubkey_path])
-      stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
   stdoutdata, _ = proc.communicate()
   assert proc.returncode == 0, \
       "Failed to extract pubkey for {}:\n{}".format(
@@ -551,9 +561,10 @@ def _BuildBootableImage(sourcedir, fs_config_file, info_dict=None,
     fn = os.path.join(sourcedir, "recovery_dtbo")
     cmd.extend(["--recovery_dtbo", fn])
 
-  p = Run(cmd, stdout=subprocess.PIPE)
+  proc = Run(cmd)
-  p.communicate()
+  output, _ = proc.communicate()
-  assert p.returncode == 0, "mkbootimg of %s image failed" % (partition_name,)
+  assert proc.returncode == 0, \
+      "Failed to run mkbootimg of {}:\n{}".format(partition_name, output)
 
   if (info_dict.get("boot_signer") == "true" and
       info_dict.get("verity_key")):
@@ -568,9 +579,10 @@ def _BuildBootableImage(sourcedir, fs_config_file, info_dict=None,
     cmd.extend([path, img.name,
                 info_dict["verity_key"] + ".pk8",
                 info_dict["verity_key"] + ".x509.pem", img.name])
-    p = Run(cmd, stdout=subprocess.PIPE)
+    proc = Run(cmd)
-    p.communicate()
+    output, _ = proc.communicate()
-    assert p.returncode == 0, "boot_signer of %s image failed" % path
+    assert proc.returncode == 0, \
+        "Failed to run boot_signer of {} image:\n{}".format(path, output)
 
   # Sign the image if vboot is non-empty.
   elif info_dict.get("vboot"):
@@ -588,9 +600,10 @@ def _BuildBootableImage(sourcedir, fs_config_file, info_dict=None,
            info_dict["vboot_subkey"] + ".vbprivk",
            img_keyblock.name,
            img.name]
-    p = Run(cmd, stdout=subprocess.PIPE)
+    proc = Run(cmd)
-    p.communicate()
+    proc.communicate()
-    assert p.returncode == 0, "vboot_signer of %s image failed" % path
+    assert proc.returncode == 0, \
+        "Failed to run vboot_signer of {} image:\n{}".format(path, output)
 
     # Clean up the temp files.
     img_unsigned.close()
@@ -607,10 +620,11 @@ def _BuildBootableImage(sourcedir, fs_config_file, info_dict=None,
     args = info_dict.get("avb_" + partition_name + "_add_hash_footer_args")
     if args and args.strip():
       cmd.extend(shlex.split(args))
-    p = Run(cmd, stdout=subprocess.PIPE)
+    proc = Run(cmd)
-    p.communicate()
+    output, _ = proc.communicate()
-    assert p.returncode == 0, "avbtool add_hash_footer of %s failed" % (
+    assert proc.returncode == 0, \
-        partition_name,)
+        "Failed to run 'avbtool add_hash_footer' of {}:\n{}".format(
+            partition_name, output)
 
   img.seek(os.SEEK_SET, 0)
   data = img.read()
@@ -682,9 +696,9 @@ def UnzipTemp(filename, pattern=None):
     cmd = ["unzip", "-o", "-q", filename, "-d", dirname]
     if pattern is not None:
       cmd.extend(pattern)
-    p = Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+    proc = Run(cmd)
-    stdoutdata, _ = p.communicate()
+    stdoutdata, _ = proc.communicate()
-    if p.returncode != 0:
+    if proc.returncode != 0:
       raise ExternalError(
           "Failed to unzip input target-files \"{}\":\n{}".format(
               filename, stdoutdata))
@@ -926,15 +940,14 @@ def SignFile(input_name, output_name, key, password, min_api_level=None,
               key + OPTIONS.private_key_suffix,
               input_name, output_name])
 
-  p = Run(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE,
+  proc = Run(cmd, stdin=subprocess.PIPE)
-          stderr=subprocess.STDOUT)
   if password is not None:
     password += "\n"
-  stdoutdata, _ = p.communicate(password)
+  stdoutdata, _ = proc.communicate(password)
-  if p.returncode != 0:
+  if proc.returncode != 0:
     raise ExternalError(
         "Failed to run signapk.jar: return code {}:\n{}".format(
-            p.returncode, stdoutdata))
+            proc.returncode, stdoutdata))
 
 
 def CheckSize(data, target, info_dict):
@@ -1267,8 +1280,7 @@ class PasswordManager(object):
         first_line = i + 4
     f.close()
 
-    p = Run([self.editor, "+%d" % (first_line,), self.pwfile])
+    Run([self.editor, "+%d" % (first_line,), self.pwfile]).communicate()
-    _, _ = p.communicate()
 
     return self.ReadFile()
 
@@ -1396,10 +1408,10 @@ def ZipDelete(zip_filename, entries):
   if isinstance(entries, basestring):
     entries = [entries]
   cmd = ["zip", "-d", zip_filename] + entries
-  proc = Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+  proc = Run(cmd)
   stdoutdata, _ = proc.communicate()
-  assert proc.returncode == 0, "Failed to delete %s:\n%s" % (entries,
+  assert proc.returncode == 0, \
-                                                             stdoutdata)
+      "Failed to delete {}:\n{}".format(entries, stdoutdata)
 
 
 def ZipClose(zip_file):
@@ -1860,9 +1872,9 @@ class BlockDifference(object):
                     '--output={}.new.dat.br'.format(self.path),
                     '{}.new.dat'.format(self.path)]
       print("Compressing {}.new.dat with brotli".format(self.partition))
-      p = Run(brotli_cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+      proc = Run(brotli_cmd)
-      stdoutdata, _ = p.communicate()
+      stdoutdata, _ = proc.communicate()
-      assert p.returncode == 0, \
+      assert proc.returncode == 0, \
           'Failed to compress {}.new.dat with brotli:\n{}'.format(
               self.partition, stdoutdata)
 

tools/releasetools/ota_from_target_files.py

@@ -394,8 +394,7 @@ class PayloadSigner(object):
       signing_key = common.MakeTempFile(prefix="key-", suffix=".key")
       cmd.extend(["-out", signing_key])
 
-      get_signing_key = common.Run(cmd, verbose=False, stdout=subprocess.PIPE,
+      get_signing_key = common.Run(cmd, verbose=False)
-                                   stderr=subprocess.STDOUT)
       stdoutdata, _ = get_signing_key.communicate()
       assert get_signing_key.returncode == 0, \
           "Failed to get signing key: {}".format(stdoutdata)
@@ -411,7 +410,7 @@ class PayloadSigner(object):
     """Signs the given input file. Returns the output filename."""
     out_file = common.MakeTempFile(prefix="signed-", suffix=".bin")
     cmd = [self.signer] + self.signer_args + ['-in', in_file, '-out', out_file]
-    signing = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+    signing = common.Run(cmd)
     stdoutdata, _ = signing.communicate()
     assert signing.returncode == 0, \
         "Failed to sign the input file: {}".format(stdoutdata)

tools/releasetools/test_add_img_to_target_files.py

@@ -16,7 +16,6 @@
 
 import os
 import os.path
-import subprocess
 import unittest
 import zipfile
 
@@ -45,9 +44,11 @@ class AddImagesToTargetFilesTest(unittest.TestCase):
 
     # Calls an external binary to convert the proto message.
     cmd = ["care_map_generator", "--parse_proto", file_name, text_file]
-    p = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+    proc = common.Run(cmd)
-    p.communicate()
+    output, _ = proc.communicate()
-    self.assertEqual(0, p.returncode)
+    self.assertEqual(
+        0, proc.returncode,
+        "Failed to run care_map_generator:\n{}".format(output))
 
     with open(text_file, 'r') as verify_fp:
       plain_text = verify_fp.read()

tools/releasetools/test_ota_from_target_files.py

@@ -17,7 +17,6 @@
 import copy
 import os
 import os.path
-import subprocess
 import unittest
 import zipfile
 
@@ -1024,11 +1023,11 @@ class AbOtaPropertyFilesTest(PropertyFilesTest):
            '--signature_size', str(self.SIGNATURE_SIZE),
            '--metadata_hash_file', metadata_sig_file,
            '--payload_hash_file', payload_sig_file]
-    proc = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+    proc = common.Run(cmd)
     stdoutdata, _ = proc.communicate()
     self.assertEqual(
         0, proc.returncode,
-        'Failed to run brillo_update_payload: {}'.format(stdoutdata))
+        'Failed to run brillo_update_payload:\n{}'.format(stdoutdata))
 
     signed_metadata_sig_file = payload_signer.Sign(metadata_sig_file)
 

tools/releasetools/test_validate_target_files.py

@@ -21,7 +21,6 @@ from __future__ import print_function
 import os
 import os.path
 import shutil
-import subprocess
 import unittest
 
 import build_image
@@ -44,7 +43,7 @@ class ValidateTargetFilesTest(unittest.TestCase):
       kernel_fp.write(os.urandom(10))
 
     cmd = ['mkbootimg', '--kernel', kernel, '-o', output_file]
-    proc = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+    proc = common.Run(cmd)
     stdoutdata, _ = proc.communicate()
     self.assertEqual(
         0, proc.returncode,
@@ -53,7 +52,7 @@ class ValidateTargetFilesTest(unittest.TestCase):
     cmd = ['boot_signer', '/boot', output_file,
            os.path.join(self.testdata_dir, 'testkey.pk8'),
            os.path.join(self.testdata_dir, 'testkey.x509.pem'), output_file]
-    proc = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+    proc = common.Run(cmd)
     stdoutdata, _ = proc.communicate()
     self.assertEqual(
         0, proc.returncode,
@@ -123,7 +122,7 @@ class ValidateTargetFilesTest(unittest.TestCase):
     system_root = common.MakeTempDir()
     cmd = ['mkuserimg_mke2fs', '-s', system_root, output_file, 'ext4',
            '/system', str(image_size), '-j', '0']
-    proc = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+    proc = common.Run(cmd)
     stdoutdata, _ = proc.communicate()
     self.assertEqual(
         0, proc.returncode,

tools/releasetools/validate_target_files.py

@@ -35,7 +35,6 @@ import filecmp
 import logging
 import os.path
 import re
-import subprocess
 import zipfile
 
 import common
@@ -256,7 +255,7 @@ def ValidateVerifiedBootImages(input_tmp, info_dict, options):
         continue
 
       cmd = ['boot_signer', '-verify', image_path, '-certificate', verity_key]
-      proc = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+      proc = common.Run(cmd)
       stdoutdata, _ = proc.communicate()
       assert proc.returncode == 0, \
           'Failed to verify {} with boot_signer:\n{}'.format(image, stdoutdata)
@@ -299,7 +298,7 @@ def ValidateVerifiedBootImages(input_tmp, info_dict, options):
         continue
 
       cmd = ['verity_verifier', image_path, '-mincrypt', verity_key_mincrypt]
-      proc = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+      proc = common.Run(cmd)
       stdoutdata, _ = proc.communicate()
       assert proc.returncode == 0, \
           'Failed to verify {} with verity_verifier (key: {}):\n{}'.format(
@@ -328,7 +327,7 @@ def ValidateVerifiedBootImages(input_tmp, info_dict, options):
             partition, info_dict, options[key_name])
         cmd.extend(["--expected_chain_partition", chained_partition_arg])
 
-    proc = common.Run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
+    proc = common.Run(cmd)
     stdoutdata, _ = proc.communicate()
     assert proc.returncode == 0, \
         'Failed to verify {} with verity_verifier (key: {}):\n{}'.format(