Remove rietveld support from tryserver recipe module

Bug: 770408
Recipe-Nontrivial-Roll: build
Change-Id: Icafd4bd11009975c40e53d93901589c07ecd738e
Reviewed-on: https://chromium-review.googlesource.com/707723
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Reviewed-by: Robbie Iannucci <iannucci@chromium.org>

recipes/README.recipes.md

@@ -671,21 +671,17 @@ Returns:
   given is unknown.
 ### *recipe_modules* / [tryserver](/recipes/recipe_modules/tryserver)
 
-[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]
+[DEPS](/recipes/recipe_modules/tryserver/__init__.py#5): [gerrit](#recipe_modules-gerrit), [git](#recipe_modules-git), [git\_cl](#recipe_modules-git_cl), [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#12)([RecipeApi][recipe_engine/wkt/RecipeApi]):**
 
-— **def [add\_failure\_reason](/recipes/recipe_modules/tryserver/api.py#148)(self, reason):**
+— **def [add\_failure\_reason](/recipes/recipe_modules/tryserver/api.py#108)(self, reason):**
 
 Records a more detailed reason why build is failing.
 
 The reason can be any JSON-serializable object.
 
-&emsp; **@property**<br>&mdash; **def [can\_apply\_issue](/recipes/recipe_modules/tryserver/api.py#23)(self):**
-
-Returns true iff the properties exist to apply_issue from rietveld.
-
-&mdash; **def [get\_files\_affected\_by\_patch](/recipes/recipe_modules/tryserver/api.py#46)(self, patch_root=None, \*\*kwargs):**
+&mdash; **def [get\_files\_affected\_by\_patch](/recipes/recipe_modules/tryserver/api.py#38)(self, patch_root, \*\*kwargs):**
 
 Returns list of paths to files affected by the patch.
 
@@ -695,38 +691,34 @@ Argument:
 
 Returned paths will be relative to to patch_root.
 
-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.
-
-&mdash; **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#211)(self, tag, patch_text=None):**
+&mdash; **def [get\_footer](/recipes/recipe_modules/tryserver/api.py#162)(self, tag, patch_text=None):**
 
 Gets a specific tag from a CL description
 
-&mdash; **def [get\_footers](/recipes/recipe_modules/tryserver/api.py#184)(self, patch_text=None):**
+&mdash; **def [get\_footers](/recipes/recipe_modules/tryserver/api.py#144)(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.
 
-&emsp; **@property**<br>&mdash; **def [is\_gerrit\_issue](/recipes/recipe_modules/tryserver/api.py#30)(self):**
+&emsp; **@property**<br>&mdash; **def [is\_gerrit\_issue](/recipes/recipe_modules/tryserver/api.py#22)(self):**
 
 Returns true iff the properties exist to match a Gerrit issue.
 
-&emsp; **@property**<br>&mdash; **def [is\_patch\_in\_git](/recipes/recipe_modules/tryserver/api.py#40)(self):**
+&emsp; **@property**<br>&mdash; **def [is\_patch\_in\_git](/recipes/recipe_modules/tryserver/api.py#32)(self):**
 
 &emsp; **@property**<br>&mdash; **def [is\_tryserver](/recipes/recipe_modules/tryserver/api.py#17)(self):**
 
-Returns true iff we can apply_issue or patch.
+Returns true iff we have a change to check out.
 
-&mdash; **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#215)(self, footer):**
+&mdash; **def [normalize\_footer\_name](/recipes/recipe_modules/tryserver/api.py#166)(self, footer):**
 
-&mdash; **def [set\_compile\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#127)(self):**
+&mdash; **def [set\_compile\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#87)(self):**
 
 Mark the tryjob result as a compile failure.
 
-&emsp; **@contextlib.contextmanager**<br>&mdash; **def [set\_failure\_hash](/recipes/recipe_modules/tryserver/api.py#157)(self):**
+&emsp; **@contextlib.contextmanager**<br>&mdash; **def [set\_failure\_hash](/recipes/recipe_modules/tryserver/api.py#117)(self):**
 
 Context manager that sets a failure_hash build property on StepFailure.
 
@@ -735,7 +727,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#139)(self):**
+&mdash; **def [set\_invalid\_test\_results\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#99)(self):**
 
 Mark the tryjob result as having invalid test results.
 
@@ -743,18 +735,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).
 
-&mdash; **def [set\_patch\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#123)(self):**
+&mdash; **def [set\_patch\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#83)(self):**
 
 Mark the tryjob result as failure to apply the patch.
 
-&mdash; **def [set\_subproject\_tag](/recipes/recipe_modules/tryserver/api.py#105)(self, subproject_tag):**
+&mdash; **def [set\_subproject\_tag](/recipes/recipe_modules/tryserver/api.py#65)(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.
 
-&mdash; **def [set\_test\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#131)(self):**
+&mdash; **def [set\_test\_failure\_tryjob\_result](/recipes/recipe_modules/tryserver/api.py#91)(self):**
 
 Mark the tryjob result as a test failure.
 

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

@@ -95,7 +95,6 @@
       "@@@STEP_LOG_LINE@json.output@  \"step_text\": \"Some step text\"@@@",
       "@@@STEP_LOG_LINE@json.output@}@@@",
       "@@@STEP_LOG_END@json.output@@@",
-      "@@@SET_BUILD_PROPERTY@failure_type@\"PATCH_FAILURE\"@@@",
       "@@@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\"@@@",

recipes/recipe_modules/tryserver/__init__.py

@@ -14,5 +14,4 @@ DEPS = [
   'recipe_engine/python',
   'recipe_engine/raw_io',
   'recipe_engine/step',
-  'rietveld',
 ]

recipes/recipe_modules/tryserver/api.py

@@ -16,16 +16,8 @@ class TryserverApi(recipe_api.RecipeApi):
 
   @property
   def is_tryserver(self):
-    """Returns true iff we can apply_issue or patch."""
-    return (
-        self.can_apply_issue or self.is_patch_in_git or self.is_gerrit_issue)
-
-  @property
-  def can_apply_issue(self):
-    """Returns true iff the properties exist to apply_issue from rietveld."""
-    return (self.m.properties.get('rietveld')
-            and 'issue' in self.m.properties
-            and 'patchset' in self.m.properties)
+    """Returns true iff we have a change to check out."""
+    return (self.is_patch_in_git or self.is_gerrit_issue)
 
   @property
   def is_gerrit_issue(self):
@@ -43,7 +35,7 @@ class TryserverApi(recipe_api.RecipeApi):
             self.m.properties.get('patch_repo_url') and
             self.m.properties.get('patch_ref'))
 
-  def get_files_affected_by_patch(self, patch_root=None, **kwargs):
+  def get_files_affected_by_patch(self, patch_root, **kwargs):
     """Returns list of paths to files affected by the patch.
 
     Argument:
@@ -51,16 +43,7 @@ class TryserverApi(recipe_api.RecipeApi):
         api.gclient.calculate_patch_root(patch_project)
 
     Returned paths will be relative to to patch_root.
-
-    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.
     """
-    # patch_root must be set! None is for backwards compataibility and will be
-    # removed.
-    if patch_root is None:
-      return self._old_get_files_affected_by_patch()
-
     cwd = self.m.context.cwd or self.m.path['start_dir'].join(patch_root)
     with self.m.context(cwd=cwd):
       step_result = self.m.git(
@@ -79,29 +62,6 @@ class TryserverApi(recipe_api.RecipeApi):
     step_result.presentation.logs['files'] = paths
     return paths
 
-
-  def _old_get_files_affected_by_patch(self):
-    issue_root = self.m.rietveld.calculate_issue_root()
-    cwd = self.m.path['checkout'].join(issue_root) if issue_root else None
-
-    with self.m.context(cwd=cwd):
-      step_result = self.m.git(
-          '-c', 'core.quotePath=false', 'diff', '--cached', '--name-only',
-          name='git diff to analyze patch',
-          stdout=self.m.raw_io.output(),
-          step_test_data=lambda:
-            self.m.raw_io.test_api.stream_output('foo.cc'))
-    paths = step_result.stdout.split()
-    if issue_root:
-      paths = [self.m.path.join(issue_root, path) for path in paths]
-    if self.m.platform.is_win:
-      # Looks like "analyze" wants POSIX slashes even on Windows (since git
-      # uses that format even on Windows).
-      paths = [path.replace('\\', '/') for path in paths]
-
-    step_result.presentation.logs['files'] = paths
-    return paths
-
   def set_subproject_tag(self, subproject_tag):
     """Adds a subproject tag to the build.
 
@@ -188,19 +148,10 @@ class TryserverApi(recipe_api.RecipeApi):
     git-footers documentation for more information.
     """
     if patch_text is None:
-      if self.is_gerrit_issue:
-        patch_text = self.m.gerrit.get_change_description(
-            self.m.properties['patch_gerrit_url'],
-            self.m.properties['patch_issue'],
-            self.m.properties['patch_set'])
-      elif self.can_apply_issue:  # pragma: no cover
-        patch_url = (
-            self.m.properties['rietveld'].rstrip('/') + '/' +
-            str(self.m.properties['issue']))
-        patch_text = self.m.git_cl.get_description(
-            patch_url=patch_url, codereview='rietveld').stdout
-      else:  # pragma: no cover
-        raise recipe_api.StepFailure('Unknown patch storage.')
+      patch_text = self.m.gerrit.get_change_description(
+          self.m.properties['patch_gerrit_url'],
+          self.m.properties['patch_issue'],
+          self.m.properties['patch_set'])
 
     result = self.m.python(
         'parse description', self.package_repo_resource('git_footers.py'),

recipes/recipe_modules/tryserver/examples/full.expected/with_wrong_patch.json

@@ -8,7 +8,6 @@
       "--cached",
       "--name-only"
     ],
-    "cwd": "[START_DIR]",
     "infra_step": true,
     "name": "git diff to analyze patch",
     "stdout": "/path/to/tmp/",

recipes/recipe_modules/tryserver/examples/full.py

@@ -28,7 +28,7 @@ def RunSteps(api):
             'Foo', api.properties['patch_text']))])
     return
 
-  if api.tryserver.can_apply_issue or api.tryserver.is_gerrit_issue:
+  if api.tryserver.is_gerrit_issue:
     api.tryserver.get_footers()
   api.tryserver.get_files_affected_by_patch(
       api.properties.get('test_patch_root'))
@@ -50,22 +50,29 @@ def RunSteps(api):
 def GenTests(api):
   description_step = api.override_step_data(
       'git_cl description', stdout=api.raw_io.output_text('foobar'))
+  # The 'test_patch_root' property used below is just so that these
+  # tests can avoid using the gclient module to calculate the
+  # patch root. Normal users would use gclient.calculate_patch_root().
   yield (api.test('with_git_patch') +
          api.properties(
-              path_config='buildbot',
-              patch_storage='git',
-              patch_project='v8',
-              patch_repo_url='http://patch.url/',
-              patch_ref='johndoe#123.diff'))
+             path_config='buildbot',
+             patch_storage='git',
+             patch_project='v8',
+             patch_repo_url='http://patch.url/',
+             patch_ref='johndoe#123.diff',
+             test_patch_root='v8'))
 
   yield (api.test('with_git_patch_luci') +
          api.properties(
              patch_storage='git',
              patch_project='v8',
              patch_repo_url='http://patch.url/',
-             patch_ref='johndoe#123.diff'))
+             patch_ref='johndoe#123.diff',
+             test_patch_root='v8'))
 
-  yield (api.test('with_wrong_patch') + api.platform('win', 32))
+  yield (api.test('with_wrong_patch') +
+         api.platform('win', 32) +
+         api.properties(test_patch_root=''))
 
   yield (api.test('with_gerrit_patch') +
          api.properties.tryserver(gerrit_project='infra/infra'))