[gms updater] Fixes and tests to prepare activation

build/android/play_services/update.py

 #!/usr/bin/env python
 # Copyright 2015 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 help uploading and downloading the Google Play services client
 library to and from a Google Cloud storage.
 '''
 
@@ skipping 19 matching lines @@
 import download_from_google_storage
 import upload_to_google_storage
 
 
 # Directory where the SHA1 files for the zip and the license are stored
 # It should be managed by git to provided information about new versions.
 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.
 CONFIG_DEFAULT_PATH = os.path.join(constants.DIR_SOURCE_ROOT, 'build',
                                    'android', 'play_services', 'config.json')
 
 LICENSE_FILE_NAME = 'LICENSE'
 LIBRARY_FILE_NAME = 'google_play_services_library.zip'
 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)
   subparsers = parser.add_subparsers(title='commands')
 
   # Download arguments
   parser_download = subparsers.add_parser(
       'download',
       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(
       'sdk',
       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(
       'upload',
       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):
   '''
   Downloads the Google Play services client library from a Google Cloud Storage
   bucket and installs it to
   //third_party/android_tools/sdk/extras/google/google_play_services.
 
   A license check will be made, and the user might have to accept the license
   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')
 
   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)
   bucket_path = _VerifyBucketPathFormat(args.bucket,
                                         config.version_number,
                                         args.dry_run)
 
   tmp_root = tempfile.mkdtemp()
   try:
-    if not os.environ.get('CHROME_HEADLESS'):
-      if not os.path.isdir(paths.package):
-        os.makedirs(paths.package)
+    # 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)
+    # 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):
+    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 '
                         'retry downloading it.', __file__)
         return 0
 
     new_lib_zip = os.path.join(tmp_root, LIBRARY_FILE_NAME)
     _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)
 
     logging.debug('Copying %s to %s', new_license, old_license)
     shutil.copy(new_license, old_license)
 
     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)
 
   return 0
 
 
 def UpdateSdk(args):
   '''
   Uses the Android SDK Manager to update or download the local Google Play
   services library. Its usual installation path is
@@ skipping 188 matching lines @@
       print license_part
       sys.stdout.flush()
 
   # Need to put the prompt on a separate line otherwise the gclient hook buffer
   # only prints it after we received an input.
   print 'Do you accept the license? [y/n]: '
   sys.stdout.flush()
   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
   running in dry run mode or not. Returns a supposedly safe path to use with
   Gsutil.
   '''
 
   if is_dry_run:
     bucket_path = os.path.abspath(os.path.join(bucket_name,
                                                str(version_number)))
@@ skipping 25 matching lines @@
   def call(self, *args):
     logging.debug('Calling command "%s"', str(args))
     return cmd_helper.GetCmdStatusOutputAndError(args)
 
   def check_call(self, *args):
     logging.debug('Calling command "%s"', str(args))
     return cmd_helper.GetCmdStatusOutputAndError(args)
 
 
 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?