From 33b199ffaf7d8f238da0515e160f29a6f28aadfe Mon Sep 17 00:00:00 2001 From: Ben Pastene Date: Wed, 20 Nov 2024 23:29:33 +0000 Subject: [PATCH] Surface git errors in the build summary for presubmit builds Occasionally someone will upload a CL whose diff was already landed in another CL. In the past, the error message from the presubmit build for their CL has led to confusion. So this tries to clarify it by: - putting the git stdout from the git cmd in the build summary when it fails - adding a hint about the identical diff in the build summary when it sees "nothing to commit" in git's stdout eg: Before this CL, such builds fail like: https://ci.chromium.org/ui/p/chromium/builders/try/chromium_presubmit/3035841/infra With this CL, they'll fail like: https://ci.chromium.org/ui/p/chromium/builders/try.shadow/chromium_presubmit/3883/infra Bug: None Change-Id: I8002e19efce3cae5a11d2e616e4db596afb3b50c Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6039934 Reviewed-by: Josip Sokcevic Commit-Queue: Ben Pastene --- recipes/README.recipes.md | 8 ++-- recipes/recipe_modules/presubmit/__init__.py | 27 +++++++------- recipes/recipe_modules/presubmit/api.py | 37 +++++++++++++------ .../recipe_modules/presubmit/tests/prepare.py | 33 +++++++++++++---- 4 files changed, 69 insertions(+), 36 deletions(-) diff --git a/recipes/README.recipes.md b/recipes/README.recipes.md index 8200747b7..65dc82bf5 100644 --- a/recipes/README.recipes.md +++ b/recipes/README.recipes.md @@ -825,7 +825,7 @@ Raises: — **def [initialize](/recipes/recipe_modules/osx_sdk/api.py#55)(self):** ### *recipe_modules* / [presubmit](/recipes/recipe_modules/presubmit) -[DEPS](/recipes/recipe_modules/presubmit/__init__.py#13): [bot\_update](#recipe_modules-bot_update), [depot\_tools](#recipe_modules-depot_tools), [gclient](#recipe_modules-gclient), [git](#recipe_modules-git), [tryserver](#recipe_modules-tryserver), [recipe\_engine/buildbucket][recipe_engine/recipe_modules/buildbucket], [recipe\_engine/context][recipe_engine/recipe_modules/context], [recipe\_engine/cv][recipe_engine/recipe_modules/cv], [recipe\_engine/json][recipe_engine/recipe_modules/json], [recipe\_engine/path][recipe_engine/recipe_modules/path], [recipe\_engine/properties][recipe_engine/recipe_modules/properties], [recipe\_engine/resultdb][recipe_engine/recipe_modules/resultdb], [recipe\_engine/step][recipe_engine/recipe_modules/step] +[DEPS](/recipes/recipe_modules/presubmit/__init__.py#13): [bot\_update](#recipe_modules-bot_update), [depot\_tools](#recipe_modules-depot_tools), [gclient](#recipe_modules-gclient), [git](#recipe_modules-git), [tryserver](#recipe_modules-tryserver), [recipe\_engine/buildbucket][recipe_engine/recipe_modules/buildbucket], [recipe\_engine/context][recipe_engine/recipe_modules/context], [recipe\_engine/cv][recipe_engine/recipe_modules/cv], [recipe\_engine/json][recipe_engine/recipe_modules/json], [recipe\_engine/path][recipe_engine/recipe_modules/path], [recipe\_engine/properties][recipe_engine/recipe_modules/properties], [recipe\_engine/raw\_io][recipe_engine/recipe_modules/raw_io], [recipe\_engine/resultdb][recipe_engine/recipe_modules/resultdb], [recipe\_engine/step][recipe_engine/recipe_modules/step] #### **class [PresubmitApi](/recipes/recipe_modules/presubmit/api.py#16)([RecipeApi][recipe_engine/wkt/RecipeApi]):** @@ -834,7 +834,7 @@ Raises: Returns a presubmit step. -— **def [execute](/recipes/recipe_modules/presubmit/api.py#103)(self, bot_update_step, skip_owners=False, run_all=False):** +— **def [execute](/recipes/recipe_modules/presubmit/api.py#116)(self, bot_update_step, skip_owners=False, run_all=False):** Runs presubmit and sets summary markdown if applicable. @@ -1212,10 +1212,10 @@ Move things around in a loop! — **def [RunSteps](/recipes/recipe_modules/presubmit/tests/execute.py#25)(api):** ### *recipes* / [presubmit:tests/prepare](/recipes/recipe_modules/presubmit/tests/prepare.py) -[DEPS](/recipes/recipe_modules/presubmit/tests/prepare.py#11): [gclient](#recipe_modules-gclient), [presubmit](#recipe_modules-presubmit), [recipe\_engine/buildbucket][recipe_engine/recipe_modules/buildbucket], [recipe\_engine/context][recipe_engine/recipe_modules/context], [recipe\_engine/path][recipe_engine/recipe_modules/path], [recipe\_engine/properties][recipe_engine/recipe_modules/properties], [recipe\_engine/runtime][recipe_engine/recipe_modules/runtime] +[DEPS](/recipes/recipe_modules/presubmit/tests/prepare.py#11): [gclient](#recipe_modules-gclient), [presubmit](#recipe_modules-presubmit), [recipe\_engine/buildbucket][recipe_engine/recipe_modules/buildbucket], [recipe\_engine/context][recipe_engine/recipe_modules/context], [recipe\_engine/path][recipe_engine/recipe_modules/path], [recipe\_engine/properties][recipe_engine/recipe_modules/properties], [recipe\_engine/raw\_io][recipe_engine/recipe_modules/raw_io], [recipe\_engine/runtime][recipe_engine/recipe_modules/runtime] -— **def [RunSteps](/recipes/recipe_modules/presubmit/tests/prepare.py#28)(api, patch_project, patch_repository_url):** +— **def [RunSteps](/recipes/recipe_modules/presubmit/tests/prepare.py#29)(api, patch_project, patch_repository_url):** ### *recipes* / [tryserver:examples/full](/recipes/recipe_modules/tryserver/examples/full.py) [DEPS](/recipes/recipe_modules/tryserver/examples/full.py#7): [gerrit](#recipe_modules-gerrit), [tryserver](#recipe_modules-tryserver), [recipe\_engine/buildbucket][recipe_engine/recipe_modules/buildbucket], [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/raw\_io][recipe_engine/recipe_modules/raw_io], [recipe\_engine/step][recipe_engine/recipe_modules/step] diff --git a/recipes/recipe_modules/presubmit/__init__.py b/recipes/recipe_modules/presubmit/__init__.py index c1150c2ce..686800974 100644 --- a/recipes/recipe_modules/presubmit/__init__.py +++ b/recipes/recipe_modules/presubmit/__init__.py @@ -11,19 +11,20 @@ from PB.recipe_modules.depot_tools.presubmit import properties PYTHON_VERSION_COMPATIBILITY = 'PY3' DEPS = [ - 'bot_update', - 'depot_tools', - 'gclient', - 'git', - 'recipe_engine/buildbucket', - 'recipe_engine/context', - 'recipe_engine/cv', - 'recipe_engine/json', - 'recipe_engine/path', - 'recipe_engine/properties', - 'recipe_engine/step', - 'recipe_engine/resultdb', - 'tryserver', + 'bot_update', + 'depot_tools', + 'gclient', + 'git', + 'recipe_engine/buildbucket', + 'recipe_engine/context', + 'recipe_engine/cv', + 'recipe_engine/json', + 'recipe_engine/path', + 'recipe_engine/properties', + 'recipe_engine/raw_io', + 'recipe_engine/resultdb', + 'recipe_engine/step', + 'tryserver', ] diff --git a/recipes/recipe_modules/presubmit/api.py b/recipes/recipe_modules/presubmit/api.py index 1eb6807a4..2719db654 100644 --- a/recipes/recipe_modules/presubmit/api.py +++ b/recipes/recipe_modules/presubmit/api.py @@ -81,18 +81,31 @@ class PresubmitApi(recipe_api.RecipeApi): # TODO(unowned): Consider either: # - extracting user name & email address from the issue, or # - using a dedicated and clearly nonexistent name/email address - self.m.git('-c', - 'user.email=commit-bot@chromium.org', - '-c', - 'user.name=The Commit Bot', - '-c', - 'diff.ignoreSubmodules=all', - 'commit', - '-a', - '-m', - 'Committed patch', - name='commit-git-patch', - infra_step=False) + step_result = self.m.git( + '-c', + 'user.email=commit-bot@chromium.org', + '-c', + 'user.name=The Commit Bot', + '-c', + 'diff.ignoreSubmodules=all', + 'commit', + '-a', + '-m', + 'Committed patch', + name='commit-git-patch', + raise_on_failure=False, + stdout=self.m.raw_io.output_text('stdout', + add_output_log='on_failure'), + infra_step=False, + ) + if step_result.retcode: + failure_md_lines = ['Failed to apply patch.'] + if step_result.stdout: + failure_md_lines += step_result.stdout.splitlines() + [''] + if 'nothing to commit' in step_result.stdout: + failure_md_lines.append( + 'Was an identical diff already submitted elsewhere?') + raise self.m.step.StepFailure('
'.join(failure_md_lines)) if self._runhooks: with self.m.context(cwd=self.m.path.checkout_dir): diff --git a/recipes/recipe_modules/presubmit/tests/prepare.py b/recipes/recipe_modules/presubmit/tests/prepare.py index fef0e7e7b..620eeba4e 100644 --- a/recipes/recipe_modules/presubmit/tests/prepare.py +++ b/recipes/recipe_modules/presubmit/tests/prepare.py @@ -9,13 +9,14 @@ from recipe_engine import recipe_api PYTHON_VERSION_COMPATIBILITY = 'PY3' DEPS = [ - 'gclient', - 'presubmit', - 'recipe_engine/buildbucket', - 'recipe_engine/context', - 'recipe_engine/path', - 'recipe_engine/properties', - 'recipe_engine/runtime', + 'gclient', + 'presubmit', + 'recipe_engine/buildbucket', + 'recipe_engine/context', + 'recipe_engine/path', + 'recipe_engine/properties', + 'recipe_engine/raw_io', + 'recipe_engine/runtime', ] @@ -43,3 +44,21 @@ def GenTests(api): api.post_process(post_process.MustRun, 'gclient runhooks') + api.post_process(post_process.StatusSuccess) + api.post_process(post_process.DropExpectation)) + + yield api.test( + 'failed_commit', + api.runtime(is_experimental=False), + api.buildbucket.try_build(project='infra'), + api.step_data( + 'commit-git-patch', + retcode=1, + stdout=api.raw_io.output_text( + 'nothing to commit, working tree clean'), + ), + api.expect_status('FAILURE'), + api.post_check( + post_process.SummaryMarkdownRE, + 'Was an identical diff already submitted elsewhere?', + ), + api.post_process(post_process.DropExpectation), + )