[Telemetry] Switch RunShellCommand clients to check_return=True

telemetry/telemetry/internal/platform/android_platform_backend.py

 # Copyright 2013 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
 import os
 import posixpath
 import re
 import subprocess
 import tempfile
@@ skipping 254 matching lines @@
     This can be used to make memory measurements more stable. Requires root.
     """
     if not self._can_elevate_privilege:
       logging.warning('Cannot run purge_ashmem. Requires a rooted device.')
       return
 
     if not android_prebuilt_profiler_helper.InstallOnDevice(
         self._device, 'purge_ashmem'):
       raise Exception('Error installing purge_ashmem.')
     output = self._device.RunShellCommand([
-      android_prebuilt_profiler_helper.GetDevicePath('purge_ashmem')])
+      android_prebuilt_profiler_helper.GetDevicePath('purge_ashmem')],
+      check_return=True)
     for l in output:
       logging.info(l)
 
   @decorators.Deprecated(
       2017, 11, 4,
       'Clients should use tracing and memory-infra in new Telemetry '
       'benchmarks. See for context: https://crbug.com/632021')
   def GetMemoryStats(self, pid):
     memory_usage = self._device.GetMemoryUsageForPid(pid)
     if not memory_usage:
@@ skipping 44 matching lines @@
 
   def FlushEntireSystemCache(self):
     cache = cache_control.CacheControl(self._device)
     cache.DropRamCaches()
 
   def FlushSystemCacheForDirectory(self, directory):
     raise NotImplementedError()
 
   def FlushDnsCache(self):
     self._device.RunShellCommand(
-        ['ndc', 'resolver', 'flushdefaultif'], as_root=True)
+        ['ndc', 'resolver', 'flushdefaultif'], as_root=True, check_return=True)
 
   def StopApplication(self, application):
     """Stop the given |application|.
 
     Args:
        application: The full package name string of the application to stop.
     """
     self._device.ForceStop(application)
 
   def KillApplication(self, application):
@@ skipping 11 matching lines @@
       self, application, parameters=None, elevate_privilege=False):
     """Launches the given |application| with a list of |parameters| on the OS.
 
     Args:
       application: The full package name string of the application to launch.
       parameters: A list of parameters to be passed to the ActivityManager.
       elevate_privilege: Currently unimplemented on Android.
     """
     if elevate_privilege:
       raise NotImplementedError("elevate_privilege isn't supported on android.")
+    # TODO(catapult:#3215): Migrate to StartActivity.
     cmd = ['am', 'start']
     if parameters:
       cmd.extend(parameters)
     cmd.append(application)
-    result_lines = self._device.RunShellCommand(cmd)
+    result_lines = self._device.RunShellCommand(cmd, check_return=True)
     for line in result_lines:
       if line.startswith('Error: '):
         raise ValueError('Failed to start "%s" with error\n  %s' %
                          (application, line))
 
   def IsApplicationRunning(self, application):
     return len(self._device.GetPids(application)) > 0
 
   def CanLaunchApplication(self, application):
     if not self._installed_applications:
       self._installed_applications = self._device.RunShellCommand(
-          ['pm', 'list', 'packages'])
+          ['pm', 'list', 'packages'], check_return=True)
     return 'package:' + application in self._installed_applications
 
   def InstallApplication(self, application):
     self._installed_applications = None
     self._device.Install(application)
 
   @decorators.Cache
   def CanCaptureVideo(self):
     return self.GetOSVersionName() >= 'K'
 
@@ skipping 47 matching lines @@
 
   def GetFileContents(self, fname):
     if not self._can_elevate_privilege:
       logging.warning('%s cannot be retrieved on non-rooted device.', fname)
       return ''
     return self._device.ReadFile(fname, as_root=True)
 
   def GetPsOutput(self, columns, pid=None):
     assert columns == ['pid', 'name'] or columns == ['pid'], \
         'Only know how to return pid and name. Requested: ' + columns
+    # TODO(catapult:#3215): Migrate to GetPids.
     cmd = ['ps']
     if pid:
       cmd.extend(['-p', str(pid)])
-    ps = self._device.RunShellCommand(cmd, large_output=True)[1:]
+    ps = self._device.RunShellCommand(
+        cmd, check_return=True, large_output=True)[1:]
     output = []
     for line in ps:
       data = line.split()
       curr_pid = data[1]
       curr_name = data[-1]
       if columns == ['pid', 'name']:
         output.append([curr_pid, curr_name])
       else:
         output.append([curr_pid])
     return output
 
   def RunCommand(self, command):
-    return '\n'.join(self._device.RunShellCommand(command))
+    return self._device.RunShellCommand(
+        command, check_return=True, raw_output=True)
 
   @staticmethod
   def ParseCStateSample(sample):
     sample_stats = {}
     for cpu in sample:
       values = sample[cpu].splitlines()
       # Each state has three values after excluding the time value.
       num_states = (len(values) - 1) / 3
       names = values[:num_states]
       times = values[num_states:2 * num_states]
@@ skipping 100 matching lines @@
     # the base name; so we now need to get the base name from the directory.
     if not profile_base:
       profile_base = os.path.basename(profile_parent)
 
     saved_profile_location = '/sdcard/profile/%s' % profile_base
     self._device.PushChangedFiles([(new_profile_dir, saved_profile_location)])
 
     profile_dir = self._GetProfileDir(package)
     self._EfficientDeviceDirectoryCopy(
         saved_profile_location, profile_dir)
-    dumpsys = self._device.RunShellCommand(['dumpsys', 'package', package])
+    dumpsys = self._device.RunShellCommand(
+        ['dumpsys', 'package', package], check_return=True)
     id_line = next(line for line in dumpsys if 'userId=' in line)
     uid = re.search(r'\d+', id_line).group()
     files = self._device.ListDirectory(profile_dir, as_root=True)
     paths = [posixpath.join(profile_dir, f) for f in files if f != 'lib']
     for path in paths:
+      # TODO(crbug.com/628617): Implement without ignoring shell errors.
       # Note: need to pass command as a string for the shell to expand the *'s.
       extended_path = '%s %s/* %s/*/* %s/*/*/*' % (path, path, path, path)
       self._device.RunShellCommand(
-          'chown %s.%s %s' % (uid, uid, extended_path))
+          'chown %s.%s %s' % (uid, uid, extended_path), check_return=False)
 
   def _EfficientDeviceDirectoryCopy(self, source, dest):
     if not self._device_copy_script:
       self._device.adb.Push(
           _DEVICE_COPY_SCRIPT_FILE,
           _DEVICE_COPY_SCRIPT_LOCATION)
-      self._device_copy_script = _DEVICE_COPY_SCRIPT_FILE
+      self._device_copy_script = _DEVICE_COPY_SCRIPT_LOCATION
     self._device.RunShellCommand(
-        ['sh', self._device_copy_script, source, dest])
+        ['sh', self._device_copy_script, source, dest], check_return=True)
 
   def GetPortPairForForwarding(self, local_port):
     return forwarders.PortPair(local_port=local_port, remote_port=0)
 
   def RemoveProfile(self, package, ignore_list):
     """Delete application profile on device.
 
     Args:
       package: The full package name string of the application for which the
         profile is to be deleted.
       ignore_list: List of files to keep.
     """
     profile_dir = self._GetProfileDir(package)
     if not self._device.PathExists(profile_dir):
       return
     files = [
       posixpath.join(profile_dir, f)
       for f in self._device.ListDirectory(profile_dir, as_root=True)
       if f not in ignore_list]
     if not files:
       return
-    cmd = ['rm', '-r']
-    cmd.extend(files)
-    self._device.RunShellCommand(cmd, as_root=True, check_return=True)
+    self._device.RemovePath(files, recursive=True, as_root=True)
 
   def PullProfile(self, package, output_profile_path):
     """Copy application profile from device to host machine.
 
     Args:
       package: The full package name string of the application for which the
         profile is to be copied.
       output_profile_dir: Location where profile to be stored on host machine.
     """
     profile_dir = self._GetProfileDir(package)
     logging.info("Pulling profile directory from device: '%s'->'%s'.",
                  profile_dir, output_profile_path)
     # To minimize bandwidth it might be good to look at whether all the data
     # pulled down is really needed e.g. .pak files.
     if not os.path.exists(output_profile_path):
       os.makedirs(output_profile_path)
-    files = self._device.ListDirectory(profile_dir, as_root=True)
-    for f in files:
+    problem_files = []
+    for filename in self._device.ListDirectory(profile_dir, as_root=True):
       # Don't pull lib, since it is created by the installer.
-      if f != 'lib':
-        source = posixpath.join(profile_dir, f)
-        dest = os.path.join(output_profile_path, f)
-        try:
-          self._device.PullFile(source, dest, timeout=240)
-        except device_errors.CommandFailedError:
-          logging.exception('Failed to pull %s to %s', source, dest)
+      if filename == 'lib':
+        continue
+      source = posixpath.join(profile_dir, filename)
+      dest = os.path.join(output_profile_path, filename)
+      try:
+        self._device.PullFile(source, dest, timeout=240)
+      except device_errors.CommandFailedError:
+        problem_files.append(source)
+    if problem_files:
+      # Some paths (e.g. 'files', 'app_textures') consistently fail to be
+      # pulled from the device.
+      logging.warning(
+          'There were errors retrieving the following paths from the profile:')
+      for filepath in problem_files:
+        logging.warning('- %s', filepath)
 
   def _GetProfileDir(self, package):
     """Returns the on-device location where the application profile is stored
     based on Android convention.
 
     Args:
       package: The full package name string of the application.
     """
     return '/data/data/%s/' % package
 
   def SetDebugApp(self, package):
     """Set application to debugging.
 
     Args:
       package: The full package name string of the application.
     """
     if self._device.IsUserBuild():
       logging.debug('User build device, setting debug app')
       self._device.RunShellCommand(
-          ['am', 'set-debug-app', '--persistent', package])
+          ['am', 'set-debug-app', '--persistent', package],
+          check_return=True)
 
   def GetLogCat(self, number_of_lines=500):
     """Returns most recent lines of logcat dump.
 
     Args:
       number_of_lines: Number of lines of log to return.
     """
     def decode_line(line):
       try:
         uline = unicode(line, encoding='utf-8')
@@ skipping 95 matching lines @@
     # Use linux instead of Android because when determining what tests to run on
     # a bot the individual device could be down, which would make BattOr tests
     # not run on any device. BattOrs communicate with the host and not android
     # devices.
     return battor_wrapper.IsBattOrConnected('linux')
 
   def Log(self, message):
     """Prints line to logcat."""
     TELEMETRY_LOGCAT_TAG = 'Telemetry'
     self._device.RunShellCommand(
-        ['log', '-p', 'i', '-t', TELEMETRY_LOGCAT_TAG, message])
+        ['log', '-p', 'i', '-t', TELEMETRY_LOGCAT_TAG, message],
+        check_return=True)
 
   def WaitForTemperature(self, temp):
     # Temperature is in tenths of a degree C, so we convert to that scale.
     self._battery.LetBatteryCoolToTemperature(temp * 10)
 
 def _FixPossibleAdbInstability():
   """Host side workaround for crbug.com/268450 (adb instability).
 
   The adb server has a race which is mitigated by binding to a single core.
   """
   if not psutil:
     return
   for process in psutil.process_iter():
     try:
       if psutil.version_info >= (2, 0):
         if 'adb' in process.name():
           process.cpu_affinity([0])
       else:
         if 'adb' in process.name:
           process.set_cpu_affinity([0])
     except (psutil.NoSuchProcess, psutil.AccessDenied):
       logging.warn('Failed to set adb process CPU affinity')