From ee04b9a2f4890af563a21a69871d20fd6a586ce4 Mon Sep 17 00:00:00 2001 From: Edward Lesmes Date: Mon, 26 Mar 2018 19:28:31 -0400 Subject: [PATCH] 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 Reviewed-by: Robbie Iannucci --- recipes/README.recipes.md | 12 ++++++------ recipes/recipe_modules/bot_update/__init__.py | 11 +++++++++++ recipes/recipe_modules/bot_update/api.py | 10 ++++++---- ...riment.json => apply_patch_on_gclient.json} | 5 +++-- .../recipe_modules/bot_update/examples/full.py | 10 ++++------ .../bot_update/resources/bot_update.py | 18 +++++++++--------- recipes/recipe_modules/bot_update/test_api.py | 9 +++++++++ tests/bot_update_coverage_test.py | 4 ++-- 8 files changed, 50 insertions(+), 29 deletions(-) rename recipes/recipe_modules/bot_update/examples/full.expected/{enable_gclient_experiment.json => apply_patch_on_gclient.json} (99%) diff --git a/recipes/README.recipes.md b/recipes/README.recipes.md index 959d0c12e..b3bda8108 100644 --- a/recipes/README.recipes.md +++ b/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. -  **@property**
— **def [last\_returned\_properties](/recipes/recipe_modules/bot_update/api.py#39)(self):** +  **@property**
— **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] diff --git a/recipes/recipe_modules/bot_update/__init__.py b/recipes/recipe_modules/bot_update/__init__.py index 15fe97f27..9694929c7 100644 --- a/recipes/recipe_modules/bot_update/__init__.py +++ b/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={}, + ), } diff --git a/recipes/recipe_modules/bot_update/api.py b/recipes/recipe_modules/bot_update/api.py index 8552e20be..d778e7288 100644 --- a/recipes/recipe_modules/bot_update/api.py +++ b/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 diff --git a/recipes/recipe_modules/bot_update/examples/full.expected/enable_gclient_experiment.json b/recipes/recipe_modules/bot_update/examples/full.expected/apply_patch_on_gclient.json similarity index 99% rename from recipes/recipe_modules/bot_update/examples/full.expected/enable_gclient_experiment.json rename to recipes/recipe_modules/bot_update/examples/full.expected/apply_patch_on_gclient.json index 70caf7703..81fdb28cf 100644 --- a/recipes/recipe_modules/bot_update/examples/full.expected/enable_gclient_experiment.json +++ b/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": [ diff --git a/recipes/recipe_modules/bot_update/examples/full.py b/recipes/recipe_modules/bot_update/examples/full.py index eeb256588..88a936a6f 100644 --- a/recipes/recipe_modules/bot_update/examples/full.py +++ b/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', diff --git a/recipes/recipe_modules/bot_update/resources/bot_update.py b/recipes/recipe_modules/bot_update/resources/bot_update.py index b240828e4..5efb4c2e9 100755 --- a/recipes/recipe_modules/bot_update/resources/bot_update.py +++ b/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.' diff --git a/recipes/recipe_modules/bot_update/test_api.py b/recipes/recipe_modules/bot_update/test_api.py index 5c86aca00..54213d749 100644 --- a/recipes/recipe_modules/bot_update/test_api.py +++ b/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 diff --git a/tests/bot_update_coverage_test.py b/tests/bot_update_coverage_test.py index 315e566bb..f623e6daa 100755 --- a/tests/bot_update_coverage_test.py +++ b/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')