Add GN (the build tool) to the chromium.linux buildbots and try jobs

Add GN (the build tool) to the chromium.linux buildbots and try jobs

This patch adds three things:
1. a recipe for building chromium w/ GN and running the gn_unittests
2. a new linux_gn bot to the chromium.linux waterfall that runs the above
   recipe
3. a few additional steps on the linux and linux_rel chromium trybots
   to run GN, put the output into a separate Debug_gn (or Release_gn)
   dir, compile in that dir as well, and run the gn_unittests from
   that dir.

We do step 3 because at this time the cost of running GN + compiling
the few targets and running the gn_uniitests is *way* smaller than
the cost of setting up an entire new fleet of GN try bots that are
part of the default set.

When the GN build gets closer to building all of chrome, we will split
it off into its own pool of bots and remove these steps from the
chromium.recipe.

R=iannucci@chromium.org, phajdan.jr@chromium.org
BUG=353690, 353854, 353855, 353881
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=260291

[Dirk Pranke, 2014-03-26 00:30:18]

Please take a look. This is a single all-in-one patch with all of the changes
(starting over in a new CL since the others were too hard to follow).

If we land this as-is, then the chromium linux try bots will be affected
immediately, and then we'll need to restart the chromium.linux master.

I can also split things up into multiple CLs if we'd prefer to make it easier to
revert one piece or another if there are problems, once we agree that this all
looks right.

[iannucci, 2014-03-26 09:38:49]

Probably needs a rebase on top of Paweł's latest patch

https://chromiumcodereview.appspot.com/211473006/diff/50001/masters/master.ch...
File masters/master.chromium.linux/master_linux_cfg.py (right):

https://chromiumcodereview.appspot.com/211473006/diff/50001/masters/master.ch...
masters/master.chromium.linux/master_linux_cfg.py:57: {'name': 'Linux GN (dbg)',
'recipe': 'gn_buildbot'},
it occurs to me that 'chromium_gn' may be a better name for this recipe.

https://chromiumcodereview.appspot.com/211473006/diff/50001/scripts/slave/rec...
File scripts/slave/recipes/chromium_trybot.py (right):

https://chromiumcodereview.appspot.com/211473006/diff/50001/scripts/slave/rec...
scripts/slave/recipes/chromium_trybot.py:274: gn_output_dir)
we may want to set 'can_fail_build=False, abort_on_failure=False' for these,
until we're sure they should be green (maybe we're sure now?) Same for the clone
of these steps below.

https://chromiumcodereview.appspot.com/211473006/diff/50001/scripts/slave/rec...
scripts/slave/recipes/chromium_trybot.py:310: # trybots, and assume that making
runtest() work w/ a differe output dir
*different

Though it would be /best/ if we could avoid runtest.py entirely and just
implement the bits and pieces which are actually necessary. I'm assuming the
annotations you're referring to are the test case summaries in step_text?

https://chromiumcodereview.appspot.com/211473006/diff/50001/scripts/slave/rec...
scripts/slave/recipes/chromium_trybot.py:608: lambda a, b: a + b,
isn't this the same as sum?

[Paweł Hajdan Jr., 2014-03-26 11:44:59]

"not lgtm" because of my comment about "gn_unittests"

Otherwise looks good, generally minor comments.

Feel free to just let Robbie do the review but please do not change
chromium_trybot without my review.

https://codereview.chromium.org/211473006/diff/50001/masters/master.chromium....
File masters/master.chromium.linux/master_linux_cfg.py (right):

https://codereview.chromium.org/211473006/diff/50001/masters/master.chromium....
masters/master.chromium.linux/master_linux_cfg.py:57: {'name': 'Linux GN (dbg)',
'recipe': 'gn_buildbot'},
On 2014/03/26 09:38:49, iannucci wrote:
> it occurs to me that 'chromium_gn' may be a better name for this recipe.

+1, but not blocking

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:253: should_run_gn =
api.properties.get('buildername') in ('linux', 'linux_rel')
This recipe is not running on any of these builders. This should probably be
linux_chromium_rel.

