From cb3fd431fd4945e0c745743d0e6d18cb4b604d1b Mon Sep 17 00:00:00 2001 From: "tandrii@chromium.org" Date: Mon, 25 Apr 2016 14:18:33 +0000 Subject: [PATCH] Generalize patch_project to patch root conversion. IMPORTANT: See bug http://crbug.com/605563#c3 for deployment and emergency roll-back plan. This patch is first of a series. It provides a cleaner and generic way to apply patches to non-root of first solution. R=phajdan.jr@chromium.org,machenbach@chromium.org BUG=605563 Review URL: https://codereview.chromium.org/1917433002 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@300141 0039d316-1c4b-4281-b951-d872f2087c98 --- recipe_modules/bot_update/__init__.py | 1 + recipe_modules/bot_update/api.py | 17 ++++++ .../example.expected/gerrit_no_reset.json | 2 +- .../example.expected/tryjob_crbug605563.json | 59 +++++++++++++++++++ .../example.expected/tryjob_gerrit_angle.json | 50 ++++++++++++++++ .../tryjob_v8_head_by_default.json | 52 ++++++++++++++++ recipe_modules/bot_update/example.py | 28 ++++++++- recipe_modules/gclient/api.py | 40 +++++++++++++ recipe_modules/gclient/config.py | 39 ++++++++---- recipe_modules/rietveld/api.py | 5 ++ 10 files changed, 278 insertions(+), 15 deletions(-) create mode 100644 recipe_modules/bot_update/example.expected/tryjob_crbug605563.json create mode 100644 recipe_modules/bot_update/example.expected/tryjob_gerrit_angle.json create mode 100644 recipe_modules/bot_update/example.expected/tryjob_v8_head_by_default.json diff --git a/recipe_modules/bot_update/__init__.py b/recipe_modules/bot_update/__init__.py index dde0d6914..11364c78f 100644 --- a/recipe_modules/bot_update/__init__.py +++ b/recipe_modules/bot_update/__init__.py @@ -21,6 +21,7 @@ PROPERTIES = { 'issue': Property(default=None), 'patchset': Property(default=None), 'patch_url': Property(default=None), + 'patch_project': Property(default=None), 'repository': Property(default=None), 'event.patchSet.ref': Property(default=None, param_name="gerrit_ref"), 'rietveld': Property(default=None), diff --git a/recipe_modules/bot_update/api.py b/recipe_modules/bot_update/api.py index 31a27335c..1f93e6179 100644 --- a/recipe_modules/bot_update/api.py +++ b/recipe_modules/bot_update/api.py @@ -85,6 +85,17 @@ class BotUpdateApi(recipe_api.RecipeApi): # Construct our bot_update command. This basically be inclusive of # everything required for bot_update to know: root = patch_root + if root == 'TODO(TANDRII): REMOVE THIS TRANSITION TO patch_projects': + # This special condition is here for initial rollout of this code, + # because it's hard to test this change without rolling into build + # repository. + # After the switch to new code is complete, this special TODOstring will + # be removed in favor of "root is None" + assert patch_project_roots is None + root = self.m.gclient.calculate_patch_root( + self.m.properties.get('patch_project'), cfg) + # TODO(tandrii): get rid the condition below after transition. + if root is None: root = cfg.solutions[0].name additional = self.m.rietveld.calculate_issue_root(patch_project_roots) @@ -128,6 +139,12 @@ class BotUpdateApi(recipe_api.RecipeApi): else: email_file = key_file = None + # Allow patch_project's revision if necessary. + # This is important for projects which are checked out as DEPS of the + # gclient solution. + self.m.gclient.set_patch_project_revision( + self.m.properties.get('patch_project'), cfg) + rev_map = cfg.got_revision_mapping.as_jsonish() flags = [ diff --git a/recipe_modules/bot_update/example.expected/gerrit_no_reset.json b/recipe_modules/bot_update/example.expected/gerrit_no_reset.json index 734b43e5a..710e17420 100644 --- a/recipe_modules/bot_update/example.expected/gerrit_no_reset.json +++ b/recipe_modules/bot_update/example.expected/gerrit_no_reset.json @@ -42,4 +42,4 @@ "recipe_result": null, "status_code": 0 } -] +] \ No newline at end of file diff --git a/recipe_modules/bot_update/example.expected/tryjob_crbug605563.json b/recipe_modules/bot_update/example.expected/tryjob_crbug605563.json new file mode 100644 index 000000000..c2b62debd --- /dev/null +++ b/recipe_modules/bot_update/example.expected/tryjob_crbug605563.json @@ -0,0 +1,59 @@ +[ + { + "cmd": [ + "python", + "-u", + "RECIPE_MODULE[depot_tools::bot_update]/resources/bot_update.py", + "--master", + "tryserver.chromium.linux", + "--builder", + "linux_rel", + "--slave", + "totallyaslave-c4", + "--spec", + "cache_dir = '[GIT_CACHE]'\nsolutions = [{'deps_file': '.DEPS.git', 'managed': True, 'name': 'src', 'url': 'svn://svn.chromium.org/chrome/trunk/src'}]", + "--root", + "src", + "--revision_mapping_file", + "{\"src\": \"got_cr_revision\"}", + "--git-cache-dir", + "[GIT_CACHE]", + "--patch_url", + "http://src.chromium.org/foo/bar", + "--output_json", + "/path/to/tmp/json", + "--revision", + "src@HEAD" + ], + "cwd": "[SLAVE_BUILD]", + "env": { + "PATH": "%(PATH)s:RECIPE_PACKAGE_REPO[depot_tools]" + }, + "name": "bot_update", + "~followup_annotations": [ + "@@@STEP_TEXT@Some step text@@@", + "@@@STEP_LOG_LINE@json.output@{@@@", + "@@@STEP_LOG_LINE@json.output@ \"did_run\": true, @@@", + "@@@STEP_LOG_LINE@json.output@ \"fixed_revisions\": {@@@", + "@@@STEP_LOG_LINE@json.output@ \"src\": \"HEAD\"@@@", + "@@@STEP_LOG_LINE@json.output@ }, @@@", + "@@@STEP_LOG_LINE@json.output@ \"patch_failure\": false, @@@", + "@@@STEP_LOG_LINE@json.output@ \"patch_root\": \"src\", @@@", + "@@@STEP_LOG_LINE@json.output@ \"properties\": {@@@", + "@@@STEP_LOG_LINE@json.output@ \"got_cr_revision\": \"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\", @@@", + "@@@STEP_LOG_LINE@json.output@ \"got_cr_revision_cp\": \"refs/heads/master@{#170242}\"@@@", + "@@@STEP_LOG_LINE@json.output@ }, @@@", + "@@@STEP_LOG_LINE@json.output@ \"root\": \"src\", @@@", + "@@@STEP_LOG_LINE@json.output@ \"step_text\": \"Some step text\"@@@", + "@@@STEP_LOG_LINE@json.output@}@@@", + "@@@STEP_LOG_END@json.output@@@", + "@@@SET_BUILD_PROPERTY@got_cr_revision@\"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\"@@@", + "@@@SET_BUILD_PROPERTY@got_cr_revision_cp@\"refs/heads/master@{#170242}\"@@@" + ] + }, + { + "name": "$result", + "recipe_result": null, + "status_code": 0 + } +] \ No newline at end of file diff --git a/recipe_modules/bot_update/example.expected/tryjob_gerrit_angle.json b/recipe_modules/bot_update/example.expected/tryjob_gerrit_angle.json new file mode 100644 index 000000000..b4b7311f1 --- /dev/null +++ b/recipe_modules/bot_update/example.expected/tryjob_gerrit_angle.json @@ -0,0 +1,50 @@ +[ + { + "cmd": [ + "python", + "-u", + "RECIPE_MODULE[depot_tools::bot_update]/resources/bot_update.py", + "--master", + "chromium.testing.master", + "--builder", + "TestBuilder", + "--slave", + "TestSlavename", + "--spec", + "cache_dir = '[GIT_CACHE]'\nsolutions = [{'deps_file': '.DEPS.git', 'managed': True, 'name': 'src', 'url': 'svn://svn.chromium.org/chrome/trunk/src'}]", + "--root", + "src/third_party/angle", + "--revision_mapping_file", + "{\"src\": \"got_cr_revision\"}", + "--git-cache-dir", + "[GIT_CACHE]", + "--gerrit_repo", + "https://chromium.googlesource.com/angle/angle", + "--gerrit_ref", + "refs/changes/11/338811/3", + "--output_json", + "/path/to/tmp/json", + "--revision", + "src@HEAD", + "--revision", + "src/third_party/angle@HEAD" + ], + "cwd": "[SLAVE_BUILD]", + "env": { + "PATH": "%(PATH)s:RECIPE_PACKAGE_REPO[depot_tools]" + }, + "name": "bot_update", + "~followup_annotations": [ + "@@@STEP_LOG_LINE@json.output@{@@@", + "@@@STEP_LOG_LINE@json.output@ \"did_run\": false, @@@", + "@@@STEP_LOG_LINE@json.output@ \"patch_failure\": false@@@", + "@@@STEP_LOG_LINE@json.output@}@@@", + "@@@STEP_LOG_END@json.output@@@" + ] + }, + { + "name": "$result", + "recipe_result": null, + "status_code": 0 + } +] diff --git a/recipe_modules/bot_update/example.expected/tryjob_v8_head_by_default.json b/recipe_modules/bot_update/example.expected/tryjob_v8_head_by_default.json new file mode 100644 index 000000000..f4db111c0 --- /dev/null +++ b/recipe_modules/bot_update/example.expected/tryjob_v8_head_by_default.json @@ -0,0 +1,52 @@ +[ + { + "cmd": [ + "python", + "-u", + "RECIPE_MODULE[depot_tools::bot_update]/resources/bot_update.py", + "--master", + "chromium.testing.master", + "--builder", + "TestBuilder", + "--slave", + "TestSlavename", + "--spec", + "cache_dir = '[GIT_CACHE]'\nsolutions = [{'deps_file': '.DEPS.git', 'managed': True, 'name': 'src', 'url': 'svn://svn.chromium.org/chrome/trunk/src'}]", + "--root", + "src/v8", + "--revision_mapping_file", + "{\"src\": \"got_cr_revision\"}", + "--git-cache-dir", + "[GIT_CACHE]", + "--issue", + "12853011", + "--patchset", + "1", + "--rietveld_server", + "https://codereview.chromium.org", + "--output_json", + "/path/to/tmp/json", + "--revision", + "src@HEAD", + "--revision", + "src/v8@HEAD" + ], + "cwd": "[SLAVE_BUILD]", + "env": { + "PATH": "%(PATH)s:RECIPE_PACKAGE_REPO[depot_tools]" + }, + "name": "bot_update", + "~followup_annotations": [ + "@@@STEP_LOG_LINE@json.output@{@@@", + "@@@STEP_LOG_LINE@json.output@ \"did_run\": false, @@@", + "@@@STEP_LOG_LINE@json.output@ \"patch_failure\": false@@@", + "@@@STEP_LOG_LINE@json.output@}@@@", + "@@@STEP_LOG_END@json.output@@@" + ] + }, + { + "name": "$result", + "recipe_result": null, + "status_code": 0 + } +] \ No newline at end of file diff --git a/recipe_modules/bot_update/example.py b/recipe_modules/bot_update/example.py index 99362ffcf..6aea997fe 100644 --- a/recipe_modules/bot_update/example.py +++ b/recipe_modules/bot_update/example.py @@ -18,8 +18,11 @@ def RunSteps(api): soln.url = 'svn://svn.chromium.org/chrome/trunk/src' soln.revision = api.properties.get('revision') api.gclient.c = src_cfg - api.gclient.c.revisions = api.properties.get('revisions', {}) + api.gclient.c.revisions.update(api.properties.get('revisions', {})) api.gclient.c.got_revision_mapping['src'] = 'got_cr_revision' + api.gclient.c.patch_projects['v8'] = ('src/v8', 'HEAD') + api.gclient.c.patch_projects['angle/angle'] = ('src/third_party/angle', + 'HEAD') patch = api.properties.get('patch', True) clobber = True if api.properties.get('clobber') else False force = True if api.properties.get('force') else False @@ -31,6 +34,9 @@ def RunSteps(api): root_solution_revision = api.properties.get('root_solution_revision') suffix = api.properties.get('suffix') gerrit_no_reset = True if api.properties.get('gerrit_no_reset') else False + # TODO(tandrii): remove this after transition. http://crbug.com/605563. + crbug605563 = ('TODO(TANDRII): REMOVE THIS TRANSITION TO patch_projects' + if api.properties.get('crbug605563') else None) api.bot_update.ensure_checkout(force=force, no_shallow=no_shallow, patch=patch, @@ -40,7 +46,8 @@ def RunSteps(api): clobber=clobber, root_solution_revision=root_solution_revision, suffix=suffix, - gerrit_no_reset=gerrit_no_reset) + gerrit_no_reset=gerrit_no_reset, + patch_root=crbug605563) def GenTests(api): @@ -72,6 +79,15 @@ def GenTests(api): patchset=654321, patch_url='http://src.chromium.org/foo/bar' ) + yield api.test('tryjob_crbug605563') + api.properties( + mastername='tryserver.chromium.linux', + buildername='linux_rel', + slavename='totallyaslave-c4', + issue=12345, + patchset=654321, + patch_url='http://src.chromium.org/foo/bar', + crbug605563=True, + ) yield api.test('trychange') + api.properties( mastername='tryserver.chromium.linux', buildername='linux_rel', @@ -161,3 +177,11 @@ def GenTests(api): patch_project='v8', revisions={'src/v8': 'abc'} ) + yield api.test('tryjob_v8_head_by_default') + api.properties.tryserver( + patch_project='v8', + crbug605563=True, + ) + yield api.test('tryjob_gerrit_angle') + api.properties.tryserver_gerrit( + full_project_name='angle/angle', + crbug605563=True, + ) diff --git a/recipe_modules/gclient/api.py b/recipe_modules/gclient/api.py index 2d9ed949f..6b44dab1d 100644 --- a/recipe_modules/gclient/api.py +++ b/recipe_modules/gclient/api.py @@ -148,6 +148,7 @@ class GclientApi(recipe_api.RecipeApi): def sync(self, cfg, with_branch_heads=False, **kwargs): revisions = [] + self.set_patch_project_revision(self.m.properties.get('patch_project'), cfg) for i, s in enumerate(cfg.solutions): if s.safesync_url: # prefer safesync_url in gclient mode continue @@ -336,3 +337,42 @@ class GclientApi(recipe_api.RecipeApi): args=[self.m.path['slave_build']], infra_step=True, ) + + def calculate_patch_root(self, patch_project, gclient_config=None): + """Returns path where a patch should be applied to based patch_project. + + Maps "patch_project" to a path of directories relative to checkout's root, + which describe where to place the patch. + + For now, considers only first solution (c.solutions[0]), but in theory can + be extended to all of them. + + See patch_projects solution config property. + + Returns: + Relative path, including solution's root. + If patch_project is not given or not recognized, it'll be just first + solution root. + """ + cfg = gclient_config or self.c + root, _ = cfg.patch_projects.get(patch_project, ('', '')) + if root: + # Note, that c.patch_projects contains patch roots as + # slash(/)-separated path, which are roots of the respective project repos + # and include actual solution name in them. + return self.m.path.join(*root.split('/')) + # Default case - assume patch is for first solution, as this is what most + # projects rely on. + return cfg.solutions[0].name + + def set_patch_project_revision(self, patch_project, gclient_config=None): + """Updates config revision corresponding to patch_project. + + Useful for bot_update only, as this is the only consumer of gclient's config + revision map. This doesn't overwrite the revision if it was already set. + """ + assert patch_project is None or isinstance(patch_project, basestring) + cfg = gclient_config or self.c + path, revision = cfg.patch_projects.get(patch_project, (None, None)) + if path and revision and path not in cfg.revisions: + cfg.revisions[path] = revision diff --git a/recipe_modules/gclient/config.py b/recipe_modules/gclient/config.py index c300edd50..1803a17bf 100644 --- a/recipe_modules/gclient/config.py +++ b/recipe_modules/gclient/config.py @@ -63,6 +63,20 @@ def BaseConfig(USE_MIRROR=True, GIT_MODE=False, CACHE_DIR=None, parent_got_revision_mapping = Dict(hidden=True), delete_unversioned_trees = Single(bool, empty_val=True, required=False), + # Maps patch_project to (solution/path, revision). + # - solution/path is then used to apply patches as patch root in + # bot_update. + # - if revision is given, it's passed verbatim to bot_update for + # corresponding dependency. + # This is essentially a whitelist of which projects inside a solution + # can be patched automatically by bot_update based on PATCH_PROJECT + # property. + # For example, bare chromium solution has this entry in patch_projects + # 'angle/angle': ('src/third_party/angle', 'HEAD') + # then a patch to Angle project can be applied to a chromium src's + # checkout after first updating Angle's repo to its master's HEAD. + patch_projects = Dict(value_type=tuple, hidden=True), + # Check out refs/branch-heads. # TODO (machenbach): Only implemented for bot_update atm. with_branch_heads = Single( @@ -73,6 +87,8 @@ def BaseConfig(USE_MIRROR=True, GIT_MODE=False, CACHE_DIR=None, GIT_MODE = Static(bool(GIT_MODE)), USE_MIRROR = Static(bool(USE_MIRROR)), + # TODO(tandrii): remove PATCH_PROJECT field. + # DON'T USE THIS. WILL BE REMOVED. PATCH_PROJECT = Static(str(PATCH_PROJECT), hidden=True), BUILDSPEC_VERSION= Static(BUILDSPEC_VERSION, hidden=True), ) @@ -146,18 +162,10 @@ def chromium_bare(c): p['parent_got_v8_revision'] = 'v8_revision' p['parent_got_webrtc_revision'] = 'webrtc_revision' - # Patch project revisions are applied whenever patch_project is set. E.g. if - # a v8 stand-alone patch is sent to a chromium trybot, patch_project is v8 - # and can be used to sync v8 to HEAD instead of the pinned chromium - # version. - patch_project_revisions = { - 'v8': ('src/v8', 'HEAD'), - } - - patch_revision = patch_project_revisions.get(c.PATCH_PROJECT) - # TODO(phajdan.jr): Move to proper repo and add coverage. - if patch_revision: # pragma: no cover - c.revisions[patch_revision[0]] = patch_revision[1] + p = c.patch_projects + p['v8'] = ('src/v8', 'HEAD') + p['angle/angle'] = ('src/third_party/angle', None) + p['blink'] = ('src/third_party/WebKit', None) @config_ctx(includes=['chromium_bare']) def chromium_empty(c): @@ -538,6 +546,10 @@ def infra(c): soln.url = 'https://chromium.googlesource.com/infra/infra.git' c.got_revision_mapping['infra'] = 'got_revision' + p = c.patch_projects + p['luci-py'] = ('infra/luci', 'HEAD') + p['recipes-py'] = ('infra/recipes-py', 'HEAD') + @config_ctx(config_vars={'GIT_MODE': True}) def infra_internal(c): # pragma: no cover soln = c.solutions.add() @@ -572,6 +584,7 @@ def luci_py(c): # luci-py is checked out as part of infra just to have appengine # pre-installed, as that's what luci-py PRESUBMIT relies on. c.revisions['infra'] = 'origin/master' + # TODO(tandrii): make use of c.patch_projects. c.revisions['infra/luci'] = ( gclient_api.RevisionFallbackChain('origin/master')) m = c.got_revision_mapping @@ -581,6 +594,7 @@ def luci_py(c): @config_ctx(includes=['infra']) def recipes_py(c): c.revisions['infra'] = 'origin/master' + # TODO(tandrii): make use of c.patch_projects. c.revisions['infra/recipes-py'] = ( gclient_api.RevisionFallbackChain('origin/master')) m = c.got_revision_mapping @@ -634,6 +648,7 @@ def angle_top_of_tree(c): # pragma: no cover Sets up ToT instead of the DEPS-pinned revision for ANGLE. """ + # TODO(tandrii): I think patch_projects in bare_chromium fixed this. c.solutions[0].revision = 'HEAD' c.revisions['src/third_party/angle'] = 'HEAD' diff --git a/recipe_modules/rietveld/api.py b/recipe_modules/rietveld/api.py index aa7374fc3..b52ec0bc9 100644 --- a/recipe_modules/rietveld/api.py +++ b/recipe_modules/rietveld/api.py @@ -11,6 +11,11 @@ class RietveldApi(recipe_api.RecipeApi): def calculate_issue_root(self, extra_patch_project_roots=None): """Returns path where a patch should be applied to based on "patch_project". + YOU SHOULD NOT USE THIS METHOD. Put this into gclient's config as + patch_projects instead, and with luck you won't need to use + calculate_patch_root from gclient api. + TODO(tandrii): remove this method completely. See http://crbug.com/605563. + Maps Rietveld's "patch_project" to a path of directories relative to api.gclient.c.solutions[0].name which describe where to place the patch.