🚀 Geneate base_unittest's Android .isolate file at build-time

Geneate base_unittest's Android .isolate file at build-time

This is a v1. A better future version would have a GN primitive to
produce the file.

Since the generation runs in parallel with other test targets, there's no added
build latency.

BUG=589318, 593416

[agrieve, 2016-03-15 20:07:52]

agrieve@chromium.org changed reviewers:
+ dpranke@chromium.org, jbudorick@chromium.org, thakis@chromium.org

[agrieve, 2016-03-15 20:07:53]

Here's something that's much closer to our endgame.

[jbudorick, 2016-03-15 20:23:14]

https://codereview.chromium.org/1805643002/diff/1/testing/test.gni
File testing/test.gni (right):

https://codereview.chromium.org/1805643002/diff/1/testing/test.gni#newcode33
testing/test.gni:33: _requires_apk_isolate =
Why define this outside of is_android?

[jbudorick, 2016-03-15 20:23:36]

On 2016/03/15 20:23:14, jbudorick wrote:
> https://codereview.chromium.org/1805643002/diff/1/testing/test.gni
> File testing/test.gni (right):
> 
> https://codereview.chromium.org/1805643002/diff/1/testing/test.gni#newcode33
> testing/test.gni:33: _requires_apk_isolate =
> Why define this outside of is_android?

also, nit: s/Geneate/Generate/ in the description

[agrieve, 2016-03-15 20:34:08]

https://codereview.chromium.org/1805643002/diff/1/testing/test.gni
File testing/test.gni (right):

https://codereview.chromium.org/1805643002/diff/1/testing/test.gni#newcode33
testing/test.gni:33: _requires_apk_isolate =
On 2016/03/15 20:23:14, jbudorick wrote:
> Why define this outside of is_android?

I thought it'd be nice to have it live right beside data = in the callers. I
have to access it here to mark it as used for non-android platforms.

Definitely don't feel strongly about this though, and I think we can get rid of
the variable once gn supports a primitive for writing the deps (we'll just write
them for every test and ignore it if it's empty)

[Dirk Pranke, 2016-03-15 22:32:42]

dpranke@chromium.org changed reviewers:
+ brettw@chromium.org

[Dirk Pranke, 2016-03-15 22:32:42]

this does not look good to me.

I think we should stick with using the checked-in isolates until we have a
proper fix (or fixes).

Two obvious downsides to this patch as-is:

Running GN always causes *.ninja files to be written and other scripts to be
executed. I would be nervous about executing this at the same time as an
on-going build even if we were generating files into a different build
directory. If we're generating files into the *same* build directory (which is
what you're doing here), that's even worse.

Second, running GN isn't that expensive, but it will peg all of the cores for a
couple seconds. In addition, I have no idea what would happen if we tried to run
N GN's in parallel, so you'd probably need to either implement locking in your
wrapper script or force all of the GN's to be run out of a single ninja pool of
size 1, neither of which makes me terribly excited.

[agrieve, 2016-03-16 01:06:24]

On 2016/03/15 22:32:42, Dirk Pranke wrote:
> this does not look good to me.
> 
> I think we should stick with using the checked-in isolates until we have a
> proper fix (or fixes).
> 
> Two obvious downsides to this patch as-is:
> 
> Running GN always causes *.ninja files to be written and other scripts to be
> executed. I would be nervous about executing this at the same time as an
> on-going build even if we were generating files into a different build
> directory. If we're generating files into the *same* build directory (which is
> what you're doing here), that's even worse.

I'm not seeing ninja files being written by this (uses gn desc, not gn gen). But
it makes sense that exec_script()s are running (although they're not supposed to
have side-effects?).

> 
> Second, running GN isn't that expensive, but it will peg all of the cores for
a
> couple seconds. In addition, I have no idea what would happen if we tried to
run
> N GN's in parallel, so you'd probably need to either implement locking in your
> wrapper script or force all of the GN's to be run out of a single ninja pool
of
> size 1, neither of which makes me terribly excited.

Just tried running 100 at the same time via:
for i in $(seq 1 100); do (build/android/gyp/gen-android-test-isolate.py
out-gn/Release //base:base_unittests&) ; done

My machine was a bit unresponsive, but I could still use vim and execute
commands.

How about until we have a write_runtime_deps() builtin, I just add a call to
os.nice(10)?

[Dirk Pranke, 2016-03-16 01:13:47]

What's wrong w/ just using the checked-in files until we have the real fix?

[agrieve, 2016-03-16 01:36:17]

On 2016/03/16 01:13:47, Dirk Pranke wrote:
> What's wrong w/ just using the checked-in files until we have the real fix?

That was my initial thought too, but then
https://codereview.chromium.org/1784273003/ didn't go so well. And now that (I
think) we've agreed upon the endgame, manually adding all the .isolate does seem
like wasted work.

Just realized as well that I can remove requires_apk_isolate = true, and check
whether isolate_file != "" to enable this. Updated to do so. This now (should)
fix all swarming tests.

If you're still too nervous about, I'll probably look at adding the
write_runtime_deps() to GN and just go straight to that instead of this
intermediate step.

[Dirk Pranke, 2016-03-16 01:43:08]

On 2016/03/16 01:36:17, agrieve wrote:
> On 2016/03/16 01:13:47, Dirk Pranke wrote:
> > What's wrong w/ just using the checked-in files until we have the real fix?
> 
> That was my initial thought too, but then
> https://codereview.chromium.org/1784273003/ didn't go so well. And now that (I
> think) we've agreed upon the endgame, manually adding all the .isolate does
seem
> like wasted work.
> 
> Just realized as well that I can remove requires_apk_isolate = true, and check
> whether isolate_file != "" to enable this. Updated to do so. This now (should)
> fix all swarming tests.
> 
> If you're still too nervous about, I'll probably look at adding the
> write_runtime_deps() to GN and just go straight to that instead of this
> intermediate step.

