Make tryserver API only update the presentation of its own steps.

To sanely support concurrency, I need to make `api.step.nest` strictly scoped; i.e. once you leave the nest context it closes all the steps within that context. This changes some wild 'active_step' uses to generate a step for the module's own purpose. R=martiniss@chromium.org, nodir@chromium.org, tandrii@chromium.org Recipe-Nontrivial-Roll: build Bug: 910369 Change-Id: I19aef31118faf9d3d078a2c5a0ebd48a4bfebd43 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1630044 Commit-Queue: Robbie Iannucci <iannucci@chromium.org> Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

Make tryserver API only update the presentation of its own steps.
To sanely support concurrency, I need to make `api.step.nest` strictly scoped; i.e. once you leave the nest context it closes all the steps within that context. This changes some wild 'active_step' uses to generate a step for the module's own purpose. R=martiniss@chromium.org, nodir@chromium.org, tandrii@chromium.org Recipe-Nontrivial-Roll: build Bug: 910369 Change-Id: I19aef31118faf9d3d078a2c5a0ebd48a4bfebd43 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/1630044 Commit-Queue: Robbie Iannucci <iannucci@chromium.org> Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
778c7f11 · Robert Iannucci · Commit Bot · 26af0d34 · 778c7f11 · 778c7f11
Commit 778c7f11 authored May 28, 2019 by Robert Iannucci Committed by Commit Bot May 28, 2019
9 changed files
--- a/recipes/README.recipes.md
+++ b/recipes/README.recipes.md
@@ -753,7 +753,7 @@ Return a presubmit step.

 #### **class [TryserverApi](/recipes/recipe_modules/tryserver/api.py#11)([RecipeApi][recipe_engine/wkt/RecipeApi]):**

