[gms updater] Fixes and tests to prepare activation

build/android/play_services/update.py

Index: build/android/play_services/update.py
diff --git a/build/android/play_services/update.py b/build/android/play_services/update.py
index b2d57ad359f49c7c271246d4f0b629b332b551d0..f2bec07b372569dbf56c908c9e0200dc3d69484b 100755
--- a/build/android/play_services/update.py
+++ b/build/android/play_services/update.py
@@ -37,7 +37,7 @@ SHA1_DIRECTORY = os.path.join(constants.DIR_SOURCE_ROOT, 'build', 'android',
                               'play_services')
 
 # Default bucket used for storing the files.
-GMS_CLOUD_STORAGE = 'chrome-sdk-extras'
+GMS_CLOUD_STORAGE = 'chromium-android-tools/play-services'
 
 # Path to the default configuration file. It exposes the currently installed
 # version of the library in a human readable way.
@@ -51,7 +51,7 @@ GMS_PACKAGE_ID = 'extra-google-google_play_services'  # used by sdk manager
 LICENSE_PATTERN = re.compile(r'^Pkg\.License=(?P<text>.*)$', re.MULTILINE)
 
 
-def Main():
+def Main(raw_args):

[jbudorick, 2015/11/04 16:56:32]

nit: missed this before, but s/Main/main/

[dgn, 2015/11/04 18:43:58]

Done.

   parser = argparse.ArgumentParser(
       description=__doc__ + 'Please see the subcommand help for more details.',
       formatter_class=utils.DefaultsRawHelpFormatter)
@@ -63,12 +63,9 @@ def Main():
       help='download the library from the cloud storage',
       description=Download.__doc__,
       formatter_class=utils.DefaultsRawHelpFormatter)
-  parser_download.add_argument('-f', '--force',
-                               action='store_true',
-                               help=('run even if the local version is '
-                                     'already up to date'))
   parser_download.set_defaults(func=Download)
-  AddCommonArguments(parser_download)
+  AddArguments(parser_download, bucket=True, config=True, dry_run=True,
+                     force=True, sdk_root=True, verbose=True)

[jbudorick, 2015/11/04 16:56:32]

nit: indentation

[dgn, 2015/11/04 18:43:58]

Done.

 
   # SDK Update arguments
   parser_sdk = subparsers.add_parser(
@@ -76,14 +73,8 @@ def Main():
       help='update the local sdk using the Android SDK Manager',
       description=UpdateSdk.__doc__,
       formatter_class=utils.DefaultsRawHelpFormatter)
-  parser_sdk.add_argument('--sdk-root',
-                          help=('base path to the Android SDK tools to use to '
-                                'update the library'),
-                          default=constants.ANDROID_SDK_ROOT)
-  parser_sdk.add_argument('-v', '--verbose',
-                          action='store_true',
-                          help='print debug information')
   parser_sdk.set_defaults(func=UpdateSdk)
+  AddArguments(parser_sdk, verbose=True, sdk_root=True)
 
   # Upload arguments
   parser_upload = subparsers.add_parser(
@@ -91,49 +82,63 @@ def Main():
       help='upload the library to the cloud storage',
       description=Upload.__doc__,
       formatter_class=utils.DefaultsRawHelpFormatter)
-  parser_upload.add_argument('-f', '--force',
-                             action='store_true',
-                             help=('run even if the checked in version is '
-                                   'already up to date'))
-  parser_upload.add_argument('--sdk-root',
-                             help=('base path to the Android SDK tools to use '
-                                   'to update the library'),
-                             default=constants.ANDROID_SDK_ROOT)
+
   parser_upload.add_argument('--skip-git',
                              action='store_true',
                              help="don't commit the changes at the end")
   parser_upload.set_defaults(func=Upload)
-  AddCommonArguments(parser_upload)
+  AddArguments(parser_upload, bucket=True, config=True, dry_run=True,
+                     force=True, sdk_root=True, verbose=True)

[jbudorick, 2015/11/04 16:56:32]

nit: indentation

[dgn, 2015/11/04 18:43:58]

Done.

 
-  args = parser.parse_args()
+  args = parser.parse_args(raw_args)
   if args.verbose:
     logging.basicConfig(level=logging.DEBUG)
