Merge "releasetools: Move build_image.RunCommand into common.py." am: b2dd1b6365

am: 6c22649500

Change-Id: I6257243b7bee2ea8fdc45cd3f1841417307e0f73

tools/releasetools/build_image.py

@@ -31,7 +31,6 @@ import os.path
 import re
 import shlex
 import shutil
-import subprocess
 import sys
 
 import common
@@ -52,54 +51,21 @@ class BuildImageError(Exception):
     Exception.__init__(self, message)
 
 
-def RunCommand(cmd, verbose=None, env=None):
-  """Echo and run the given command.
-
-  Args:
-    cmd: the command represented as a list of strings.
-    verbose: show commands being executed.
-    env: a dictionary of additional environment variables.
-  Returns:
-    A tuple of the output and the exit code.
-  """
-  env_copy = None
-  if env is not None:
-    env_copy = os.environ.copy()
-    env_copy.update(env)
-  if verbose is None:
-    verbose = OPTIONS.verbose
-  if verbose:
-    print("Running: " + " ".join(cmd))
-  p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
-                       env=env_copy)
-  output, _ = p.communicate()
-
-  if verbose:
-    print(output.rstrip())
-  return (output, p.returncode)
-
-
 def GetVerityFECSize(partition_size):
   cmd = ["fec", "-s", str(partition_size)]
-  output, exit_code = RunCommand(cmd, False)
+  output = common.RunAndCheckOutput(cmd, verbose=False)
-  if exit_code != 0:
-    raise BuildImageError("Failed to GetVerityFECSize:\n{}".format(output))
   return int(output)
 
 
 def GetVerityTreeSize(partition_size):
   cmd = ["build_verity_tree", "-s", str(partition_size)]
-  output, exit_code = RunCommand(cmd, False)
+  output = common.RunAndCheckOutput(cmd, verbose=False)
-  if exit_code != 0:
-    raise BuildImageError("Failed to GetVerityTreeSize:\n{}".format(output))
   return int(output)
 
 
 def GetVerityMetadataSize(partition_size):
   cmd = ["build_verity_metadata.py", "size", str(partition_size)]
-  output, exit_code = RunCommand(cmd, False)
+  output = common.RunAndCheckOutput(cmd, verbose=False)
-  if exit_code != 0:
-    raise BuildImageError("Failed to GetVerityMetadataSize:\n{}".format(output))
   return int(output)
 
 
@@ -125,10 +91,12 @@ def GetDiskUsage(path):
   Raises:
     BuildImageError: On error.
   """
-  env = {"POSIXLY_CORRECT": "1"}
+  env_copy = os.environ.copy()
+  env_copy["POSIXLY_CORRECT"] = "1"
   cmd = ["du", "-s", path]
-  output, exit_code = RunCommand(cmd, verbose=False, env=env)
+  try:
-  if exit_code != 0:
+    output = common.RunAndCheckOutput(cmd, verbose=False, env=env_copy)
+  except common.ExternalError:
     raise BuildImageError("Failed to get disk usage:\n{}".format(output))
   # POSIX du returns number of blocks with block size 512
   return int(output.split()[0]) * 512
@@ -160,16 +128,13 @@ def AVBCalcMaxImageSize(avbtool, footer_type, partition_size, additional_args):
     The maximum image size.
 
   Raises:
-    BuildImageError: On error or getting invalid image size.
+    BuildImageError: On invalid image size.
   """
   cmd = [avbtool, "add_%s_footer" % footer_type,
          "--partition_size", str(partition_size), "--calc_max_image_size"]
   cmd.extend(shlex.split(additional_args))
 
