From 31f3e63cbc8d86bc4b012bd44b1e44336002a23d Mon Sep 17 00:00:00 2001 From: Daniel Jacques Date: Fri, 14 Jul 2017 01:12:23 +0000 Subject: [PATCH] Revert "Revert "[tryserver] Remove unused methods."" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit cc27ecb005d4c62be5580d6d22c9b246a84d81c9. Reason for revert: Published this method downstream to internal repo. Original change's description: > Revert "[tryserver] Remove unused methods." > > This reverts commit 133ac1ab8de3617f0a68b0011d4c231083f8983c. > > Reason for revert: Turns out these are used by internal recipes. > > Original change's description: > > [tryserver] Remove unused methods. > > > > R=​agable@chromium.org, dnj@chromium.org, hinoka@chromium.org > > > > Bug: > > Change-Id: I82a11f31c8c1c4c4a2b461090e5aee637f8821c2 > > Reviewed-on: https://chromium-review.googlesource.com/569019 > > Reviewed-by: Nodir Turakulov > > Reviewed-by: Aaron Gable > > Commit-Queue: Robbie Iannucci > > TBR=iannucci@chromium.org,hinoka@chromium.org,agable@chromium.org,dnj@chromium.org,nodir@chromium.org > > Change-Id: Ib1d4192520a36f649f1f9b31e2928027667311d4 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Reviewed-on: https://chromium-review.googlesource.com/570988 > Reviewed-by: Daniel Jacques > Commit-Queue: Daniel Jacques TBR=iannucci@chromium.org,hinoka@chromium.org,agable@chromium.org,dnj@chromium.org,nodir@chromium.org Change-Id: Id7ac3555d40162e4204ceac5e96c2e3864c67aba No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/570781 Reviewed-by: Daniel Jacques Commit-Queue: Daniel Jacques --- recipes/README.recipes.md | 52 ++++-------- recipes/recipe_modules/tryserver/api.py | 80 +------------------ .../full.expected/with_git_patch.json | 69 ---------------- .../full.expected/with_git_patch_luci.json | 34 +++++++- .../full.expected/with_rietveld_patch.json | 20 ----- .../with_rietveld_patch_new.json | 20 ----- .../recipe_modules/tryserver/examples/full.py | 1 - 7 files changed, 51 insertions(+), 225 deletions(-) diff --git a/recipes/README.recipes.md b/recipes/README.recipes.md index 9d130133c..0047667c8 100644 --- a/recipes/README.recipes.md +++ b/recipes/README.recipes.md @@ -658,29 +658,21 @@ Returns: [DEPS](/recipes/recipe_modules/tryserver/__init__.py#5): [gerrit](#recipe_modules-gerrit), [git](#recipe_modules-git), [git\_cl](#recipe_modules-git_cl), [rietveld](#recipe_modules-rietveld), [recipe\_engine/context][recipe_engine/recipe_modules/context], [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] -#### **class [TryserverApi](/recipes/recipe_modules/tryserver/api.py#16)([RecipeApi][recipe_engine/wkt/RecipeApi]):** +#### **class [TryserverApi](/recipes/recipe_modules/tryserver/api.py#12)([RecipeApi][recipe_engine/wkt/RecipeApi]):** -— **def [\_\_init\_\_](/recipes/recipe_modules/tryserver/api.py#17)(self, \*args, \*\*kwargs):** +— **def [\_\_init\_\_](/recipes/recipe_modules/tryserver/api.py#13)(self, \*args, \*\*kwargs):** -— **def [add\_failure\_reason](/recipes/recipe_modules/tryserver/api.py#224)(self, reason):** +— **def [add\_failure\_reason](/recipes/recipe_modules/tryserver/api.py#146)(self, reason):** Records a more detailed reason why build is failing. The reason can be any JSON-serializable object. -— **def [apply\_from\_git](/recipes/recipe_modules/tryserver/api.py#67)(self, cwd):** - -Downloads patch from given git repo and ref and applies it - -  **@property**
— **def [can\_apply\_issue](/recipes/recipe_modules/tryserver/api.py#27)(self):** +  **@property**
— **def [can\_apply\_issue](/recipes/recipe_modules/tryserver/api.py#23)(self):** Returns true iff the properties exist to apply_issue from rietveld. -— **def [determine\_patch\_storage](/recipes/recipe_modules/tryserver/api.py#94)(self):** - -Determines patch_storage automatically based on properties. - -— **def [get\_files\_affected\_by\_patch](/recipes/recipe_modules/tryserver/api.py#124)(self, patch_root=None, \*\*kwargs):** +— **def [get\_files\_affected\_by\_patch](/recipes/recipe_modules/tryserver/api.py#46)(self, patch_root=None, \*\*kwargs):** Returns list of paths to files affected by the patch. @@ -694,44 +686,34 @@ TODO(tandrii): remove this doc. Unless you use patch_root=None, in which case old behavior is used which returns paths relative to checkout aka solution[0].name. -— **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#287)(self, tag, patch_text=None):** +— **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#209)(self, tag, patch_text=None):** Gets a specific tag from a CL description -— **def [get\_footers](/recipes/recipe_modules/tryserver/api.py#260)(self, patch_text=None):** +— **def [get\_footers](/recipes/recipe_modules/tryserver/api.py#182)(self, patch_text=None):** Retrieves footers from the patch description. footers are machine readable tags embedded in commit messages. See git-footers documentation for more information. -  **@property**
— **def [is\_gerrit\_issue](/recipes/recipe_modules/tryserver/api.py#34)(self):** +  **@property**
— **def [is\_gerrit\_issue](/recipes/recipe_modules/tryserver/api.py#30)(self):** Returns true iff the properties exist to match a Gerrit issue. -  **@property**
— **def [is\_patch\_in\_git](/recipes/recipe_modules/tryserver/api.py#44)(self):** +  **@property**
— **def [is\_patch\_in\_git](/recipes/recipe_modules/tryserver/api.py#40)(self):** -  **@property**
— **def [is\_tryserver](/recipes/recipe_modules/tryserver/api.py#21)(self):** +  **@property**
— **def [is\_tryserver](/recipes/recipe_modules/tryserver/api.py#17)(self):** Returns true iff we can apply_issue or patch. -— **def [maybe\_apply\_issue](/recipes/recipe_modules/tryserver/api.py#103)(self, cwd=None, authentication=None):** - -If we're a trybot, apply a codereview issue. - -Args: - cwd: If specified, apply the patch from the specified directory. - authentication: authentication scheme whenever apply_issue.py is called. - This is only used if the patch comes from Rietveld. Possible values: - None, 'oauth2' (see also api.rietveld.apply_issue.) - -— **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#291)(self, footer):** +— **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#213)(self, footer):** -— **def [set\_compile\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#203)(self):** +— **def [set\_compile\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#125)(self):** Mark the tryjob result as a compile failure. -  **@contextlib.contextmanager**
— **def [set\_failure\_hash](/recipes/recipe_modules/tryserver/api.py#233)(self):** +  **@contextlib.contextmanager**
— **def [set\_failure\_hash](/recipes/recipe_modules/tryserver/api.py#155)(self):** Context manager that sets a failure_hash build property on StepFailure. @@ -740,7 +722,7 @@ for the same reason. For example, if a patch is bad (breaks something), we'd expect it to always break in the same way. Different failures for the same patch are usually a sign of flakiness. -— **def [set\_invalid\_test\_results\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#215)(self):** +— **def [set\_invalid\_test\_results\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#137)(self):** Mark the tryjob result as having invalid test results. @@ -748,18 +730,18 @@ This means we run some tests, but the results were not valid (e.g. no list of specific test cases that failed, or too many tests failing, etc). -— **def [set\_patch\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#199)(self):** +— **def [set\_patch\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#121)(self):** Mark the tryjob result as failure to apply the patch. -— **def [set\_subproject\_tag](/recipes/recipe_modules/tryserver/api.py#181)(self, subproject_tag):** +— **def [set\_subproject\_tag](/recipes/recipe_modules/tryserver/api.py#103)(self, subproject_tag):** Adds a subproject tag to the build. This can be used to distinguish between builds that execute different steps depending on what was patched, e.g. blink vs. pure chromium patches. -— **def [set\_test\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#207)(self):** +— **def [set\_test\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#129)(self):** Mark the tryjob result as a test failure. diff --git a/recipes/recipe_modules/tryserver/api.py b/recipes/recipe_modules/tryserver/api.py index 89f68b7b7..a78e7a021 100644 --- a/recipes/recipe_modules/tryserver/api.py +++ b/recipes/recipe_modules/tryserver/api.py @@ -9,10 +9,6 @@ import hashlib from recipe_engine import recipe_api -PATCH_STORAGE_RIETVELD = 'rietveld' -PATCH_STORAGE_GIT = 'git' - - class TryserverApi(recipe_api.RecipeApi): def __init__(self, *args, **kwargs): super(TryserverApi, self).__init__(*args, **kwargs) @@ -43,84 +39,10 @@ class TryserverApi(recipe_api.RecipeApi): @property def is_patch_in_git(self): - return (self.m.properties.get('patch_storage') == PATCH_STORAGE_GIT and + return (self.m.properties.get('patch_storage') == 'git' and self.m.properties.get('patch_repo_url') and self.m.properties.get('patch_ref')) - def _apply_patch_step(self, patch_file=None, patch_content=None, root=None): - assert not (patch_file and patch_content), ( - 'Please only specify either patch_file or patch_content, not both!') - patch_cmd = [ - 'patch', - '--dir', root or self.m.path['checkout'], - '--force', - '--forward', - '--remove-empty-files', - '--strip', '0', - ] - if patch_file: - patch_cmd.extend(['--input', patch_file]) - - self.m.step('apply patch', patch_cmd, - stdin=patch_content) - - def apply_from_git(self, cwd): - """Downloads patch from given git repo and ref and applies it""" - # TODO(nodir): accept these properties as parameters - patch_repo_url = self.m.properties['patch_repo_url'] - patch_ref = self.m.properties['patch_ref'] - - patch_dir = self.m.path.mkdtemp('patch') - try: - build_path = self.m.path['build'] - except KeyError: - raise self.m.step.StepFailure( - 'path["build"] is not defined. ' - 'Possibly this is a LUCI build. ' - 'tryserver.apply_from_git is not supported in LUCI builds.') - - git_setup_py = build_path.join('scripts', 'slave', 'git_setup.py') - git_setup_args = ['--path', patch_dir, '--url', patch_repo_url] - patch_path = patch_dir.join('patch.diff') - - self.m.python('patch git setup', git_setup_py, git_setup_args) - with self.m.context(cwd=patch_dir): - self.m.git('fetch', 'origin', patch_ref, name='patch fetch') - self.m.git('clean', '-f', '-d', '-x', name='patch clean') - self.m.git('checkout', '-f', 'FETCH_HEAD', name='patch git checkout') - self._apply_patch_step(patch_file=patch_path, root=cwd) - self.m.step('remove patch', ['rm', '-rf', patch_dir]) - - def determine_patch_storage(self): - """Determines patch_storage automatically based on properties.""" - storage = self.m.properties.get('patch_storage') - if storage: - return storage - - if self.can_apply_issue: - return PATCH_STORAGE_RIETVELD - - def maybe_apply_issue(self, cwd=None, authentication=None): - """If we're a trybot, apply a codereview issue. - - Args: - cwd: If specified, apply the patch from the specified directory. - authentication: authentication scheme whenever apply_issue.py is called. - This is only used if the patch comes from Rietveld. Possible values: - None, 'oauth2' (see also api.rietveld.apply_issue.) - """ - storage = self.determine_patch_storage() - - if storage == PATCH_STORAGE_RIETVELD: - return self.m.rietveld.apply_issue( - self.m.rietveld.calculate_issue_root(), - authentication=authentication) - elif storage == PATCH_STORAGE_GIT: - return self.apply_from_git(cwd) - else: - # Since this method is "maybe", we don't raise an Exception. - pass - def get_files_affected_by_patch(self, patch_root=None, **kwargs): """Returns list of paths to files affected by the patch. diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch.json index 105d99507..de0706341 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch.json @@ -1,73 +1,4 @@ [ - { - "cmd": [ - "python", - "-u", - "[BUILD]/scripts/slave/git_setup.py", - "--path", - "[TMP_BASE]/patch_tmp_1", - "--url", - "http://patch.url/" - ], - "name": "patch git setup" - }, - { - "cmd": [ - "git", - "fetch", - "origin", - "johndoe#123.diff" - ], - "cwd": "[TMP_BASE]/patch_tmp_1", - "infra_step": true, - "name": "patch fetch" - }, - { - "cmd": [ - "git", - "clean", - "-f", - "-d", - "-x" - ], - "cwd": "[TMP_BASE]/patch_tmp_1", - "infra_step": true, - "name": "patch clean" - }, - { - "cmd": [ - "git", - "checkout", - "-f", - "FETCH_HEAD" - ], - "cwd": "[TMP_BASE]/patch_tmp_1", - "infra_step": true, - "name": "patch git checkout" - }, - { - "cmd": [ - "patch", - "--dir", - "[START_DIR]", - "--force", - "--forward", - "--remove-empty-files", - "--strip", - "0", - "--input", - "[TMP_BASE]/patch_tmp_1/patch.diff" - ], - "name": "apply patch" - }, - { - "cmd": [ - "rm", - "-rf", - "[TMP_BASE]/patch_tmp_1" - ], - "name": "remove patch" - }, { "cmd": [ "git", diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch_luci.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch_luci.json index 70f1df03f..de0706341 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch_luci.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch_luci.json @@ -1,7 +1,39 @@ [ + { + "cmd": [ + "git", + "diff", + "--cached", + "--name-only" + ], + "cwd": "[START_DIR]/v8", + "infra_step": true, + "name": "git diff to analyze patch", + "stdout": "/path/to/tmp/", + "~followup_annotations": [ + "@@@STEP_LOG_LINE@files@v8/foo.cc@@@", + "@@@STEP_LOG_END@files@@@", + "@@@SET_BUILD_PROPERTY@failure_type@\"INVALID_TEST_RESULTS\"@@@", + "@@@SET_BUILD_PROPERTY@subproject_tag@\"v8\"@@@" + ] + }, + { + "cmd": [ + "python", + "-u", + "import sys; sys.exit(1)" + ], + "name": "fail", + "~followup_annotations": [ + "step returned non-zero exit code: 1", + "@@@STEP_TEXT@foo@@@", + "@@@STEP_FAILURE@@@", + "@@@SET_BUILD_PROPERTY@failure_hash@\"c2311ad770732eade3d2f48247abd147e40a70e7\"@@@" + ] + }, { "name": "$result", - "reason": "path[\"build\"] is not defined. Possibly this is a LUCI build. tryserver.apply_from_git is not supported in LUCI builds.", + "reason": "Step('fail') failed with return_code 1", "recipe_result": null, "status_code": 1 } diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch.json index 291677b60..48457e576 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch.json @@ -1,24 +1,4 @@ [ - { - "cmd": [ - "python", - "-u", - "RECIPE_PACKAGE_REPO[depot_tools]/apply_issue.py", - "-r", - "[START_DIR]", - "-i", - "12853011", - "-p", - "1", - "-s", - "https://codereview.chromium.org", - "--no-auth" - ], - "name": "apply_issue", - "~followup_annotations": [ - "@@@STEP_LINK@Applied issue 12853011@https://codereview.chromium.org/12853011@@@" - ] - }, { "cmd": [ "RECIPE_PACKAGE_REPO[depot_tools]/git_cl.py", diff --git a/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch_new.json b/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch_new.json index 4138eca50..950948d88 100644 --- a/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch_new.json +++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_rietveld_patch_new.json @@ -1,24 +1,4 @@ [ - { - "cmd": [ - "python", - "-u", - "RECIPE_PACKAGE_REPO[depot_tools]/apply_issue.py", - "-r", - "[START_DIR]", - "-i", - "12853011", - "-p", - "1", - "-s", - "https://codereview.chromium.org", - "--no-auth" - ], - "name": "apply_issue", - "~followup_annotations": [ - "@@@STEP_LINK@Applied issue 12853011@https://codereview.chromium.org/12853011@@@" - ] - }, { "cmd": [ "RECIPE_PACKAGE_REPO[depot_tools]/git_cl.py", diff --git a/recipes/recipe_modules/tryserver/examples/full.py b/recipes/recipe_modules/tryserver/examples/full.py index 08a149a4d..4a6a776aa 100644 --- a/recipes/recipe_modules/tryserver/examples/full.py +++ b/recipes/recipe_modules/tryserver/examples/full.py @@ -28,7 +28,6 @@ def RunSteps(api): 'Foo', api.properties['patch_text']))]) return - api.tryserver.maybe_apply_issue() if api.tryserver.can_apply_issue or api.tryserver.is_gerrit_issue: api.tryserver.get_footers() api.tryserver.get_files_affected_by_patch(