-  logging_utils.ColorStreamHandler.MakeDefault()
+  logging_utils.ColorStreamHandler.MakeDefault(not _IsBotEnvironment())
   return args.func(args)
 
-
-def AddCommonArguments(parser):
+def AddArguments(parser, bucket=False, config=False, dry_run=False,

[jbudorick, 2015/11/04 16:56:32]

Split this into two functions without all of these flags: one that adds bucket,
config, dry_run, and force, and one the adds verbose and sdk_root. Call both for
parser_upload and parser_download, and call only the latter for parser_sdk.

[dgn, 2015/11/04 18:43:58]

Done.

+                       force=False, sdk_root=False, verbose=False):

[jbudorick, 2015/11/04 16:56:32]

nit: indentation

[dgn, 2015/11/04 18:43:58]

Done.

   '''
   Defines the common arguments on subparser rather than the main one. This
   allows to put arguments after the command: `foo.py upload --debug --force`
   instead of `foo.py --debug upload --force`
   '''
 
-  parser.add_argument('--bucket',
-                      help='name of the bucket where the files are stored',
-                      default=GMS_CLOUD_STORAGE)
-  parser.add_argument('--config',
-                      help='JSON Configuration file',
-                      default=CONFIG_DEFAULT_PATH)
-  parser.add_argument('--dry-run',
-                      action='store_true',
-                      help=('run the script in dry run mode. Files will be '
-                            'copied to a local directory instead of the cloud '
-                            'storage. The bucket name will be as path to that '
-                            'directory relative to the repository root.'))
-  parser.add_argument('-v', '--verbose',
-                      action='store_true',
-                      help='print debug information')
+  if bucket:
+    parser.add_argument('--bucket',
+                        help='name of the bucket where the files are stored',
+                        default=GMS_CLOUD_STORAGE)
+
+  if config:
+    parser.add_argument('--config',
+                        help='JSON Configuration file',
+                        default=CONFIG_DEFAULT_PATH)
+
+  if dry_run:
+    parser.add_argument('--dry-run',
+                        action='store_true',
+                        help=('run the script in dry run mode. Files will be '
+                              'copied to a local directory instead of the '
+                              'cloud storage. The bucket name will be as path '
+                              'to that directory relative to the repository '
+                              'root.'))
+
+  if force:
+    parser.add_argument('-f', '--force',
+                               action='store_true',
+                               help=('run even if the library is already '
+                                     ' up to date'))
+
+  if sdk_root:
+    parser.add_argument('--sdk-root',
+                         help=('base path to the Android SDK tools to use when'
+                               'updating the library'),
+                         default=constants.ANDROID_SDK_ROOT)
+
+  if verbose:
+    parser.add_argument('-v', '--verbose',
+                        action='store_true',
+                        help='print debug information')
 
 
 def Download(args):