-  output, exit_code = RunCommand(cmd)
+  output = common.RunAndCheckOutput(cmd)
-  if exit_code != 0:
-    raise BuildImageError(
-        "Failed to calculate max image size:\n{}".format(output))
   image_size = int(output)
   if image_size <= 0:
     raise BuildImageError(
@@ -250,9 +215,6 @@ def AVBAddFooter(image_path, avbtool, footer_type, partition_size,
     salt: The salt to use (a hexadecimal string) or None.
     additional_args: Additional arguments to pass to "avbtool add_hash_footer"
         or "avbtool add_hashtree_footer".
-
-  Raises:
-    BuildImageError: On error.
   """
   cmd = [avbtool, "add_%s_footer" % footer_type,
          "--partition_size", partition_size,
@@ -266,10 +228,7 @@ def AVBAddFooter(image_path, avbtool, footer_type, partition_size,
 
   cmd.extend(shlex.split(additional_args))
 
-  output, exit_code = RunCommand(cmd)
+  common.RunAndCheckOutput(cmd)
-  if exit_code != 0:
-    raise BuildImageError(
-        "Failed to add AVB footer:\n{}".format(output))
 
 
 def AdjustPartitionSizeForVerity(partition_size, fec_supported):
@@ -324,19 +283,13 @@ def BuildVerityFEC(sparse_image_path, verity_path, verity_fec_path,
                    padding_size):
   cmd = ["fec", "-e", "-p", str(padding_size), sparse_image_path,
          verity_path, verity_fec_path]
-  output, exit_code = RunCommand(cmd)
+  common.RunAndCheckOutput(cmd)
-  if exit_code != 0:
-    raise BuildImageError(
-        "Failed to build FEC data:\n{}".format(output))
 
 
 def BuildVerityTree(sparse_image_path, verity_image_path):
   cmd = ["build_verity_tree", "-A", FIXED_SALT, sparse_image_path,
          verity_image_path]
-  output, exit_code = RunCommand(cmd)
+  output = common.RunAndCheckOutput(cmd)
-  if exit_code != 0:
-    raise BuildImageError(
-        "Failed to build verity tree:\n{}".format(output))
   root, salt = output.split()
   return root, salt
 
@@ -350,10 +303,7 @@ def BuildVerityMetadata(image_size, verity_metadata_path, root_hash, salt,
     cmd.append("--signer_args=\"%s\"" % (' '.join(signer_args),))
   if verity_disable:
     cmd.append("--verity_disable")
-  output, exit_code = RunCommand(cmd)
+  common.RunAndCheckOutput(cmd)
-  if exit_code != 0:
-    raise BuildImageError(
-        "Failed to build verity metadata:\n{}".format(output))
 
 
 def Append2Simg(sparse_image_path, unsparse_image_path, error_message):
@@ -367,9 +317,10 @@ def Append2Simg(sparse_image_path, unsparse_image_path, error_message):
     BuildImageError: On error.
   """
   cmd = ["append2simg", sparse_image_path, unsparse_image_path]
-  output, exit_code = RunCommand(cmd)
+  try:
-  if exit_code != 0:
+    common.RunAndCheckOutput(cmd)
-    raise BuildImageError("{}:\n{}".format(error_message, output))
+  except:
+    raise BuildImageError(error_message)
 
 
 def Append(target, file_to_append, error_message):
@@ -413,12 +364,11 @@ def UnsparseImage(sparse_image_path, replace=True):
     else:
       return unsparse_image_path
   inflate_command = ["simg2img", sparse_image_path, unsparse_image_path]
-  inflate_output, exit_code = RunCommand(inflate_command)
+  try:
-  if exit_code != 0:
+    common.RunAndCheckOutput(inflate_command)
+  except:
     os.remove(unsparse_image_path)
-    raise BuildImageError(
+    raise
-        "Error: '{}' failed with exit code {}:\n{}".format(
-            inflate_command, exit_code, inflate_output))
   return unsparse_image_path
 
 
@@ -475,10 +425,7 @@ def MakeVerityEnabledImage(out_file, fec_supported, prop_dict):
 def ConvertBlockMapToBaseFs(block_map_file):
   base_fs_file = common.MakeTempFile(prefix="script_gen_", suffix=".base_fs")
   convert_command = ["blk_alloc_to_base_fs", block_map_file, base_fs_file]
-  output, exit_code = RunCommand(convert_command)
+  common.RunAndCheckOutput(convert_command)
-  if exit_code != 0:
-    raise BuildImageError(
-        "Failed to call blk_alloc_to_base_fs:\n{}".format(output))
   return base_fs_file
 
 
@@ -729,12 +676,15 @@ def BuildImage(in_dir, prop_dict, out_file, target_out=None):
     raise BuildImageError(
         "Error: unknown filesystem type: {}".format(fs_type))
 
-  mkfs_output, exit_code = RunCommand(build_command)
+  try:
-  if exit_code != 0:
+    mkfs_output = common.RunAndCheckOutput(build_command)
+  except:
     try:
       du = GetDiskUsage(in_dir)
       du_str = "{} bytes ({} MB)".format(du, du // BYTES_IN_MB)
-    except BuildImageError as e:
+    # Suppress any errors from GetDiskUsage() to avoid hiding the real errors
+    # from common.RunAndCheckOutput().
+    except Exception as e:  # pylint: disable=broad-except
       print(e, file=sys.stderr)
       du_str = "unknown"
     print(
@@ -750,10 +700,7 @@ def BuildImage(in_dir, prop_dict, out_file, target_out=None):
             int(prop_dict["image_size"]) // BYTES_IN_MB,
             int(prop_dict["partition_size"]),
             int(prop_dict["partition_size"]) // BYTES_IN_MB))
-
+    raise
-    raise BuildImageError(
-        "Error: '{}' failed with exit code {}:\n{}".format(
-            build_command, exit_code, mkfs_output))
 
   # Check if there's enough headroom space available for ext4 image.
   if "partition_headroom" in prop_dict and fs_type.startswith("ext4"):
@@ -792,15 +739,12 @@ def BuildImage(in_dir, prop_dict, out_file, target_out=None):
     # Run e2fsck on the inflated image file
     e2fsck_command = ["e2fsck", "-f", "-n", unsparse_image]
     # TODO(b/112062612): work around e2fsck failure with SANITIZE_HOST=address
-    env4e2fsck = {"ASAN_OPTIONS": "detect_odr_violation=0"}
+    env4e2fsck = os.environ.copy()
-    e2fsck_output, exit_code = RunCommand(e2fsck_command, env=env4e2fsck)
+    env4e2fsck["ASAN_OPTIONS"] = "detect_odr_violation=0"
-
+    try:
-    os.remove(unsparse_image)
+      common.RunAndCheckOutput(e2fsck_command, env=env4e2fsck)
-
+    finally:
-    if exit_code != 0:
+      os.remove(unsparse_image)
-      raise BuildImageError(
-          "Error: '{}' failed with exit code {}:\n{}".format(
-              e2fsck_command, exit_code, e2fsck_output))
 
 
 def ImagePropFromGlobalDict(glob_dict, mount_point):

tools/releasetools/common.py

@@ -37,6 +37,7 @@ from hashlib import sha1, sha256
 import blockimgdiff
 import sparse_img
 
+
 class Options(object):
   def __init__(self):
     platform_search_path = {
@@ -72,16 +73,13 @@ class Options(object):
 
 OPTIONS = Options()
 
-
 # Values for "certificate" in apkcerts that mean special things.
 SPECIAL_CERT_STRINGS = ("PRESIGNED", "EXTERNAL")
 
-
 # The partitions allowed to be signed by AVB (Android verified boot 2.0).
 AVB_PARTITIONS = ('boot', 'recovery', 'system', 'vendor', 'product',
                   'product_services', 'dtbo', 'odm')
 
-
 # Partitions that should have their care_map added to META/care_map.pb
 PARTITIONS_WITH_CARE_MAP = ('system', 'vendor', 'product', 'product_services',
                             'odm')
@@ -144,6 +142,36 @@ def Run(args, verbose=None, **kwargs):
   return subprocess.Popen(args, **kwargs)
 
 
+def RunAndCheckOutput(args, verbose=None, **kwargs):
+  """Runs the given command and returns the output.
+
+  Args:
+    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:
+    The output string.
+
+  Raises:
+    ExternalError: On non-zero exit from the command.
+  """
+  if verbose is None:
+    verbose = OPTIONS.verbose
+  proc = Run(args, verbose=verbose, **kwargs)
+  output, _ = proc.communicate()
+  if verbose:
+    print("{}".format(output.rstrip()))
+  if proc.returncode != 0:
+    raise ExternalError(
+        "Failed to run command '{}' (exit code {}):\n{}".format(
+            args, proc.returncode, output))
+  return output
+
+
 def RoundUpTo4K(value):
   rounded_up = value + 4095
   return rounded_up - (rounded_up % 4096)
@@ -445,20 +473,13 @@ def GetAvbChainedPartitionArg(partition, info_dict, key=None):
   Returns:
     A string of form "partition:rollback_index_location:key" that can be used to
     build or verify vbmeta image.
-
-  Raises:
-    AssertionError: When it fails to extract the public key with avbtool.
   """
   if key is None:
     key = info_dict["avb_" + partition + "_key_path"]
   avbtool = os.getenv('AVBTOOL') or info_dict["avb_avbtool"]
   pubkey_path = MakeTempFile(prefix="avb-", suffix=".pubkey")
-  proc = Run(
+  RunAndCheckOutput(
       [avbtool, "extract_public_key", "--key", key, "--output", pubkey_path])
-  stdoutdata, _ = proc.communicate()
-  assert proc.returncode == 0, \
-      "Failed to extract pubkey for {}:\n{}".format(
-          partition, stdoutdata)
 
   rollback_index_location = info_dict[
       "avb_" + partition + "_rollback_index_location"]
@@ -561,10 +582,7 @@ def _BuildBootableImage(sourcedir, fs_config_file, info_dict=None,
     fn = os.path.join(sourcedir, "recovery_dtbo")
     cmd.extend(["--recovery_dtbo", fn])
 
-  proc = Run(cmd)
+  RunAndCheckOutput(cmd)
-  output, _ = proc.communicate()
-  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")):
@@ -579,10 +597,7 @@ 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])
-    proc = Run(cmd)
+    RunAndCheckOutput(cmd)
-    output, _ = proc.communicate()
-    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"):
@@ -600,10 +615,7 @@ def _BuildBootableImage(sourcedir, fs_config_file, info_dict=None,
            info_dict["vboot_subkey"] + ".vbprivk",
            img_keyblock.name,
            img.name]
-    proc = Run(cmd)
+    RunAndCheckOutput(cmd)
-    proc.communicate()
-    assert proc.returncode == 0, \
-        "Failed to run vboot_signer of {} image:\n{}".format(path, output)
 
     # Clean up the temp files.
     img_unsigned.close()
@@ -620,11 +632,7 @@ 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))
-    proc = Run(cmd)
+    RunAndCheckOutput(cmd)
-    output, _ = proc.communicate()
-    assert proc.returncode == 0, \
-        "Failed to run 'avbtool add_hash_footer' of {}:\n{}".format(
-            partition_name, output)
 
   img.seek(os.SEEK_SET, 0)
   data = img.read()
@@ -696,12 +704,7 @@ def UnzipTemp(filename, pattern=None):
     cmd = ["unzip", "-o", "-q", filename, "-d", dirname]
     if pattern is not None:
       cmd.extend(pattern)
-    proc = Run(cmd)
+    RunAndCheckOutput(cmd)
-    stdoutdata, _ = proc.communicate()
-    if proc.returncode != 0:
-      raise ExternalError(
-          "Failed to unzip input target-files \"{}\":\n{}".format(
-              filename, stdoutdata))
 
   tmp = MakeTempDir(prefix="targetfiles-")
   m = re.match(r"^(.*[.]zip)\+(.*[.]zip)$", filename, re.IGNORECASE)
@@ -1280,7 +1283,7 @@ class PasswordManager(object):
         first_line = i + 4
     f.close()
 
-    Run([self.editor, "+%d" % (first_line,), self.pwfile]).communicate()
+    RunAndCheckOutput([self.editor, "+%d" % (first_line,), self.pwfile])
 
     return self.ReadFile()
 
@@ -1408,10 +1411,7 @@ def ZipDelete(zip_filename, entries):
   if isinstance(entries, basestring):
     entries = [entries]
   cmd = ["zip", "-d", zip_filename] + entries
-  proc = Run(cmd)
+  RunAndCheckOutput(cmd)
-  stdoutdata, _ = proc.communicate()
-  assert proc.returncode == 0, \
-      "Failed to delete {}:\n{}".format(entries, stdoutdata)
 
 
 def ZipClose(zip_file):
@@ -1872,11 +1872,7 @@ class BlockDifference(object):
                     '--output={}.new.dat.br'.format(self.path),
                     '{}.new.dat'.format(self.path)]
       print("Compressing {}.new.dat with brotli".format(self.partition))
-      proc = Run(brotli_cmd)
+      RunAndCheckOutput(brotli_cmd)
-      stdoutdata, _ = proc.communicate()
-      assert proc.returncode == 0, \
-          'Failed to compress {}.new.dat with brotli:\n{}'.format(
-              self.partition, stdoutdata)
 
       new_data_name = '{}.new.dat.br'.format(self.partition)
       ZipWrite(output_zip,

tools/releasetools/test_build_image.py

@@ -23,7 +23,7 @@ import unittest
 import common
 from build_image import (
     AVBCalcMinPartitionSize, BLOCK_SIZE, BuildImageError, CheckHeadroom,
-    RunCommand, SetUpInDirAndFsConfig)
+    SetUpInDirAndFsConfig)
 
 
 class BuildImageTest(unittest.TestCase):
@@ -91,8 +91,9 @@ class BuildImageTest(unittest.TestCase):
     output_image = common.MakeTempFile(suffix='.img')
     command = ['mkuserimg_mke2fs', input_dir, output_image, 'ext4',
                '/system', '409600', '-j', '0']
-    ext4fs_output, exit_code = RunCommand(command)
+    proc = common.Run(command)
-    self.assertEqual(0, exit_code)
+    ext4fs_output, _ = proc.communicate()
+    self.assertEqual(0, proc.returncode)
 
     prop_dict = {
         'fs_type' : 'ext4',

tools/releasetools/test_common.py

@@ -334,8 +334,8 @@ class CommonZipTest(unittest.TestCase):
         self.assertFalse('Test2' in entries)
         self.assertTrue('Test3' in entries)
 
-      self.assertRaises(AssertionError, common.ZipDelete, zip_file.name,
+      self.assertRaises(
-                        'Test2')
+          common.ExternalError, common.ZipDelete, zip_file.name, 'Test2')
       with zipfile.ZipFile(zip_file.name, 'r') as check_zip:
         entries = check_zip.namelist()
         self.assertTrue('Test1' in entries)
@@ -782,7 +782,8 @@ class CommonUtilsTest(unittest.TestCase):
         'avb_system_rollback_index_location': 2,
     }
     self.assertRaises(
-        AssertionError, common.GetAvbChainedPartitionArg, 'system', info_dict)
+        common.ExternalError, common.GetAvbChainedPartitionArg, 'system',
+        info_dict)
 
   INFO_DICT_DEFAULT = {
       'recovery_api_version': 3,