From 1b24ae8922d7cf77ece8697b4768d1a774e95603 Mon Sep 17 00:00:00 2001 From: Chris Craik Date: Mon, 13 Mar 2017 11:01:00 -0700 Subject: Sync to internal master Syncs to upstream revision e2a178a9c0cdc4e6aa745af113bc9b7ae3651aa8 via update.py --local= Test: ran systrace.py locally Change-Id: Ieb2b634438a4e408b46d2fb9621ed19d65520cf4 --- catapult/devil/PRESUBMIT.py | 26 +- catapult/devil/devil/__init__.py | 4 + catapult/devil/devil/android/device_errors.py | 48 +++- catapult/devil/devil/android/device_errors_test.py | 72 ++++++ catapult/devil/devil/android/device_temp_file.py | 7 + catapult/devil/devil/android/device_utils.py | 85 +++++-- catapult/devil/devil/android/device_utils_test.py | 114 ++++++++- catapult/devil/devil/android/flag_changer.py | 263 ++++++++++++++------- .../devil/devil/android/flag_changer_devicetest.py | 77 ++++++ catapult/devil/devil/android/flag_changer_test.py | 132 +++++++++++ catapult/devil/devil/android/forwarder.py | 26 +- catapult/devil/devil/android/perf/perf_control.py | 69 +++++- .../android/sdk/adb_compatibility_devicetest.py | 2 +- catapult/devil/devil/android/sdk/adb_wrapper.py | 13 + .../devil/devil/android/tools/adb_run_shell_cmd.py | 6 + catapult/devil/devil/android/tools/cpufreq.py | 87 +++++++ .../devil/devil/android/tools/device_monitor.py | 19 +- .../devil/android/tools/device_monitor_test.py | 15 +- .../devil/devil/android/tools/device_recovery.py | 13 +- .../devil/devil/android/tools/provision_devices.py | 72 ++++-- .../devil/devil/android/tools/script_common.py | 8 +- .../devil/devil/android/tools/wait_for_devices.py | 50 ++++ catapult/devil/devil/base_error.py | 7 + catapult/devil/devil/devil_dependencies.json | 4 +- catapult/devil/devil/utils/cmd_helper.py | 7 +- catapult/devil/devil/utils/reset_usb.py | 9 +- catapult/devil/devil/utils/signal_handler.py | 26 +- 27 files changed, 1069 insertions(+), 192 deletions(-) create mode 100755 catapult/devil/devil/android/device_errors_test.py create mode 100644 catapult/devil/devil/android/flag_changer_devicetest.py create mode 100755 catapult/devil/devil/android/flag_changer_test.py create mode 100755 catapult/devil/devil/android/tools/cpufreq.py create mode 100755 catapult/devil/devil/android/tools/wait_for_devices.py (limited to 'catapult/devil') diff --git a/catapult/devil/PRESUBMIT.py b/catapult/devil/PRESUBMIT.py index edec3e16..289a5c65 100644 --- a/catapult/devil/PRESUBMIT.py +++ b/catapult/devil/PRESUBMIT.py @@ -26,21 +26,17 @@ def _RunUnitTests(input_api, output_api): 'PYTHONPATH': ':'.join([J(), J('..')]), }) - return input_api.canned_checks.RunUnitTests( - input_api, - output_api, - unit_tests=[ - J('devil_env_test.py'), - J('android', 'battery_utils_test.py'), - J('android', 'device_utils_test.py'), - J('android', 'fastboot_utils_test.py'), - J('android', 'md5sum_test.py'), - J('android', 'logcat_monitor_test.py'), - J('android', 'tools', 'script_common_test.py'), - J('utils', 'cmd_helper_test.py'), - J('utils', 'timeout_retry_unittest.py'), - ], - env=test_env) + message_type = (output_api.PresubmitError if input_api.is_committing + else output_api.PresubmitPromptWarning) + + return input_api.RunTests([ + input_api.Command( + name='devil/bin/run_py_tests', + cmd=[ + input_api.os_path.join( + input_api.PresubmitLocalPath(), 'bin', 'run_py_tests')], + kwargs={'env': test_env}, + message=message_type)]) def _EnsureNoPylibUse(input_api, output_api): diff --git a/catapult/devil/devil/__init__.py b/catapult/devil/devil/__init__.py index 50b23dff..7de59c94 100644 --- a/catapult/devil/devil/__init__.py +++ b/catapult/devil/devil/__init__.py @@ -1,3 +1,7 @@ # Copyright 2015 The Chromium Authors. All rights reserved. # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. + +import logging + +logging.getLogger('devil').addHandler(logging.NullHandler()) diff --git a/catapult/devil/devil/android/device_errors.py b/catapult/devil/devil/android/device_errors.py index 67faa488..568e4974 100644 --- a/catapult/devil/devil/android/device_errors.py +++ b/catapult/devil/devil/android/device_errors.py @@ -15,11 +15,19 @@ class CommandFailedError(base_error.BaseError): """Exception for command failures.""" def __init__(self, message, device_serial=None): - if device_serial is not None: - message = '(device: %s) %s' % (device_serial, message) + device_leader = '(device: %s)' % device_serial + if device_serial is not None and not message.startswith(device_leader): + message = '%s %s' % (device_leader, message) self.device_serial = device_serial super(CommandFailedError, self).__init__(message) + def __eq__(self, other): + return (super(CommandFailedError, self).__eq__(other) + and self.device_serial == other.device_serial) + + def __ne__(self, other): + return not self == other + class _BaseCommandFailedError(CommandFailedError): """Base Exception for adb and fastboot command failures.""" @@ -42,6 +50,27 @@ class _BaseCommandFailedError(CommandFailedError): message = ''.join(message) super(_BaseCommandFailedError, self).__init__(message, device_serial) + def __eq__(self, other): + return (super(_BaseCommandFailedError, self).__eq__(other) + and self.args == other.args + and self.output == other.output + and self.status == other.status) + + def __ne__(self, other): + return not self == other + + def __reduce__(self): + """Support pickling.""" + result = [None, None, None, None, None] + super_result = super(_BaseCommandFailedError, self).__reduce__() + for i in range(len(super_result)): + result[i] = super_result[i] + + # Update the args used to reconstruct this exception. + result[1] = ( + self.args, self.output, self.status, self.device_serial, self.message) + return tuple(result) + class AdbCommandFailedError(_BaseCommandFailedError): """Exception for adb command failures.""" @@ -91,6 +120,17 @@ class AdbShellCommandFailedError(AdbCommandFailedError): super(AdbShellCommandFailedError, self).__init__( ['shell', command], output, status, device_serial, message) + def __reduce__(self): + """Support pickling.""" + result = [None, None, None, None, None] + super_result = super(AdbShellCommandFailedError, self).__reduce__() + for i in range(len(super_result)): + result[i] = super_result[i] + + # Update the args used to reconstruct this exception. + result[1] = (self.command, self.output, self.status, self.device_serial) + return tuple(result) + class CommandTimeoutError(base_error.BaseError): """Exception for command timeouts.""" @@ -105,9 +145,9 @@ class DeviceUnreachableError(base_error.BaseError): class NoDevicesError(base_error.BaseError): """Exception for having no devices attached.""" - def __init__(self): + def __init__(self, msg=None): super(NoDevicesError, self).__init__( - 'No devices attached.', is_infra_error=True) + msg or 'No devices attached.', is_infra_error=True) class MultipleDevicesError(base_error.BaseError): diff --git a/catapult/devil/devil/android/device_errors_test.py b/catapult/devil/devil/android/device_errors_test.py new file mode 100755 index 00000000..68a4f167 --- /dev/null +++ b/catapult/devil/devil/android/device_errors_test.py @@ -0,0 +1,72 @@ +#! /usr/bin/env python +# Copyright 2017 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +import pickle +import sys +import unittest + +from devil.android import device_errors + + +class DeviceErrorsTest(unittest.TestCase): + + def assertIsPicklable(self, original): + pickled = pickle.dumps(original) + reconstructed = pickle.loads(pickled) + self.assertEquals(original, reconstructed) + + def testPicklable_AdbCommandFailedError(self): + original = device_errors.AdbCommandFailedError( + ['these', 'are', 'adb', 'args'], 'adb failure output', status=':(', + device_serial='0123456789abcdef') + self.assertIsPicklable(original) + + def testPicklable_AdbShellCommandFailedError(self): + original = device_errors.AdbShellCommandFailedError( + 'foo', 'erroneous foo output', '1', device_serial='0123456789abcdef') + self.assertIsPicklable(original) + + def testPicklable_CommandFailedError(self): + original = device_errors.CommandFailedError( + 'sample command failed') + self.assertIsPicklable(original) + + def testPicklable_CommandTimeoutError(self): + original = device_errors.CommandTimeoutError( + 'My fake command timed out :(') + self.assertIsPicklable(original) + + def testPicklable_DeviceChargingError(self): + original = device_errors.DeviceChargingError( + 'Fake device failed to charge') + self.assertIsPicklable(original) + + def testPicklable_DeviceUnreachableError(self): + original = device_errors.DeviceUnreachableError + self.assertIsPicklable(original) + + def testPicklable_FastbootCommandFailedError(self): + original = device_errors.FastbootCommandFailedError( + ['these', 'are', 'fastboot', 'args'], 'fastboot failure output', + status=':(', device_serial='0123456789abcdef') + self.assertIsPicklable(original) + + def testPicklable_MultipleDevicesError(self): + # TODO(jbudorick): Implement this after implementing a stable DeviceUtils + # fake. https://github.com/catapult-project/catapult/issues/3145 + pass + + def testPicklable_NoAdbError(self): + original = device_errors.NoAdbError() + self.assertIsPicklable(original) + + def testPicklable_NoDevicesError(self): + original = device_errors.NoDevicesError() + self.assertIsPicklable(original) + + + +if __name__ == '__main__': + sys.exit(unittest.main()) diff --git a/catapult/devil/devil/android/device_temp_file.py b/catapult/devil/devil/android/device_temp_file.py index 75488c50..4d0c7adb 100644 --- a/catapult/devil/devil/android/device_temp_file.py +++ b/catapult/devil/devil/android/device_temp_file.py @@ -26,7 +26,14 @@ class DeviceTempFile(object): suffix: The suffix of the name of the temp file. prefix: The prefix of the name of the temp file. dir: The directory on the device where to place the temp file. + Raises: + ValueError if any of suffix, prefix, or dir are None. """ + if None in (dir, prefix, suffix): + m = 'Provided None path component. (dir: %s, prefix: %s, suffix: %s)' % ( + dir, prefix, suffix) + raise ValueError(m) + self._adb = adb # Python's random module use 52-bit numbers according to its docs. random_hex = hex(random.randint(0, 2 ** 52))[2:] diff --git a/catapult/devil/devil/android/device_utils.py b/catapult/devil/devil/android/device_utils.py index 0d16b81e..3d8e2034 100644 --- a/catapult/devil/devil/android/device_utils.py +++ b/catapult/devil/devil/android/device_utils.py @@ -16,6 +16,7 @@ import logging import multiprocessing import os import posixpath +import pprint import re import shutil import stat @@ -72,12 +73,14 @@ _RESTART_ADBD_SCRIPT = """ # Not all permissions can be set. _PERMISSIONS_BLACKLIST = [ + 'android.permission.ACCESS_LOCATION_EXTRA_COMMANDS', 'android.permission.ACCESS_MOCK_LOCATION', 'android.permission.ACCESS_NETWORK_STATE', 'android.permission.ACCESS_WIFI_STATE', 'android.permission.AUTHENTICATE_ACCOUNTS', 'android.permission.BLUETOOTH', 'android.permission.BLUETOOTH_ADMIN', + 'android.permission.DISABLE_KEYGUARD', 'android.permission.DOWNLOAD_WITHOUT_NOTIFICATION', 'android.permission.INTERNET', 'android.permission.MANAGE_ACCOUNTS', @@ -96,6 +99,7 @@ _PERMISSIONS_BLACKLIST = [ 'com.android.browser.permission.WRITE_HISTORY_BOOKMARKS', 'com.android.launcher.permission.INSTALL_SHORTCUT', 'com.chrome.permission.DEVICE_EXTRAS', + 'com.google.android.apps.now.CURRENT_ACCOUNT_ACCESS', 'com.google.android.c2dm.permission.RECEIVE', 'com.google.android.providers.gsf.permission.READ_GSERVICES', 'com.sec.enterprise.knox.MDM_CONTENT_PROVIDER', @@ -496,15 +500,11 @@ class DeviceUtils(object): apks = [] for line in output: if not line.startswith('package:'): - # KitKat on x86 may show following warnings that is safe to ignore. - if (line.startswith('WARNING: linker: libdvm.so has text relocations.') - and version_codes.KITKAT <= self.build_version_sdk - and self.build_version_sdk <= version_codes.KITKAT_WATCH - and self.product_cpu_abi == 'x86'): - continue - raise device_errors.CommandFailedError( - 'pm path returned: %r' % '\n'.join(output), str(self)) + continue apks.append(line[len('package:'):]) + if not apks and output: + raise device_errors.CommandFailedError( + 'pm path returned: %r' % '\n'.join(output), str(self)) self._cache['package_apk_paths'][package] = list(apks) return apks @@ -538,19 +538,19 @@ class DeviceUtils(object): package: Name of the package. Returns: - The package's data directory, or None if the package doesn't exist on the - device. + The package's data directory. + Raises: + CommandFailedError if the package's data directory can't be found, + whether because it's not installed or otherwise. """ - try: - output = self._RunPipedShellCommand( - 'pm dump %s | grep dataDir=' % cmd_helper.SingleQuote(package)) - for line in output: - _, _, dataDir = line.partition('dataDir=') - if dataDir: - return dataDir - except device_errors.CommandFailedError: - logger.exception('Could not find data directory for %s', package) - return None + output = self._RunPipedShellCommand( + 'pm dump %s | grep dataDir=' % cmd_helper.SingleQuote(package)) + for line in output: + _, _, dataDir = line.partition('dataDir=') + if dataDir: + return dataDir + raise device_errors.CommandFailedError( + 'Could not find data directory for %s', package) @decorators.WithTimeoutAndRetriesFromInstance() def WaitUntilFullyBooted(self, wifi=False, timeout=None, retries=None): @@ -702,6 +702,12 @@ class DeviceUtils(object): if len(all_apks) == 1: logger.warning('split-select did not select any from %s', split_apks) + missing_apks = [apk for apk in all_apks if not os.path.exists(apk)] + if missing_apks: + raise device_errors.CommandFailedError( + 'Attempted to install non-existent apks: %s' + % pprint.pformat(missing_apks)) + package_name = base_apk.GetPackageName() device_apk_paths = self._GetApplicationPathsInternal(package_name) @@ -794,8 +800,9 @@ class DeviceUtils(object): @decorators.WithTimeoutAndRetriesFromInstance() def RunShellCommand(self, cmd, check_return=False, cwd=None, env=None, - as_root=False, single_line=False, large_output=False, - raw_output=False, timeout=None, retries=None): + run_as=None, as_root=False, single_line=False, + large_output=False, raw_output=False, timeout=None, + retries=None): """Run an ADB shell command. The command to run |cmd| should be a sequence of program arguments or else @@ -824,6 +831,8 @@ class DeviceUtils(object): be checked. cwd: The device directory in which the command should be run. env: The environment variables with which the command should be run. + run_as: A string containing the package as which the command should be + run. as_root: A boolean indicating whether the shell command should be run with root privileges. single_line: A boolean indicating if only a single line of output is @@ -904,6 +913,9 @@ class DeviceUtils(object): cmd = '%s %s' % (env, cmd) if cwd: cmd = 'cd %s && %s' % (cmd_helper.SingleQuote(cwd), cmd) + if run_as: + cmd = 'run-as %s sh -c %s' % (cmd_helper.SingleQuote(run_as), + cmd_helper.SingleQuote(cmd)) if as_root and self.NeedsSU(): # "su -c sh -c" allows using shell features in |cmd| cmd = self._Su('sh -c %s' % cmd_helper.SingleQuote(cmd)) @@ -1493,6 +1505,33 @@ class DeviceUtils(object): except device_errors.CommandFailedError: return False + @decorators.WithTimeoutAndRetriesFromInstance() + def RemovePath(self, device_path, force=False, recursive=False, + as_root=False, timeout=None, retries=None): + """Removes the given path(s) from the device. + + Args: + device_path: A string containing the absolute path to the file on the + device, or an iterable of paths to check. + force: Whether to remove the path(s) with force (-f). + recursive: Whether to remove any directories in the path(s) recursively. + as_root: Whether root permissions should be use to remove the given + path(s). + timeout: timeout in seconds + retries: number of retries + """ + args = ['rm'] + if force: + args.append('-f') + if recursive: + args.append('-r') + if isinstance(device_path, basestring): + args.append(device_path) + else: + args.extend(device_path) + self.RunShellCommand(args, as_root=as_root, check_return=True) + + @decorators.WithTimeoutAndRetriesFromInstance() def PullFile(self, device_path, host_path, timeout=None, retries=None): """Pull a file from the device. @@ -2350,7 +2389,7 @@ class DeviceUtils(object): A device serial, or a list of device serials (optional). Returns: - A list of one or more DeviceUtils instances. + A list of DeviceUtils instances. Raises: NoDevicesError: Raised when no non-blacklisted devices exist and diff --git a/catapult/devil/devil/android/device_utils_test.py b/catapult/devil/devil/android/device_utils_test.py index 18fda546..a77d1202 100755 --- a/catapult/devil/devil/android/device_utils_test.py +++ b/catapult/devil/devil/android/device_utils_test.py @@ -361,6 +361,15 @@ class DeviceUtilsGetApplicationPathsInternalTest(DeviceUtilsTest): self.assertEquals([], self.device._GetApplicationPathsInternal('not.installed.app')) + def testGetApplicationPathsInternal_garbageFirstLine(self): + with self.assertCalls( + (self.call.device.GetProp('ro.build.version.sdk', cache=True), '19'), + (self.call.device.RunShellCommand( + ['pm', 'path', 'android'], check_return=True), + ['garbage first line'])): + with self.assertRaises(device_errors.CommandFailedError): + self.device._GetApplicationPathsInternal('android') + def testGetApplicationPathsInternal_fails(self): with self.assertCalls( (self.call.device.GetProp('ro.build.version.sdk', cache=True), '19'), @@ -417,7 +426,8 @@ class DeviceUtilsGetApplicationDataDirectoryTest(DeviceUtilsTest): self.call.device._RunPipedShellCommand( 'pm dump foo.bar.baz | grep dataDir='), self.ShellError()): - self.assertIsNone(self.device.GetApplicationDataDirectory('foo.bar.baz')) + with self.assertRaises(device_errors.CommandFailedError): + self.device.GetApplicationDataDirectory('foo.bar.baz') @mock.patch('time.sleep', mock.Mock()) @@ -614,6 +624,7 @@ class DeviceUtilsInstallTest(DeviceUtilsTest): def testInstall_noPriorInstall(self): with self.patch_call(self.call.device.build_version_sdk, return_value=23): with self.assertCalls( + (mock.call.os.path.exists('/fake/test/app.apk'), True), (self.call.device._GetApplicationPathsInternal('test.package'), []), self.call.adb.Install('/fake/test/app.apk', reinstall=False, allow_downgrade=False), @@ -623,6 +634,7 @@ class DeviceUtilsInstallTest(DeviceUtilsTest): def testInstall_permissionsPreM(self): with self.patch_call(self.call.device.build_version_sdk, return_value=20): with self.assertCalls( + (mock.call.os.path.exists('/fake/test/app.apk'), True), (self.call.device._GetApplicationPathsInternal('test.package'), []), (self.call.adb.Install('/fake/test/app.apk', reinstall=False, allow_downgrade=False))): @@ -631,6 +643,7 @@ class DeviceUtilsInstallTest(DeviceUtilsTest): def testInstall_findPermissions(self): with self.patch_call(self.call.device.build_version_sdk, return_value=23): with self.assertCalls( + (mock.call.os.path.exists('/fake/test/app.apk'), True), (self.call.device._GetApplicationPathsInternal('test.package'), []), (self.call.adb.Install('/fake/test/app.apk', reinstall=False, allow_downgrade=False)), @@ -639,6 +652,7 @@ class DeviceUtilsInstallTest(DeviceUtilsTest): def testInstall_passPermissions(self): with self.assertCalls( + (mock.call.os.path.exists('/fake/test/app.apk'), True), (self.call.device._GetApplicationPathsInternal('test.package'), []), (self.call.adb.Install('/fake/test/app.apk', reinstall=False, allow_downgrade=False)), @@ -648,6 +662,7 @@ class DeviceUtilsInstallTest(DeviceUtilsTest): def testInstall_differentPriorInstall(self): with self.assertCalls( + (mock.call.os.path.exists('/fake/test/app.apk'), True), (self.call.device._GetApplicationPathsInternal('test.package'), ['/fake/data/app/test.package.apk']), (self.call.device._ComputeStaleApks('test.package', @@ -661,6 +676,7 @@ class DeviceUtilsInstallTest(DeviceUtilsTest): def testInstall_differentPriorInstall_reinstall(self): with self.assertCalls( + (mock.call.os.path.exists('/fake/test/app.apk'), True), (self.call.device._GetApplicationPathsInternal('test.package'), ['/fake/data/app/test.package.apk']), (self.call.device._ComputeStaleApks('test.package', @@ -673,6 +689,7 @@ class DeviceUtilsInstallTest(DeviceUtilsTest): def testInstall_identicalPriorInstall_reinstall(self): with self.assertCalls( + (mock.call.os.path.exists('/fake/test/app.apk'), True), (self.call.device._GetApplicationPathsInternal('test.package'), ['/fake/data/app/test.package.apk']), (self.call.device._ComputeStaleApks('test.package', @@ -682,8 +699,15 @@ class DeviceUtilsInstallTest(DeviceUtilsTest): self.device.Install(DeviceUtilsInstallTest.mock_apk, reinstall=True, retries=0, permissions=[]) + def testInstall_missingApk(self): + with self.assertCalls( + (mock.call.os.path.exists('/fake/test/app.apk'), False)): + with self.assertRaises(device_errors.CommandFailedError): + self.device.Install(DeviceUtilsInstallTest.mock_apk, retries=0) + def testInstall_fails(self): with self.assertCalls( + (mock.call.os.path.exists('/fake/test/app.apk'), True), (self.call.device._GetApplicationPathsInternal('test.package'), []), (self.call.adb.Install('/fake/test/app.apk', reinstall=False, allow_downgrade=False), @@ -693,6 +717,7 @@ class DeviceUtilsInstallTest(DeviceUtilsTest): def testInstall_downgrade(self): with self.assertCalls( + (mock.call.os.path.exists('/fake/test/app.apk'), True), (self.call.device._GetApplicationPathsInternal('test.package'), ['/fake/data/app/test.package.apk']), (self.call.device._ComputeStaleApks('test.package', @@ -716,6 +741,8 @@ class DeviceUtilsInstallSplitApkTest(DeviceUtilsTest): ['split1.apk', 'split2.apk', 'split3.apk'], allow_cached_props=False), ['split2.apk']), + (mock.call.os.path.exists('base.apk'), True), + (mock.call.os.path.exists('split2.apk'), True), (self.call.device._GetApplicationPathsInternal('test.package'), []), (self.call.adb.InstallMultiple( ['base.apk', 'split2.apk'], partial=None, reinstall=False, @@ -731,6 +758,8 @@ class DeviceUtilsInstallSplitApkTest(DeviceUtilsTest): ['split1.apk', 'split2.apk', 'split3.apk'], allow_cached_props=False), ['split2.apk']), + (mock.call.os.path.exists('base.apk'), True), + (mock.call.os.path.exists('split2.apk'), True), (self.call.device._GetApplicationPathsInternal('test.package'), ['base-on-device.apk', 'split2-on-device.apk']), (self.call.device._ComputeStaleApks('test.package', @@ -751,6 +780,8 @@ class DeviceUtilsInstallSplitApkTest(DeviceUtilsTest): ['split1.apk', 'split2.apk', 'split3.apk'], allow_cached_props=False), ['split2.apk']), + (mock.call.os.path.exists('base.apk'), True), + (mock.call.os.path.exists('split2.apk'), True), (self.call.device._GetApplicationPathsInternal('test.package'), ['base-on-device.apk', 'split2-on-device.apk']), (self.call.device._ComputeStaleApks('test.package', @@ -764,6 +795,21 @@ class DeviceUtilsInstallSplitApkTest(DeviceUtilsTest): reinstall=True, permissions=[], retries=0, allow_downgrade=True) + def testInstallSplitApk_missingSplit(self): + with self.assertCalls( + (self.call.device._CheckSdkLevel(21)), + (mock.call.devil.android.sdk.split_select.SelectSplits( + self.device, 'base.apk', + ['split1.apk', 'split2.apk', 'split3.apk'], + allow_cached_props=False), + ['split2.apk']), + (mock.call.os.path.exists('base.apk'), True), + (mock.call.os.path.exists('split2.apk'), False)): + with self.assertRaises(device_errors.CommandFailedError): + self.device.InstallSplitApk(DeviceUtilsInstallSplitApkTest.mock_apk, + ['split1.apk', 'split2.apk', 'split3.apk'], permissions=[], + retries=0) + class DeviceUtilsUninstallTest(DeviceUtilsTest): @@ -847,7 +893,7 @@ class DeviceUtilsRunShellCommandTest(DeviceUtilsTest): self.assertEquals([payload], self.device.RunShellCommand(['echo', payload])) - def testRunShellCommand_withHugeCmdAndSU(self): + def testRunShellCommand_withHugeCmdAndSu(self): payload = 'hi! ' * 1024 expected_cmd_without_su = """sh -c 'echo '"'"'%s'"'"''""" % payload expected_cmd = 'su -c %s' % expected_cmd_without_su @@ -871,6 +917,31 @@ class DeviceUtilsRunShellCommandTest(DeviceUtilsTest): (self.call.adb.Shell(expected_cmd), '')): self.device.RunShellCommand('setprop service.adb.root 0', as_root=True) + def testRunShellCommand_withRunAs(self): + expected_cmd_without_run_as = "sh -c 'mkdir -p files'" + expected_cmd = ( + 'run-as org.devil.test_package %s' % expected_cmd_without_run_as) + with self.assertCall(self.call.adb.Shell(expected_cmd), ''): + self.device.RunShellCommand( + ['mkdir', '-p', 'files'], + run_as='org.devil.test_package') + + def testRunShellCommand_withRunAsAndSu(self): + expected_cmd_with_nothing = "sh -c 'mkdir -p files'" + expected_cmd_with_run_as = ( + 'run-as org.devil.test_package %s' % expected_cmd_with_nothing) + expected_cmd_without_su = ( + 'sh -c %s' % cmd_helper.SingleQuote(expected_cmd_with_run_as)) + expected_cmd = 'su -c %s' % expected_cmd_without_su + with self.assertCalls( + (self.call.device.NeedsSU(), True), + (self.call.device._Su(expected_cmd_without_su), expected_cmd), + (self.call.adb.Shell(expected_cmd), '')): + self.device.RunShellCommand( + ['mkdir', '-p', 'files'], + run_as='org.devil.test_package', + as_root=True) + def testRunShellCommand_manyLines(self): cmd = 'ls /some/path' with self.assertCall(self.call.adb.Shell(cmd), 'file1\nfile2\nfile3\n'): @@ -1600,6 +1671,45 @@ class DeviceUtilsPathExistsTest(DeviceUtilsTest): self.assertFalse(self.device.FileExists('/path/file.not.exists')) +class DeviceUtilsRemovePathTest(DeviceUtilsTest): + + def testRemovePath_regular(self): + with self.assertCall( + self.call.device.RunShellCommand( + ['rm', 'some file'], as_root=False, check_return=True), + []): + self.device.RemovePath('some file') + + def testRemovePath_withForce(self): + with self.assertCall( + self.call.device.RunShellCommand( + ['rm', '-f', 'some file'], as_root=False, check_return=True), + []): + self.device.RemovePath('some file', force=True) + + def testRemovePath_recursively(self): + with self.assertCall( + self.call.device.RunShellCommand( + ['rm', '-r', '/remove/this/dir'], as_root=False, check_return=True), + []): + self.device.RemovePath('/remove/this/dir', recursive=True) + + def testRemovePath_withRoot(self): + with self.assertCall( + self.call.device.RunShellCommand( + ['rm', 'some file'], as_root=True, check_return=True), + []): + self.device.RemovePath('some file', as_root=True) + + def testRemovePath_manyPaths(self): + with self.assertCall( + self.call.device.RunShellCommand( + ['rm', 'eeny', 'meeny', 'miny', 'moe'], + as_root=False, check_return=True), + []): + self.device.RemovePath(['eeny', 'meeny', 'miny', 'moe']) + + class DeviceUtilsPullFileTest(DeviceUtilsTest): def testPullFile_existsOnDevice(self): diff --git a/catapult/devil/devil/android/flag_changer.py b/catapult/devil/devil/android/flag_changer.py index 5de54eaa..2c8cc3c2 100644 --- a/catapult/devil/devil/android/flag_changer.py +++ b/catapult/devil/devil/android/flag_changer.py @@ -3,12 +3,19 @@ # found in the LICENSE file. import logging - -from devil.android import device_errors +import posixpath +import re logger = logging.getLogger(__name__) +_CMDLINE_DIR = '/data/local/tmp' +_CMDLINE_DIR_LEGACY = '/data/local' +_RE_NEEDS_QUOTING = re.compile(r'[^\w-]') # Not in: alphanumeric or hyphens. +_QUOTES = '"\'' # Either a single or a double quote. +_ESCAPE = '\\' # A backslash. + + class FlagChanger(object): """Changes the flags Chrome runs with. @@ -22,26 +29,50 @@ class FlagChanger(object): Args: device: A DeviceUtils instance. - cmdline_file: Path to the command line file on the device. + cmdline_file: Name of the command line file where to store flags. """ self._device = device - # Unrooted devices have limited access to the file system. - # Place files in /data/local/tmp/ rather than /data/local/ - if not device.HasRoot() and not '/data/local/tmp/' in cmdline_file: - self._cmdline_file = cmdline_file.replace('/data/local/', - '/data/local/tmp/') + unused_dir, basename = posixpath.split(cmdline_file) + self._cmdline_path = posixpath.join(_CMDLINE_DIR, basename) + + # TODO(catapult:#3112): Make this fail instead of warn after all clients + # have been switched. + if unused_dir: + logging.warning( + 'cmdline_file argument of %s() should be a file name only (not a' + ' full path).', type(self).__name__) + if cmdline_file != self._cmdline_path: + logging.warning( + 'Client supplied %r, but %r will be used instead.', + cmdline_file, self._cmdline_path) + + cmdline_path_legacy = posixpath.join(_CMDLINE_DIR_LEGACY, basename) + if self._device.PathExists(cmdline_path_legacy): + logging.warning( + 'Removing legacy command line file %r.', cmdline_path_legacy) + self._device.RemovePath(cmdline_path_legacy, as_root=True) + + self._state_stack = [None] # Actual state is set by GetCurrentFlags(). + self.GetCurrentFlags() + + def GetCurrentFlags(self): + """Read the current flags currently stored in the device. + + Also updates the internal state of the flag_changer. + + Returns: + A list of flags. + """ + if self._device.PathExists(self._cmdline_path): + command_line = self._device.ReadFile(self._cmdline_path).strip() else: - self._cmdline_file = cmdline_file - - stored_flags = '' - if self._device.PathExists(self._cmdline_file): - try: - stored_flags = self._device.ReadFile(self._cmdline_file).strip() - except device_errors.CommandFailedError: - pass + command_line = '' + flags = _ParseFlags(command_line) + # Store the flags as a set to facilitate adding and removing flags. - self._state_stack = [set(self._TokenizeFlags(stored_flags))] + self._state_stack[-1] = set(flags) + return flags def ReplaceFlags(self, flags): """Replaces the flags in the command line with the ones provided. @@ -52,10 +83,13 @@ class FlagChanger(object): flags: A sequence of command line flags to set, eg. ['--single-process']. Note: this should include flags only, not the name of a command to run (ie. there is no need to start the sequence with 'chrome'). + + Returns: + A list with the flags now stored on the device. """ new_flags = set(flags) self._state_stack.append(new_flags) - self._UpdateCommandLineFile() + return self._UpdateCommandLineFile() def AddFlags(self, flags): """Appends flags to the command line if they aren't already there. @@ -64,8 +98,11 @@ class FlagChanger(object): Args: flags: A sequence of flags to add on, eg. ['--single-process']. + + Returns: + A list with the flags now stored on the device. """ - self.PushFlags(add=flags) + return self.PushFlags(add=flags) def RemoveFlags(self, flags): """Removes flags from the command line, if they exist. @@ -80,8 +117,11 @@ class FlagChanger(object): that we expect a complete match when removing flags; if you want to remove a switch with a value, you must use the exact string used to add it in the first place. + + Returns: + A list with the flags now stored on the device. """ - self.PushFlags(remove=flags) + return self.PushFlags(remove=flags) def PushFlags(self, add=None, remove=None): """Appends and removes flags to/from the command line if they aren't already @@ -94,91 +134,142 @@ class FlagChanger(object): expect a complete match when removing flags; if you want to remove a switch with a value, you must use the exact string used to add it in the first place. + + Returns: + A list with the flags now stored on the device. """ new_flags = self._state_stack[-1].copy() if add: new_flags.update(add) if remove: new_flags.difference_update(remove) - self.ReplaceFlags(new_flags) + return self.ReplaceFlags(new_flags) def Restore(self): """Restores the flags to their state prior to the last AddFlags or RemoveFlags call. + + Returns: + A list with the flags now stored on the device. """ # The initial state must always remain on the stack. assert len(self._state_stack) > 1, ( "Mismatch between calls to Add/RemoveFlags and Restore") self._state_stack.pop() - self._UpdateCommandLineFile() + return self._UpdateCommandLineFile() def _UpdateCommandLineFile(self): - """Writes out the command line to the file, or removes it if empty.""" - current_flags = list(self._state_stack[-1]) - logger.info('Current flags: %s', current_flags) - # Root is not required to write to /data/local/tmp/. - use_root = '/data/local/tmp/' not in self._cmdline_file - if current_flags: - # The first command line argument doesn't matter as we are not actually - # launching the chrome executable using this command line. - cmd_line = ' '.join(['_'] + current_flags) - self._device.WriteFile( - self._cmdline_file, cmd_line, as_root=use_root) - file_contents = self._device.ReadFile( - self._cmdline_file, as_root=use_root).rstrip() - assert file_contents == cmd_line, ( - 'Failed to set the command line file at %s' % self._cmdline_file) - else: - self._device.RunShellCommand('rm ' + self._cmdline_file, - as_root=use_root) - assert not self._device.FileExists(self._cmdline_file), ( - 'Failed to remove the command line file at %s' % self._cmdline_file) - - @staticmethod - def _TokenizeFlags(line): - """Changes the string containing the command line into a list of flags. - - Follows similar logic to CommandLine.java::tokenizeQuotedArguments: - * Flags are split using whitespace, unless the whitespace is within a - pair of quotation marks. - * Unlike the Java version, we keep the quotation marks around switch - values since we need them to re-create the file when new flags are - appended. + """Writes out the command line to the file, or removes it if empty. - Args: - line: A string containing the entire command line. The first token is - assumed to be the program name. + Returns: + A list with the flags now stored on the device. """ - if not line: - return [] - - tokenized_flags = [] - current_flag = "" - within_quotations = False - - # Move through the string character by character and build up each flag - # along the way. - for c in line.strip(): - if c is '"': - if len(current_flag) > 0 and current_flag[-1] == '\\': - # Last char was a backslash; pop it, and treat this " as a literal. - current_flag = current_flag[0:-1] + '"' - else: - within_quotations = not within_quotations - current_flag += c - elif not within_quotations and (c is ' ' or c is '\t'): - if current_flag is not "": - tokenized_flags.append(current_flag) - current_flag = "" - else: - current_flag += c + command_line = _SerializeFlags(self._state_stack[-1]) + if command_line is not None: + self._device.WriteFile(self._cmdline_path, command_line) + else: + self._device.RunShellCommand('rm ' + self._cmdline_path) + + current_flags = self.GetCurrentFlags() + logger.info('Flags now set on the device: %s', current_flags) + return current_flags - # Tack on the last flag. - if not current_flag: - if within_quotations: - logger.warn('Unterminated quoted argument: ' + line) + +def _ParseFlags(line): + """Parse the string containing the command line into a list of flags. + + It's a direct port of CommandLine.java::tokenizeQuotedArguments. + + The first token is assumed to be the (unused) program name and stripped off + from the list of flags. + + Args: + line: A string containing the entire command line. The first token is + assumed to be the program name. + + Returns: + A list of flags, with quoting removed. + """ + flags = [] + current_quote = None + current_flag = None + + for c in line: + # Detect start or end of quote block. + if (current_quote is None and c in _QUOTES) or c == current_quote: + if current_flag is not None and current_flag[-1] == _ESCAPE: + # Last char was a backslash; pop it, and treat c as a literal. + current_flag = current_flag[:-1] + c + else: + current_quote = c if current_quote is None else None + elif current_quote is None and c.isspace(): + if current_flag is not None: + flags.append(current_flag) + current_flag = None else: - tokenized_flags.append(current_flag) + if current_flag is None: + current_flag = '' + current_flag += c + + if current_flag is not None: + if current_quote is not None: + logger.warning('Unterminated quoted argument: ' + current_flag) + flags.append(current_flag) + + # Return everything but the program name. + return flags[1:] + + +def _SerializeFlags(flags): + """Serialize a sequence of flags into a command line string. + + Args: + flags: A sequence of strings with individual flags. + + Returns: + A line with the command line contents to save; or None if the sequence of + flags is empty. + """ + if flags: + # The first command line argument doesn't matter as we are not actually + # launching the chrome executable using this command line. + args = ['_'] + args.extend(_QuoteFlag(f) for f in flags) + return ' '.join(args) + else: + return None + + +def _QuoteFlag(flag): + """Validate and quote a single flag. + + Args: + A string with the flag to quote. + + Returns: + A string with the flag quoted so that it can be parsed by the algorithm + in _ParseFlags; or None if the flag does not appear to be valid. + """ + if '=' in flag: + key, value = flag.split('=', 1) + else: + key, value = flag, None + + if not flag or _RE_NEEDS_QUOTING.search(key): + # Probably not a valid flag, but quote the whole thing so it can be + # parsed back correctly. + return '"%s"' % flag.replace('"', r'\"') - # Return everything but the program name. - return tokenized_flags[1:] + if value is None: + return key + else: + # TODO(catapult:#3112): Remove this check when all clients comply. + if value[0] in _QUOTES and value[0] == value[-1]: + logging.warning( + 'Flag %s appears to be quoted, so will be passed as-is.', flag) + logging.warning( + 'Note: this behavior will be changed in the future. ' + 'Clients should pass values unquoted to prevent double-quoting.') + elif _RE_NEEDS_QUOTING.search(value): + value = '"%s"' % value.replace('"', r'\"') + return '='.join([key, value]) diff --git a/catapult/devil/devil/android/flag_changer_devicetest.py b/catapult/devil/devil/android/flag_changer_devicetest.py new file mode 100644 index 00000000..f5d19d60 --- /dev/null +++ b/catapult/devil/devil/android/flag_changer_devicetest.py @@ -0,0 +1,77 @@ +#!/usr/bin/env python +# Copyright 2017 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. +""" +Unit tests for the contents of flag_changer.py. +The test will invoke real devices +""" + +import os +import posixpath +import sys +import unittest + +if __name__ == '__main__': + sys.path.append( + os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', ))) + +from devil.android import device_test_case +from devil.android import device_utils +from devil.android import flag_changer +from devil.android.sdk import adb_wrapper + + +_CMDLINE_FILE = 'dummy-command-line' + + +class FlagChangerTest(device_test_case.DeviceTestCase): + + def setUp(self): + super(FlagChangerTest, self).setUp() + self.adb = adb_wrapper.AdbWrapper(self.serial) + self.adb.WaitForDevice() + self.device = device_utils.DeviceUtils( + self.adb, default_timeout=10, default_retries=0) + # pylint: disable=protected-access + self.cmdline_path = posixpath.join(flag_changer._CMDLINE_DIR, _CMDLINE_FILE) + self.cmdline_path_legacy = posixpath.join( + flag_changer._CMDLINE_DIR_LEGACY, _CMDLINE_FILE) + + def tearDown(self): + self.device.RemovePath( + [self.cmdline_path, self.cmdline_path_legacy], force=True, as_root=True) + + def testFlagChanger(self): + if not self.device.HasRoot(): + self.skipTest('Test needs a rooted device') + + # Write some custom chrome command line flags. + self.device.WriteFile( + self.cmdline_path, 'chrome --some --old --flags') + + # Write some more flags on a command line file in the legacy location. + self.device.WriteFile( + self.cmdline_path_legacy, 'some --stray --flags', as_root=True) + self.assertTrue(self.device.PathExists(self.cmdline_path_legacy)) + + changer = flag_changer.FlagChanger(self.device, _CMDLINE_FILE) + + # Legacy command line file is removed, ensuring Chrome picks up the + # right file. + self.assertFalse(self.device.PathExists(self.cmdline_path_legacy)) + + # Write some new files, and check they are set. + new_flags = ['--my', '--new', '--flags=with special value'] + self.assertItemsEqual( + changer.ReplaceFlags(new_flags), + new_flags) + + # Restore and go back to the old flags. + self.assertItemsEqual( + changer.Restore(), + ['--some', '--old', '--flags']) + + +if __name__ == '__main__': + unittest.main() diff --git a/catapult/devil/devil/android/flag_changer_test.py b/catapult/devil/devil/android/flag_changer_test.py new file mode 100755 index 00000000..f692bd62 --- /dev/null +++ b/catapult/devil/devil/android/flag_changer_test.py @@ -0,0 +1,132 @@ +#!/usr/bin/env python +# Copyright 2017 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +import posixpath +import unittest + +from devil.android import flag_changer + + +_CMDLINE_FILE = 'chrome-command-line' + + +class _FakeDevice(object): + def __init__(self): + self.build_type = 'user' + self.has_root = True + self.file_system = {} + + def HasRoot(self): + return self.has_root + + def PathExists(self, filepath): + return filepath in self.file_system + + def RemovePath(self, path, **_kwargs): + self.file_system.pop(path) + + def WriteFile(self, path, contents, **_kwargs): + self.file_system[path] = contents + + def ReadFile(self, path, **_kwargs): + return self.file_system[path] + + +class FlagChangerTest(unittest.TestCase): + def setUp(self): + self.device = _FakeDevice() + # pylint: disable=protected-access + self.cmdline_path = posixpath.join(flag_changer._CMDLINE_DIR, _CMDLINE_FILE) + self.cmdline_path_legacy = posixpath.join( + flag_changer._CMDLINE_DIR_LEGACY, _CMDLINE_FILE) + + def testFlagChanger_removeLegacyCmdLine(self): + self.device.WriteFile(self.cmdline_path_legacy, 'chrome --old --stuff') + self.assertTrue(self.device.PathExists(self.cmdline_path_legacy)) + + changer = flag_changer.FlagChanger(self.device, 'chrome-command-line') + self.assertEquals( + changer._cmdline_path, # pylint: disable=protected-access + self.cmdline_path) + self.assertFalse(self.device.PathExists(self.cmdline_path_legacy)) + + +class ParseSerializeFlagsTest(unittest.TestCase): + def _testQuoteFlag(self, flag, expected_quoted_flag): + # Start with an unquoted flag, check that it's quoted as expected. + # pylint: disable=protected-access + quoted_flag = flag_changer._QuoteFlag(flag) + self.assertEqual(quoted_flag, expected_quoted_flag) + # Check that it survives a round-trip. + parsed_flags = flag_changer._ParseFlags('_ %s' % quoted_flag) + self.assertEqual(len(parsed_flags), 1) + self.assertEqual(flag, parsed_flags[0]) + + def testQuoteFlag_simple(self): + self._testQuoteFlag('--simple-flag', '--simple-flag') + + def testQuoteFlag_withSimpleValue(self): + self._testQuoteFlag('--key=value', '--key=value') + + def testQuoteFlag_withQuotedValue1(self): + self._testQuoteFlag('--key=valueA valueB', '--key="valueA valueB"') + + def testQuoteFlag_withQuotedValue2(self): + self._testQuoteFlag( + '--key=this "should" work', r'--key="this \"should\" work"') + + def testQuoteFlag_withQuotedValue3(self): + self._testQuoteFlag( + "--key=this is 'fine' too", '''--key="this is 'fine' too"''') + + def testQuoteFlag_withQuotedValue4(self): + with self.assertRaises(AssertionError): + # TODO(catapult:#3112) This test is broken in the current implementation; + # flags that appear to be quoted are left as-is and, thus, do not + # survive the round-trip. + self._testQuoteFlag( + "--key='I really want to keep these quotes'", + '''--key="'I really want to keep these quotes'"''') + + def testQuoteFlag_withQuotedValue5(self): + self._testQuoteFlag( + "--this is a strange=flag", '"--this is a strange=flag"') + + def _testParseCmdLine(self, command_line, expected_flags): + # Start with a command line, check that flags are parsed as expected. + # pylint: disable=protected-access + flags = flag_changer._ParseFlags(command_line) + self.assertItemsEqual(flags, expected_flags) + + # Check that flags survive a round-trip. + # Note: Although new_command_line and command_line may not match, they + # should describe the same set of flags. + new_command_line = flag_changer._SerializeFlags(flags) + new_flags = flag_changer._ParseFlags(new_command_line) + self.assertItemsEqual(new_flags, expected_flags) + + def testParseCmdLine_simple(self): + self._testParseCmdLine( + 'chrome --foo --bar="a b" --baz=true --fine="ok"', + ['--foo', '--bar=a b', '--baz=true', '--fine=ok']) + + def testParseCmdLine_withFancyQuotes(self): + self._testParseCmdLine( + r'''_ --foo="this 'is' ok" + --bar='this \'is\' too' + --baz="this \'is\' tricky" + ''', + ["--foo=this 'is' ok", + "--bar=this 'is' too", + r"--baz=this \'is\' tricky"]) + + def testParseCmdLine_withUnterminatedQuote(self): + self._testParseCmdLine( + '_ --foo --bar="I forgot something', + ['--foo', '--bar=I forgot something']) + + +if __name__ == '__main__': + unittest.main(verbosity=2) diff --git a/catapult/devil/devil/android/forwarder.py b/catapult/devil/devil/android/forwarder.py index a85d5f65..b5a2bf14 100644 --- a/catapult/devil/devil/android/forwarder.py +++ b/catapult/devil/devil/android/forwarder.py @@ -18,6 +18,11 @@ from devil.utils import cmd_helper logger = logging.getLogger(__name__) +# If passed as the device port, this will tell the forwarder to allocate +# a dynamic port on the device. The actual port can then be retrieved with +# Forwarder.DevicePortForHostPort. +DYNAMIC_DEVICE_PORT = 0 + def _GetProcessStartTime(pid): return psutil.Process(pid).create_time @@ -155,11 +160,13 @@ class Forwarder(object): 'Failed to kill the device forwarder after map failure: %s', str(e)) _LogMapFailureDiagnostics(device) + formatted_output = ('\n'.join(output) if isinstance(output, list) + else output) raise HostForwarderError( '`%s` exited with %d:\n%s' % ( ' '.join(map_cmd), exit_code, - '\n'.join(output))) + formatted_output)) tokens = output.split(':') if len(tokens) != 2: raise HostForwarderError( @@ -209,7 +216,10 @@ class Forwarder(object): if exit_code != 0: error_msg = [ '`%s` exited with %d' % (' '.join(unmap_all_cmd), exit_code)] - error_msg += output + if isinstance(output, list): + error_msg += output + else: + error_msg += [output] raise HostForwarderError('\n'.join(error_msg)) # Clean out any entries from the device & host map. @@ -229,9 +239,9 @@ class Forwarder(object): def DevicePortForHostPort(host_port): """Returns the device port that corresponds to a given host port.""" with _FileLock(Forwarder._LOCK_PATH): - _, device_port = Forwarder._GetInstanceLocked( + serial_and_port = Forwarder._GetInstanceLocked( None)._host_to_device_port_map.get(host_port) - return device_port + return serial_and_port[1] if serial_and_port else None @staticmethod def RemoveHostLog(): @@ -312,7 +322,7 @@ class Forwarder(object): '`%s` exited with %d:\n%s', ' '.join(unmap_cmd), exit_code, - '\n'.join(output)) + '\n'.join(output) if isinstance(output, list) else output) @staticmethod def _GetPidForLock(): @@ -405,8 +415,10 @@ class Forwarder(object): kill_cmd, Forwarder._TIMEOUT) if exit_code != 0: raise HostForwarderError( - '%s exited with %d:\n%s' % (self._host_forwarder_path, exit_code, - '\n'.join(output))) + '%s exited with %d:\n%s' % ( + self._host_forwarder_path, + exit_code, + '\n'.join(output) if isinstance(output, list) else output)) except cmd_helper.TimeoutError as e: raise HostForwarderError( '`%s` timed out:\n%s' % (' '.join(kill_cmd), e.output)) diff --git a/catapult/devil/devil/android/perf/perf_control.py b/catapult/devil/devil/android/perf/perf_control.py index 225b7c95..d52d64e6 100644 --- a/catapult/devil/devil/android/perf/perf_control.py +++ b/catapult/devil/devil/android/perf/perf_control.py @@ -13,9 +13,11 @@ logger = logging.getLogger(__name__) class PerfControl(object): """Provides methods for setting the performance mode of a device.""" + + _AVAILABLE_GOVERNORS_REL_PATH = 'cpufreq/scaling_available_governors' + _CPU_FILE_PATTERN = re.compile(r'^cpu\d+$') _CPU_PATH = '/sys/devices/system/cpu' _KERNEL_MAX = '/sys/devices/system/cpu/kernel_max' - _CPU_FILE_PATTERN = re.compile(r'^cpu\d+$') def __init__(self, device): self._device = device @@ -26,8 +28,14 @@ class PerfControl(object): assert self._cpu_files, 'Failed to detect CPUs.' self._cpu_file_list = ' '.join(self._cpu_files) logger.info('CPUs found: %s', self._cpu_file_list) + self._have_mpdecision = self._device.FileExists('/system/bin/mpdecision') + raw = self._ReadEachCpuFile(self._AVAILABLE_GOVERNORS_REL_PATH) + self._available_governors = [ + (cpu, raw_governors.strip().split() if not exit_code else None) + for cpu, raw_governors, exit_code in raw] + def SetHighPerfMode(self): """Sets the highest stable performance mode for the device.""" try: @@ -46,21 +54,21 @@ class PerfControl(object): self._ForceAllCpusOnline(True) if not self._AllCpusAreOnline(): logger.warning('Failed to force CPUs online. Results may be NOISY!') - self._SetScalingGovernorInternal('performance') + self.SetScalingGovernor('performance') elif 'Nexus 5' == product_model: self._ForceAllCpusOnline(True) if not self._AllCpusAreOnline(): logger.warning('Failed to force CPUs online. Results may be NOISY!') - self._SetScalingGovernorInternal('performance') + self.SetScalingGovernor('performance') self._SetScalingMaxFreq(1190400) self._SetMaxGpuClock(200000000) else: - self._SetScalingGovernorInternal('performance') + self.SetScalingGovernor('performance') def SetPerfProfilingMode(self): """Enables all cores for reliable perf profiling.""" self._ForceAllCpusOnline(True) - self._SetScalingGovernorInternal('performance') + self.SetScalingGovernor('performance') if not self._AllCpusAreOnline(): if not self._device.HasRoot(): raise RuntimeError('Need root to force CPUs online.') @@ -84,7 +92,7 @@ class PerfControl(object): 'Nexus 7': 'interactive', 'Nexus 10': 'interactive' }.get(product_model, 'ondemand') - self._SetScalingGovernorInternal(governor_mode) + self.SetScalingGovernor(governor_mode) self._ForceAllCpusOnline(False) def GetCpuInfo(self): @@ -108,17 +116,58 @@ class PerfControl(object): return zip(self._cpu_files, output[0::2], (int(c) for c in output[1::2])) def _WriteEachCpuFile(self, path, value): + self._ConditionallyWriteEachCpuFile(path, value, condition='true') + + def _ConditionallyWriteEachCpuFile(self, path, value, condition): + template = ( + '{condition} && test -e "$CPU/{path}" && echo {value} > "$CPU/{path}"') results = self._ForEachCpu( - 'test -e "$CPU/{path}" && echo {value} > "$CPU/{path}"'.format( - path=path, value=value)) + template.format(path=path, value=value, condition=condition)) cpus = ' '.join(cpu for (cpu, _, status) in results if status == 0) if cpus: logger.info('Successfully set %s to %r on: %s', path, value, cpus) else: logger.warning('Failed to set %s to %r on any cpus', path, value) - def _SetScalingGovernorInternal(self, value): - self._WriteEachCpuFile('cpufreq/scaling_governor', value) + def _ReadEachCpuFile(self, path): + return self._ForEachCpu( + 'cat "$CPU/{path}"'.format(path=path)) + + def SetScalingGovernor(self, value): + """Sets the scaling governor to the given value on all possible CPUs. + + This does not attempt to set a governor to a value not reported as available + on the corresponding CPU. + + Args: + value: [string] The new governor value. + """ + condition = 'test -e "{path}" && grep -q {value} {path}'.format( + path=('${CPU}/%s' % self._AVAILABLE_GOVERNORS_REL_PATH), + value=value) + self._ConditionallyWriteEachCpuFile( + 'cpufreq/scaling_governor', value, condition) + + def GetScalingGovernor(self): + """Gets the currently set governor for each CPU. + + Returns: + An iterable of 2-tuples, each containing the cpu and the current + governor. + """ + raw = self._ReadEachCpuFile('cpufreq/scaling_governor') + return [ + (cpu, raw_governor.strip() if not exit_code else None) + for cpu, raw_governor, exit_code in raw] + + def ListAvailableGovernors(self): + """Returns the list of available governors for each CPU. + + Returns: + An iterable of 2-tuples, each containing the cpu and a list of available + governors for that cpu. + """ + return self._available_governors def _SetScalingMaxFreq(self, value): self._WriteEachCpuFile('cpufreq/scaling_max_freq', '%d' % value) diff --git a/catapult/devil/devil/android/sdk/adb_compatibility_devicetest.py b/catapult/devil/devil/android/sdk/adb_compatibility_devicetest.py index 49a4971e..cbe2a1b6 100644 --- a/catapult/devil/devil/android/sdk/adb_compatibility_devicetest.py +++ b/catapult/devil/devil/android/sdk/adb_compatibility_devicetest.py @@ -83,7 +83,7 @@ class AdbCompatibilityTest(device_test_case.DeviceTestCase): adb_wrapper.AdbWrapper.StartServer() adb_pids = _hostAdbPids() - self.assertEqual(1, len(adb_pids)) + self.assertGreaterEqual(len(adb_pids), 1) kill_server_status, _ = cmd_helper.GetCmdStatusAndOutput( [adb_wrapper.AdbWrapper.GetAdbPath(), 'kill-server']) diff --git a/catapult/devil/devil/android/sdk/adb_wrapper.py b/catapult/devil/devil/android/sdk/adb_wrapper.py index 924aaa64..8654d269 100644 --- a/catapult/devil/devil/android/sdk/adb_wrapper.py +++ b/catapult/devil/devil/android/sdk/adb_wrapper.py @@ -637,7 +637,20 @@ class AdbWrapper(object): Args: timeout: (optional) Timeout per try in seconds. retries: (optional) Number of retries to attempt. + Returns: + The output of adb forward --list as a string. """ + if (distutils.version.LooseVersion(self.Version()) >= + distutils.version.LooseVersion('1.0.36')): + # Starting in 1.0.36, this can occasionally fail with a protocol fault. + # As this interrupts all connections with all devices, we instead just + # return an empty list. This may give clients an inaccurate result, but + # that's usually better than crashing the adb server. + + # TODO(jbudorick): Determine an appropriate upper version bound for this + # once b/31811775 is fixed. + return '' + return self._RunDeviceAdbCmd(['forward', '--list'], timeout, retries) def JDWP(self, timeout=DEFAULT_TIMEOUT, retries=DEFAULT_RETRIES): diff --git a/catapult/devil/devil/android/tools/adb_run_shell_cmd.py b/catapult/devil/devil/android/tools/adb_run_shell_cmd.py index a826ab12..77b67e84 100755 --- a/catapult/devil/devil/android/tools/adb_run_shell_cmd.py +++ b/catapult/devil/devil/android/tools/adb_run_shell_cmd.py @@ -5,8 +5,14 @@ import argparse import json +import os import sys +if __name__ == '__main__': + sys.path.append( + os.path.abspath(os.path.join(os.path.dirname(__file__), + '..', '..', '..'))) + from devil.android import device_blacklist from devil.android import device_utils from devil.utils import run_tests_helper diff --git a/catapult/devil/devil/android/tools/cpufreq.py b/catapult/devil/devil/android/tools/cpufreq.py new file mode 100755 index 00000000..97deaf04 --- /dev/null +++ b/catapult/devil/devil/android/tools/cpufreq.py @@ -0,0 +1,87 @@ +#! /usr/bin/env python +# Copyright 2016 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +"""A script to manipulate device CPU frequency.""" + +import argparse +import os +import pprint +import sys + +if __name__ == '__main__': + sys.path.append( + os.path.abspath(os.path.join(os.path.dirname(__file__), + '..', '..', '..'))) + +from devil import devil_env +from devil.android import device_utils +from devil.android.perf import perf_control +from devil.utils import run_tests_helper + + +def SetScalingGovernor(device, args): + p = perf_control.PerfControl(device) + p.SetScalingGovernor(args.governor) + + +def GetScalingGovernor(device, _args): + p = perf_control.PerfControl(device) + for cpu, governor in p.GetScalingGovernor(): + print '%s %s: %s' % (str(device), cpu, governor) + + +def ListAvailableGovernors(device, _args): + p = perf_control.PerfControl(device) + for cpu, governors in p.ListAvailableGovernors(): + print '%s %s: %s' % (str(device), cpu, pprint.pformat(governors)) + + +def main(raw_args): + parser = argparse.ArgumentParser() + parser.add_argument( + '--adb-path', + help='ADB binary path.') + parser.add_argument( + '--device', dest='devices', action='append', default=[], + help='Devices for which the governor should be set. Defaults to all.') + parser.add_argument( + '-v', '--verbose', action='count', + help='Log more.') + + subparsers = parser.add_subparsers() + + set_governor = subparsers.add_parser('set-governor') + set_governor.add_argument( + 'governor', + help='Desired CPU governor.') + set_governor.set_defaults(func=SetScalingGovernor) + + get_governor = subparsers.add_parser('get-governor') + get_governor.set_defaults(func=GetScalingGovernor) + + list_governors = subparsers.add_parser('list-governors') + list_governors.set_defaults(func=ListAvailableGovernors) + + args = parser.parse_args(raw_args) + + run_tests_helper.SetLogLevel(args.verbose) + + devil_dynamic_config = devil_env.EmptyConfig() + if args.adb_path: + devil_dynamic_config['dependencies'].update( + devil_env.LocalConfigItem( + 'adb', devil_env.GetPlatform(), args.adb_path)) + devil_env.config.Initialize(configs=[devil_dynamic_config]) + + devices = device_utils.DeviceUtils.HealthyDevices(device_arg=args.devices) + + parallel_devices = device_utils.DeviceUtils.parallel(devices) + parallel_devices.pMap(args.func, args) + + return 0 + + +if __name__ == '__main__': + sys.exit(main(sys.argv[1:])) diff --git a/catapult/devil/devil/android/tools/device_monitor.py b/catapult/devil/devil/android/tools/device_monitor.py index 86a84dd7..6139c770 100755 --- a/catapult/devil/devil/android/tools/device_monitor.py +++ b/catapult/devil/devil/android/tools/device_monitor.py @@ -13,6 +13,7 @@ import argparse import collections import json import logging +import logging.handlers import os import re import sys @@ -133,9 +134,9 @@ def get_device_status(device): for f in files: try: sensor_name = device.ReadFile(f).strip() - temp = device.ReadFile(f[:-4] + 'temp').strip() # s/type^/temp + temp = float(device.ReadFile(f[:-4] + 'temp').strip()) # s/type^/temp status['temp'][sensor_name] = temp - except device_errors.AdbShellCommandFailedError: + except (device_errors.AdbShellCommandFailedError, ValueError): logging.exception('Unable to read thermal sensor %s', f) # Uptime @@ -185,6 +186,15 @@ def main(argv): parser.add_argument('--blacklist-file', help='Path to device blacklist file.') args = parser.parse_args(argv) + logger = logging.getLogger() + logger.setLevel(logging.DEBUG) + handler = logging.handlers.RotatingFileHandler( + '/tmp/device_monitor.log', maxBytes=10 * 1024 * 1024, backupCount=5) + fmt = logging.Formatter('%(asctime)s %(levelname)s %(message)s', + datefmt='%y%m%d %H:%M:%S') + handler.setFormatter(fmt) + logger.addHandler(handler) + devil_dynamic_config = devil_env.EmptyConfig() if args.adb_path: devil_dynamic_config['dependencies'].update( @@ -195,10 +205,15 @@ def main(argv): blacklist = (device_blacklist.Blacklist(args.blacklist_file) if args.blacklist_file else None) + + logging.info('Device monitor running with pid %d, adb: %s, blacklist: %s', + os.getpid(), args.adb_path, args.blacklist_file) while True: + start = time.time() status_dict = get_all_status(blacklist) with open(DEVICE_FILE, 'wb') as f: json.dump(status_dict, f, indent=2, sort_keys=True) + logging.info('Got status of all devices in %.2fs.', time.time() - start) time.sleep(60) diff --git a/catapult/devil/devil/android/tools/device_monitor_test.py b/catapult/devil/devil/android/tools/device_monitor_test.py index 7079cf64..9c597938 100755 --- a/catapult/devil/devil/android/tools/device_monitor_test.py +++ b/catapult/devil/devil/android/tools/device_monitor_test.py @@ -54,7 +54,7 @@ class DeviceMonitorTest(unittest.TestCase): 'device_cereal': { 'processes': 5, 'temp': { - 'CPU-therm': '30' + 'CPU-therm': 30.0 }, 'battery': { 'temperature': 123, @@ -157,6 +157,19 @@ class DeviceMonitorTest(unittest.TestCase): status = device_monitor.get_all_status(blacklist) self.assertEquals(expected_status, status['devices']) + @mock.patch('devil.android.battery_utils.BatteryUtils') + @mock.patch('devil.android.device_utils.DeviceUtils.HealthyDevices') + def test_brokenTempValue(self, get_devices, get_battery): + self.file_contents['/sys/class/thermal/thermal_zone0/temp'] = 'n0t a numb3r' + get_devices.return_value = [self.device] + get_battery.return_value = self.battery + + expected_status_no_temp = self.expected_status.copy() + expected_status_no_temp['device_cereal'].pop('temp') + + status = device_monitor.get_all_status(None) + self.assertEquals(self.expected_status, status['devices']) + if __name__ == '__main__': sys.exit(unittest.main()) diff --git a/catapult/devil/devil/android/tools/device_recovery.py b/catapult/devil/devil/android/tools/device_recovery.py index a7d53338..57857b1e 100755 --- a/catapult/devil/devil/android/tools/device_recovery.py +++ b/catapult/devil/devil/android/tools/device_recovery.py @@ -75,9 +75,14 @@ def RecoverDevice(device, blacklist, should_reboot=lambda device: True): 'Attempting alternative reboot.', str(device)) # The device drops offline before we can grab the exit code, so # we don't check for status. - device.adb.Root() - device.adb.Shell('echo b > /proc/sysrq-trigger', expect_status=None, - timeout=5, retries=0) + try: + device.adb.Root() + finally: + # We are already in a failure mode, attempt to reboot regardless of + # what device.adb.Root() returns. If the sysrq reboot fails an + # exception willbe thrown at that level. + device.adb.Shell('echo b > /proc/sysrq-trigger', expect_status=None, + timeout=5, retries=0) except device_errors.CommandFailedError: logger.exception('Failed to reboot %s.', str(device)) if blacklist: @@ -161,7 +166,7 @@ def RecoverDevices(devices, blacklist, enable_usb_reset=False): device_utils.DeviceUtils.parallel(devices).pMap( RecoverDevice, blacklist, - should_reboot=lambda device: device in should_reboot_device) + should_reboot=lambda device: device.serial in should_reboot_device) def main(): diff --git a/catapult/devil/devil/android/tools/provision_devices.py b/catapult/devil/devil/android/tools/provision_devices.py index 2c4406f0..df75539f 100755 --- a/catapult/devil/devil/android/tools/provision_devices.py +++ b/catapult/devil/devil/android/tools/provision_devices.py @@ -39,6 +39,7 @@ from devil.android import device_utils from devil.android import settings from devil.android.constants import chrome from devil.android.sdk import adb_wrapper +from devil.android.sdk import intent from devil.android.sdk import keyevent from devil.android.sdk import version_codes from devil.android.tools import script_common @@ -87,7 +88,13 @@ def ProvisionDevices( blacklist = (device_blacklist.Blacklist(blacklist_file) if blacklist_file else None) - devices = script_common.GetDevices(devices, blacklist) + try: + devices = script_common.GetDevices(devices, blacklist) + except device_errors.NoDevicesError: + logging.error('No available devices to provision.') + if blacklist: + logging.error('Local device blacklist: %s', blacklist.Read()) + raise devices = [d for d in devices if not emulators or d.adb.is_emulator] parallel_devices = device_utils.DeviceUtils.parallel(devices) @@ -449,6 +456,8 @@ def SetDate(device): _set_and_verify_date, wait_period=1, max_tries=2): raise device_errors.CommandFailedError( 'Failed to set date & time.', device_serial=str(device)) + device.BroadcastIntent( + intent.Intent(action='android.intent.action.TIME_SET')) def LogDeviceProperties(device): @@ -490,25 +499,16 @@ def main(raw_args): parser = argparse.ArgumentParser( description='Provision Android devices with settings required for bots.') parser.add_argument( - '-d', '--device', metavar='SERIAL', action='append', dest='devices', - help='the serial number of the device to be provisioned ' - '(the default is to provision all devices attached)') + '--adb-key-files', type=str, nargs='+', + help='list of adb keys to push to device') parser.add_argument( '--adb-path', help='Absolute path to the adb binary to use.') parser.add_argument('--blacklist-file', help='Device blacklist JSON file.') parser.add_argument( - '--skip-wipe', action='store_true', default=False, - help="don't wipe device data during provisioning") - parser.add_argument( - '--reboot-timeout', metavar='SECS', type=int, - help='when wiping the device, max number of seconds to' - ' wait after each reboot ' - '(default: %s)' % _DEFAULT_TIMEOUTS.HELP_TEXT) - parser.add_argument( - '--min-battery-level', type=int, metavar='NUM', - help='wait for the device to reach this minimum battery' - ' level before trying to continue') + '-d', '--device', metavar='SERIAL', action='append', dest='devices', + help='the serial number of the device to be provisioned ' + '(the default is to provision all devices attached)') parser.add_argument( '--disable-location', action='store_true', help='disable Google location services on devices') @@ -525,24 +525,49 @@ def main(raw_args): parser.add_argument( '--disable-system-chrome', action='store_true', help='Disable the system chrome from devices.') + parser.add_argument( + '--emulators', action='store_true', + help='provision only emulators and ignore usb devices') + parser.add_argument( + '--max-battery-temp', type=int, metavar='NUM', + help='Wait for the battery to have this temp or lower.') + parser.add_argument( + '--min-battery-level', type=int, metavar='NUM', + help='wait for the device to reach this minimum battery' + ' level before trying to continue') + parser.add_argument( + '--output-device-blacklist', + help='Json file to output the device blacklist.') + parser.add_argument( + '--reboot-timeout', metavar='SECS', type=int, + help='when wiping the device, max number of seconds to' + ' wait after each reboot ' + '(default: %s)' % _DEFAULT_TIMEOUTS.HELP_TEXT) parser.add_argument( '--remove-system-webview', action='store_true', help='Remove the system webview from devices.') parser.add_argument( - '--adb-key-files', type=str, nargs='+', - help='list of adb keys to push to device') + '--skip-wipe', action='store_true', default=False, + help="don't wipe device data during provisioning") parser.add_argument( '-v', '--verbose', action='count', default=1, help='Log more information.') + + # No-op arguments for compatibility with build/android/provision_devices.py. + # TODO(jbudorick): Remove these once all callers have stopped using them. parser.add_argument( - '--max-battery-temp', type=int, metavar='NUM', - help='Wait for the battery to have this temp or lower.') + '--chrome-specific-wipe', action='store_true', + help=argparse.SUPPRESS) parser.add_argument( - '--output-device-blacklist', - help='Json file to output the device blacklist.') + '--phase', action='append', + help=argparse.SUPPRESS) parser.add_argument( - '--emulators', action='store_true', - help='provision only emulators and ignore usb devices') + '-r', '--auto-reconnect', action='store_true', + help=argparse.SUPPRESS) + parser.add_argument( + '-t', '--target', + help=argparse.SUPPRESS) + args = parser.parse_args(raw_args) run_tests_helper.SetLogLevel(args.verbose) @@ -573,6 +598,7 @@ def main(raw_args): remove_system_webview=args.remove_system_webview, wipe=not args.skip_wipe) except (device_errors.DeviceUnreachableError, device_errors.NoDevicesError): + logging.exception('Unable to provision local devices.') return exit_codes.INFRA diff --git a/catapult/devil/devil/android/tools/script_common.py b/catapult/devil/devil/android/tools/script_common.py index 8e462c35..f91ad5ee 100644 --- a/catapult/devil/devil/android/tools/script_common.py +++ b/catapult/devil/devil/android/tools/script_common.py @@ -9,11 +9,11 @@ from devil.android import device_utils def GetDevices(requested_devices, blacklist_file): if not isinstance(blacklist_file, device_blacklist.Blacklist): - blacklist = (device_blacklist.Blacklist(blacklist_file) - if blacklist_file - else None) + blacklist_file = (device_blacklist.Blacklist(blacklist_file) + if blacklist_file + else None) - devices = device_utils.DeviceUtils.HealthyDevices(blacklist) + devices = device_utils.DeviceUtils.HealthyDevices(blacklist_file) if not devices: raise device_errors.NoDevicesError() elif requested_devices: diff --git a/catapult/devil/devil/android/tools/wait_for_devices.py b/catapult/devil/devil/android/tools/wait_for_devices.py new file mode 100755 index 00000000..4bde2cd4 --- /dev/null +++ b/catapult/devil/devil/android/tools/wait_for_devices.py @@ -0,0 +1,50 @@ +#!/usr/bin/env python +# Copyright 2016 The Chromium Authors. All rights reserved. +# Use of this source code is governed by a BSD-style license that can be +# found in the LICENSE file. + +"""Waits for the given devices to be available.""" + +import argparse +import os +import sys + +if __name__ == '__main__': + sys.path.append( + os.path.abspath(os.path.join(os.path.dirname(__file__), + '..', '..', '..'))) + +from devil import devil_env +from devil.android import device_utils +from devil.utils import run_tests_helper + + +def main(raw_args): + parser = argparse.ArgumentParser() + parser.add_argument('-v', '--verbose', action='count', help='Log more.') + parser.add_argument('-t', '--timeout', default=30, type=int, + help='Seconds to wait for the devices.') + parser.add_argument('--adb-path', help='ADB binary to use.') + parser.add_argument('device_serials', nargs='*', metavar='SERIAL', + help='Serials of the devices to wait for.') + + args = parser.parse_args(raw_args) + + run_tests_helper.SetLogLevel(args.verbose) + + devil_dynamic_config = devil_env.EmptyConfig() + if args.adb_path: + devil_dynamic_config['dependencies'].update( + devil_env.LocalConfigItem( + 'adb', devil_env.GetPlatform(), args.adb_path)) + devil_env.config.Initialize(configs=[devil_dynamic_config]) + + devices = device_utils.DeviceUtils.HealthyDevices( + device_arg=args.device_serials) + parallel_devices = device_utils.DeviceUtils.parallel(devices) + parallel_devices.WaitUntilFullyBooted(timeout=args.timeout) + return 0 + + +if __name__ == '__main__': + sys.exit(main(sys.argv[1:])) diff --git a/catapult/devil/devil/base_error.py b/catapult/devil/devil/base_error.py index dadf4da2..4b896613 100644 --- a/catapult/devil/devil/base_error.py +++ b/catapult/devil/devil/base_error.py @@ -10,6 +10,13 @@ class BaseError(Exception): super(BaseError, self).__init__(message) self._is_infra_error = is_infra_error + def __eq__(self, other): + return (self.message == other.message + and self.is_infra_error == other.is_infra_error) + + def __ne__(self, other): + return not self == other + @property def is_infra_error(self): """Property to indicate if error was caused by an infrastructure issue.""" diff --git a/catapult/devil/devil/devil_dependencies.json b/catapult/devil/devil/devil_dependencies.json index 5c2b8711..e5522930 100644 --- a/catapult/devil/devil/devil_dependencies.json +++ b/catapult/devil/devil/devil_dependencies.json @@ -91,7 +91,7 @@ "file_info": { "android_arm64-v8a": { "cloud_storage_hash": "4e7d2dedd9c6321fdc152b06869e09a3c5817904", - "download_path": "../bin/deps/andorid/arm64-v8a/bin/md5sum_device" + "download_path": "../bin/deps/android/arm64-v8a/bin/md5sum_device" }, "android_armeabi-v7a": { "cloud_storage_hash": "39fd90af0f8828202b687f7128393759181c5e2e", @@ -124,4 +124,4 @@ } } } -} \ No newline at end of file +} diff --git a/catapult/devil/devil/utils/cmd_helper.py b/catapult/devil/devil/utils/cmd_helper.py index 36fd87be..269be5bb 100644 --- a/catapult/devil/devil/utils/cmd_helper.py +++ b/catapult/devil/devil/utils/cmd_helper.py @@ -236,9 +236,10 @@ def _IterProcessStdout(process, timeout=None, buffer_size=4096, break finally: try: - # Make sure the process doesn't stick around if we fail with an - # exception. - process.kill() + if process.returncode is None: + # Make sure the process doesn't stick around if we fail with an + # exception. + process.kill() except OSError: pass process.wait() diff --git a/catapult/devil/devil/utils/reset_usb.py b/catapult/devil/devil/utils/reset_usb.py index 947c8e29..0335227d 100755 --- a/catapult/devil/devil/utils/reset_usb.py +++ b/catapult/devil/devil/utils/reset_usb.py @@ -6,9 +6,15 @@ import argparse import fcntl import logging +import os import re import sys +if __name__ == '__main__': + sys.path.append( + os.path.abspath(os.path.join(os.path.dirname(__file__), + '..', '..'))) + from devil.android import device_errors from devil.utils import lsusb from devil.utils import run_tests_helper @@ -47,7 +53,8 @@ def reset_android_usb(serial): reset_usb(bus, device) else: raise device_errors.DeviceUnreachableError( - 'Unable to determine bus or device for device %s' % serial) + 'Unable to determine bus(%s) or device(%s) for device %s' + % (bus, device, serial)) def reset_all_android_devices(): diff --git a/catapult/devil/devil/utils/signal_handler.py b/catapult/devil/devil/utils/signal_handler.py index 566bef94..1230f8df 100644 --- a/catapult/devil/devil/utils/signal_handler.py +++ b/catapult/devil/devil/utils/signal_handler.py @@ -6,6 +6,23 @@ import contextlib import signal +@contextlib.contextmanager +def SignalHandler(signalnum, handler): + """Sets the signal handler for the given signal in the wrapped context. + + Args: + signum: The signal for which a handler should be added. + additional_handler: The handler to add. + """ + existing_handler = signal.getsignal(signalnum) + + try: + signal.signal(signalnum, handler) + yield + finally: + signal.signal(signalnum, existing_handler) + + @contextlib.contextmanager def AddSignalHandler(signalnum, additional_handler): """Adds a signal handler for the given signal in the wrapped context. @@ -24,7 +41,8 @@ def AddSignalHandler(signalnum, additional_handler): existing_handler(signum, frame) additional_handler(signum, frame) - signal.signal(signalnum, handler) - yield - signal.signal(signalnum, existing_handler) - + try: + signal.signal(signalnum, handler) + yield + finally: + signal.signal(signalnum, existing_handler) -- cgit v1.2.3