Add swarming support for MB (and the GN bots) in the chromium recipes.

scripts/slave/recipe_modules/chromium/api.py

Index: scripts/slave/recipe_modules/chromium/api.py
diff --git a/scripts/slave/recipe_modules/chromium/api.py b/scripts/slave/recipe_modules/chromium/api.py
index caea92bccf1bb4e179b7b1eadb39eec46316421d..5a0eb0a45a73b0144baa4471ed1eae2d8579ea7c 100644
--- a/scripts/slave/recipe_modules/chromium/api.py
+++ b/scripts/slave/recipe_modules/chromium/api.py
@@ -579,24 +579,33 @@ class ChromiumApi(recipe_api.RecipeApi):
             '--args=%s' % ' '.join(gn_args),
         ])
 
+  def run_mb_if_necessary(self, mastername, buildername, tests):

[Paweł Hajdan Jr., 2015/06/11 10:11:24]

Why is it still a separate method and not part of e.g. compile? It's better than
previous patch, but still somewhat error-prone and raising questions why we do
things in slightly different order between chromium and chromium_trybot recipe.
Ideally such differences should not exist.

[Dirk Pranke, 2015/06/11 16:57:52]

This call should run before we call analyze(), which happens outside
of compile() and doesn't happen in the chromium recipe at all.

I think I understand your concern about not wanting to have too many lines of
code to keep in sync between the two recipes, but I'm also concerned about
merging unrelated bits of code together in the modules.

For example, compile() should be about the compile step (calling ninja), and
not do a lot of other things. get_test_spec() should be about reading the
*.json files, not also deleting temp files from the bot.

Perhaps a better way to ensure that there is more top-level code
shared between the two recipes, maybe you should add a main_chromium_steps()
module and have the two recipes call that, instead?

(I wouldn't want to make such a change as part of this patch, though).

[Paweł Hajdan Jr., 2015/06/12 09:18:44]

Why does it need to run before analyze?

I'm also slightly confused because this patch makes chromium recipe call
run_mb_if_necessary, which seems to contradict the "doesn't happen in the
chromium recipe at all" part - or did you mean something else?
Good point. How about extracting the test spec parts out of prepare_checkout,
and also extracting the runhooks part out (and mb would then be part of
runhooks)?

Would that work for this scenario?

I wouldn't go as far as introducing main_chromium_steps (the name doesn't make
it obvious what it does), but I think it's important to make this API guide
users naturally towards the right thing. I prefer having natural dependencies on
some calls (e.g. next steps require the test spec, so you have to call test spec
method; or things will obviously break without runhooks, so you need to call the
runhooks method) rather than making people remember where exactly run_mb needs
to be called. That's why I'm trying to make it fit together with some of the
larger logical code chunks.

I wouldn't like to delay the review for too long either, so we can resolve the
remaining concerns e.g. via IM.

[Dirk Pranke, 2015/06/12 16:00:01]

My main concern is that if there's something wrong in MB, GYP, or GN files
that would cause any command to fail, I want the "generate files" command to
fail first, rather than the "analyze" command; I think having analyze fail and
abort the build before "generate" even got a chance to run would be confusing.
 
I meant that analyze doesn't run in the chromium recipe.
 
I'm not sure; at this point we're starting to talk about fairly large changes
to the structure of things.

I wouldn't want the call to be mb to be part of runhooks, because I don't
think it reflects reality (mb isn't a part of runhooks any more than it's a
part of compile).

I actually think it's better to be explicit about what steps are called when
in the main recipes, and so it's better that the main recipes do things 
like calling runhooks and mb directly; that way the top function of the 
code clearly maps to the step you see in the build; I think bundling
steps into methods can make things less clear and harder to maintain.

That's not always true - prepare_checkout() is an okay thing, for example,
because there's no step on the waterfall called prepare_checkout(), and
the name is fairly generic. I think when we do bundle things we should
be careful to avoid confusion. run_tests_and_deapply_as_needed() is
another good example of something that is not confusing.

Would make sense to land this roughly as-is so we can un-block me 
and then you could re-structure things in a follow-on patch to show 
me what you have in mind? Or, you can keep trying to help me 
understand what you have in mind and I can try a follow-on patch.

[Paweł Hajdan Jr., 2015/06/15 10:15:48]

I think the scenario is real, but wouldn't "analyze" fail with the same error as
"generate"?

Aren't we worried to much about confusion here?