-&mdash; **def [add\_failure\_reason](/recipes/recipe_modules/tryserver/api.py#215)(self, reason):**
+&mdash; **def [add\_failure\_reason](/recipes/recipe_modules/tryserver/api.py#220)(self, reason):**

 Records a more detailed reason why build is failing.

@@ -793,11 +793,11 @@ Argument:

 Returned paths will be relative to to patch_root.

-&mdash; **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#271)(self, tag, patch_text=None):**
+&mdash; **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#278)(self, tag, patch_text=None):**

 Gets a specific tag from a CL description

-&mdash; **def [get\_footers](/recipes/recipe_modules/tryserver/api.py#251)(self, patch_text=None):**
+&mdash; **def [get\_footers](/recipes/recipe_modules/tryserver/api.py#258)(self, patch_text=None):**

 Retrieves footers from the patch description.

@@ -816,20 +816,20 @@ Returns true iff the properties exist to match a Gerrit issue.

 Returns true iff we have a change to check out.

-&mdash; **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#275)(self, footer):**
+&mdash; **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#282)(self, footer):**

-&mdash; **def [set\_compile\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#194)(self):**
+&mdash; **def [set\_compile\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#199)(self):**

 Mark the tryjob result as a compile failure.

-&mdash; **def [set\_do\_not\_retry\_build](/recipes/recipe_modules/tryserver/api.py#181)(self):**
+&mdash; **def [set\_do\_not\_retry\_build](/recipes/recipe_modules/tryserver/api.py#185)(self):**

 A flag to indicate the build should not be retried by the CQ.

 This mechanism is used to reduce CQ duration when retrying will likely
 return an identical result.

-&emsp; **@contextlib.contextmanager**<br>&mdash; **def [set\_failure\_hash](/recipes/recipe_modules/tryserver/api.py#224)(self):**
+&emsp; **@contextlib.contextmanager**<br>&mdash; **def [set\_failure\_hash](/recipes/recipe_modules/tryserver/api.py#229)(self):**

 Context manager that sets a failure_hash build property on StepFailure.

@@ -838,7 +838,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.

-&mdash; **def [set\_invalid\_test\_results\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#206)(self):**
+&mdash; **def [set\_invalid\_test\_results\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#211)(self):**

 Mark the tryjob result as having invalid test results.

@@ -846,7 +846,7 @@ 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).

-&mdash; **def [set\_patch\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#190)(self):**
+&mdash; **def [set\_patch\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#195)(self):**

 Mark the tryjob result as failure to apply the patch.

@@ -857,7 +857,7 @@ 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.

-&mdash; **def [set\_test\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#198)(self):**
+&mdash; **def [set\_test\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#203)(self):**

 Mark the tryjob result as a test failure.


--- 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
@@ -144,8 +144,16 @@
    "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@failure_type@\"PATCH_FAILURE\"@@@"
+      "@@@SET_BUILD_PROPERTY@$recipe_engine/buildbucket/output_gitiles_commit@{\"host\": \"fake.org\", \"id\": \"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\", \"position\": 170242, \"project\": \"src\", \"ref\": \"refs/heads/master\"}@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@PATCH_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"PATCH_FAILURE\"@@@",
+      "@@@STEP_FAILURE@@@"
    ]
  },
  {

--- a/recipes/recipe_modules/tryserver/api.py
+++ b/recipes/recipe_modules/tryserver/api.py
@@ -168,15 +168,19 @@ class TryserverApi(recipe_api.RecipeApi):
    """
    assert self.is_tryserver

-    step_result = self.m.step.active_result
+    step_result = self.m.step('TRYJOB SET SUBPROJECT_TAG', cmd=None)
    step_result.presentation.properties['subproject_tag'] = subproject_tag
+    step_result.presentation.step_text = subproject_tag

  def _set_failure_type(self, failure_type):
    if not self.is_tryserver:
      return

-    step_result = self.m.step.active_result
+    # TODO(iannucci): add API to set properties regardless of the current step.
+    step_result = self.m.step('TRYJOB FAILURE', cmd=None)
    step_result.presentation.properties['failure_type'] = failure_type
+    step_result.presentation.step_text = failure_type
+    step_result.presentation.status = 'FAILURE'

  def set_do_not_retry_build(self):
    """A flag to indicate the build should not be retried by the CQ.
@@ -184,7 +188,8 @@ class TryserverApi(recipe_api.RecipeApi):
    This mechanism is used to reduce CQ duration when retrying will likely
    return an identical result.
    """
-    step_result = self.m.step.active_result
+    # TODO(iannucci): add API to set properties regardless of the current step.
+    step_result = self.m.step('TRYJOB DO NOT RETRY', cmd=None)
    step_result.presentation.properties['do_not_retry'] = True

  def set_patch_failure_tryjob_result(self):
@@ -236,6 +241,8 @@ class TryserverApi(recipe_api.RecipeApi):
    except self.m.step.StepFailure as e:
      self.add_failure_reason(e.reason)

+      # TODO(iannucci): add API to set properties regardless of the current
+      # step.
      try:
        step_result = self.m.step.active_result
      except ValueError:

--- a/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch.json
+++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch.json
@@ -116,12 +116,60 @@
    "stdout": "/path/to/tmp/",
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@files@None/foo.cc@@@",
-      "@@@STEP_LOG_END@files@@@",
-      "@@@SET_BUILD_PROPERTY@do_not_retry@true@@@",
-      "@@@SET_BUILD_PROPERTY@failure_type@\"INVALID_TEST_RESULTS\"@@@",
+      "@@@STEP_LOG_END@files@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB SET SUBPROJECT_TAG",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@v8@@@",
      "@@@SET_BUILD_PROPERTY@subproject_tag@\"v8\"@@@"
    ]
  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@PATCH_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"PATCH_FAILURE\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB DO NOT RETRY",
+    "~followup_annotations": [
+      "@@@SET_BUILD_PROPERTY@do_not_retry@true@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE (2)",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@COMPILE_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"COMPILE_FAILURE\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE (3)",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@TEST_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"TEST_FAILURE\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE (4)",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@INVALID_TEST_RESULTS@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"INVALID_TEST_RESULTS\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
  {
    "cmd": [
      "python",

--- a/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch_and_target_ref.json
+++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_gerrit_patch_and_target_ref.json
@@ -116,12 +116,60 @@
    "stdout": "/path/to/tmp/",
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@files@None/foo.cc@@@",
-      "@@@STEP_LOG_END@files@@@",
-      "@@@SET_BUILD_PROPERTY@do_not_retry@true@@@",
-      "@@@SET_BUILD_PROPERTY@failure_type@\"INVALID_TEST_RESULTS\"@@@",
+      "@@@STEP_LOG_END@files@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB SET SUBPROJECT_TAG",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@v8@@@",
      "@@@SET_BUILD_PROPERTY@subproject_tag@\"v8\"@@@"
    ]
  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@PATCH_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"PATCH_FAILURE\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB DO NOT RETRY",
+    "~followup_annotations": [
+      "@@@SET_BUILD_PROPERTY@do_not_retry@true@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE (2)",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@COMPILE_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"COMPILE_FAILURE\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE (3)",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@TEST_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"TEST_FAILURE\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE (4)",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@INVALID_TEST_RESULTS@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"INVALID_TEST_RESULTS\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
  {
    "cmd": [
      "python",

--- a/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch.json
+++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_git_patch.json
@@ -14,12 +14,60 @@
    "stdout": "/path/to/tmp/",
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@files@v8/foo.cc@@@",
-      "@@@STEP_LOG_END@files@@@",
-      "@@@SET_BUILD_PROPERTY@do_not_retry@true@@@",
-      "@@@SET_BUILD_PROPERTY@failure_type@\"INVALID_TEST_RESULTS\"@@@",
+      "@@@STEP_LOG_END@files@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB SET SUBPROJECT_TAG",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@v8@@@",
      "@@@SET_BUILD_PROPERTY@subproject_tag@\"v8\"@@@"
    ]
  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@PATCH_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"PATCH_FAILURE\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB DO NOT RETRY",
+    "~followup_annotations": [
+      "@@@SET_BUILD_PROPERTY@do_not_retry@true@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE (2)",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@COMPILE_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"COMPILE_FAILURE\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE (3)",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@TEST_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"TEST_FAILURE\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE (4)",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@INVALID_TEST_RESULTS@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"INVALID_TEST_RESULTS\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
  {
    "cmd": [
      "python",

--- 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
@@ -14,12 +14,60 @@
    "stdout": "/path/to/tmp/",
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@files@v8/foo.cc@@@",
-      "@@@STEP_LOG_END@files@@@",
-      "@@@SET_BUILD_PROPERTY@do_not_retry@true@@@",
-      "@@@SET_BUILD_PROPERTY@failure_type@\"INVALID_TEST_RESULTS\"@@@",
+      "@@@STEP_LOG_END@files@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB SET SUBPROJECT_TAG",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@v8@@@",
      "@@@SET_BUILD_PROPERTY@subproject_tag@\"v8\"@@@"
    ]
  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@PATCH_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"PATCH_FAILURE\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB DO NOT RETRY",
+    "~followup_annotations": [
+      "@@@SET_BUILD_PROPERTY@do_not_retry@true@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE (2)",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@COMPILE_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"COMPILE_FAILURE\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE (3)",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@TEST_FAILURE@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"TEST_FAILURE\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB FAILURE (4)",
+    "~followup_annotations": [
+      "@@@STEP_TEXT@INVALID_TEST_RESULTS@@@",
+      "@@@SET_BUILD_PROPERTY@failure_type@\"INVALID_TEST_RESULTS\"@@@",
+      "@@@STEP_FAILURE@@@"
+    ]
+  },
  {
    "cmd": [
      "python",

--- a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json
+++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json
@@ -13,7 +13,13 @@
    "stdout": "/path/to/tmp/",
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@files@foo.cc@@@",
-      "@@@STEP_LOG_END@files@@@",
+      "@@@STEP_LOG_END@files@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB DO NOT RETRY",
+    "~followup_annotations": [
      "@@@SET_BUILD_PROPERTY@do_not_retry@true@@@"
    ]
  },

--- a/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json
+++ b/recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch_new.json
@@ -14,7 +14,13 @@
    "stdout": "/path/to/tmp/",
    "~followup_annotations": [
      "@@@STEP_LOG_LINE@files@sub/project/foo.cc@@@",
-      "@@@STEP_LOG_END@files@@@",
+      "@@@STEP_LOG_END@files@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "TRYJOB DO NOT RETRY",
+    "~followup_annotations": [
      "@@@SET_BUILD_PROPERTY@do_not_retry@true@@@"
    ]
  },