Make patch failures from bot_update fail in ensure_checkout

Patch failures in bot_update return exit code 87 and 88, which are ignored by ensure_checkout. This causes the bot_update step in recipes to be successful despite a patch failure. This change removes these exit codes from the accepted return codes for this step. Bug: 1207685 Change-Id: I7bd71732a99184c96196659c6953e4d4b610c9b9 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2893686 Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org> Commit-Queue: Gavin Mak <gavinmak@google.com>
4 years ago · 6b0a611c2c
parent 6a3e09171c
commit 6b0a611c2c
4 changed files with 47 additions and 94 deletions
--- a/recipes/README.recipes.md
+++ b/recipes/README.recipes.md
@ -58,7 +58,7 @@ Recipe module to ensure a checkout is consistent on a bot.

 Wrapper for easy calling of bot_update.

-&mdash; **def [deapply\_patch](/recipes/recipe_modules/bot_update/api.py#539)(self, bot_update_step):**
+&mdash; **def [deapply\_patch](/recipes/recipe_modules/bot_update/api.py#529)(self, bot_update_step):**

 Deapplies a patch, taking care of DEPS and solution revisions properly.
    
@ -95,7 +95,7 @@ Args:
    bot_update module ONLY supports one change. Users may specify a change
    via tryserver.set_change() and explicitly set this flag False.

-&mdash; **def [get\_project\_revision\_properties](/recipes/recipe_modules/bot_update/api.py#516)(self, project_name, gclient_config=None):**
+&mdash; **def [get\_project\_revision\_properties](/recipes/recipe_modules/bot_update/api.py#506)(self, project_name, gclient_config=None):**

 Returns all property names used for storing the checked-out revision of
 a given project.
@ -111,7 +111,7 @@ Returns (list of str): All properties that'll hold the checked-out revision

 &emsp; **@property**<br>&mdash; **def [last\_returned\_properties](/recipes/recipe_modules/bot_update/api.py#45)(self):**

-&mdash; **def [resolve\_fixed\_revision](/recipes/recipe_modules/bot_update/api.py#467)(self, bot_update_json, name):**
+&mdash; **def [resolve\_fixed\_revision](/recipes/recipe_modules/bot_update/api.py#457)(self, bot_update_json, name):**

 Sets a fixed revision for a single dependency using project revision
 properties.
--- a/recipes/recipe_modules/bot_update/api.py
+++ b/recipes/recipe_modules/bot_update/api.py
@ -313,12 +313,7 @@ class BotUpdateApi(recipe_api.RecipeApi):
    # Ah hah! Now that everything is in place, lets run bot_update!
    step_result = None
    try:
-      # 87 and 88 are the 'patch failure' codes for patch download and patch
-      # apply, respectively. We don't actually use the error codes, and instead
-      # rely on emitted json to determine cause of failure.
-      step_result = self(
-           name, cmd, step_test_data=step_test_data,
-           ok_ret=(0, 87, 88), **kwargs)
+      step_result = self(name, cmd, step_test_data=step_test_data, **kwargs)
    except self.m.step.StepFailure as f:
      step_result = f.result
      raise
@ -338,6 +333,24 @@ class BotUpdateApi(recipe_api.RecipeApi):
          step_text = result['step_text']
          step_result.presentation.step_text = step_text

+        if result.get('patch_failure'):
+          patch_body = result.get('failed_patch_body')
+          if patch_body:
+            step_result.presentation.logs['patch error'] = (
+                patch_body.splitlines())
+
+          if result.get('patch_apply_return_code') == 3:
+            # This is download failure, hence an infra failure.
+            raise self.m.step.InfraFailure(
+                'Patch failure: Git reported a download failure')
+          else:
+            # This is actual patch failure.
+            self.m.tryserver.set_patch_failure_tryjob_result()
+            self.m.cq.set_do_not_retry_build()
+            raise self.m.step.StepFailure(
+                'Patch failure: See patch error log attached to bot_update. '
+                'Try rebasing?')
+
        if add_blamelists and 'manifest' in result:
          blamelist_pins = []
          for name in revisions:
@ -399,29 +412,6 @@ class BotUpdateApi(recipe_api.RecipeApi):
        # Set the "checkout" path for the main solution.
        # This is used by the Chromium module to figure out where to look for
        # the checkout.
-        # If there is a patch failure, emit another step that said things
-        # failed.
-        if result.get('patch_failure'):
-          return_code = result.get('patch_apply_return_code')
-          patch_body = result.get('failed_patch_body')
-          try:
-            if return_code == 3:
-              # This is download failure, hence an infra failure.
-              with self.m.context(infra_steps=True):
-                self.m.python.failing_step(
-                    'Patch failure', 'Git reported a download failure')
-            else:
-              # This is actual patch failure.
-              self.m.tryserver.set_patch_failure_tryjob_result()
-              self.m.cq.set_do_not_retry_build()
-              self.m.python.failing_step(
-                  'Patch failure', 'See attached log. Try rebasing?')
-          except self.m.step.StepFailure as e:
-            if patch_body:
-              e.result.presentation.logs['patch error'] = (
-                  patch_body.splitlines())
-            raise e
-
        # bot_update actually just sets root to be the folder name of the
        # first solution.
        if (result.get('did_run')
--- a/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_fail_patch.json
+++ b/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_fail_patch.json
@ -142,6 +142,19 @@
      "@@@STEP_LOG_LINE@json.output@  \"step_text\": \"Some step text\"@@@",
      "@@@STEP_LOG_LINE@json.output@}@@@",
      "@@@STEP_LOG_END@json.output@@@",
+      "@@@STEP_LOG_LINE@patch error@Downloading patch...@@@",
+      "@@@STEP_LOG_LINE@patch error@Applying the patch...@@@",
+      "@@@STEP_LOG_LINE@patch error@Patch: foo/bar.py@@@",
+      "@@@STEP_LOG_LINE@patch error@Index: foo/bar.py@@@",
+      "@@@STEP_LOG_LINE@patch error@diff --git a/foo/bar.py b/foo/bar.py@@@",
+      "@@@STEP_LOG_LINE@patch error@index HASH..HASH MODE@@@",
+      "@@@STEP_LOG_LINE@patch error@--- a/foo/bar.py@@@",
+      "@@@STEP_LOG_LINE@patch error@+++ b/foo/bar.py@@@",
+      "@@@STEP_LOG_LINE@patch error@context@@@",
+      "@@@STEP_LOG_LINE@patch error@+something@@@",
+      "@@@STEP_LOG_LINE@patch error@-something@@@",
+      "@@@STEP_LOG_LINE@patch error@more context@@@",
+      "@@@STEP_LOG_END@patch error@@@",
      "@@@SET_BUILD_PROPERTY@got_angle_revision@\"fac9503c46405f77757b9a728eb85b8d7bc6080c\"@@@",
      "@@@SET_BUILD_PROPERTY@got_angle_revision_cp@\"refs/heads/master@{#297276}\"@@@",
      "@@@SET_BUILD_PROPERTY@got_cr_revision@\"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\"@@@",
@ -149,14 +162,8 @@
      "@@@SET_BUILD_PROPERTY@got_revision@\"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\"@@@",
      "@@@SET_BUILD_PROPERTY@got_revision_cp@\"refs/heads/master@{#170242}\"@@@",
      "@@@SET_BUILD_PROPERTY@got_v8_revision@\"801ada225ddc271c132c3a35f03975671d43e399\"@@@",
-      "@@@SET_BUILD_PROPERTY@got_v8_revision_cp@\"refs/heads/master@{#43426}\"@@@"
-    ]
-  },
-  {
-    "cmd": [],
-    "name": "set_output_gitiles_commit",
-    "~followup_annotations": [
-      "@@@SET_BUILD_PROPERTY@$recipe_engine/buildbucket/output_gitiles_commit@{\"host\": \"fake.org\", \"id\": \"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\", \"position\": 170242, \"project\": \"src\", \"ref\": \"refs/heads/master\"}@@@"
+      "@@@SET_BUILD_PROPERTY@got_v8_revision_cp@\"refs/heads/master@{#43426}\"@@@",
+      "@@@STEP_EXCEPTION@@@"
    ]
  },
  {
@ -176,35 +183,10 @@
      "@@@SET_BUILD_PROPERTY@do_not_retry@true@@@"
    ]
  },
-  {
-    "cmd": [
-      "python",
-      "-u",
-      "import sys; sys.exit(1)"
-    ],
-    "name": "Patch failure",
-    "~followup_annotations": [
-      "@@@STEP_TEXT@See attached log. Try rebasing?@@@",
-      "@@@STEP_LOG_LINE@patch error@Downloading patch...@@@",
-      "@@@STEP_LOG_LINE@patch error@Applying the patch...@@@",
-      "@@@STEP_LOG_LINE@patch error@Patch: foo/bar.py@@@",
-      "@@@STEP_LOG_LINE@patch error@Index: foo/bar.py@@@",
-      "@@@STEP_LOG_LINE@patch error@diff --git a/foo/bar.py b/foo/bar.py@@@",
-      "@@@STEP_LOG_LINE@patch error@index HASH..HASH MODE@@@",
-      "@@@STEP_LOG_LINE@patch error@--- a/foo/bar.py@@@",
-      "@@@STEP_LOG_LINE@patch error@+++ b/foo/bar.py@@@",
-      "@@@STEP_LOG_LINE@patch error@context@@@",
-      "@@@STEP_LOG_LINE@patch error@+something@@@",
-      "@@@STEP_LOG_LINE@patch error@-something@@@",
-      "@@@STEP_LOG_LINE@patch error@more context@@@",
-      "@@@STEP_LOG_END@patch error@@@",
-      "@@@STEP_FAILURE@@@"
-    ]
-  },
  {
    "failure": {
      "failure": {},
-      "humanReason": "Step('Patch failure') (retcode: 1)"
+      "humanReason": "Patch failure: See patch error log attached to bot_update. Try rebasing?"
    },
    "name": "$result"
  }
--- a/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_fail_patch_download.json
+++ b/recipes/recipe_modules/bot_update/examples/full.expected/tryjob_fail_patch_download.json
@ -142,33 +142,6 @@
      "@@@STEP_LOG_LINE@json.output@  \"step_text\": \"Some step text\"@@@",
      "@@@STEP_LOG_LINE@json.output@}@@@",
      "@@@STEP_LOG_END@json.output@@@",
-      "@@@SET_BUILD_PROPERTY@got_angle_revision@\"fac9503c46405f77757b9a728eb85b8d7bc6080c\"@@@",
-      "@@@SET_BUILD_PROPERTY@got_angle_revision_cp@\"refs/heads/master@{#297276}\"@@@",
-      "@@@SET_BUILD_PROPERTY@got_cr_revision@\"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\"@@@",
-      "@@@SET_BUILD_PROPERTY@got_cr_revision_cp@\"refs/heads/master@{#170242}\"@@@",
-      "@@@SET_BUILD_PROPERTY@got_revision@\"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\"@@@",
-      "@@@SET_BUILD_PROPERTY@got_revision_cp@\"refs/heads/master@{#170242}\"@@@",
-      "@@@SET_BUILD_PROPERTY@got_v8_revision@\"801ada225ddc271c132c3a35f03975671d43e399\"@@@",
-      "@@@SET_BUILD_PROPERTY@got_v8_revision_cp@\"refs/heads/master@{#43426}\"@@@"
-    ]
-  },
-  {
-    "cmd": [],
-    "name": "set_output_gitiles_commit",
-    "~followup_annotations": [
-      "@@@SET_BUILD_PROPERTY@$recipe_engine/buildbucket/output_gitiles_commit@{\"host\": \"fake.org\", \"id\": \"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\", \"position\": 170242, \"project\": \"src\", \"ref\": \"refs/heads/master\"}@@@"
-    ]
-  },
-  {
-    "cmd": [
-      "python",
-      "-u",
-      "import sys; sys.exit(1)"
-    ],
-    "infra_step": true,
-    "name": "Patch failure",
-    "~followup_annotations": [
-      "@@@STEP_TEXT@Git reported a download failure@@@",
      "@@@STEP_LOG_LINE@patch error@Downloading patch...@@@",
      "@@@STEP_LOG_LINE@patch error@Applying the patch...@@@",
      "@@@STEP_LOG_LINE@patch error@Patch: foo/bar.py@@@",
@ -182,12 +155,20 @@
      "@@@STEP_LOG_LINE@patch error@-something@@@",
      "@@@STEP_LOG_LINE@patch error@more context@@@",
      "@@@STEP_LOG_END@patch error@@@",
+      "@@@SET_BUILD_PROPERTY@got_angle_revision@\"fac9503c46405f77757b9a728eb85b8d7bc6080c\"@@@",
+      "@@@SET_BUILD_PROPERTY@got_angle_revision_cp@\"refs/heads/master@{#297276}\"@@@",
+      "@@@SET_BUILD_PROPERTY@got_cr_revision@\"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\"@@@",
+      "@@@SET_BUILD_PROPERTY@got_cr_revision_cp@\"refs/heads/master@{#170242}\"@@@",
+      "@@@SET_BUILD_PROPERTY@got_revision@\"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\"@@@",
+      "@@@SET_BUILD_PROPERTY@got_revision_cp@\"refs/heads/master@{#170242}\"@@@",
+      "@@@SET_BUILD_PROPERTY@got_v8_revision@\"801ada225ddc271c132c3a35f03975671d43e399\"@@@",
+      "@@@SET_BUILD_PROPERTY@got_v8_revision_cp@\"refs/heads/master@{#43426}\"@@@",
      "@@@STEP_EXCEPTION@@@"
    ]
  },
  {
    "failure": {
-      "humanReason": "Infra Failure: Step('Patch failure') (retcode: 1)"
+      "humanReason": "Patch failure: Git reported a download failure"
    },
    "name": "$result"
  }