[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..4f67cba901a77876d35308042ca3fecd4c82b166 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):
   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)
+  AddBasicArguments(parser_download)
+  AddBucketArguments(parser_download)
 
   # 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)
+  AddBasicArguments(parser_sdk)
 
   # Upload arguments
   parser_upload = subparsers.add_parser(
@@ -91,49 +82,59 @@ 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)
+  AddBasicArguments(parser_upload)
+  AddBucketArguments(parser_upload)
 
-  args = parser.parse_args()
+  args = parser.parse_args(raw_args)

[jbudorick, 2015/11/04 18:46:07]

parse_args() will pick up sys.argv[1:] and parse those by default, so the only
reason to pass something to parse_args is if you're doing something unusual
(which doesn't appear to be the case here).

[dgn, 2015/11/05 12:15:10]

It's the case in update_test.py, where I call main() with command line
arguments:
https://codereview.chromium.org/1418833005/diff/140001/build/android/play_ser...

I could call the underlying functions directly, but I preferred this way since
it tests the argument parsing, is easier to write (avoid having to rebuild the
args object), and I believe is also more useful for demo/doc purposes. Should I
change that?

[jbudorick, 2015/11/05 16:03:02]

oooooh I missed that. This is fine, then.

   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 AddBasicArguments(parser):
   '''
   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('--sdk-root',
+                      help=('base path to the Android SDK tools to use when'
+                            'updating the library'),
+                      default=constants.ANDROID_SDK_ROOT)
+
+  parser.add_argument('-v', '--verbose',
+                      action='store_true',
+                      help='print debug information')
+
+
+def AddBucketArguments(parser):
   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',
+                            '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('-f', '--force',
                       action='store_true',
-                      help='print debug information')
+                      help=('run even if the library is already '
+                            ' up to date'))
 
 
 def Download(args):
@@ -146,7 +147,20 @@ 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)
+
+  if os.path.isdir(paths.package) and not os.access(paths.package, os.W_OK):
+    logging.error('Failed updating the Google Play Services library. '
+                  'The location is not writable. Please remove the '
+                  'directory (%s) and try again.', paths.package)
+    return -2
 
   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 +168,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 +178,21 @@ 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,
+                        args.verbose, args.dry_run)
+
+    if (not _IsBotEnvironment() and
+        not _CheckLicenseAgreement(new_license, old_license,
+                                   config.version_number)):
         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 +203,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)

[jbudorick, 2015/11/04 18:48:10]

In light of the root ownership issue, we might want to give users some hints
here if one of these two calls fails.

[dgn, 2015/11/05 12:15:10]

Done. I also wrapped the calls below, as the license and sha1 files don't go
under that path.

 
+    # 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 +224,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)
 
@@ -381,7 +405,8 @@ def _ExtractLicenseFile(license_path, play_services_package_dir):
     license_file.write(match.group('text'))
 
 
-def _CheckLicenseAgreement(expected_license_path, actual_license_path):
+def _CheckLicenseAgreement(expected_license_path, actual_license_path,
+                           version_number):
   '''
   Checks that the new license is the one already accepted by the user. If it
   isn't, it prompts the user to accept it. Returns whether the expected license
@@ -392,13 +417,16 @@ def _CheckLicenseAgreement(expected_license_path, actual_license_path):
     return True
 
   with open(expected_license_path) as license_file:
+    # Uses plain print rather than logging to make sure this is not formatted
+    # by the logger.
+    print ('Updating the Google Play services SDK to '
+           'version %d.' % version_number)
+
     # The output is buffered when running as part of gclient hooks. We split
     # the text here and flush is explicitly to avoid having part of it dropped
     # out.
     # Note: text contains *escaped* new lines, so we split by '\\n', not '\n'.
     for license_part in license_file.read().split('\\n'):
-      # Uses plain print rather than logging to make sure this is not formatted
-      # by the logger.
       print license_part
       sys.stdout.flush()
 
@@ -409,6 +437,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 +486,4 @@ class DummyGsutil(download_from_google_storage.Gsutil):
 
 
 if __name__ == '__main__':
-  sys.exit(Main())
+  sys.exit(main(sys.argv[1:]))