Add support for GN's --runtime-deps-list-file flag to MB.

Add support for --runtime-deps-list-file flag to MB.

This allows us to compute what files are needed at runtime
to run each of the specified targets while we are also
generating the .ninja files to build the targets; since GN
has integrated support for this, we don't need to build
.isolate.gen.json or .isolate files at compile time like
we do for GYP.

As far as the implementation goes, while
--runtime-deps-list-file takes a list of GN labels (like
//base:base_unittests), we actually only have a list of ninja
build targets (like 'base_unittests'). So, we need to add a
mapping of ninja target -> GN label somewhere.

I think the best place to do this is in //testing/buildbot ,
and there I've updated the manage.py presubmit checks to
make sure that we have mappings for all of the targets
listed in the *.json config files. We can't easily check
that the mapping is right, but at least we can verify
that it does exist.

R=phajdan.jr@chromium.org, maruel@chromium.org
BUG=480053
Committed: https://crrev.com/74559b593fa49d493b07fce25a2fbf809c041b7f
Cr-Commit-Position: refs/heads/master@{#333802}

[Dirk Pranke, 2015-06-09 02:40:14]

dpranke@chromium.org changed reviewers:
- brettw@chromium.org, maruel@chromium.org

[Dirk Pranke, 2015-06-10 06:39:55]

dpranke@chromium.org changed reviewers:
+ maruel@chromium.org, phajdan.jr@chromium.org

[Dirk Pranke, 2015-06-10 06:43:17]

Please take a look? 

I think this and https://codereview.chromium.org/1177593002/ should be what's
needed to enable swarming on the GN bots (this patch needs to land before the
recipe-side change).

https://codereview.chromium.org/1169923008/diff/40001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/1169923008/diff/40001/tools/mb/mb.py#newcode423
tools/mb/mb.py:423: self.PrintJSON(outp)
this is an unrelated bug that pylint revealed :).

[Paweł Hajdan Jr., 2015-06-10 09:30:08]

LGTM

https://codereview.chromium.org/1169923008/diff/40001/testing/buildbot/manage.py
File testing/buildbot/manage.py (right):

https://codereview.chromium.org/1169923008/diff/40001/testing/buildbot/manage...
testing/buildbot/manage.py:134: if (not d['test'] in ninja_targets and
nit: Consider "d['test'] not in ninja_targets" instead - would be more readable
and consistent with line below.

[M-A Ruel, 2015-06-10 14:53:40]

https://codereview.chromium.org/1169923008/diff/40001/testing/buildbot/manage.py
File testing/buildbot/manage.py (right):

https://codereview.chromium.org/1169923008/diff/40001/testing/buildbot/manage...
testing/buildbot/manage.py:254: extra_targets = set(ninja_targets.keys()) -
ninja_targets_seen
set(ninja_targets)

https://codereview.chromium.org/1169923008/diff/40001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/1169923008/diff/40001/tools/mb/mb.py#newcode350
tools/mb/mb.py:350: ninja_targets_to_labels =
ast.literal_eval(self.ReadFile(os.path.join(
I trick we used in isolate Go is to pre-process the file to remove all lines
that start with a #, roughly;

content = '\n'.join(
 l for l in self.ReadFile(os.path.join(...)).splitlines()
 if not l.lstrip().startswith('#'))

ninja_targets_to_labels = json.loads(content)

To be clear; that's a total abuse but it works fine enough. The only annoying
thing is the trailing comma but  we can live with it.

I don't mind either way. It's just that in Go, ast wasn't an option.

https://codereview.chromium.org/1169923008/diff/40001/tools/mb/mb.py#newcode356
tools/mb/mb.py:356: (target, "//testing/buildbot/ninja_to_gn.pyl"))
Use single quotes

https://codereview.chromium.org/1169923008/diff/40001/tools/mb/mb.py#newcode361
tools/mb/mb.py:361: cmd.append('--runtime-deps-list-file=%s' %
gn_runtime_deps_path)
use two separate args, it works better on Windows.

https://codereview.chromium.org/1169923008/diff/40001/tools/mb/mb.py#newcode366
tools/mb/mb.py:366: for target in swarming_targets:
Where is this variable defined? I don't see how this code can work.

[Dirk Pranke, 2015-06-10 16:07:59]

https://codereview.chromium.org/1169923008/diff/40001/testing/buildbot/manage.py
File testing/buildbot/manage.py (right):

https://codereview.chromium.org/1169923008/diff/40001/testing/buildbot/manage...
testing/buildbot/manage.py:134: if (not d['test'] in ninja_targets and
On 2015/06/10 09:30:08, Paweł Hajdan Jr. wrote:
> nit: Consider "d['test'] not in ninja_targets" instead - would be more
readable
> and consistent with line below.

will fix.

https://codereview.chromium.org/1169923008/diff/40001/testing/buildbot/manage...
testing/buildbot/manage.py:254: extra_targets = set(ninja_targets.keys()) -
ninja_targets_seen
On 2015/06/10 14:53:40, M-A Ruel wrote:
> set(ninja_targets)

will fix.

https://codereview.chromium.org/1169923008/diff/40001/tools/mb/mb.py
File tools/mb/mb.py (right):

https://codereview.chromium.org/1169923008/diff/40001/tools/mb/mb.py#newcode350
tools/mb/mb.py:350: ninja_targets_to_labels =
ast.literal_eval(self.ReadFile(os.path.join(
On 2015/06/10 14:53:40, M-A Ruel wrote:
> I trick we used in isolate Go is to pre-process the file to remove all lines
> that start with a #, roughly;
> 
> content = '\n'.join(
>  l for l in self.ReadFile(os.path.join(...)).splitlines()
>  if not l.lstrip().startswith('#'))
> 
> ninja_targets_to_labels = json.loads(content)
> 
> To be clear; that's a total abuse but it works fine enough. The only annoying
> thing is the trailing comma but  we can live with it.
> 
> I don't mind either way. It's just that in Go, ast wasn't an option.

That is a good suggestion, but since .pyl / ast.literal_eval() is an option,
I'll stick w/ that for now ...

https://codereview.chromium.org/1169923008/diff/40001/tools/mb/mb.py#newcode356
tools/mb/mb.py:356: (target, "//testing/buildbot/ninja_to_gn.pyl"))
On 2015/06/10 14:53:40, M-A Ruel wrote:
> Use single quotes

will fix.

https://codereview.chromium.org/1169923008/diff/40001/tools/mb/mb.py#newcode361
tools/mb/mb.py:361: cmd.append('--runtime-deps-list-file=%s' %
gn_runtime_deps_path)
On 2015/06/10 14:53:40, M-A Ruel wrote:
> use two separate args, it works better on Windows.

I can't; GN only supports --foo=bar style arguments (argue w/ Brett about that
one).

https://codereview.chromium.org/1169923008/diff/40001/tools/mb/mb.py#newcode366
tools/mb/mb.py:366: for target in swarming_targets:
On 2015/06/10 14:53:40, M-A Ruel wrote:
> Where is this variable defined? I don't see how this code can work.

Whoops, you're right. This should be looping over gn_labels, but apparently
even that isn't actually defined. Will fix.

[Dirk Pranke, 2015-06-10 19:38:13]

Patch updated; please take another look?

I should look and see if my coverage has gotten bad enough now that I need to
add another couple unit tests ...

[M-A Ruel, 2015-06-10 19:43:55]

lgtm

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

The CQ bit was checked by dpranke@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1169923008/#ps60001
(title: "update w/ review feedback, fix lint errors")

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

The CQ bit was unchecked by dpranke@chromium.org

[Dirk Pranke, 2015-06-10 19:47:35]

My coverage numbers are indeed getting pretty bad now, but I'll leave that for
another day to avoid confusing the patches.

[Dirk Pranke, 2015-06-10 19:47:38]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2015-06-10 19:48:16]

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

[commit-bot: I haz the power, 2015-06-10 19:55:57]

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

[commit-bot: I haz the power, 2015-06-10 19:55:58]

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

[Dirk Pranke, 2015-06-10 20:07:54]

On 2015/06/10 19:55:58, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

In which the change that I approved yesterday bites me today ... at least the
good news is that the presubmit did exactly what it was supposed to do.

[Dirk Pranke, 2015-06-10 20:07:57]

The CQ bit was checked by dpranke@chromium.org

[Dirk Pranke, 2015-06-10 20:07:58]

The patchset sent to the CQ was uploaded after l-g-t-m from maruel@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1169923008/#ps80001
(title: "merge to HEAD, add nacl_helper_nonsfi_unittests to fix presubmit
failure")

[commit-bot: I haz the power, 2015-06-10 20:09:07]

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

[commit-bot: I haz the power, 2015-06-10 21:25:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-10 21:26:36]

Patchset 5 (id:??) landed as
https://crrev.com/74559b593fa49d493b07fce25a2fbf809c041b7f
Cr-Commit-Position: refs/heads/master@{#333802}

[Nico, 2015-07-15 17:09:30]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-07-15 17:09:31]

https://codereview.chromium.org/1169923008/diff/80001/testing/buildbot/ninja_...
File testing/buildbot/ninja_to_gn.pyl (right):

https://codereview.chromium.org/1169923008/diff/80001/testing/buildbot/ninja_...
testing/buildbot/ninja_to_gn.pyl:12: # hierarchical).
I just happened to see this file. Is this really needed? The hierarchical test
names work with gn too for most (all?) of the tests in this file, no?