Implemented retries in download_sdk_extras.py

build/download_sdk_extras.py

 #!/usr/bin/env python
 # Copyright 2014 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.
 
 """Script to download sdk/extras packages on the bots from google storage.
 
 The script expects arguments that specify zips file in the google storage
 bucket named: <dir in SDK extras>_<package name>_<version>.zip. The file will
 be extracted in the android_tools/sdk/extras directory on the test bots. This
@@ skipping 27 matching lines @@
 
 def clean_and_extract(dir_name, package_name, zip_file):
   local_dir = '%s/%s/%s' % (SDK_EXTRAS_PATH, dir_name, package_name)
   if os.path.exists(local_dir):
     shutil.rmtree(local_dir)
   local_zip = '%s/%s' % (SDK_EXTRAS_PATH, zip_file)
   with zipfile.ZipFile(local_zip) as z:
     z.extractall(path=SDK_EXTRAS_PATH)
 
 
+def download_package(remote_file, local_file):
+  """Download a file from GCS.
+
+  Returns:
+    success (bool): True if the download succeeded, False otherwise.
+  """
+  if not os.path.exists(local_file):

[Dirk Pranke, 2015/07/31 00:35:59]

it's confusing to me that a routine called download_package() doesn't
unconditionally download something.

Can you either pull the os.path.exists() check outside of the function, or
rename this to download_package_if_needed() or some such?

+    try:
+      subprocess.check_call(['python', GSUTIL_PATH, '--force-version', '4.7',
+                             'cp', remote_file, local_file])
+    except subprocess.CalledProcessError:
+      print ('WARNING: Failed to download SDK packages. If this bot compiles '
+             'for Android, it may have errors.')
+      return False
+  return True
+
 def main():
   if not os.environ.get('CHROME_HEADLESS'):
     # This is not a buildbot checkout.
     return 0
   # Update the android_sdk_extras.json file to update downloaded packages.
   with open(SDK_EXTRAS_JSON_FILE) as json_file:
     packages = json.load(json_file)
   for package in packages:
     local_zip = '%s/%s' % (SDK_EXTRAS_PATH, package['zip'])
-    if not os.path.exists(local_zip):
-      package_zip = '%s/%s' % (SDK_EXTRAS_BUCKET, package['zip'])
+    package_zip = '%s/%s' % (SDK_EXTRAS_BUCKET, package['zip'])
+    for attempt in xrange(2):
+      print '(%d) Downloading package %s' % (attempt + 1, package['zip'])
+      if not download_package(package_zip, local_zip):
+        return 0  # hiding downloading failures on purpose.

[Dirk Pranke, 2015/07/31 00:35:59]

I don't know what "hiding" means in this context, and this comment doesn't tell
me *why* we want to do this. Can you be more verbose?

       try:
-        subprocess.check_call(['python', GSUTIL_PATH, '--force-version', '4.7',
-                               'cp', package_zip, local_zip])
-      except subprocess.CalledProcessError:
-        print ('WARNING: Failed to download SDK packages. If this bot compiles '
-               'for Android, it may have errors.')
-        return 0
-    # Always clean dir and extract zip to ensure correct contents.
-    clean_and_extract(package['dir_name'], package['package'], package['zip'])
+        # Always clean dir and extract zip to ensure correct contents.
+        clean_and_extract(package['dir_name'],
+                          package['package'],
+                          package['zip'])
+        break
+      except zipfile.BadZipfile:
+        print 'Failed unpacking zip file. Deleting and retrying...'
+        os.remove(local_zip)
+        continue

[Dirk Pranke, 2015/07/31 00:35:59]

this continue is unnecessary, no?

+
+    else:
+      print ('WARNING: Failed to unpack SDK packages. If this bot compiles '
+             'for Android, it may have errors.')
+      return 1
 
 
 if __name__ == '__main__':
   sys.exit(main())