merge run and check-rc steps

infra/bots/recipe_modules/flavor/gn_android_flavor.py

 # 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.
 
 import default_flavor
 import subprocess
 
 # Data should go under in _data_dir, which may be preserved across runs.
 _data_dir = '/sdcard/revenge_of_the_skiabot/'
 # Executables go under _bin_dir, which, well, allows executable files.
@@ skipping 19 matching lines @@
 
   def _run(self, title, *cmd, **kwargs):
     self.m.vars.default_env = {k: v for (k,v)
                                in self.m.vars.default_env.iteritems()
                                if k in ['PATH']}
     return self.m.run(self.m.step, title, cmd=list(cmd),
                       cwd=self.m.vars.skia_dir, env={}, **kwargs)
 
   def _adb(self, title, *cmd, **kwargs):
     self._ever_ran_adb = True
-    # The only non-infra adb call (check rc) happens to not use _adb().
+    # The only non-infra adb steps (dm / nanobench) happen to not use _adb().
     if 'infra_step' not in kwargs:
       kwargs['infra_step'] = True
     return self._run(title, 'adb', *cmd, **kwargs)
 
   def compile(self, unused_target, **kwargs):
     compiler      = self.m.vars.builder_cfg.get('compiler')
     configuration = self.m.vars.builder_cfg.get('configuration')
     extra_config  = self.m.vars.builder_cfg.get('extra_config', '')
     os            = self.m.vars.builder_cfg.get('os')
     target_arch   = self.m.vars.builder_cfg.get('target_arch')
@@ skipping 20 matching lines @@
               infra_step=True)
     self._run('gn gen', 'gn', 'gen', self.out_dir, '--args=' + gn_args)
     self._run('ninja', 'ninja', '-C', self.out_dir)
 
   def install(self):
     self._adb('mkdir ' + self.device_dirs.resource_dir,
               'shell', 'mkdir', '-p', self.device_dirs.resource_dir)
 
   def cleanup_steps(self):
     if self._ever_ran_adb:
+      self._adb('dump log ', 'logcat', '-d')

[borenet, 2016/09/14 16:52:24]

Nit: trailing space?

[mtklein_C, 2016/09/14 17:30:31]

Done.

       self._adb('reboot', 'reboot')
       self._adb('kill adb server', 'kill-server')
 
   def step(self, name, cmd, env=None, **kwargs):
     app = self.m.vars.skia_out.join(self.m.vars.configuration, cmd[0])
     self._adb('push %s' % cmd[0],
               'push', app, _bin_dir)
 
     sh = '%s.sh' % cmd[0]
     self.m.run.writefile(self.m.vars.tmp_dir.join(sh),
         'set -x; %s%s; echo $? >%src' %
         (_bin_dir, subprocess.list2cmdline(map(str, cmd)), _bin_dir))
     self._adb('push %s' % sh,
               'push', self.m.vars.tmp_dir.join(sh), _bin_dir)
 
     self._adb('clear log', 'logcat', '-c')
-    self._adb(cmd[0], 'shell', 'sh', _bin_dir + sh)
-    self._adb('dump log ', 'logcat', '-d')
-
-    self.m.python.inline('check %s rc' % cmd[0], """
+    self.m.python.inline('%s' % cmd[0], """
     import subprocess
     import sys
+    bin_dir = sys.argv[1]
+    sh      = sys.argv[2]
+    subprocess.check_call(['adb', 'shell', 'sh', bin_dir + sh])
     sys.exit(int(subprocess.check_output(['adb', 'shell', 'cat',
-                                          '%src'])))
-    """ % _bin_dir)
+                                          bin_dir + 'rc'])))

[borenet, 2016/09/14 16:52:24]

Please break this into a few lines and add a helpful error message, eg:

try:
  output = subprocess.check_output(['adb', 'shell', 'cat', bin_dir + 'rc'])
except:
  print 'Failed to obtain return code from file'
  sys.exit(1)
try:
  rc = int(output)
except:
  print 'Got non-integer return code: "%s"' % output
  sys.exit(1)
sys.exit(rc)

[mtklein_C, 2016/09/14 17:30:31]

Done, mostly.  We don't expect this 'adb shell ...' to fail any more or less
than the others, so I didn't check for CalledProcessError, but I did add a check
here for ValueError.

[borenet, 2016/09/14 17:44:35]

Thanks. Mainly just want to distinguish between "thing failed" and "couldn't
read return code", plus having the python stacktrace at the end of the DM log is
kind of unintelligible.

+    """, args=[_bin_dir, sh])
 
   def copy_file_to_device(self, host, device):
     self._adb('push %s %s' % (host, device), 'push', host, device)
 
   def copy_directory_contents_to_device(self, host, device):
     # Copy the tree, avoiding hidden directories and resolving symlinks.
     self.m.python.inline('push %s/* %s' % (host, device), """
     import os
     import subprocess
     import sys
@@ skipping 16 matching lines @@
   def read_file_on_device(self, path):
     return self._adb('read %s' % path,
                      'shell', 'cat', path, stdout=self.m.raw_io.output()).stdout
 
   def remove_file_on_device(self, path):
     self._adb('rm %s' % path, 'shell', 'rm', '-f', path)
 
   def create_clean_device_dir(self, path):
     self._adb('rm %s' % path, 'shell', 'rm', '-rf', path)
     self._adb('mkdir %s' % path, 'shell', 'mkdir', '-p', path)