[no-sync] bot_update: Remove previous no-sync exp changes and implement new one.

Bug: 1339472
Change-Id: I250dd72dd806d26ba6cbe213280ecd65fbc66123
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3789864
Commit-Queue: Joanna Wang <jojwang@chromium.org>
Reviewed-by: Gavin Mak <gavinmak@google.com>

recipes/recipe_modules/bot_update/resources/bot_update.py

@@ -83,10 +83,9 @@ GOT_REVISION_MAPPINGS = {
 }
 
 # List of bot update experiments
-EXP_NO_SYNC = 'no_sync'  # Don't fetch/sync if current revision is recent enough
-
-# Don't sync if the checkout is less than 6 hours old.
-NO_SYNC_MAX_DELAY_S = 6 * 60 * 60
+# Gclient will skip deps syncing if there have been no DEPS changes
+# since the last sync on the bot.
+EXP_NO_SYNC = 'no-sync'
 
 GCLIENT_TEMPLATE = """solutions = %(solutions)s
 
@@ -411,10 +410,14 @@ def git_config_if_not_set(key, value):
       git('config', '--global', '--unset', key)
 
 
-def gclient_sync(
-    with_branch_heads, with_tags, revisions,
-    patch_refs, gerrit_reset,
-    gerrit_rebase_patch_ref, download_topics=False):
+def gclient_sync(with_branch_heads,
+                 with_tags,
+                 revisions,
+                 patch_refs,
+                 gerrit_reset,
+                 gerrit_rebase_patch_ref,
+                 download_topics=False,
+                 experiments=None):
   args = ['sync', '--verbose', '--reset', '--force',
           '--nohooks', '--noprehooks', '--delete_unversioned_trees']
   if with_branch_heads:
@@ -436,6 +439,9 @@ def gclient_sync(
     if download_topics:
       args.append('--download-topics')
 
+  if EXP_NO_SYNC in experiments:
+    args.extend(['--experiment', EXP_NO_SYNC])
+
   try:
     call_gclient(*args)
   except SubprocessFailed as e:
@@ -652,47 +658,22 @@ def _set_git_config(fn):
 
 
 def git_checkouts(solutions, revisions, refs, no_fetch_tags, git_cache_dir,
-                  cleanup_dir, enforce_fetch, experiments):
+                  cleanup_dir, enforce_fetch):
   build_dir = os.getcwd()
-  synced = []
   for sln in solutions:
     sln_dir = path.join(build_dir, sln['name'])
-    did_sync = _git_checkout(
-        sln, sln_dir, revisions, refs, no_fetch_tags, git_cache_dir,
-        cleanup_dir, enforce_fetch, experiments)
-    if did_sync:
-      synced.append(sln['name'])
-  return synced
-
-
-def _git_checkout_needs_sync(sln_url, sln_dir, refs):
-  if not path.exists(sln_dir):
-    return True
-  for ref in refs:
-    try:
-      remote_ref = ref_to_remote_ref(ref)
-      commit_time = git('show', '-s', '--format=%ct', remote_ref, cwd=sln_dir)
-      commit_time = int(commit_time)
-    except SubprocessError:
-      return True
-    if time.time() - commit_time >= NO_SYNC_MAX_DELAY_S:
-      return True
-  return False
+    _git_checkout(sln, sln_dir, revisions, refs, no_fetch_tags, git_cache_dir,
+                  cleanup_dir, enforce_fetch)
 
 
 def _git_checkout(sln, sln_dir, revisions, refs, no_fetch_tags, git_cache_dir,
-                  cleanup_dir, enforce_fetch, experiments):
+                  cleanup_dir, enforce_fetch):
   name = sln['name']
   url = sln['url']
 
   branch, revision = get_target_branch_and_revision(name, url, revisions)
   pin = revision if COMMIT_HASH_RE.match(revision) else None
 
-  if (EXP_NO_SYNC in experiments
-      and not _git_checkout_needs_sync(url, sln_dir, refs)):
-    git('checkout', '--force', pin or branch, '--', cwd=sln_dir)
-    return False
-
   populate_cmd = (['cache', 'populate', '-v', '--cache-dir', git_cache_dir, url,
                    '--reset-fetch-config'])
   if no_fetch_tags:
@@ -756,7 +737,7 @@ def _git_checkout(sln, sln_dir, revisions, refs, no_fetch_tags, git_cache_dir,
       # happens to have the exact same name.
       git('checkout', '--force', pin or branch, '--', cwd=sln_dir)
       git('clean', '-dff', cwd=sln_dir)
-      return True
+      return
     except SubprocessFailed as e:
       # Exited abnormally, there's probably something wrong.
       print('Something failed: %s.' % str(e))
@@ -767,8 +748,6 @@ def _git_checkout(sln, sln_dir, revisions, refs, no_fetch_tags, git_cache_dir,
       else:
         raise
 
-  return True
-
 
 def _git_disable_gc(cwd):
   git('config', 'gc.auto', '0', cwd=cwd)
@@ -838,9 +817,8 @@ def ensure_checkout(solutions, revisions, first_sln, target_os, target_os_only,
   # invoking DEPS.
   print('Fetching Git checkout')
 
-  synced_solutions = git_checkouts(
-      solutions, revisions, refs, no_fetch_tags, git_cache_dir, cleanup_dir,
-      enforce_fetch, experiments)
+  git_checkouts(solutions, revisions, refs, no_fetch_tags, git_cache_dir,
+                cleanup_dir, enforce_fetch)
 
   # Ensure our build/ directory is set up with the correct .gclient file.
   gclient_configure(solutions, target_os, target_os_only, target_cpu,
@@ -860,14 +838,10 @@ 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_sync(
-      BRANCH_HEADS_REFSPEC in refs,
-      TAGS_REFSPEC in refs,
-      gc_revisions,
-      patch_refs,
-      gerrit_reset,
-      gerrit_rebase_patch_ref,
-      download_topics)
+  gclient_sync(BRANCH_HEADS_REFSPEC in refs, TAGS_REFSPEC in refs, gc_revisions,
+               patch_refs, gerrit_reset, gerrit_rebase_patch_ref,
+               download_topics, experiments)
+
 
   # Now that gclient_sync has finished, we should revert any .DEPS.git so that
   # presubmit doesn't complain about it being modified.
@@ -880,8 +854,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 synced_solutions
-
 
 def parse_revisions(revisions, root):
   """Turn a list of revision specs into a nice dictionary.