Actually, given the questions about its stability, can we create dedicated
tryserver builders for this? They can still use the large pool of Linux slaves.

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:273: yield
api.chromium.compile_with_ninja('compile (gn with patch)',
Shouldn't this _replace_ api.chromium.compile for GN bots?

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:294: yield api.path.rmcontents('slave
gn build directory', gn_output_dir)
Instead of this, could you add force_clobber to the api.chromium methods you're
adding?

This would be consistent with api.chromium.compile, and maybe run_gn and
compile_with_ninja can be just one method.

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:314:
cmd=[gn_output_dir.join('gn_unittests',
Sorry, all new code added here MUST support retries without patch.

We've had to add pre-existing telemetry tests below, and they're causing
annoying flakes, even though they run on the main waterfall.

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:602: api.step_data('compile (gn with
patch)', api.json.output(None)) +
These shouldn't be needed after my latest changes. :)

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:619: api.step_data('gclient revert') +
This is likely a no-op.

[Dirk Pranke, 2014-03-26 16:52:28]

I have several questions about Paweł's comments, please take a look ...

https://codereview.chromium.org/211473006/diff/50001/masters/master.chromium....
File masters/master.chromium.linux/master_linux_cfg.py (right):

https://codereview.chromium.org/211473006/diff/50001/masters/master.chromium....
masters/master.chromium.linux/master_linux_cfg.py:57: {'name': 'Linux GN (dbg)',
'recipe': 'gn_buildbot'},
On 2014/03/26 11:44:59, Paweł Hajdan Jr. wrote:
> On 2014/03/26 09:38:49, iannucci wrote:
> > it occurs to me that 'chromium_gn' may be a better name for this recipe.
> 
> +1, but not blocking

Yeah, it probably makes sense to rename. Will do so.

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:253: should_run_gn =
api.properties.get('buildername') in ('linux', 'linux_rel')
On 2014/03/26 11:44:59, Paweł Hajdan Jr. wrote:
> This recipe is not running on any of these builders. This should probably be
> linux_chromium_rel.

Right! Will fix.

> 
> Actually, given the questions about its stability, can we create dedicated
> tryserver builders for this? They can still use the large pool of Linux
slaves.

No, we can't. That's the whole point of this ... we can't currently afford to
double the load of the try jobs (twice the checkout space in particular) for
something that takes less than a minute to run the new steps.

Also, as Robbie pointed out earlier, since we're building into a new builddir,
we can't reuse the pool of slaves either.

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:273: yield
api.chromium.compile_with_ninja('compile (gn with patch)',
On 2014/03/26 11:44:59, Paweł Hajdan Jr. wrote:
> Shouldn't this _replace_ api.chromium.compile for GN bots?

No. This and other comments make me think that you didn't understand what I'm
doing in this patch. The try job needs to do *two* compiles, the normal one and
the gn one, into two different directories.

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:274: gn_output_dir)
On 2014/03/26 09:38:49, iannucci wrote:
> we may want to set 'can_fail_build=False, abort_on_failure=False' for these,
> until we're sure they should be green (maybe we're sure now?) Same for the
clone
> of these steps below.

Good point, will do.

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:294: yield api.path.rmcontents('slave
gn build directory', gn_output_dir)
On 2014/03/26 11:44:59, Paweł Hajdan Jr. wrote:
> Instead of this, could you add force_clobber to the api.chromium methods
you're
> adding?

Nope :(. force_clobber doesn't support custom output dirs, and I don't think we
should add the param to that method (and the others) for something that should
be a relatively-short-lived thing.

> 
> This would be consistent with api.chromium.compile, and maybe run_gn and
> compile_with_ninja can be just one method.

See above. 

It's also not clear to me if run_gn and compile_with_ninja should even be one
step at this point, since we will definitely want to see the failures
differently in the job.

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:310: # trybots, and assume that making
runtest() work w/ a differe output dir
On 2014/03/26 09:38:49, iannucci wrote:
> *different
> 
> Though it would be /best/ if we could avoid runtest.py entirely and just
> implement the bits and pieces which are actually necessary. I'm assuming the
> annotations you're referring to are the test case summaries in step_text?

Yes, and I agree that we should probably try to bypass runtest completely and
just get the annotations.

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:314:
cmd=[gn_output_dir.join('gn_unittests',
On 2014/03/26 11:44:59, Paweł Hajdan Jr. wrote:
> Sorry, all new code added here MUST support retries without patch.
> 
> We've had to add pre-existing telemetry tests below, and they're causing
> annoying flakes, even though they run on the main waterfall.

I forgot to add a TODO -- and will do so -- but I thought we discussed this
yesterday and agreed that adding a TODO a la the telemetry tests would be good
enough?

There's very few gn_unittests, almost no one will be working on them, and they
complete in less than a second. They should not be flaky, I don't think it will
be easy to distinguish a flaky failure from a patch-induced failure, and I will
be perfectly fine w/ flakes being considered failures for now (and I believe
Brett will be, too).

I don't want to start out assuming they will be flaky and writing extra code to
handle it, especially considering that we can't use the normal retry logic for
this (another good reason to make this part of its own recipe).

The analogy to telemetry isn't very good; telemetry is a far more complex set of
tests than gn_unittests.

How does that sound?

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:602: api.step_data('compile (gn with
patch)', api.json.output(None)) +
On 2014/03/26 11:44:59, Paweł Hajdan Jr. wrote:
> These shouldn't be needed after my latest changes. :)

Why not?

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:608: lambda a, b: a + b,
On 2014/03/26 09:38:49, iannucci wrote:
> isn't this the same as sum?

Think so :). I was just cutting and pasting. Will take a look.

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:619: api.step_data('gclient revert') +
On 2014/03/26 11:44:59, Paweł Hajdan Jr. wrote:
> This is likely a no-op.

I don't know what that means; are you saying I don't need the line?

[Dirk Pranke, 2014-03-26 19:16:35]

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
File scripts/slave/recipes/chromium_trybot.py (right):

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:274: gn_output_dir)

Oh, I misread this. No, they should be green.

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:314:
cmd=[gn_output_dir.join('gn_unittests',
On 2014/03/26 16:52:28, Dirk Pranke wrote:
> On 2014/03/26 11:44:59, Paweł Hajdan Jr. wrote:
> > Sorry, all new code added here MUST support retries without patch.
> >

Actually, per further discussion w/ Robbie, I'm going to remove gn_unittests
from this change so that this problem goes away, and
then add a separate change so that we can discuss how best to handle testing for
the GN-based binaries there.

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:602: api.step_data('compile (gn with
patch)', api.json.output(None)) +
On 2014/03/26 16:52:28, Dirk Pranke wrote:
> On 2014/03/26 11:44:59, Paweł Hajdan Jr. wrote:
> > These shouldn't be needed after my latest changes. :)
> 
> Why not?

Never mind, I think I've got it now.

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:608: lambda a, b: a + b,
On 2014/03/26 16:52:28, Dirk Pranke wrote:
> On 2014/03/26 09:38:49, iannucci wrote:
> > isn't this the same as sum?
> 
> Think so :). I was just cutting and pasting. Will take a look.

cleaned up on merge.

https://codereview.chromium.org/211473006/diff/50001/scripts/slave/recipes/ch...
scripts/slave/recipes/chromium_trybot.py:619: api.step_data('gclient revert') +
On 2014/03/26 16:52:28, Dirk Pranke wrote:
> On 2014/03/26 11:44:59, Paweł Hajdan Jr. wrote:
> > This is likely a no-op.
> 
> I don't know what that means; are you saying I don't need the line?

cleaned up on merge.

[Dirk Pranke, 2014-03-26 19:28:02]

Okay, here's a new version of the patch. I've removed the gn_unittests steps,
and incorporated all of the other review feedback.

Please take another look?

[iannucci, 2014-03-26 22:19:02]

lgtm

[iannucci, 2014-03-27 01:33:57]

down with relpaths (not lgtm in that regard).

https://codereview.chromium.org/211473006/diff/180001/scripts/slave/recipes/c...
File
scripts/slave/recipes/chromium_gn.expected/full_chromium_linux_Linux_GN__dbg_.json
(right):

https://codereview.chromium.org/211473006/diff/180001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_gn.expected/full_chromium_linux_Linux_GN__dbg_.json:89:
"out/Debug",
noooooooo! :)

(as we discussed) relative paths on bots are a really bad thing (they lead to a
lot of hard to debug cruft). We should make gn happy with an absolute path.

Maybe if gn was run in [CHECKOUT]/src it could do the relative path calculation
if it needs to do so internally?

https://codereview.chromium.org/211473006/diff/180001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium_gn.py (right):

https://codereview.chromium.org/211473006/diff/180001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_gn.py:23: }[api.properties.get('buildername')]
yeah, we need a real mechanism for this. Bug filed here of how we might do this:
crbug.com/356974

https://codereview.chromium.org/211473006/diff/180001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_gn.py:30: yield api.chromium.run_gn('out/' +
build_config)
Oh, that's actually worse than I thought :D. I guess we don't even have the
'out' string available in a non-absolute path form.

[brettw, 2014-03-27 04:54:03]

https://codereview.chromium.org/211473006/diff/180001/scripts/slave/recipes/c...
File
scripts/slave/recipes/chromium_gn.expected/full_chromium_linux_Linux_GN__dbg_.json
(right):

https://codereview.chromium.org/211473006/diff/180001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_gn.expected/full_chromium_linux_Linux_GN__dbg_.json:88:
"-o",
Delete this.

https://codereview.chromium.org/211473006/diff/180001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_gn.expected/full_chromium_linux_Linux_GN__dbg_.json:89:
"out/Debug",
You can just change this to //out/Debug and there should be no ambiguity.

[Dirk Pranke, 2014-03-27 22:13:12]

Okay, the latest patch resolves the relative path issues, and has been tested to
work on a buildbot config locally; I am testing the tryserver config right now
as well.

Please take another look?

[Dirk Pranke, 2014-03-28 01:57:14]

Okay, I've tested the patch locally for both buildbot and trybot, and things
seem happy. 

Barring something weird happening, I'll plan to land this first thing tomorrow
MTV time.

I'll assume that, having fixed Robbie's no lgtm and Paweł's no lgtm concerns,
Robbie's prior lgtm will be good enough, but please feel free to chime in if you
disagree. It would be great to get a second stamp of approval, though.

[Paweł Hajdan Jr., 2014-03-28 09:45:04]

"not lgtm" for the chromium_trybot changes - I'm really sorry about that, but
I'm convinced this is important enough to *keep working*.

Feel totally free to commit everything else - it Looks Good To Me, and is
actually very good. Thank you for working on that.

https://codereview.chromium.org/211473006/diff/220001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium_gn.py (right):

https://codereview.chromium.org/211473006/diff/220001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_gn.py:16: 
nit: Remove redundant empty line.

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

https://codereview.chromium.org/211473006/diff/220001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_trybot.py:297: api.step_history['compile (gn with
patch)'].retcode != 0)):
For the fallback to be useful please mark compile_with_ninja above and run_gn
with abort_on_failure=False, can_fail_build=False.

This can actually get pretty complex, and so if you want to land this patch I
strongly recommend extracting the trybot recipe changes to a separate CL. In
fact, if you just create a separate gn trybot recipe it can make it much easier.

[Dirk Pranke, 2014-03-28 15:59:43]

https://codereview.chromium.org/211473006/diff/220001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium_gn.py (right):

https://codereview.chromium.org/211473006/diff/220001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_gn.py:16: 
On 2014/03/28 09:45:04, Paweł Hajdan Jr. wrote:
> nit: Remove redundant empty line.

will do.

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

https://codereview.chromium.org/211473006/diff/220001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_trybot.py:297: api.step_history['compile (gn with
patch)'].retcode != 0)):
On 2014/03/28 09:45:04, Paweł Hajdan Jr. wrote:
> For the fallback to be useful please mark compile_with_ninja above and run_gn
> with abort_on_failure=False, can_fail_build=False.

Good catch, will do.

> 
> This can actually get pretty complex, and so if you want to land this patch I
> strongly recommend extracting the trybot recipe changes to a separate CL. In
> fact, if you just create a separate gn trybot recipe it can make it much
easier.

I started out this whole effort by trying to create separate CLs for trybots vs.
buildbots, and the figuring out what was changing where was too complicated
(none of us could keep track of which feedback was in which CL :).

Case in point: I can't create a separate gn trybot recipe, because we don't have
separate GN trybots, though I had hoped to do that at first. The changes *have*
to be in this file because we're changing the existing chromium trybot behavior.

As soon as we have separate standalone GN trybots, I will create a separate
recipe.

At any rate, changing the above two lines is simple enough. Was there anything
else keeping this from landing?

[Dirk Pranke, 2014-03-28 20:00:24]

Please take another look?

Note that the large delta between patchsets 5 and 6 in
unittest_should_run_gn_compile_failure is because running gn will no longer
abort the build, and so we're running steps that should've been run before, but
weren't.

[Paweł Hajdan Jr., 2014-03-28 20:56:20]

LGTM. I appreciate your patience with me.

Thank you for working on this.

[Dirk Pranke, 2014-03-28 21:46:57]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2014-03-28 21:47:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dpranke@chromium.org/211473006/260001

[commit-bot: I haz the power, 2014-03-28 21:48:27]

Change committed as 260291

[iannucci, 2014-03-28 22:40:42]

some other comments

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

https://codereview.chromium.org/211473006/diff/260001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:246: return self.m.step(name=name,
cmd=[ninja_path, '-C', output_dir], **kwargs)
Really not convinced that output_dir should be specified from outside the
chromium module here. The configuration for chromium has a build_config_fs which
could be set in a 'gn' configuration item. Then self.output_dir would just be
correct (though it would still be an absolute path)

https://codereview.chromium.org/211473006/diff/260001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:255: gn_args = ['--root=' +
str(self.m.path['checkout']), 'gen', output_arg]
is gn using a non-standard cli parser, which is why this can't be 2 tokens?

