Enable set_output_commit to be used with RevisionFallbackChain.

When set_output_commit is True and the repo associated with the
got_revision property does not use commit positions, buiders whose
gclient config uses RevisionFallbackChain for that repo will raise an
exception when attempting set the output commit because a string is
expected for the revision value, not a RevisionFallbackChain object.
This changes the set_output_commit code to resolve the revision so that
the output commit can be set in those cases.

Bug: 1159749, 1141886
Change-Id: Iddf1d49871028d9500eefb0eed07e36ce7e6b0d6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2612223
Auto-Submit: Garrett Beaty <gbeaty@chromium.org>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>
Commit-Queue: Josip Sokcevic <sokcevic@google.com>

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.
 
-— **def [deapply\_patch](/recipes/recipe_modules/bot_update/api.py#528)(self, bot_update_step):**
+— **def [deapply\_patch](/recipes/recipe_modules/bot_update/api.py#530)(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.
 
-— **def [get\_project\_revision\_properties](/recipes/recipe_modules/bot_update/api.py#505)(self, project_name, gclient_config=None):**
+— **def [get\_project\_revision\_properties](/recipes/recipe_modules/bot_update/api.py#507)(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#39)(self):**
 
-&mdash; **def [resolve\_fixed\_revision](/recipes/recipe_modules/bot_update/api.py#456)(self, bot_update_json, name):**
+&mdash; **def [resolve\_fixed\_revision](/recipes/recipe_modules/bot_update/api.py#458)(self, bot_update_json, name):**
 
 Sets a fixed revision for a single dependency using project revision
 properties.
@@ -955,7 +955,7 @@ Raises:
 
 [DEPS](/recipes/recipe_modules/bot_update/examples/full.py#5): [bot\_update](#recipe_modules-bot_update), [gclient](#recipe_modules-gclient), [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/runtime][recipe_engine/recipe_modules/runtime]
 
-&mdash; **def [RunSteps](/recipes/recipe_modules/bot_update/examples/full.py#22)(api):**
+&mdash; **def [RunSteps](/recipes/recipe_modules/bot_update/examples/full.py#23)(api):**
 ### *recipes* / [bot\_update:tests/do\_not\_retry\_patch\_failures\_in\_cq](/recipes/recipe_modules/bot_update/tests/do_not_retry_patch_failures_in_cq.py)
 
 [DEPS](/recipes/recipe_modules/bot_update/tests/do_not_retry_patch_failures_in_cq.py#5): [bot\_update](#recipe_modules-bot_update), [gclient](#recipe_modules-gclient), [recipe\_engine/buildbucket][recipe_engine/recipe_modules/buildbucket], [recipe\_engine/cq][recipe_engine/recipe_modules/cq], [recipe\_engine/properties][recipe_engine/recipe_modules/properties], [recipe\_engine/step][recipe_engine/recipe_modules/step]

recipes/recipe_modules/bot_update/api.py

@@ -363,7 +363,9 @@ class BotUpdateApi(recipe_api.RecipeApi):
 
           # Determine the output ref.
           got_revision_cp = self._last_returned_properties.get('got_revision_cp')
-          in_rev = revisions.get(out_solution)
+          in_rev = self.m.gclient.resolve_revision(revisions.get(out_solution))
+          if not in_rev:
+            in_rev = 'HEAD'
           if got_revision_cp:
             # If commit position string is available, read the ref from there.
             out_commit.ref, out_commit.position = (
@@ -380,7 +382,7 @@ class BotUpdateApi(recipe_api.RecipeApi):
             out_commit.ref = in_commit.ref
           else: # pragma: no cover
             assert False, (
-                'Unsupposed case. '
+                'Unsupported case. '
                 'Call buildbucket.set_output_gitiles_commit directly.'
             )
           self.m.buildbucket.set_output_gitiles_commit(out_commit)

recipes/recipe_modules/bot_update/examples/full.expected/revision_fallback_chain_set_output_commit.json

+[
+  {
+    "cmd": [
+      "python",
+      "-u",
+      "RECIPE_MODULE[depot_tools::bot_update]/resources/bot_update.py",
+      "--spec-path",
+      "cache_dir = '[CACHE]/git'\nsolutions = [{'deps_file': '.DEPS.git', 'managed': True, 'name': 'src', 'url': 'https://chromium.googlesource.com/chromium/src.git'}]",
+      "--revision_mapping_file",
+      "{\"got_angle_revision\": \"src/third_party/angle\", \"got_cr_revision\": \"src\", \"got_revision\": \"src\", \"got_v8_revision\": \"src/v8\"}",
+      "--git-cache-dir",
+      "[CACHE]/git",
+      "--cleanup-dir",
+      "[CLEANUP]/bot_update",
+      "--output_json",
+      "/path/to/tmp/json",
+      "--refs",
+      "refs/heads/master",
+      "--disable-syntax-validation"
+    ],
+    "env": {
+      "GIT_HTTP_LOW_SPEED_LIMIT": "102400",
+      "GIT_HTTP_LOW_SPEED_TIME": "300"
+    },
+    "env_suffixes": {
+      "DEPOT_TOOLS_UPDATE": [
+        "0"
+      ],
+      "PATH": [
+        "RECIPE_REPO[depot_tools]"
+      ]
+    },
+    "infra_step": true,
+    "name": "bot_update (without patch)",
+    "~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@  \"manifest\": {@@@",
+      "@@@STEP_LOG_LINE@json.output@    \"src\": {@@@",
+      "@@@STEP_LOG_LINE@json.output@      \"repository\": \"https://fake.org/src.git\", @@@",
+      "@@@STEP_LOG_LINE@json.output@      \"revision\": \"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\"@@@",
+      "@@@STEP_LOG_LINE@json.output@    }@@@",
+      "@@@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_revision\": \"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\"@@@",
+      "@@@STEP_LOG_LINE@json.output@  }, @@@",
+      "@@@STEP_LOG_LINE@json.output@  \"root\": \"src\", @@@",
+      "@@@STEP_LOG_LINE@json.output@  \"source_manifest\": {@@@",
+      "@@@STEP_LOG_LINE@json.output@    \"directories\": {@@@",
+      "@@@STEP_LOG_LINE@json.output@      \"src\": {@@@",
+      "@@@STEP_LOG_LINE@json.output@        \"git_checkout\": {@@@",
+      "@@@STEP_LOG_LINE@json.output@          \"repo_url\": \"https://fake.org/src.git\", @@@",
+      "@@@STEP_LOG_LINE@json.output@          \"revision\": \"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\"@@@",
+      "@@@STEP_LOG_LINE@json.output@        }@@@",
+      "@@@STEP_LOG_LINE@json.output@      }@@@",
+      "@@@STEP_LOG_LINE@json.output@    }, @@@",
+      "@@@STEP_LOG_LINE@json.output@    \"version\": 0@@@",
+      "@@@STEP_LOG_LINE@json.output@  }, @@@",
+      "@@@STEP_LOG_LINE@json.output@  \"step_text\": \"Some step text\"@@@",
+      "@@@STEP_LOG_LINE@json.output@}@@@",
+      "@@@STEP_LOG_END@json.output@@@",
+      "@@@SET_BUILD_PROPERTY@got_revision@\"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\"@@@"
+    ]
+  },
+  {
+    "cmd": [],
+    "name": "set_output_gitiles_commit",
+    "~followup_annotations": [
+      "@@@SET_BUILD_PROPERTY@$recipe_engine/buildbucket/output_gitiles_commit@{\"host\": \"fake.org\", \"id\": \"f27fede2220bcd326aee3e86ddfd4ebd0fe58cb9\", \"project\": \"src\", \"ref\": \"refs/heads/master\"}@@@"
+    ]
+  },
+  {
+    "name": "$result"
+  }
+]
\ No newline at end of file

recipes/recipe_modules/bot_update/examples/full.py

@@ -17,6 +17,7 @@ DEPS = [
 
 from recipe_engine import types
 
+from RECIPE_MODULES.depot_tools import gclient
 from PB.go.chromium.org.luci.buildbucket.proto.build import Build
 
 def RunSteps(api):
@@ -27,7 +28,10 @@ def RunSteps(api):
   soln = src_cfg.solutions.add()
   soln.name = 'src'
   soln.url = 'https://chromium.googlesource.com/chromium/src.git'
-  soln.revision = commit.id or commit.ref or None
+  if api.properties.get('revision_fallback_chain'):
+    soln.revision = gclient.api.RevisionFallbackChain()
+  else:
+    soln.revision = commit.id or commit.ref or None
   api.gclient.c = src_cfg
   api.gclient.c.revisions.update(api.properties.get('revisions', {}))
   if api.properties.get('deprecated_got_revision_mapping'):
@@ -238,6 +242,20 @@ def GenTests(api):
       ci_build(revision='origin/master')
   )
 
+  yield (
+      api.test('revision_fallback_chain_set_output_commit') +
+      ci_build() +
+      api.properties(
+          set_output_commit=True,
+          revision_fallback_chain=True,
+      ) +
+      # Don't set commit position properties so that the set_output_commit code
+      # attempts to do comparisons on the revision value
+      api.step_data('bot_update (without patch)', api.bot_update.output_json(
+          root='src', first_sln='src', revision_mapping={'got_revision': 'src'},
+          commit_positions=False))
+  )
+
   yield (
       api.test('add_blamelists') +
       ci_build() +

recipes/recipe_modules/bot_update/test_api.py

@@ -9,7 +9,7 @@ from recipe_engine import recipe_test_api
 
 class BotUpdateTestApi(recipe_test_api.RecipeTestApi):
   def output_json(self, root, first_sln, revision_mapping, fail_patch=False,
-                  fixed_revisions=None):
+                  fixed_revisions=None, commit_positions=True):
     """Deterministically synthesize json.output test data for gclient's
     --output-json option.
     """
@@ -34,11 +34,12 @@ class BotUpdateTestApi(recipe_test_api.RecipeTestApi):
         property_name: revisions[project_name]
         for property_name, project_name in revision_mapping.items()
     }
-    properties.update({
-        '%s_cp' % property_name: ('refs/heads/master@{#%s}' %
-                                  self.gen_commit_position(project_name))
-        for property_name, project_name in revision_mapping.items()
-    })
+    if commit_positions:
+      properties.update({
+          '%s_cp' % property_name: ('refs/heads/master@{#%s}' %
+                                    self.gen_commit_position(project_name))
+          for property_name, project_name in revision_mapping.items()
+      })
 
     output.update({
         'patch_root': root or first_sln,