bot_update: Make gclient experiment a property instead of an argument.

Is easier to configure experimental bots this way.

Bug: 643346
Change-Id: Idb6d3a68b02949dce71dbcba38e8ef756c467830
Reviewed-on: https://chromium-review.googlesource.com/981515
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>

recipes/README.recipes.md

@@ -42,18 +42,18 @@ Recipe module to ensure a checkout is consistent on a bot.
 
 #### **class [BotUpdateApi](/recipes/recipe_modules/bot_update/api.py#11)([RecipeApi][recipe_engine/wkt/RecipeApi]):**
 
-— **def [\_\_call\_\_](/recipes/recipe_modules/bot_update/api.py#30)(self, name, cmd, \*\*kwargs):**
+— **def [\_\_call\_\_](/recipes/recipe_modules/bot_update/api.py#32)(self, name, cmd, \*\*kwargs):**
 
 Wrapper for easy calling of bot_update.
 
-— **def [apply\_gerrit\_ref](/recipes/recipe_modules/bot_update/api.py#45)(self, root, gerrit_no_reset=False, gerrit_no_rebase_patch_ref=False, gerrit_repo=None, gerrit_ref=None, step_name='apply_gerrit', \*\*kwargs):**
+— **def [apply\_gerrit\_ref](/recipes/recipe_modules/bot_update/api.py#47)(self, root, gerrit_no_reset=False, gerrit_no_rebase_patch_ref=False, gerrit_repo=None, gerrit_ref=None, step_name='apply_gerrit', \*\*kwargs):**
 
-— **def [deapply\_patch](/recipes/recipe_modules/bot_update/api.py#395)(self, bot_update_step):**
+— **def [deapply\_patch](/recipes/recipe_modules/bot_update/api.py#397)(self, bot_update_step):**
 
 Deapplies a patch, taking care of DEPS and solution revisions properly.
     
 
-— **def [ensure\_checkout](/recipes/recipe_modules/bot_update/api.py#67)(self, gclient_config=None, suffix=None, patch=True, update_presentation=True, patch_root=None, no_shallow=False, with_branch_heads=False, with_tags=False, refs=None, patch_oauth2=None, oauth2_json=None, use_site_config_creds=None, clobber=False, root_solution_revision=None, rietveld=None, issue=None, patchset=None, gerrit_no_reset=False, gerrit_no_rebase_patch_ref=False, disable_syntax_validation=False, manifest_name=None, enable_gclient_experiment=False, \*\*kwargs):**
+— **def [ensure\_checkout](/recipes/recipe_modules/bot_update/api.py#69)(self, gclient_config=None, suffix=None, patch=True, update_presentation=True, patch_root=None, no_shallow=False, with_branch_heads=False, with_tags=False, refs=None, patch_oauth2=None, oauth2_json=None, use_site_config_creds=None, clobber=False, root_solution_revision=None, rietveld=None, issue=None, patchset=None, gerrit_no_reset=False, gerrit_no_rebase_patch_ref=False, disable_syntax_validation=False, manifest_name=None, \*\*kwargs):**
 
 Args:
   gclient_config: The gclient configuration to use when running bot_update.
@@ -64,7 +64,7 @@ Args:
   manifest_name: The name of the manifest to upload to LogDog.  This must
     be unique for the whole build.
 
-— **def [get\_project\_revision\_properties](/recipes/recipe_modules/bot_update/api.py#372)(self, project_name, gclient_config=None):**
+— **def [get\_project\_revision\_properties](/recipes/recipe_modules/bot_update/api.py#374)(self, project_name, gclient_config=None):**
 
 Returns all property names used for storing the checked-out revision of
 a given project.
@@ -78,7 +78,7 @@ Args:
 Returns (list of str): All properties that'll hold the checked-out revision
     of the given project. An empty list if no such properties exist.
 
-&emsp; **@property**<br>&mdash; **def [last\_returned\_properties](/recipes/recipe_modules/bot_update/api.py#39)(self):**
+&emsp; **@property**<br>&mdash; **def [last\_returned\_properties](/recipes/recipe_modules/bot_update/api.py#41)(self):**
 ### *recipe_modules* / [cipd](/recipes/recipe_modules/cipd)
 
 [DEPS](/recipes/recipe_modules/cipd/__init__.py#1): [infra\_paths](#recipe_modules-infra_paths), [recipe\_engine/json][recipe_engine/recipe_modules/json], [recipe\_engine/path][recipe_engine/recipe_modules/path], [recipe\_engine/platform][recipe_engine/recipe_modules/platform], [recipe\_engine/properties][recipe_engine/recipe_modules/properties], [recipe\_engine/python][recipe_engine/recipe_modules/python], [recipe\_engine/raw\_io][recipe_engine/recipe_modules/raw_io], [recipe\_engine/step][recipe_engine/recipe_modules/step]

recipes/recipe_modules/bot_update/__init__.py

@@ -16,6 +16,7 @@ DEPS = [
 ]
 
 from recipe_engine.recipe_api import Property
+from recipe_engine.config import ConfigGroup, Single
 
 PROPERTIES = {
   # Gerrit patches will have all properties about them prefixed with patch_.
@@ -37,4 +38,14 @@ PROPERTIES = {
   'fail_patch': Property(default=None, kind=str),
   'parent_got_revision': Property(default=None),
   'revision': Property(default=None),
+
+  '$depot_tools/bot_update': Property(
+      help='Properties specific to bot_update module.',
+      param_name='properties',
+      kind=ConfigGroup(
+          # Whether we should do the patching in gclient instead of bot_update
+          apply_patch_on_gclient=Single(bool),
+      ),
+      default={},
+  ),
 }

recipes/recipe_modules/bot_update/api.py

@@ -10,10 +10,12 @@ from recipe_engine import recipe_api
 
 class BotUpdateApi(recipe_api.RecipeApi):
 
-  def __init__(self, patch_issue, patch_set,
+  def __init__(self, properties, patch_issue, patch_set,
                repository, patch_repository_url, gerrit_ref, patch_ref,
                patch_gerrit_url, revision, parent_got_revision,
                deps_revision_overrides, fail_patch, *args, **kwargs):
+    self._apply_patch_on_gclient = properties.get(
+        'apply_patch_on_gclient', False)
     self._issue = patch_issue
     self._patchset = patch_set
     self._repository = repository or patch_repository_url
@@ -74,7 +76,7 @@ class BotUpdateApi(recipe_api.RecipeApi):
                       patchset=None, gerrit_no_reset=False,
                       gerrit_no_rebase_patch_ref=False,
                       disable_syntax_validation=False, manifest_name=None,
-                      enable_gclient_experiment=False, **kwargs):
+                      **kwargs):
     """
     Args:
       gclient_config: The gclient configuration to use when running bot_update.
@@ -205,8 +207,8 @@ class BotUpdateApi(recipe_api.RecipeApi):
       cmd.append('--gerrit_no_rebase_patch_ref')
     if disable_syntax_validation or cfg.disable_syntax_validation:
       cmd.append('--disable-syntax-validation')
-    if enable_gclient_experiment:
-      cmd.append('--enable-gclient-experiment')
+    if self._apply_patch_on_gclient:
+      cmd.append('--apply-patch-on-gclient')
 
     # Inject Json output for testing.
     first_sln = cfg.solutions[0].name

recipes/recipe_modules/bot_update/examples/full.expected/apply_patch_on_gclient.json

@@ -69,7 +69,7 @@
       "--revision",
       "src/third_party/angle@HEAD",
       "--disable-syntax-validation",
-      "--enable-gclient-experiment"
+      "--apply-patch-on-gclient"
     ],
     "env_prefixes": {
       "PATH": [
@@ -169,7 +169,8 @@
       "--revision",
       "src@f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9",
       "--revision",
-      "src/third_party/angle@fac9503c46405f77757b9a728eb85b8d7bc6080c"
+      "src/third_party/angle@fac9503c46405f77757b9a728eb85b8d7bc6080c",
+      "--apply-patch-on-gclient"
     ],
     "env_prefixes": {
       "PATH": [

recipes/recipe_modules/bot_update/examples/full.py

@@ -51,8 +51,6 @@ def RunSteps(api):
       api.properties.get('gerrit_no_rebase_patch_ref'))
   manifest_name = api.properties.get('manifest_name')
 
-  enable_gclient_experiment = api.properties.get('enable_gclient_experiment')
-
   if api.properties.get('test_apply_gerrit_ref'):
     prop2arg = {
         'gerrit_custom_repo': 'gerrit_repo',
@@ -82,8 +80,7 @@ def RunSteps(api):
         gerrit_no_reset=gerrit_no_reset,
         gerrit_no_rebase_patch_ref=gerrit_no_rebase_patch_ref,
         disable_syntax_validation=True,
-        manifest_name=manifest_name,
-        enable_gclient_experiment=enable_gclient_experiment)
+        manifest_name=manifest_name)
     if patch:
       api.bot_update.deapply_patch(bot_update_step)
 
@@ -196,11 +193,12 @@ def GenTests(api):
       patch_issue=338811,
       patch_set=3,
   )
-  yield api.test('enable_gclient_experiment') + api.properties.tryserver(
+  yield api.test('apply_patch_on_gclient') + api.properties.tryserver(
       gerrit_project='angle/angle',
       patch_issue=338811,
       patch_set=3,
-      enable_gclient_experiment=True,
+  ) + api.bot_update.properties(
+      apply_patch_on_gclient=True,
   )
   yield api.test('tryjob_gerrit_v8') + api.properties.tryserver(
       gerrit_project='v8/v8',

recipes/recipe_modules/bot_update/resources/bot_update.py

@@ -331,7 +331,7 @@ def gclient_configure(solutions, target_os, target_os_only, target_cpu,
 def gclient_sync(
     with_branch_heads, with_tags, shallow, revisions, break_repo_locks,
     disable_syntax_validation, gerrit_repo, gerrit_ref, gerrit_reset,
-    gerrit_rebase_patch_ref, enable_gclient_experiment):
+    gerrit_rebase_patch_ref, apply_patch_on_gclient):
   # We just need to allocate a filename.
   fd, gclient_output_file = tempfile.mkstemp(suffix='.json')
   os.close(fd)
@@ -354,7 +354,7 @@ def gclient_sync(
       revision = 'origin/master'
     args.extend(['--revision', '%s@%s' % (name, revision)])
 
-  if enable_gclient_experiment:
+  if apply_patch_on_gclient:
     # TODO(ehmaldonado): Merge gerrit_repo and gerrit_ref into a patch-ref flag
     # and add support for passing multiple patch refs.
     args.extend(['--patch-ref', gerrit_repo + '@' + gerrit_ref])
@@ -368,7 +368,7 @@ def gclient_sync(
   except SubprocessFailed as e:
     # If gclient sync is handling patching, parse the output for a patch error
     # message.
-    if enable_gclient_experiment and PATCH_ERROR_RE.search(e.output):
+    if apply_patch_on_gclient and PATCH_ERROR_RE.search(e.output):
       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)
@@ -856,7 +856,7 @@ def ensure_checkout(solutions, revisions, first_sln, target_os, target_os_only,
                     target_cpu, patch_root, gerrit_repo, gerrit_ref,
                     gerrit_rebase_patch_ref, shallow, refs, git_cache_dir,
                     cleanup_dir, gerrit_reset, disable_syntax_validation,
-                    enable_gclient_experiment):
+                    apply_patch_on_gclient):
   # Get a checkout of each solution, without DEPS or hooks.
   # Calling git directly because there is no way to run Gclient without
   # invoking DEPS.
@@ -865,7 +865,7 @@ def ensure_checkout(solutions, revisions, first_sln, target_os, target_os_only,
   git_checkouts(solutions, revisions, shallow, refs, git_cache_dir, cleanup_dir)
 
   applied_gerrit_patch = False
-  if not enable_gclient_experiment:
+  if not apply_patch_on_gclient:
     print '===Processing patch solutions==='
     patch_root = patch_root or ''
     print 'Patch root is %r' % patch_root
@@ -912,7 +912,7 @@ def ensure_checkout(solutions, revisions, first_sln, target_os, target_os_only,
       gerrit_ref,
       gerrit_reset,
       gerrit_rebase_patch_ref,
-      enable_gclient_experiment)
+      apply_patch_on_gclient)
 
   # Now that gclient_sync has finished, we should revert any .DEPS.git so that
   # presubmit doesn't complain about it being modified.
@@ -920,7 +920,7 @@ def ensure_checkout(solutions, revisions, first_sln, target_os, target_os_only,
     git('checkout', 'HEAD', '--', '.DEPS.git', cwd=first_sln)
 
   # Apply the rest of the patch here (sans DEPS)
-  if gerrit_ref and not applied_gerrit_patch and not enable_gclient_experiment:
+  if gerrit_ref and not applied_gerrit_patch and not apply_patch_on_gclient:
     # If gerrit_ref was for solution's main repository, it has already been
     # applied above. This chunk is executed only for patches to DEPS-ed in
     # git repositories.
@@ -1026,7 +1026,7 @@ def parse_args():
   parse.add_option(
       '--disable-syntax-validation', action='store_true',
       help='Disable validation of .gclient and DEPS syntax.')
-  parse.add_option('--enable-gclient-experiment', action='store_true',
+  parse.add_option('--apply-patch-on-gclient', action='store_true',
                    help='Patch the gerrit ref in gclient instead of here.')
 
   options, args = parse.parse_args()
@@ -1143,7 +1143,7 @@ def checkout(options, git_slns, specs, revisions, step_text, shallow):
           cleanup_dir=options.cleanup_dir,
           gerrit_reset=not options.gerrit_no_reset,
           disable_syntax_validation=options.disable_syntax_validation,
-          enable_gclient_experiment=options.enable_gclient_experiment)
+          apply_patch_on_gclient=options.apply_patch_on_gclient)
       gclient_output = ensure_checkout(**checkout_parameters)
     except GclientSyncFailed:
       print 'We failed gclient sync, lets delete the checkout and retry.'

recipes/recipe_modules/bot_update/test_api.py

@@ -8,6 +8,15 @@ from recipe_engine import recipe_test_api
 
 
 class BotUpdateTestApi(recipe_test_api.RecipeTestApi):
+  def properties(self, apply_patch_on_gclient):
+    ret = self.test(None)
+    ret.properties = {
+        '$depot_tools/bot_update': {
+            'apply_patch_on_gclient': apply_patch_on_gclient,
+        }
+    }
+    return ret
+
   def output_json(self, root, first_sln, revision_mapping, fail_patch=False,
                   fixed_revisions=None):
     """Deterministically synthesize json.output test data for gclient's

tests/bot_update_coverage_test.py

@@ -158,7 +158,7 @@ class BotUpdateUnittests(unittest.TestCase):
       'cleanup_dir': None,
       'gerrit_reset': None,
       'disable_syntax_validation': False,
-      'enable_gclient_experiment': False,
+      'apply_patch_on_gclient': False,
   }
 
   def setUp(self):
@@ -219,7 +219,7 @@ class BotUpdateUnittests(unittest.TestCase):
   def testEnableGclientExperiment(self):
     self.params['gerrit_ref'] = 'refs/changes/12/345/6'
     self.params['gerrit_repo'] = 'https://chromium.googlesource.com/v8/v8'
-    self.params['enable_gclient_experiment'] = True
+    self.params['apply_patch_on_gclient'] = True
     bot_update.ensure_checkout(**self.params)
     args = self.gclient.records[0]
     idx = args.index('--patch-ref')