https://codereview.chromium.org/211473006/diff/260001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium_gn.py (right):

https://codereview.chromium.org/211473006/diff/260001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_gn.py:27: yield api.chromium.run_gn('//out/' +
build_config)
I think that if this must be not-really-a-path, the chromium module should
/still/ own it (probably in the chromium config).

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

https://codereview.chromium.org/211473006/diff/260001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_trybot.py:294: retry_at_lkcr =
api.step_history['compile (with patch)'].retcode != 0
api.step_history.last_step().retcode

might be clearer here (and avoids the string name duplication)

edit: though I see that it was this way before... would be best to change it.

https://codereview.chromium.org/211473006/diff/260001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_trybot.py:506: api.step_data('compile (gn with
patch)')
can remove this empty step_data

[Dirk Pranke, 2014-03-28 23:52:06]

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

https://codereview.chromium.org/211473006/diff/260001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:246: return self.m.step(name=name,
cmd=[ninja_path, '-C', output_dir], **kwargs)
On 2014/03/28 22:40:42, iannucci wrote:
> Really not convinced that output_dir should be specified from outside the
> chromium module here. The configuration for chromium has a build_config_fs
which
> could be set in a 'gn' configuration item. Then self.output_dir would just be
> correct (though it would still be an absolute path)

I don't think that really works because we actually need two output dirs while
generating the steps. We would have to apply and deapply configs as appropriate,
and I think that just makes things a lot more complicated than passing values in
directly.

