Refactor wipe option to wipe on default and have option to skip wipe.

build/android/provision_devices.py

 #!/usr/bin/env python
 #
 # Copyright (c) 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.
 
 """Provisions Android devices with settings required for bots.
 
 Usage:
   ./provision_devices.py [-d <device serial number>]
@@ skipping 110 matching lines @@
 
 def ProvisionDevices(options):
   # TODO(jbudorick): Parallelize provisioning of all attached devices after
   # swithcing from AndroidCommands.
   if options.device is not None:
     devices = [options.device]
   else:
     devices = android_commands.GetAttachedDevices()
   for device_serial in devices:
     device = device_utils.DeviceUtils(device_serial)
-    device.old_interface.EnableAdbRoot()
-    WipeDeviceData(device)
-  try:
-    device_utils.DeviceUtils.parallel(devices).old_interface.Reboot(True)
-  except errors.DeviceUnresponsiveError:
-    pass
-  for device_serial in devices:
-    device = device_utils.DeviceUtils(device_serial)
     device.WaitUntilFullyBooted(timeout=90)
     device.old_interface.EnableAdbRoot()
     _ConfigureLocalProperties(device)
     device_settings.ConfigureContentSettingsDict(
         device, device_settings.DETERMINISTIC_DEVICE_SETTINGS)
     # TODO(tonyg): We eventually want network on. However, currently radios
     # can cause perfbots to drain faster than they charge.
     if 'perf' in os.environ.get('BUILDBOT_BUILDERNAME', '').lower():
       device_settings.ConfigureContentSettingsDict(
           device, device_settings.NETWORK_DISABLED_SETTINGS)
     device.old_interface.RunShellCommandWithSU('date -u %f' % time.time())
   try:
     device_utils.DeviceUtils.parallel(devices).old_interface.Reboot(True)
   except errors.DeviceUnresponsiveError:
     pass
   for device_serial in devices:
     device = device_utils.DeviceUtils(device_serial)
     device.WaitUntilFullyBooted(timeout=90)
     (_, props) = device.old_interface.GetShellCommandStatusAndOutput('getprop')
     for prop in props:
       print prop
   if options.auto_reconnect:
     PushAndLaunchAdbReboot(devices, options.target)
 
 
 def main(argv):
   logging.basicConfig(level=logging.INFO)
 
   parser = optparse.OptionParser()
-  parser.add_option('-w', '--wipe', action='store_true',
-                    help='Wipe device data from all attached devices.')
+  parser.add_option('--skip-wipe', action='store_true', default=False,

[jbudorick, 2014/06/12 00:17:17]

Do any of the bots use -w or --wipe?

[navabi, 2014/06/12 00:34:50]

No, there shouldn't be. If you look below that was used for manually wiping
devices. I added that before we figured out a way to wipe on every build so that
I had an easy way to wipe when we saw insufficient storage on the devices.

+                    help='Don\'t wipe device data during provisioning.')

[dnj, 2014/06/12 00:35:46]

Just use quotes here; no need to add unnecessary escaping.

[navabi, 2014/06/12 01:08:59]

Done.

   parser.add_option('-d', '--device',
                     help='The serial number of the device to be provisioned')
   parser.add_option('-t', '--target', default='Debug', help='The build target')
   parser.add_option(
       '-r', '--auto-reconnect', action='store_true',
       help='Push binary which will reboot the device on adb disconnections.')
   options, args = parser.parse_args(argv[1:])
   constants.SetBuildType(options.target)
 
   if args:
     print >> sys.stderr, 'Unused args %s' % args
     return 1
 
-  if options.wipe:
+  if not options.skip_wipe:
     devices = android_commands.GetAttachedDevices()

[dnj, 2014/06/12 00:35:46]

The old method had an explicit 'wipe' command that wiped all attached devices
and then quit. This was a single execution; people would run the script
explicitly to wipe devices. The old behavior of './provision_devices.py -w'
currently has no way of being invoked now. To wipe all devices, you must also
provision them.

Additionally, this method will wipe all attached devices even when a single
device is provided '-d'. This violates previous behavior and seems very
undesirable.

Since this script is multi-purposed, this can be recovered by adding
subcommands:
./provision_devices.py wipe-all
./provision_devices.py provision [-d device] [--skip-wipe]

Alternatively, you can re-purpose the script to just be the provision/wipe case
if the 'wipe all w/out provisioning' behavior was never used, but please update
the CL to state this.

Finally, if you do re-purpose the script to remove the 'wipe all w/out
provisioning' behavior, you should still only wipe the device specified with
'-d' if one is specified.

[navabi, 2014/06/12 01:08:59]

Done.

Good catch. The wipe without provisioning should not be necessary anymore after
this change lands. I don't think anyone other than me (and maybe you dnj) ever
used the wipe option. There is no convincing reason to wipe a bot device, which
effectively un-provisions, without re-provisioning the device. Thus, I have
re-purposed the script and fixed it to only wipe and provision the one device if
-d is specified.

     for device_serial in devices:
       device = device_utils.DeviceUtils(device_serial)
+      device.old_interface.EnableAdbRoot()
       WipeDeviceData(device)
     try:
-      (device_utils.DeviceUtils.parallel(devices)
-          .old_interface.Reboot(True).pFinish(None))
+      device_utils.DeviceUtils.parallel(devices).old_interface.Reboot(True)

[dnj, 2014/06/12 00:35:46]

You should block w/ 'pFinish' like the previous code did to make sure they are
all rebooting before you test to see if they're booted. Otherwise, you maybe
could create a race condition where 'WaitUntilFullyBooted' succeeds because the
device hasn't actually started rebooting yet.

[jbudorick, 2014/06/12 00:51:30]

The call to pFinish is vestigial. The call to Reboot will not return before the
devices have started rebooting.

[navabi, 2014/06/12 01:08:59]

Leaving as is.

     except errors.DeviceUnresponsiveError:
       pass
-  else:
-    ProvisionDevices(options)
+    for device_serial in devices:
+      device.WaitUntilFullyBooted(timeout=90)

[dnj, 2014/06/12 00:35:46]

This 'WaitUntilFullyBooted' will be called twice in a row when provisioning. If
this isn't expensive, that's cool, but I'd either remove this block (and let
'ProvisionDevices' handle it) or pass a flag into 'ProvisionDevices' that
indicates that it should not also call 'WaitUntilFullyBooted'.

[navabi, 2014/06/12 01:08:59]

Done. It's not expensive, but I removed the one in ProvisionDevices. I think it
makes more sense to have it here, after the reboot, rather in in the function
which does not necessarily reboot.

+
+  ProvisionDevices(options)
 
 
 if __name__ == '__main__':
   sys.exit(main(sys.argv))