Reland "bot_update: Don't use gclient sync output and rely on gclient revinfo."

This is a reland of 856ccef0e94f7b42964be792631b17d5c59cf310

Use a tempfile instead of stdout for gclient revinfo output

Original change's description:
> bot_update: Don't use gclient sync output and rely on gclient revinfo.
>
> In preparation for skipping gclient sync if there are no DEPS changes.
>
> Bug: 1199853
> Change-Id: Ib9b4ab803bc574a384c661765cee5e4c1de5baae
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2912259
> Reviewed-by: Anthony Polito <apolito@google.com>
> Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

Bug: 1199853
Change-Id: Ib50c4e943cbb51e4a2463beee9b7d1dee824ad29
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2910950
Reviewed-by: Gavin Mak <gavinmak@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

recipes/recipe_modules/bot_update/resources/bot_update.py

@@ -61,9 +61,6 @@ COMMIT_FOOTER_ENTRY_RE = re.compile(r'([^:]+):\s*(.*)')
 COMMIT_POSITION_FOOTER_KEY = 'Cr-Commit-Position'
 COMMIT_ORIGINAL_POSITION_FOOTER_KEY = 'Cr-Original-Commit-Position'
 
-# Regular expression to parse gclient's revinfo entries.
-REVINFO_RE = re.compile(r'^([^:]+):\s+([^@]+)@(.+)$')
-
 # Copied from scripts/recipes/chromium.py.
 GOT_REVISION_MAPPINGS = {
     CHROMIUM_SRC_URL: {
@@ -384,12 +381,7 @@ def gclient_sync(
     with_branch_heads, with_tags, revisions,
     patch_refs, gerrit_reset,
     gerrit_rebase_patch_ref):
-  # We just need to allocate a filename.
-  fd, gclient_output_file = tempfile.mkstemp(suffix='.json')
-  os.close(fd)
-
   args = ['sync', '--verbose', '--reset', '--force',
-          '--output-json', gclient_output_file,
           '--nohooks', '--noprehooks', '--delete_unversioned_trees']
   if with_branch_heads:
     args += ['--with_branch_heads']
@@ -417,15 +409,6 @@ def gclient_sync(
       raise PatchFailed(e.message, e.code, e.output)
     # Throw a GclientSyncFailed exception so we can catch this independently.
     raise GclientSyncFailed(e.message, e.code, e.output)
-  else:
-    with open(gclient_output_file) as f:
-      return json.load(f)
-  finally:
-    os.remove(gclient_output_file)
-
-
-def gclient_revinfo():
-  return call_gclient('revinfo', '-a') or ''
 
 
 def normalize_git_url(url):
@@ -453,19 +436,24 @@ def normalize_git_url(url):
 
 
 def create_manifest():
-  manifest = {}
-  output = gclient_revinfo()
-  for line in output.strip().splitlines():
-    match = REVINFO_RE.match(line.strip())
-    if match:
-      manifest[match.group(1)] = {
-        'repository': match.group(2),
-        'revision': match.group(3),
+  fd, fname = tempfile.mkstemp()
+  os.close(fd)
+  try:
+    revinfo = call_gclient(
+        'revinfo', '-a', '--ignore-dep-type', 'cipd', '--output-json', fname)
+    with open(fname) as f:
+      return {
+        path: {
+          'repository': info['url'],
+          'revision': info['rev'],
+        }
+        for path, info in json.load(f).items()
+        if info['rev'] is not None
       }
-    else:
-      print("WARNING: Couldn't match revinfo line:\n%s" % line)
-  return manifest
-
+  except ValueError, SubprocessFailed:
+    return {}
+  finally:
+    os.remove(fname)
 
 def get_commit_message_footer_map(message):
   """Returns: (dict) A dictionary of commit message footer entries.
@@ -773,28 +761,22 @@ def get_commit_position(git_path, revision='HEAD'):
   return None
 
 
-def parse_got_revision(gclient_output, got_revision_mapping):
+def parse_got_revision(manifest, got_revision_mapping):
   """Translate git gclient revision mapping to build properties."""
   properties = {}
-  solutions_output = {
+  manifest = {
       # Make sure path always ends with a single slash.
-      '%s/' % path.rstrip('/') : solution_output for path, solution_output
-      in gclient_output['solutions'].items()
+      '%s/' % path.rstrip('/'): info
+      for path, info in manifest.items()
   }
   for property_name, dir_name in got_revision_mapping.items():
     # Make sure dir_name always ends with a single slash.
     dir_name = '%s/' % dir_name.rstrip('/')
-    if dir_name not in solutions_output:
+    if dir_name not in manifest:
       continue
-    solution_output = solutions_output[dir_name]
-    if solution_output.get('scm') is None:
-      # This is an ignored DEPS, so the output got_revision should be 'None'.
-      revision = commit_position = None
-    else:
-      # Since we are using .DEPS.git, everything had better be git.
-      assert solution_output.get('scm') == 'git'
-      revision = git('rev-parse', 'HEAD', cwd=dir_name).strip()
-      commit_position = get_commit_position(dir_name)
+    info = manifest[dir_name]
+    revision = git('rev-parse', 'HEAD', cwd=dir_name).strip()
+    commit_position = get_commit_position(dir_name)
 
     properties[property_name] = revision
     if commit_position:
@@ -843,7 +825,7 @@ def ensure_checkout(solutions, revisions, first_sln, target_os, target_os_only,
   # Let gclient do the DEPS syncing.
   # The branch-head refspec is a special case because it's possible Chrome
   # src, which contains the branch-head refspecs, is DEPSed in.
-  gclient_output = gclient_sync(
+  gclient_sync(
       BRANCH_HEADS_REFSPEC in refs,
       TAGS_REFSPEC in refs,
       gc_revisions,
@@ -862,8 +844,6 @@ def ensure_checkout(solutions, revisions, first_sln, target_os, target_os_only,
   gclient_configure(solutions, target_os, target_os_only, target_cpu,
                     git_cache_dir)
 
-  return gclient_output
-
 
 def parse_revisions(revisions, root):
   """Turn a list of revision specs into a nice dictionary.
@@ -1086,12 +1066,12 @@ def checkout(options, git_slns, specs, revisions, step_text):
           git_cache_dir=options.git_cache_dir,
           cleanup_dir=options.cleanup_dir,
           gerrit_reset=not options.gerrit_no_reset)
-      gclient_output = ensure_checkout(**checkout_parameters)
+      ensure_checkout(**checkout_parameters)
       should_delete_dirty_file = True
     except GclientSyncFailed:
       print('We failed gclient sync, lets delete the checkout and retry.')
       ensure_no_checkout(dir_names, options.cleanup_dir)
-      gclient_output = ensure_checkout(**checkout_parameters)
+      ensure_checkout(**checkout_parameters)
       should_delete_dirty_file = True
   except PatchFailed as e:
     # Tell recipes information such as root, got_revision, etc.
@@ -1125,7 +1105,8 @@ def checkout(options, git_slns, specs, revisions, step_text):
   if not revision_mapping:
     revision_mapping['got_revision'] = first_sln
 
-  got_revisions = parse_got_revision(gclient_output, revision_mapping)
+  manifest = create_manifest()
+  got_revisions = parse_got_revision(manifest, revision_mapping)
 
   if not got_revisions:
     # TODO(hinoka): We should probably bail out here, but in the interest
@@ -1142,7 +1123,7 @@ def checkout(options, git_slns, specs, revisions, step_text):
             step_text=step_text,
             fixed_revisions=revisions,
             properties=got_revisions,
-            manifest=create_manifest())
+            manifest=manifest)
 
 
 def print_debug_info():

tests/bot_update_coverage_test.py

@@ -5,7 +5,6 @@
 
 import codecs
 import copy
-import json
 import os
 import sys
 import unittest
@@ -87,21 +86,11 @@ class MockedCall(object):
 
 
 class MockedGclientSync():
-  """A class producing a callable instance of gclient sync.
-
-  Because for bot_update, gclient sync also emits an output json file, we need
-  a callable object that can understand where the output json file is going, and
-  emit a (albite) fake file for bot_update to consume.
-  """
+  """A class producing a callable instance of gclient sync."""
   def __init__(self, fake_filesystem):
-    self.output = {}
-    self.fake_filesystem = fake_filesystem
     self.records = []
 
   def __call__(self, *args, **_):
-    output_json_index = args.index('--output-json') + 1
-    with self.fake_filesystem.open(args[output_json_index], 'w') as f:
-      json.dump(self.output, f)
     self.records.append(args)