diff --git a/tools/releasetools/add_img_to_target_files.py b/tools/releasetools/add_img_to_target_files.py index d7d1bc81da..2fa5f5296e 100755 --- a/tools/releasetools/add_img_to_target_files.py +++ b/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) - p.communicate() - assert p.returncode == 0, \ - "avbtool add_hash_footer of %s failed" % (img.name,) + proc = common.Run(cmd) + output, _ = proc.communicate() + assert proc.returncode == 0, \ + "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) - stdoutdata, _ = p.communicate() - assert p.returncode == 0, \ + proc = common.Run(cmd) + stdoutdata, _ = proc.communicate() + 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) - stdoutdata, _ = p.communicate() - assert p.returncode == 0, \ + proc = common.Run(cmd) + stdoutdata, _ = proc.communicate() + 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, - stderr=subprocess.STDOUT) - output, _ = p.communicate() - if OPTIONS.verbose: - print(output.rstrip()) - assert p.returncode == 0, "Failed to generate the care_map proto message." + proc = common.Run(care_map_gen_cmd) + output, _ = proc.communicate() + assert proc.returncode == 0, \ + "Failed to generate the care_map proto message:\n{}".format(output) 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) - stdoutdata, _ = p.communicate() - assert p.returncode == 0, \ + proc = common.Run(cmd) + stdoutdata, _ = proc.communicate() + assert proc.returncode == 0, \ "lpmake tool failed:\n{}".format(stdoutdata) img.Write() diff --git a/tools/releasetools/blockimgdiff.py b/tools/releasetools/blockimgdiff.py index aeb43798a4..189dba28e3 100644 --- a/tools/releasetools/blockimgdiff.py +++ b/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, - stderr=subprocess.STDOUT) - output, _ = p.communicate() + proc = common.Run(cmd, verbose=False) + output, _ = proc.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) - imgdiff_output, _ = p.communicate() - assert p.returncode == 0, \ + proc = common.Run(cmd) + imgdiff_output, _ = proc.communicate() + assert proc.returncode == 0, \ "Failed to create imgdiff patch between {} and {}:\n{}".format( src_name, tgt_name, imgdiff_output) diff --git a/tools/releasetools/check_ota_package_signature.py b/tools/releasetools/check_ota_package_signature.py index 3cac90a014..a5807095d0 100755 --- a/tools/releasetools/check_ota_package_signature.py +++ b/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, - stdoutdata) + 'Failed to verify payload with delta_generator: {}\n{}'.format( + package, stdoutdata) common.ZipClose(package_zip) # Verified successfully upon reaching here. diff --git a/tools/releasetools/common.py b/tools/releasetools/common.py index 4e2346cb98..e38167671b 100644 --- a/tools/releasetools/common.py +++ b/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 - OPTIONS.verbose will be used if not specified. + 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: + 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], - stdout=subprocess.PIPE, stderr=subprocess.STDOUT) + [avbtool, "extract_public_key", "--key", key, "--output", pubkey_path]) 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) - p.communicate() - assert p.returncode == 0, "mkbootimg of %s image failed" % (partition_name,) + proc = Run(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")): @@ -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) - p.communicate() - assert p.returncode == 0, "boot_signer of %s image failed" % path + proc = Run(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"): @@ -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) - p.communicate() - assert p.returncode == 0, "vboot_signer of %s image failed" % path + proc = Run(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() @@ -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) - p.communicate() - assert p.returncode == 0, "avbtool add_hash_footer of %s failed" % ( - partition_name,) + proc = Run(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() @@ -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) - stdoutdata, _ = p.communicate() - if p.returncode != 0: + proc = Run(cmd) + stdoutdata, _ = proc.communicate() + 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, - stderr=subprocess.STDOUT) + proc = Run(cmd, stdin=subprocess.PIPE) if password is not None: password += "\n" - stdoutdata, _ = p.communicate(password) - if p.returncode != 0: + stdoutdata, _ = proc.communicate(password) + 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]) - _, _ = p.communicate() + Run([self.editor, "+%d" % (first_line,), self.pwfile]).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, - stdoutdata) + assert proc.returncode == 0, \ + "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) - stdoutdata, _ = p.communicate() - assert p.returncode == 0, \ + proc = Run(brotli_cmd) + stdoutdata, _ = proc.communicate() + assert proc.returncode == 0, \ 'Failed to compress {}.new.dat with brotli:\n{}'.format( self.partition, stdoutdata) diff --git a/tools/releasetools/ota_from_target_files.py b/tools/releasetools/ota_from_target_files.py index 755eda9ebe..7ea53f8067 100755 --- a/tools/releasetools/ota_from_target_files.py +++ b/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, - stderr=subprocess.STDOUT) + get_signing_key = common.Run(cmd, verbose=False) 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) diff --git a/tools/releasetools/test_add_img_to_target_files.py b/tools/releasetools/test_add_img_to_target_files.py index a73746efc8..cc7b887db5 100644 --- a/tools/releasetools/test_add_img_to_target_files.py +++ b/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) - p.communicate() - self.assertEqual(0, p.returncode) + proc = common.Run(cmd) + output, _ = proc.communicate() + 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() diff --git a/tools/releasetools/test_ota_from_target_files.py b/tools/releasetools/test_ota_from_target_files.py index 1d8a786ba9..29e0d83dfc 100644 --- a/tools/releasetools/test_ota_from_target_files.py +++ b/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) diff --git a/tools/releasetools/test_validate_target_files.py b/tools/releasetools/test_validate_target_files.py index 0aaf069fc5..ecb7fde22d 100644 --- a/tools/releasetools/test_validate_target_files.py +++ b/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, diff --git a/tools/releasetools/validate_target_files.py b/tools/releasetools/validate_target_files.py index 09f800f92e..1cc4a600b1 100755 --- a/tools/releasetools/validate_target_files.py +++ b/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(