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

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

This patch adds support for swarming to bots that use the chromium
recipes (chromium.py, chromium_trybot.py) and the MB build wrapper
(so that we can use GN).

It does this by adding a hook to the run_mb() call that takes a
list of the gtest_test names (which need to match their ninja
build target names) and passes that to `mb gen --swarming-targets`
via a temp file.

MB then re-maps the ninja target names to the GN labels, and
writes a file containing the labels into the build directory,
and passes that to GN (or, it will ignore this in a GYP build).

This patch also required us to re-order a couple of the build steps
so that we would have the list of test steps we plan to run
*before* we attempt to run MB.

R=phajdan.jr@chromium.org, maruel@chromium.org
BUG=480053

[Dirk Pranke, 2015-06-10 06:31:46]

Patchset #1 (id:1) has been deleted

[Dirk Pranke, 2015-06-10 06:37:33]

Please take a look and let me know if the general approach looks mostly right,
at least. There's at least one issue I need to track down still (as noted in the
comment).

https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium.expected/full_chromium_win_Win8_Aura.json
(right):

https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.expected/full_chromium_win_Win8_Aura.json:186:
"browser_tests\n",
This is wrong, I think; I need to figure out why this didn't print the path to
placeholder file instead ...

[Paweł Hajdan Jr., 2015-06-10 10:27:19]

Looks good. Just one comment.

https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium.expected/full_chromium_win_Win8_Aura.json
(right):

https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.expected/full_chromium_win_Win8_Aura.json:186:
"browser_tests\n",
On 2015/06/10 at 06:37:33, Dirk Pranke wrote:
> This is wrong, I think; I need to figure out why this didn't print the path to
placeholder file instead ...

It's actually OK - raw_io puts the input data inline for test expectations, see
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...
("cheat and pretend like we're going to pass the data on the cmdline for test
expectation purposes").

https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_trybot.py:424: if
api.m.chromium.c.project_generator.tool == 'mb':
Why is the MB logic duplicated inline between chromium and chromium_trybot
recipes? Is there a way to share it? If not, what is preventing it?

[M-A Ruel, 2015-06-10 14:55:31]

https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium.expected/buildnumber_zero.json (right):

https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.expected/buildnumber_zero.json:182:
"[BUILD]/scripts/slave/cleanup_temp.py"
It's a bit late, unintentional?

[Dirk Pranke, 2015-06-10 16:11:40]

https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium.expected/buildnumber_zero.json (right):

https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.expected/buildnumber_zero.json:182:
"[BUILD]/scripts/slave/cleanup_temp.py"
On 2015/06/10 14:55:31, M-A Ruel wrote:
> It's a bit late, unintentional?

I'm not sure why this changed. Will investigate.

https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium.expected/full_chromium_win_Win8_Aura.json
(right):

https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium.expected/full_chromium_win_Win8_Aura.json:186:
"browser_tests\n",
On 2015/06/10 10:27:19, Paweł Hajdan Jr. wrote:
> On 2015/06/10 at 06:37:33, Dirk Pranke wrote:
> > This is wrong, I think; I need to figure out why this didn't print the path
to
> placeholder file instead ...
> 
> It's actually OK - raw_io puts the input data inline for test expectations,
see
>
https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/re...
> ("cheat and pretend like we're going to pass the data on the cmdline for test
> expectation purposes").

Ah, that explains it (and is confusing). Thanks!

https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_trybot.py:424: if
api.m.chromium.c.project_generator.tool == 'mb':
On 2015/06/10 10:27:19, Paweł Hajdan Jr. wrote:
> Why is the MB logic duplicated inline between chromium and chromium_trybot
> recipes? Is there a way to share it? If not, what is preventing it?

The way we build swarming_targets is a bit different between the two recipes, so
it's 
not clear to me that sharing the 5 lines of code makes things much clearer, but
I'll take a
stab at it so you can take a look.

[Dirk Pranke, 2015-06-10 19:29:33]

On 2015/06/10 16:11:40, Dirk Pranke wrote:
>
https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
> File scripts/slave/recipes/chromium.expected/buildnumber_zero.json (right):
> 
>
https://codereview.chromium.org/1177593002/diff/40001/scripts/slave/recipes/c...
> scripts/slave/recipes/chromium.expected/buildnumber_zero.json:182:
> "[BUILD]/scripts/slave/cleanup_temp.py"
> On 2015/06/10 14:55:31, M-A Ruel wrote:
> > It's a bit late, unintentional?
> 
> I'm not sure why this changed. Will investigate.
> 

Okay, this changed because cleanup_temp is called by
api.chromium_tests.compile(), but
the rmtree and extract_build scripts are called, confusingly, 
by api.chromium_tests.get_tests_for_builder(), which this patch moves before
compile().

So, I think this is okay (and safe).

(As an aside, I find this confusing because I would not expect a routine 
called get_tests_for_buider to have such a potentially destructive effect 
on a checkout; maybe we should move that logic into prepare_checkout() instead).

[Dirk Pranke, 2015-06-10 19:37:45]

Patch updated; please take another look?

[M-A Ruel, 2015-06-10 20:14:02]

lgtm

[Dirk Pranke, 2015-06-11 00:02:37]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2015-06-11 00:02:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177593002/60001

[commit-bot: I haz the power, 2015-06-11 00:17:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-11 00:17:03]

Try jobs failed on following builders:
  build_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/build_presubmit...)

[Dirk Pranke, 2015-06-11 00:20:30]

Bah. Paweł, can you give a formal l-g-t-m for this ?

[Paweł Hajdan Jr., 2015-06-11 10:11:25]

https://codereview.chromium.org/1177593002/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (right):

https://codereview.chromium.org/1177593002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:582: def run_mb_if_necessary(self,
mastername, buildername, tests):
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]