Also, I want this to go away soonish, when we switch to dedicated gn trybots.

https://codereview.chromium.org/211473006/diff/260001/scripts/slave/recipe_mo...
scripts/slave/recipe_modules/chromium/api.py:255: gn_args = ['--root=' +
str(self.m.path['checkout']), 'gen', output_arg]
On 2014/03/28 22:40:42, iannucci wrote:
> is gn using a non-standard cli parser, which is why this can't be 2 tokens?

Brett believes that  ['--root', path] is actually non standard and doesn't want
to support it in GN (and so it doesn't currently work in GN). He says that
chromium's arg parsing doesn't actually support that way of passing values and
wants to retain consistency w/ Chrome.

I chose to defer that battle for another time; GN's arg parsing needs to be
reworked anyway so it'll come up again as part of that.

https://codereview.chromium.org/211473006/diff/260001/scripts/slave/recipes/c...
File scripts/slave/recipes/chromium_gn.py (right):

https://codereview.chromium.org/211473006/diff/260001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_gn.py:27: yield api.chromium.run_gn('//out/' +
build_config)
On 2014/03/28 22:40:42, iannucci wrote:
> I think that if this must be not-really-a-path, the chromium module should
> /still/ own it (probably in the chromium config).

I think once we can get rid of the need to specify the custom build dir, this
should just disappear into the run_gn() method.

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

https://codereview.chromium.org/211473006/diff/260001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_trybot.py:294: retry_at_lkcr =
api.step_history['compile (with patch)'].retcode != 0
On 2014/03/28 22:40:42, iannucci wrote:
> api.step_history.last_step().retcode
> 
> might be clearer here (and avoids the string name duplication)
> 
> edit: though I see that it was this way before... would be best to change it.

In this case, that's clearer, and I agree that we should change it.

Down on lines 303-304, we actually need the results of the last two steps,
hopefully we can specify those similarly.

[Dirk Pranke, 2014-03-28 23:52:44]

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

https://codereview.chromium.org/211473006/diff/260001/scripts/slave/recipes/c...
scripts/slave/recipes/chromium_trybot.py:506: api.step_data('compile (gn with
patch)')
On 2014/03/28 22:40:42, iannucci wrote:
> can remove this empty step_data

okay.