Adding write_runtime_deps() is definitely better, if you're willing to do that
instead.

Failing that, I'd much rather manually add the .isolate files than hack around
it this way.

[Nico, 2016-03-18 21:47:01]

On 2016/03/16 01:36:17, agrieve wrote:
> On 2016/03/16 01:13:47, Dirk Pranke wrote:
> > What's wrong w/ just using the checked-in files until we have the real fix?
> 
> That was my initial thought too, but then
> https://codereview.chromium.org/1784273003/ didn't go so well. And now that (I
> think) we've agreed upon the endgame

Is the desired end state documented anywhere?

>, manually adding all the .isolate does seem
> like wasted work.
> 
> Just realized as well that I can remove requires_apk_isolate = true, and check
> whether isolate_file != "" to enable this. Updated to do so. This now (should)
> fix all swarming tests.
> 
> If you're still too nervous about, I'll probably look at adding the
> write_runtime_deps() to GN and just go straight to that instead of this
> intermediate step.

[Dirk Pranke, 2016-03-18 22:04:07]

On 2016/03/18 21:47:01, Nico wrote:
> On 2016/03/16 01:36:17, agrieve wrote:
> > ... And now that (I
> > think) we've agreed upon the endgame
> 
> Is the desired end state documented anywhere?
> 

Actually, I don't think we've agreed on the endgame yet,
but the discussion is happening in crbug.com/593416 
(linked to from here, and you're cc'ed on it).

[Nico, 2016-03-18 22:15:01]

On 2016/03/18 22:04:07, Dirk Pranke wrote:
> On 2016/03/18 21:47:01, Nico wrote:
> > On 2016/03/16 01:36:17, agrieve wrote:
> > > ... And now that (I
> > > think) we've agreed upon the endgame
> > 
> > Is the desired end state documented anywhere?
> > 
> 
> Actually, I don't think we've agreed on the endgame yet,
> but the discussion is happening in crbug.com/593416 
> (linked to from here, and you're cc'ed on it).

For some reason I stopped receiving bug mail (no threads with "Generated
.isolate files for Android Swarming Contain Unused Files" in my inbox, and also
no threads for discussions on other recent bugs that I filed / starred -- I
happened to see some via polling). Do you know who I'd talk to about missing
bugmail?

[Dirk Pranke, 2016-03-18 22:18:58]

On 2016/03/18 22:15:01, Nico wrote:
> For some reason I stopped receiving bug mail (no threads with "Generated
> .isolate files for Android Swarming Contain Unused Files" in my inbox, and
also
> no threads for discussions on other recent bugs that I filed / starred -- I
> happened to see some via polling). Do you know who I'd talk to about missing
> bugmail?

Not offhand ... maybe send email to infra-dev+monorail and/or file a bug against
monorail?