Implemented retries in download_sdk_extras.py

build/download_sdk_extras.py

Index: build/download_sdk_extras.py
diff --git a/build/download_sdk_extras.py b/build/download_sdk_extras.py
index d7c5d6cad6c9eec10690644bf7ba485189e3f287..5bcb069a44d085abd3f73fa48a2632094bfc3709 100755
--- a/build/download_sdk_extras.py
+++ b/build/download_sdk_extras.py
@@ -45,6 +45,22 @@ def clean_and_extract(dir_name, package_name, zip_file):
     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.
@@ -54,17 +70,26 @@ def main():
     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__':