https://codereview.chromium.org/1177593002/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (right):

https://codereview.chromium.org/1177593002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:582: def run_mb_if_necessary(self,
mastername, buildername, tests):
On 2015/06/11 10:11:24, Paweł Hajdan Jr. wrote:
> 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.

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]

https://codereview.chromium.org/1177593002/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (right):

https://codereview.chromium.org/1177593002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:582: def run_mb_if_necessary(self,
mastername, buildername, tests):
On 2015/06/11 at 16:57:52, Dirk Pranke wrote:
> On 2015/06/11 10:11:24, Paweł Hajdan Jr. wrote:
> > 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.
> 
> This call should run before we call analyze(), which happens outside
> of compile() and doesn't happen in the chromium recipe at all.

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?

> 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.

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]

https://codereview.chromium.org/1177593002/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (right):

https://codereview.chromium.org/1177593002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:582: def run_mb_if_necessary(self,
mastername, buildername, tests):
On 2015/06/12 09:18:44, Paweł Hajdan Jr. wrote:
> On 2015/06/11 at 16:57:52, Dirk Pranke wrote:
> > On 2015/06/11 10:11:24, Paweł Hajdan Jr. wrote:
> > > 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.
> > 
> > This call should run before we call analyze(), which happens outside
> > of compile() and doesn't happen in the chromium recipe at all.
> 
> Why does it need to run before analyze?

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'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?
>

I meant that analyze doesn't run in the chromium recipe.
 
> > 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.
> 
> 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'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]

https://codereview.chromium.org/1177593002/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (right):

https://codereview.chromium.org/1177593002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:582: def run_mb_if_necessary(self,
mastername, buildername, tests):
On 2015/06/12 at 16:00:01, Dirk Pranke wrote:
> On 2015/06/12 09:18:44, Paweł Hajdan Jr. wrote:
> > On 2015/06/11 at 16:57:52, Dirk Pranke wrote:
> > > On 2015/06/11 10:11:24, Paweł Hajdan Jr. wrote:
> > > > 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.
> > > 
> > > This call should run before we call analyze(), which happens outside
> > > of compile() and doesn't happen in the chromium recipe at all.
> > 
> > Why does it need to run before analyze?
> 
> 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 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.

> I'm not sure; at this point we're starting to talk about fairly large changes
> to the structure of things.

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 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 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?

> 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.

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?

> 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.

OK

> 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.

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:11]

https://codereview.chromium.org/1177593002/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (right):

https://codereview.chromium.org/1177593002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:582: def run_mb_if_necessary(self,
mastername, buildername, tests):
On 2015/06/15 10:15:48, Paweł Hajdan Jr. wrote:
> On 2015/06/12 at 16:00:01, Dirk Pranke wrote:
> > On 2015/06/12 09:18:44, Paweł Hajdan Jr. wrote:
> > > Why does [mb gen] need to run before analyze?
> > 
> > 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 think the scenario is real, but wouldn't "analyze" fail with the same error
as
> "generate"?

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 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 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?

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 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.
> 
> 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?

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.

> > 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.
> 
> 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. :)

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]

https://codereview.chromium.org/1177593002/diff/60001/scripts/slave/recipe_mo...
File scripts/slave/recipe_modules/chromium/api.py (right):

https://codereview.chromium.org/1177593002/diff/60001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:582: def run_mb_if_necessary(self,
mastername, buildername, tests):
On 2015/06/15 at 17:02:10, Dirk Pranke wrote:
> 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).

Makes sense, agreed.

> > > 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 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?
> 
> 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.

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.

> > > 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.
> > 
> > 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. :)
> 
> 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?

Sure. Just in case, what are the main scenarios I should test with my changes?

[Dirk Pranke, 2015-06-16 02:52:19]

Okay, I spent a little time looking at this, but not as much
as I would've liked.

The existing flow seems to be roughly:

1. run bot_update
2. call runhooks
3. read the test spec
4. compute the compile targets and test objects 
   from the test spec
5. do the same for the corresponding master if 
   you're a tryserver
6. add the GPU test steps if you're a tryserver
7. call analyze if you're a try server
8. call compile if need be (the logic for this
   seems reasonably different between tryserver
   and waterfall bot as well)

My patch hooks in mb between 6 and 7. mb has to run 
before 8, so we could move it after 7. We need to
compute the compile targets from the test steps, 
so we can't move the call to MB before step 6.

So, really, the only way to eliminate duplication 
and not add another step would be to replace step
8 with a new method in chromium_tests that called
mb and then called either chromium_tests.compile()
or chromium_tests.compile_specific_targets() as
appropriate. We could call this new step "build"
or something like that.

We could potentially also modify the runhooks
call in chromium_tests.prepare_checkout() to
disable GYP, and then separately call GYP in
"build" if we are not using MB.

However, the fact that there are two different
entry points for compile is exactly the sort of thing
that (to me) says that we should just duplicate
the lines between the two recipes, because otherwise
we end up pushing the control flow into the
chromium_tests module and just making things harder
to follow.

So, I'm not sure if the code is improved by moving 
stuff around.

Please take a stab at it and let me know if you 
come up with something you think is an improvement.

Also, I did realize when looking at this that I
think the 'run_mb_if_necessary' call is not written
to handle deapplying the patch properly, so we
probably need to fix that. I haven't figured out how
to do that yet, but I suspect it's probably easier
for you to do that, so that would be something
to look out for in your testing.

[Paweł Hajdan Jr., 2015-06-16 13:51:58]

Uploaded https://codereview.chromium.org/1187773004 with my suggested
modifications - what do you think?

[Dirk Pranke, 2015-06-17 00:58:56]

Abandoning - we landed phajdan.jr's version instead.