@@ -1072,7 +1044,6 @@ def checkout(options, git_slns, specs, revisions, step_text):
     pass
 
   should_delete_dirty_file = False
-  synced_solutions = []
   experiments = []
   if options.experiments:
     experiments = options.experiments.split(',')
@@ -1111,12 +1082,12 @@ def checkout(options, git_slns, specs, revisions, step_text):
           gerrit_reset=not options.gerrit_no_reset,
 
           experiments=experiments)
-      synced_solutions = 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)
-      synced_solutions = 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.
@@ -1128,8 +1099,7 @@ def checkout(options, git_slns, specs, revisions, step_text):
               patch_failure=True,
               failed_patch_body=e.output,
               step_text='%s PATCH FAILED' % step_text,
-              fixed_revisions=revisions,
-              synced_solutions=synced_solutions)
+              fixed_revisions=revisions)
     should_delete_dirty_file = True
     raise
   finally:
@@ -1169,8 +1139,7 @@ def checkout(options, git_slns, specs, revisions, step_text):
             step_text=step_text,
             fixed_revisions=revisions,
             properties=got_revisions,
-            manifest=manifest,
-            synced_solutions=synced_solutions)
+            manifest=manifest)
 
 
 def print_debug_info():

tests/bot_update_coverage_test.py

@@ -228,6 +228,16 @@ class BotUpdateUnittests(unittest.TestCase):
     self.assertTrue(found)
     return self.call.records
 
+  def testGclientNoSyncExperiment(self):
+    ref = 'refs/changes/12/345/6'
+    repo = 'https://chromium.googlesource.com/v8/v8'
+    self.params['patch_refs'] = ['%s@%s' % (repo, ref)]
+    self.params['experiments'] = bot_update.EXP_NO_SYNC
+    bot_update.ensure_checkout(**self.params)
+    args = self.gclient.records[0]
+    idx = args.index('--experiment')
+    self.assertEqual(args[idx+1], bot_update.EXP_NO_SYNC)
+
   def testApplyPatchOnGclient(self):
     ref = 'refs/changes/12/345/6'
     repo = 'https://chromium.googlesource.com/v8/v8'