Stop build on failed device status check, apk install and add CheckInstall step.

build/android/buildbot/bb_device_steps.py

Index: build/android/buildbot/bb_device_steps.py
diff --git a/build/android/buildbot/bb_device_steps.py b/build/android/buildbot/bb_device_steps.py
index a1f1b7ff653f877a1ec497bd6e33b8066b236c1e..cca17d39fd8aca2c945c03cd04bd2b1cf2631d42 100755
--- a/build/android/buildbot/bb_device_steps.py
+++ b/build/android/buildbot/bb_device_steps.py
@@ -61,6 +61,7 @@ INSTRUMENTATION_TESTS = dict((suite.name, suite) for suite in [
 
 VALID_TESTS = set(['chromedriver', 'ui', 'unit', 'webkit', 'webkit_layout'])
 
+NUM_CHECK_INSTALL = 3
 
 
 def SpawnCmd(command):
@@ -76,7 +77,7 @@ def SpawnCmd(command):
 
   return subprocess.Popen(command, cwd=CHROME_SRC)
 
-def RunCmd(command, flunk_on_failure=True):
+def RunCmd(command, flunk_on_failure=True, halt_on_failure=False):
   """Run a command relative to the chrome source root."""
   code = SpawnCmd(command).wait()
   print '<', ' '.join(map(pipes.quote, command))
@@ -86,6 +87,9 @@ def RunCmd(command, flunk_on_failure=True):
       buildbot_report.PrintError()
     else:
       buildbot_report.PrintWarning()
+    # Allow steps to have both halting (i.e. 1) and non-halting exit codes.
+    if code == 1 and halt_on_failure:

[Isaac (away), 2013/04/30 20:01:16]

How about if "code != 88"

[navabi, 2013/04/30 21:41:46]

Done (assuming you mean != 88 and != 0).

+      exit(1)

[Isaac (away), 2013/04/30 20:01:16]

How about an exception instead of sys.exit:

raise subprocess.CalledProcessError()

[Isaac (away), 2013/04/30 20:03:38]

actually looks like that exception takes arguments.

raise OSError() would work.

[navabi, 2013/04/30 21:41:46]

Done.

   return code
 
 
@@ -161,6 +165,18 @@ def RunChromeDriverTests():
           '--android-package=%s' % constants.CHROMIUM_TEST_SHELL_PACKAGE])
 
 
+def CheckInstall():
+  """Build bot step to see if adb install works on attached devices. """
+  buildbot_report.PrintNamedStep('Check device install')
+  # This step checks if apks can be installed on the devices. The apk installed
+  # is not important. SDK Controller is checked out as part of the Android SDK.
+  args = ['--apk',
+          'third_party/android_tools/sdk/tools/apps/SdkController/bin/'
+          'SdkControllerApp.apk']
+  for _ in range(NUM_CHECK_INSTALL):
+    RunCmd(['build/android/adb_install_apk.py'] + args, halt_on_failure=True)
+
+
 def InstallApk(options, test, print_step=False):
   """Install an apk to all phones.
 
@@ -175,7 +191,7 @@ def InstallApk(options, test, print_step=False):
   if options.target == 'Release':
     args.append('--release')
 
-  RunCmd(['build/android/adb_install_apk.py'] + args)
+  RunCmd(['build/android/adb_install_apk.py'] + args, halt_on_failure=True)

[navabi, 2013/04/27 00:25:46]

This will make the build stop if an apk install fails. Before any call to
InstallApk (most come from RunInstrumentationSuite), there is a new
CheckInstall() step to make sure APKs can be installed on attached devices.
Thus, that should catch most issues, but this will fail if something goes wrong
between the CheckInstall() step and the step to run the instrumentation tests.

Note: Unit tests do not install via InstallAPK. It installs in
test_package_apk.StripAndCopyExecutable which calls ManagedInstall. Thus,
CheckInstall is still needed as it is a step that is run regardless of what
tests are run.

[Isaac (away), 2013/04/30 20:01:16]

I'm confused why this isn't sufficient -- why do we need the check install?

[navabi, 2013/04/30 21:41:46]

Because not all test suites install apk's with adb_install_apk.py. This is
sufficient for RunInstrumentationSuite, since it calls InstallApk. The unit
tests, for example, do not install the apk using InstallApk (or even
adb_install_apk.py). They install with ManagedInstall in android_commands.py.
Thus, CheckInstall is needed to cover all cases.

[navabi, 2013/04/30 21:43:55]

For more information see the Note on the comment above. It explains the same
thing.

[Isaac (away), 2013/04/30 21:53:17]

That makes sense. Maybe in those cases we could change the test suite to early
terminate if install falls (it may do so already).

Frank, would be OK w/ us changing run_tests to return warning code 88 on
non-fatal errors (i.e. test failures)?

 
 
 def RunInstrumentationSuite(options, test):
@@ -269,13 +285,16 @@ def MainTestWrapper(options):
 
   # Device check and alert emails
   buildbot_report.PrintNamedStep('device_status_check')
-  RunCmd(['build/android/device_status_check.py'])
+  RunCmd(['build/android/device_status_check.py'], halt_on_failure=True)

[Isaac (away), 2013/04/30 21:53:17]

devise_status_check returns error codes on things that should not halt the
build.  Script needs to be changed to return warning code.

Warning code (88) should probably be put in pylib/constants.py.

[navabi, 2013/04/30 21:59:56]

I don't think that is the case. The main function of device_status_check will
either exit normally or return with code 1 if there are no devices attached.

What other error codes may be returned by device_status_check with should not
halt the build?

[Isaac (away), 2013/04/30 22:07:08]

Sorry, yes you're right.  I forgot Siva changed this is a recent CL.

 
   # Provision devices
   buildbot_report.PrintNamedStep('provision_devices')
   target = options.factory_properties.get('target', 'Debug')
   RunCmd(['build/android/provision_devices.py', '-t', target])
 
+  # Check to see if devices can install apks.
+  CheckInstall()
+
   if options.install:
     test_obj = INSTRUMENTATION_TESTS[options.install]
     InstallApk(options, test_obj, print_step=True)