@@ -146,7 +151,14 @@ def Download(args):
   if that has not been done before.
   '''
 
-  paths = _InitPaths(constants.ANDROID_SDK_ROOT)
+  if not os.path.isdir(args.sdk_root):
+    logging.debug('Did not find the android sdk directory at "%s".',
+                  args.sdk_root)
+    if not args.force:
+      logging.info('Skipping, not on an android checkout.')
+      return 0
+
+  paths = _InitPaths(args.sdk_root)
 
   new_lib_zip_sha1 = os.path.join(SHA1_DIRECTORY, LIBRARY_FILE_NAME + '.sha1')
   old_lib_zip_sha1 = os.path.join(paths.package, LIBRARY_FILE_NAME + '.sha1')
@@ -154,7 +166,7 @@ def Download(args):
   logging.debug('Comparing library hashes: %s and %s', new_lib_zip_sha1,
                 old_lib_zip_sha1)
   if utils.FileEquals(new_lib_zip_sha1, old_lib_zip_sha1) and not args.force:
-    logging.debug('The Google Play services library is up to date.')
+    logging.info('Skipping, the Google Play services library is up to date.')
     return 0
 
   config = utils.ConfigParser(args.config)
@@ -164,18 +176,20 @@ def Download(args):
 
   tmp_root = tempfile.mkdtemp()
   try:
-    if not os.environ.get('CHROME_HEADLESS'):
-      if not os.path.isdir(paths.package):
-        os.makedirs(paths.package)
-
-      # download license file from bucket/{version_number}/license.sha1
-      new_license = os.path.join(tmp_root, LICENSE_FILE_NAME)
-      old_license = os.path.join(paths.package, LICENSE_FILE_NAME)
-
-      license_sha1 = os.path.join(SHA1_DIRECTORY, LICENSE_FILE_NAME + '.sha1')
-      _DownloadFromBucket(bucket_path, license_sha1, new_license,
-                          args.verbose, args.dry_run)
-      if not _CheckLicenseAgreement(new_license, old_license):
+    # setup the destination directory
+    if not os.path.isdir(paths.package):
+      os.makedirs(paths.package)
+
+    # download license file from bucket/{version_number}/license.sha1
+    new_license = os.path.join(tmp_root, LICENSE_FILE_NAME)
+    old_license = os.path.join(paths.package, LICENSE_FILE_NAME)
+
+    license_sha1 = os.path.join(SHA1_DIRECTORY, LICENSE_FILE_NAME + '.sha1')
+    _DownloadFromBucket(bucket_path, license_sha1, new_license,

[jbudorick, 2015/11/04 16:56:32]

This is now downloading the license on the bot where it wasn't previously. I
don't envision that being a problem, but was that intentional?

[dgn, 2015/11/04 18:43:58]

Yes. It failed at some point where the paths were not initialized before. I
could have put an empty path there instead, but I figured that reducing the
differences between the 2 codepaths as much as possible would not be bad

+                        args.verbose, args.dry_run)
+
+    if (not _IsBotEnvironment() and
+        not _CheckLicenseAgreement(new_license, old_license)):
         logging.warning('Your version of the Google Play services library is '
                         'not up to date. You might run into issues building '
                         'or running the app. Please run `%s download` to '
@@ -186,11 +200,17 @@ def Download(args):
     _DownloadFromBucket(bucket_path, new_lib_zip_sha1, new_lib_zip,
                         args.verbose, args.dry_run)
 
-    # We remove only the library itself. Users having a SDK manager installed
+    # We remove the library itself. Users having a SDK manager installed
     # library before will keep the documentation and samples from it.
     shutil.rmtree(paths.lib, ignore_errors=True)
     os.makedirs(paths.lib)
 
+    # We also remove source.properties so that the SDK manager doesn't fail
+    # recognizing that it didn't install the local version.
+    source_prop = os.path.join(paths.package, 'source.properties')
+    if os.path.isfile(source_prop):
+      os.remove(source_prop)
+
     logging.debug('Extracting the library to %s', paths.lib)
     with zipfile.ZipFile(new_lib_zip, "r") as new_lib_zip_file:
       new_lib_zip_file.extractall(paths.lib)
@@ -201,6 +221,7 @@ def Download(args):
     logging.debug('Copying %s to %s', new_lib_zip_sha1, old_lib_zip_sha1)
     shutil.copy(new_lib_zip_sha1, old_lib_zip_sha1)
 
+    logging.info('Update complete.')
   finally:
     shutil.rmtree(tmp_root)
 
@@ -409,6 +430,10 @@ def _CheckLicenseAgreement(expected_license_path, actual_license_path):
   return raw_input('> ') in ('Y', 'y')
 
 
+def _IsBotEnvironment():
+  return bool(os.environ.get('CHROME_HEADLESS'))
+
+
 def _VerifyBucketPathFormat(bucket_name, version_number, is_dry_run):
   '''
   Formats and checks the download/upload path depending on whether we are
@@ -454,4 +479,4 @@ class DummyGsutil(download_from_google_storage.Gsutil):
 
 
 if __name__ == '__main__':
-  sys.exit(Main())
+  sys.exit(Main(sys.argv[1:]))

[jbudorick, 2015/11/04 16:56:32]

don't pass anything to Main, and don't pass anything to parse_args.

[dgn, 2015/11/04 18:43:58]

This is to allow providing arguments in the tests. should I make it the default
value of the parameter instead?