Make the recipe roller tryjob accept tryjobs.

scripts/slave/recipes/infra/recipe_roll_tryjob.py

 # Copyright (c) 2014 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Rolls recipes.cfg dependencies."""
 
 DEPS = [
   'depot_tools/bot_update',
   'depot_tools/gclient',
   'file',
@@ skipping 130 matching lines @@
   for proj, targets in deps.items():
     for target in targets:
       downstream_projects[target].add(proj)
 
   return deps, downstream_projects
 
 def get_url_mapping(api, headers=None):
   """Fetch the mapping of project id to url from luci-config.
 
   Args:
-    auth_token: an OAuth2 access token for querying luci-config.
+    headers: Optional authentication headers to pass to luci-config.
 
   Returns:
     A dictionary mapping project id to its luci-config project spec (among
     which there is a repo_url key).
   """
   url = 'https://luci-config.appspot.com/_ah/api/config/v1/projects'
 
   fetch_result = api.url.fetch(url, step_name='Get project urls',
       headers=headers,
       step_test_data=lambda: api.raw_io.test_api.output(json.dumps({
@@ skipping 15 matching lines @@
   for project in json.loads(fetch_result)['projects']:
     project = {str(k): str(v) for k, v in project.items()}
     mapping[project['id']] = project
   return mapping
 
 
 RietveldPatch = collections.namedtuple(
     'RietveldPatch', 'project server issue patchset')
 
 
-def parse_patches(api, patches_raw):
+def parse_patches(api, patches_raw, rietveld, issue, patchset, patch_project):

[iannucci, 2016/03/23 19:08:40]

hm.. this seems a bit awkward of an API to me. What if you had a different
function 'extract_patches_from_rietveld', and then fed the result of that into
parse_patches?

And I still maintain that 'patches_raw' should be a real json dictionary instead
of this string thing... what was the reason for not doing that again?

I would also consider having the rietveld syntax look like:

DOWNSTREAM_PATCH=recipes:<project_name>:<url>
DOWNSTREAM_PATCH=recipes:<project_name2>:<url>

Instead of cramming it all into one line. I'd also make sure that rietveld's
url-finder regex can correctly link the url portions as well.

The 'recipes' literal would allow us to use this syntax and roller mechanism for
applying other sorts of patches too (e.g. be able to have a roller which tries
to roll generic upstream changes downstream). Something like
`DOWNSTREAM_PATCH=DEPS:chromium_src:<url>`. The rollers would need to
disambiguate the type of dependency (recipes.cfg, DEPS, submodules, repo, etc.)
in order to know how to update the downstream patch with the roll information
(bumping the pinned version).

Just a thought.

[martiniss, 2016/03/23 20:38:43]

Looks great!

This syntax is only for command line invocations. I figured that people wouldn't
want to do something like patches='[{\"project\": \"build\", ......' on the
command line. It's a bit ugly but it does work. I think eventually it'd be
mostly replaced by just passing in one patch on the command line, and having
this look at the downstream patches and do smart things™ from there.

   """
   gives mapping of project to patch
     expect input of
     project1:https://a.b.c/1342342#ps1,project2:https://d.ce.f/1231231#ps1
   """
+  result = {}
+
+  if rietveld and issue and patchset and patch_project:
+    result[patch_project] = RietveldPatch(
+        patch_project, rietveld, issue, patchset)
+
   if not patches_raw:
-    return {}
-
-  result = {}
+    return result
 
   for patch_raw in patches_raw.split(','):
     project, url = patch_raw.split(':', 1)
     server, issue_and_patchset = url.rsplit('/', 1)
     issue, patchset = issue_and_patchset.split('#')
     patchset = patchset[2:]
 
     if project in result:
       api.python.failing_step(
           "Invalid patchset list",
@@ skipping 88 matching lines @@
 
   kwargs = {
       'gclient_config': config,
       'force': True,
       'cwd': checkout_path,
   }
   if patch:
     kwargs['rietveld'] = patch.server
     kwargs['issue'] = patch.issue
     kwargs['patchset'] = patch.patchset
+  else:
+    kwargs['patch'] = False
 
   api.bot_update.ensure_checkout(**kwargs)
   return repo_path
 
 
 PROJECTS_TO_TRY = [
   'build',
   'build_limited_scripts_slave',
   'recipe_engine',
   'depot_tools',
 ]
 
 PROPERTIES = {
   'patches': Property(kind=str, param_name='patches_raw', default="",
                       help="Patches to apply. Format is"
                       "project1:https://url.to.codereview/123456#ps01 where"
                       "url.to.codereview is the address of the code review site"
                       ", 123456 is the issue number, and ps01 is the patchset"
                       "number"),
+  # This recipe can be used as a tryjob by setting the rietveld, issue, and
+  # patchset properties, like a normal tryjob. If those are set, it will use
+  # those, as well as any data sent in the regular properties, as patches to
+  # apply.
+  "rietveld": Property(kind=str, default="",
+                       help="The code review site the tryjob patch isfrom"),

[iannucci, 2016/03/23 19:08:40]

This is very rietveld specific: "The Rietveld instance the issue is from"

[martiniss, 2016/03/23 20:38:43]

Done.

+  "issue": Property(kind=int, default=None,
+                    help="The issue number this patch is from"),

[iannucci, 2016/03/23 19:08:40]

The Rietveld issue number to pull data from.

[martiniss, 2016/03/23 20:38:43]

Done.

+  "patchset": Property(kind=int, default=None,
+                       help="The patchset to apply from this patch"),

[iannucci, 2016/03/23 19:08:40]

The patchset number for the supplied issue.

Though honestly why can't we just have it be one url?

[martiniss, 2016/03/23 20:38:43]

Done, and because CQ sets these properties when they send a tryjob, so we need
to accept them... AFAIK

+  "patch_project": Property(
+      kind=str, default=None,
+      help="The luci-config name of the project this patch belongs to"),

[iannucci, 2016/03/23 19:08:40]

Isn't this retrievable from rietveld directly?

[martiniss, 2016/03/23 20:38:43]

Probably, yeah. It's set by CQ though, so I figure we might as well use it.

 }
 
-def RunSteps(api, patches_raw):
+def RunSteps(api, patches_raw, rietveld, issue, patchset, patch_project):
   headers = {'Authorization': 'Bearer %s' % get_auth_token(api)}
 
-  patches = parse_patches(api, patches_raw)
+  patches = parse_patches(
+      api, patches_raw, rietveld, issue, patchset, patch_project)
 
   root_dir = api.path['slave_build']
 
   url_mapping = get_url_mapping(api, headers)
 
   # luci config project name to recipe config namedtuple
   recipe_configs = {}
 
   # List of all the projects we care about testing. luci-config names
   all_projects = set(p for p in url_mapping if p in PROJECTS_TO_TRY)
 
   recipe_configs = {
       p: get_project_config(api, p, headers) for p in all_projects}
 
-  downstream_projects, deps = get_deps_info(all_projects, recipe_configs)
+  deps, downstream_projects = get_deps_info(all_projects, recipe_configs)
 
   projs_to_test, locations = checkout_projects(
       api, all_projects, url_mapping, downstream_projects, root_dir, patches)
 
   with api.step.defer_results():
     for proj in projs_to_test:
       deps_locs = {dep: locations[dep] for dep in deps[proj]}
 
       simulation_test(
           api, proj, recipe_configs[proj], locations[proj], deps_locs)
@@ skipping 48 matching lines @@
       api.properties(
           patches="build:https://f.e.w/1#ps1,build:https://f.e.w/1#ps1")
   )
 
   yield (
       api.test('deps') +
       api.step_data("Get build deps", project('build', ['recipe_engine'])) +
       api.step_data("Get recipe_engine deps", project('recipe_engine'))
   )
 
+  yield (
+      api.test('tryjob') +
+      api.properties(
+        rietveld="https://fake.code.review",
+        issue=12345678,
+        patchset=1,
+        patch_project="build",
+      ) +
+      api.step_data("Get build deps", project('build', ['recipe_engine'])) +
+      api.step_data("Get recipe_engine deps", project('recipe_engine'))
+  )
+