I don't see a hard requirement to run MB before "analyze", and if we don't have
to do that I believe it'd simplify this code.
It may be needed. It's important for me that changes made to critical recipe
infrastructure fit well with the existing design. I think if they don't, it's
reasonable to make wider changes that help avoid technical debt (which the CQ
team would likely have to clean up).
I don't see a reason to impose such constraint here. It may make it harder to
find good solution.

Is there a practical or technical reason against making mb part of existing big
building blocks like runhooks or compile other than the philosophical argument?
Hm, maintaining this infrastructure I found opposite to be true.

As long as all steps in one of these methods are always called together, I think
it's fine and beneficial. When that stops to be true, they can be extracted out
- in fact, this has been successfully done before, and the methods have been
split into smaller and better focused ones, guided by how they were used.

The specific thing I'm worried about is duplication. "Bundling" helps keep
chromium and chromium_trybot recipes in sync, since there's less duplicated
calls between the two. Once each of them needs to make more calls, it gets
harder to easily see they're actually doing the same thing.

Are there some drawbacks I've missed with this approach?
OK
I really wouldn't like to land as-is.

One alternative we could try is I could write a patch that tries to do what this
CL does. It may involve adding mb to runhooks/compile or a similar method.
Unless you have strong opinions about that, I think I could produce something
that works and I'd be fine with. :)

[Dirk Pranke, 2015/06/15 17:02:10]

In GN, you'll probably get the same error, since analyze basically 
runs `gn gen` plus some other stuff; in GYP, you could well get 
different errors since the GYP analyzer is a completely different 
implementation from the generator.

However, I'm willing to give moving analyze in front of generate a try
in the interest of compromise.

Of course, you cannot move analyze in front of *runhooks*, since the 
checkout isn't complete until the hooks have run; we need to split 
out gyp_chromium from the rest of the hooks in order to do this 
(which is what we do w/ MB, but we'd have to restructure the 
non-MB code path to match the MB code path).

(I'll remind you that eventually all chromium bots needs to be calling
MB, not just the GN ones).
I'm not sure that we're talking about the same thing ... I"m talking 
specifically about the runhooks() and compile() methods in 
recipe_modules/chromium/api.py, which in my view aren't big 
building blocks, they are very specific wrappers around single steps.

I don't think we should to pushing any calls to MB into either of 
those methods; I feel pretty strongly that the existing methods 
in the chromium API should map onto single build steps as 
much as possible.

I'm fine w/ creating other wrapper methods in chromium_tests 
that bundle multiple steps together if need be.
I think the main drawback is when the bundling is done in a counterintuitive
way - as in the examples I gave - which causes the user to have to 
jump through a bunch of files in order to figure out when things are called 
and in what order.

But, you and I definitely have very different approaches to writing recipes, and
I don't think either of us is going to change the others' mind in this CL :). 

I will try to make the changes that you will be happy with.
I don't want to land something you're not happy with.

I can work on the changes you've described above, but I don't think 
I'll have anything to review before at least mid-afternoon my time. I 
don't expect you to be online to review at that point, but maybe you 
can take a look when you get in tomorrow and if it still isn't the 
approach you want you can take a stab at things.

Sound good?

[Paweł Hajdan Jr., 2015/06/15 17:10:00]

Makes sense, agreed.
Agreed. Yeah, the chromium API calls shouldn't bundle (too) much if at all. I
was thinking about the chromium_tests API - sorry about the confusion.
Sure. Just in case, what are the main scenarios I should test with my changes?

+    if self.c.project_generator.tool == 'mb':
+      self.run_mb(mastername, buildername,
+                  swarming_targets=[t.name for t in tests if t.uses_swarming])
+
   def run_mb(self, mastername, buildername, use_goma=True,
-             mb_config_path=None):
+             mb_config_path=None, swarming_targets=None):
     mb_config_path = (mb_config_path or
                       self.m.path['checkout'].join('tools', 'mb',
                                                    'mb_config.pyl'))
+    swarming_targets = swarming_targets or []
+
     args=[
         'gen', '-v',
         '-m', mastername,
         '-b', buildername,
         '--config-file', mb_config_path,
     ]
+
     if use_goma:
-        args += [
-            '--goma-dir', self.m.path['build'].join('goma'),
-        ]
-    args += [
-        '//out/%s' % self.c.build_config_fs
-    ]
+      args += ['--goma-dir', self.m.path['build'].join('goma')]
+
+    if swarming_targets:
+      data = '\n'.join(swarming_targets) + '\n'
+      args += ['--swarming-targets-file', self.m.raw_io.input(data)]
+
+    args += ['//out/%s' % self.c.build_config_fs]
 
     # This runs with no env being passed along, so we get a clean environment
     # without any GYP_DEFINES being present to cause confusion.