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

build/android/provision_devices.py

Index: build/android/provision_devices.py
diff --git a/build/android/provision_devices.py b/build/android/provision_devices.py
index d282737f0a177a4a095eddc9bf7973fde1a40b19..192967d73c096e96e2369864e9db3e921b98d42b 100755
--- a/build/android/provision_devices.py
+++ b/build/android/provision_devices.py
@@ -128,14 +128,6 @@ def ProvisionDevices(options):
     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)
@@ -165,8 +157,8 @@ 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')
@@ -180,18 +172,20 @@ def main(argv):
     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__':