[gyp][Android] Implement TestGypAndroid.run_built_executable.

[gyp][Android] Implement TestGypAndroid.run_built_executable.

Note that this includes buildbot steps to start and stop an Android emulator
for testing.

BUG=gyp:324
R=bradnelson@google.com, torne@chromium.org

Committed: https://code.google.com/p/gyp/source/detail?r=1943

[Torne, 2014-06-11 12:44:18]

Looks pretty good in general; haven't reviewed it in detail but I have a couple
of questions:

https://codereview.chromium.org/321953005/diff/20001/test/actions/gyptest-all.py
File test/actions/gyptest-all.py (right):

https://codereview.chromium.org/321953005/diff/20001/test/actions/gyptest-all...
test/actions/gyptest-all.py:59: subdir='subdir1')
Why do we need to add subdirs here? Perhaps we can change the generator's
behaviour to avoid needing to? I thought I had made the generator produce all
executables to <(PRODUCT_DIR) to make it easy to run them.

https://codereview.chromium.org/321953005/diff/20001/test/lib/TestGyp.py
File test/lib/TestGyp.py (right):

https://codereview.chromium.org/321953005/diff/20001/test/lib/TestGyp.py#newc...
test/lib/TestGyp.py:602: self._complete(out, kw.pop('stdout', None), None, None,
None, match)
Is there a way to return the program's exit status here?

[jbudorick, 2014-06-11 16:30:22]

https://codereview.chromium.org/321953005/diff/20001/test/actions/gyptest-all.py
File test/actions/gyptest-all.py (right):

https://codereview.chromium.org/321953005/diff/20001/test/actions/gyptest-all...
test/actions/gyptest-all.py:59: subdir='subdir1')
On 2014/06/11 12:44:18, Torne wrote:
> Why do we need to add subdirs here? Perhaps we can change the generator's
> behaviour to avoid needing to? I thought I had made the generator produce all
> executables to <(PRODUCT_DIR) to make it easy to run them.

I'm not sure if we can change how the generator decides the android_module or
not. At a glance, it appeared to me as though we may be locked in to the current
way because of our use of local-intermediates-dir and intermediates-dir-for from
android's build system. I did figure that doing so is probably beyond the scope
of this CL.

If you think that changing the generator is the correct way to go, I can remove
all of the subdir params I've added. Doing so will cause these tests to fail,
though.

As far as I can tell, executables go to the intermediate dir like everything
else.

https://codereview.chromium.org/321953005/diff/20001/test/lib/TestGyp.py
File test/lib/TestGyp.py (right):

https://codereview.chromium.org/321953005/diff/20001/test/lib/TestGyp.py#newc...
test/lib/TestGyp.py:555: if subprocess.call(adb_command, stdout=adb_out,
stderr=adb_err) != 0:
(tests should fail here for nonzero exit codes)

https://codereview.chromium.org/321953005/diff/20001/test/lib/TestGyp.py#newc...
test/lib/TestGyp.py:602: self._complete(out, kw.pop('stdout', None), None, None,
None, match)
On 2014/06/11 12:44:18, Torne wrote:
> Is there a way to return the program's exit status here?

If a program exits with a nonzero exit code, _call_adb should cause the test to
fail. (See above.) Do we need to be more specific?

[Torne, 2014-06-11 16:41:39]

https://codereview.chromium.org/321953005/diff/20001/test/actions/gyptest-all.py
File test/actions/gyptest-all.py (right):

https://codereview.chromium.org/321953005/diff/20001/test/actions/gyptest-all...
test/actions/gyptest-all.py:59: subdir='subdir1')
On 2014/06/11 16:30:22, jbudorick wrote:
> On 2014/06/11 12:44:18, Torne wrote:
> > Why do we need to add subdirs here? Perhaps we can change the generator's
> > behaviour to avoid needing to? I thought I had made the generator produce
all
> > executables to <(PRODUCT_DIR) to make it easy to run them.
> 
> I'm not sure if we can change how the generator decides the android_module or
> not. At a glance, it appeared to me as though we may be locked in to the
current
> way because of our use of local-intermediates-dir and intermediates-dir-for
from
> android's build system. I did figure that doing so is probably beyond the
scope
> of this CL.

I don't understand what the subdir parameter here is doing in the first place,
or how that's related to the android intermediate dirs, I mean.

> If you think that changing the generator is the correct way to go, I can
remove
> all of the subdir params I've added. Doing so will cause these tests to fail,
> though.

Having to special case every test that happens to use this mechanism seems
really gross, yes. We should avoid this.

> As far as I can tell, executables go to the intermediate dir like everything
> else.

Sorry, I checked and the generator currently only copies *host* executables to
<(PRODUCT_DIR) (so that they can be used by other build rules). We've never
really cared about building target executables before, so they don't have any
special handling.

https://codereview.chromium.org/321953005/diff/20001/test/lib/TestGyp.py
File test/lib/TestGyp.py (right):

https://codereview.chromium.org/321953005/diff/20001/test/lib/TestGyp.py#newc...
test/lib/TestGyp.py:602: self._complete(out, kw.pop('stdout', None), None, None,
None, match)
On 2014/06/11 16:30:22, jbudorick wrote:
> On 2014/06/11 12:44:18, Torne wrote:
> > Is there a way to return the program's exit status here?
> 
> If a program exits with a nonzero exit code, _call_adb should cause the test
to
> fail. (See above.) Do we need to be more specific?

adb does not return the error code of the program run by adb shell, it always
returns 0 unless adb itself failed. To get the error code you need to wrap the
program itself in a script that prints $? after it exits and parse the output to
retrieve it, as far as I know. This is done in various places in chromium :/

In theory the gyp tests could test the specific error code returned by the
program. I guess they probably don't currently, but it's supported by the test
framework being used here.

Whether it's worth it probably depends how difficult it is..

[jbudorick, 2014-06-11 16:57:41]

On 2014/06/11 16:41:39, Torne wrote:
>
https://codereview.chromium.org/321953005/diff/20001/test/actions/gyptest-all.py
> File test/actions/gyptest-all.py (right):
> 
>
https://codereview.chromium.org/321953005/diff/20001/test/actions/gyptest-all...
> test/actions/gyptest-all.py:59: subdir='subdir1')
> On 2014/06/11 16:30:22, jbudorick wrote:
> > On 2014/06/11 12:44:18, Torne wrote:
> > > Why do we need to add subdirs here? Perhaps we can change the generator's
> > > behaviour to avoid needing to? I thought I had made the generator produce
> all
> > > executables to <(PRODUCT_DIR) to make it easy to run them.
> > 
> > I'm not sure if we can change how the generator decides the android_module
or
> > not. At a glance, it appeared to me as though we may be locked in to the
> current
> > way because of our use of local-intermediates-dir and intermediates-dir-for
> from
> > android's build system. I did figure that doing so is probably beyond the
> scope
> > of this CL.
> 
> I don't understand what the subdir parameter here is doing in the first place,
> or how that's related to the android intermediate dirs, I mean.
> 

see TestGypAndroid.built_file_path, which:
 - gets called by run_built_executable to find the executable
 - calls android_module with any available 'subdir' kwarg to determine the
android module, then
 - gets the intermediate dir from the module name.

(The subdirectory comes in on the generator side in
AndroidMkWriter.ComputeAndroidModule, which includes the relative path to the
makefile in the android module and thus the intermediate dir.)

> > If you think that changing the generator is the correct way to go, I can
> remove
> > all of the subdir params I've added. Doing so will cause these tests to
fail,
> > though.
> 
> Having to special case every test that happens to use this mechanism seems
> really gross, yes. We should avoid this.
> 

Agreed, it is gross.

> > As far as I can tell, executables go to the intermediate dir like everything
> > else.
> 
> Sorry, I checked and the generator currently only copies *host* executables to
> <(PRODUCT_DIR) (so that they can be used by other build rules). We've never
> really cared about building target executables before, so they don't have any
> special handling.
> 
> https://codereview.chromium.org/321953005/diff/20001/test/lib/TestGyp.py
> File test/lib/TestGyp.py (right):
> 
>
https://codereview.chromium.org/321953005/diff/20001/test/lib/TestGyp.py#newc...
> test/lib/TestGyp.py:602: self._complete(out, kw.pop('stdout', None), None,
None,
> None, match)
> On 2014/06/11 16:30:22, jbudorick wrote:
> > On 2014/06/11 12:44:18, Torne wrote:
> > > Is there a way to return the program's exit status here?
> > 
> > If a program exits with a nonzero exit code, _call_adb should cause the test
> to
> > fail. (See above.) Do we need to be more specific?
> 
> adb does not return the error code of the program run by adb shell, it always
> returns 0 unless adb itself failed. To get the error code you need to wrap the
> program itself in a script that prints $? after it exits and parse the output
to
> retrieve it, as far as I know. This is done in various places in chromium :/
> 

including in code I've used and edited a lot... sigh. Should've remembered that.

> In theory the gyp tests could test the specific error code returned by the
> program. I guess they probably don't currently, but it's supported by the test
> framework being used here.
> 
> Whether it's worth it probably depends how difficult it is..

This shouldn't be too difficult.

[Torne, 2014-06-11 17:05:41]

On 2014/06/11 16:57:41, jbudorick wrote:
> On 2014/06/11 16:41:39, Torne wrote:
> >
>
https://codereview.chromium.org/321953005/diff/20001/test/actions/gyptest-all.py
> > File test/actions/gyptest-all.py (right):
> > 
> >
>
https://codereview.chromium.org/321953005/diff/20001/test/actions/gyptest-all...
> > test/actions/gyptest-all.py:59: subdir='subdir1')
> > On 2014/06/11 16:30:22, jbudorick wrote:
> > > On 2014/06/11 12:44:18, Torne wrote:
> > > > Why do we need to add subdirs here? Perhaps we can change the
generator's
> > > > behaviour to avoid needing to? I thought I had made the generator
produce
> > all
> > > > executables to <(PRODUCT_DIR) to make it easy to run them.
> > > 
> > > I'm not sure if we can change how the generator decides the android_module
> or
> > > not. At a glance, it appeared to me as though we may be locked in to the
> > current
> > > way because of our use of local-intermediates-dir and
intermediates-dir-for
> > from
> > > android's build system. I did figure that doing so is probably beyond the
> > scope
> > > of this CL.
> > 
> > I don't understand what the subdir parameter here is doing in the first
place,
> > or how that's related to the android intermediate dirs, I mean.
> > 
> 
> see TestGypAndroid.built_file_path, which:
>  - gets called by run_built_executable to find the executable
>  - calls android_module with any available 'subdir' kwarg to determine the
> android module, then
>  - gets the intermediate dir from the module name.
> 
> (The subdirectory comes in on the generator side in
> AndroidMkWriter.ComputeAndroidModule, which includes the relative path to the
> makefile in the android module and thus the intermediate dir.)

Ah, thanks. Sorry, even though I wrote half of this the details are fuzzy :)

The behaviour of the make/ninja generators is to stick all executables in the
top-level output directory no matter where they came from. It's probably okay
for the android generator to do the equivalent thing and stick all the
executables in obj/GYP/shared_intermediates since that is PRODUCT_DIR.

You could try that easily by just dropping the "and self.toolset == 'host'" in
ComputeOutput in android.py. The WebView build doesn't have any non-host
executables, so it can't change the behaviour there - as long as it doesn't make
the test situation worse it should be fine :)

> > > If you think that changing the generator is the correct way to go, I can
> > remove
> > > all of the subdir params I've added. Doing so will cause these tests to
> fail,
> > > though.
> > 
> > Having to special case every test that happens to use this mechanism seems
> > really gross, yes. We should avoid this.
> > 
> 
> Agreed, it is gross.
> 
> > > As far as I can tell, executables go to the intermediate dir like
everything
> > > else.
> > 
> > Sorry, I checked and the generator currently only copies *host* executables
to
> > <(PRODUCT_DIR) (so that they can be used by other build rules). We've never
> > really cared about building target executables before, so they don't have
any
> > special handling.
> > 
> > https://codereview.chromium.org/321953005/diff/20001/test/lib/TestGyp.py
> > File test/lib/TestGyp.py (right):
> > 
> >
>
https://codereview.chromium.org/321953005/diff/20001/test/lib/TestGyp.py#newc...
> > test/lib/TestGyp.py:602: self._complete(out, kw.pop('stdout', None), None,
> None,
> > None, match)
> > On 2014/06/11 16:30:22, jbudorick wrote:
> > > On 2014/06/11 12:44:18, Torne wrote:
> > > > Is there a way to return the program's exit status here?
> > > 
> > > If a program exits with a nonzero exit code, _call_adb should cause the
test
> > to
> > > fail. (See above.) Do we need to be more specific?
> > 
> > adb does not return the error code of the program run by adb shell, it
always
> > returns 0 unless adb itself failed. To get the error code you need to wrap
the
> > program itself in a script that prints $? after it exits and parse the
output
> to
> > retrieve it, as far as I know. This is done in various places in chromium :/
> > 
> 
> including in code I've used and edited a lot... sigh. Should've remembered
that.

It'd be really nice if adb *did* propagate this error code :)

> 
> > In theory the gyp tests could test the specific error code returned by the
> > program. I guess they probably don't currently, but it's supported by the
test
> > framework being used here.
> > 
> > Whether it's worth it probably depends how difficult it is..
> 
> This shouldn't be too difficult.

[jbudorick, 2014-06-12 02:50:50]

Good news: using the shared gyp intermediates dir seems to work, and the result
is much less gross than having to specific subdir all over the place.

Bad news: I'm still working on the adb return code thing. It's only applicable
for adb shell (_call_adb currently does any adb command, not just shell), so we
can't necessarily return it. I think there are two options here:
 - handle shell separately
 - fail the test on nonzero exit codes from shell commands within _call_adb

WDYT?

[Torne, 2014-06-12 09:38:49]

Just do whatever is reasonable to support for the return code thing. The tests
don't currently appear to rely on testing it; we can always revisit it later.
This change is already a huge improvement to our testing here and we should get
it in.

https://codereview.chromium.org/321953005/diff/40001/pylib/gyp/generator/andr...
File pylib/gyp/generator/android.py (right):

https://codereview.chromium.org/321953005/diff/40001/pylib/gyp/generator/andr...
pylib/gyp/generator/android.py:881: self.WriteLn('LOCAL_MODULE_PATH :=
$(gyp_shared_intermediate_dir)')
We probably want a comment here to explain that we expect executables to be for
build/test purposes only, which is why we're installing target executables to a
path that won't actually be included in the system image. This might surprise
someone later ;)

[jbudorick, 2014-06-12 19:50:05]

On 2014/06/12 09:38:49, Torne wrote:
> Just do whatever is reasonable to support for the return code thing. The tests
> don't currently appear to rely on testing it; we can always revisit it later.
> This change is already a huge improvement to our testing here and we should
get
> it in.
> 

Tests now fail if an adb shell command exits with a nonzero value.

>
https://codereview.chromium.org/321953005/diff/40001/pylib/gyp/generator/andr...
> File pylib/gyp/generator/android.py (right):
> 
>
https://codereview.chromium.org/321953005/diff/40001/pylib/gyp/generator/andr...
> pylib/gyp/generator/android.py:881: self.WriteLn('LOCAL_MODULE_PATH :=
> $(gyp_shared_intermediate_dir)')
> We probably want a comment here to explain that we expect executables to be
for
> build/test purposes only, which is why we're installing target executables to
a
> path that won't actually be included in the system image. This might surprise
> someone later ;)

Done.

[Torne, 2014-06-13 09:53:13]

+bradnelson

Bradley, can you review the change to the buildbot run script? We need to bring
up an android emulator so we can actually run the test binaries on the
gyp-android bot.

John, can you send this through the gyp trybots with "git try" or "gcl try"? The
Rietveld-based tryjob triggering doesn't work for gyp. I want to see that
gyp-android definitely is green ;)

The latest version of the change LGTM.

[jbudorick, 2014-06-14 01:20:08]

On 2014/06/13 09:53:13, Torne wrote:
> +bradnelson
> 
> Bradley, can you review the change to the buildbot run script? We need to
bring
> up an android emulator so we can actually run the test binaries on the
> gyp-android bot.
> 
> John, can you send this through the gyp trybots with "git try" or "gcl try"?
The
> Rietveld-based tryjob triggering doesn't work for gyp. I want to see that
> gyp-android definitely is green ;)
> 
> The latest version of the change LGTM.

Had to make a few minor changes to get the trybot working with the emulator, but
you can see the results here:

http://build.chromium.org/p/tryserver.nacl/builders/gyp-android/builds/952

As you can see, there's still the six failing tests. What should I do with
those? Disable them for android for now?

[bradn, 2014-06-14 16:17:49]

lgtm on buildbot

[Torne, 2014-06-16 15:18:09]

On 2014/06/14 01:20:08, jbudorick wrote:
> On 2014/06/13 09:53:13, Torne wrote:
> > +bradnelson
> > 
> > Bradley, can you review the change to the buildbot run script? We need to
> bring
> > up an android emulator so we can actually run the test binaries on the
> > gyp-android bot.
> > 
> > John, can you send this through the gyp trybots with "git try" or "gcl try"?
> The
> > Rietveld-based tryjob triggering doesn't work for gyp. I want to see that
> > gyp-android definitely is green ;)
> > 
> > The latest version of the change LGTM.
> 
> Had to make a few minor changes to get the trybot working with the emulator,
but
> you can see the results here:
> 
> http://build.chromium.org/p/tryserver.nacl/builders/gyp-android/builds/952
> 
> As you can see, there's still the six failing tests. What should I do with
> those? Disable them for android for now?

Yeah, file a gyp bug for me to investigate/fix them and then disable them with a
comment pointing at the bug. Thanks a lot!

[jbudorick, 2014-06-16 17:23:33]

On 2014/06/16 15:18:09, Torne wrote:
> On 2014/06/14 01:20:08, jbudorick wrote:
> > On 2014/06/13 09:53:13, Torne wrote:
> > > +bradnelson
> > > 
> > > Bradley, can you review the change to the buildbot run script? We need to
> > bring
> > > up an android emulator so we can actually run the test binaries on the
> > > gyp-android bot.
> > > 
> > > John, can you send this through the gyp trybots with "git try" or "gcl
try"?
> > The
> > > Rietveld-based tryjob triggering doesn't work for gyp. I want to see that
> > > gyp-android definitely is green ;)
> > > 
> > > The latest version of the change LGTM.
> > 
> > Had to make a few minor changes to get the trybot working with the emulator,
> but
> > you can see the results here:
> > 
> > http://build.chromium.org/p/tryserver.nacl/builders/gyp-android/builds/952
> > 
> > As you can see, there's still the six failing tests. What should I do with
> > those? Disable them for android for now?
> 
> Yeah, file a gyp bug for me to investigate/fix them and then disable them with
a
> comment pointing at the bug. Thanks a lot!

Filed https://code.google.com/p/gyp/issues/detail?id=436 and commented
accordingly.

[Torne, 2014-06-17 10:55:42]

On 2014/06/16 17:23:33, jbudorick wrote:
> On 2014/06/16 15:18:09, Torne wrote:
> > On 2014/06/14 01:20:08, jbudorick wrote:
> > > On 2014/06/13 09:53:13, Torne wrote:
> > > > +bradnelson
> > > > 
> > > > Bradley, can you review the change to the buildbot run script? We need
to
> > > bring
> > > > up an android emulator so we can actually run the test binaries on the
> > > > gyp-android bot.
> > > > 
> > > > John, can you send this through the gyp trybots with "git try" or "gcl
> try"?
> > > The
> > > > Rietveld-based tryjob triggering doesn't work for gyp. I want to see
that
> > > > gyp-android definitely is green ;)
> > > > 
> > > > The latest version of the change LGTM.
> > > 
> > > Had to make a few minor changes to get the trybot working with the
emulator,
> > but
> > > you can see the results here:
> > > 
> > > http://build.chromium.org/p/tryserver.nacl/builders/gyp-android/builds/952
> > > 
> > > As you can see, there's still the six failing tests. What should I do with
> > > those? Disable them for android for now?
> > 
> > Yeah, file a gyp bug for me to investigate/fix them and then disable them
with
> a
> > comment pointing at the bug. Thanks a lot!
> 
> Filed https://code.google.com/p/gyp/issues/detail?id=436 and commented
> accordingly.

Thanks a lot John; I'm going to go ahead and land this. I will investigate the
disabled tests soon and try and fix them. There's one or two more bits of test
system work it'd be good to get done; we can chat about this offline?

[Torne, 2014-06-17 11:39:38]

Committed patchset #8 manually as r1943 (presubmit successful).

[jbudorick, 2014-06-17 13:50:28]

On 2014/06/17 10:55:42, Torne wrote:
> On 2014/06/16 17:23:33, jbudorick wrote:
> > On 2014/06/16 15:18:09, Torne wrote:
> > > On 2014/06/14 01:20:08, jbudorick wrote:
> > > > On 2014/06/13 09:53:13, Torne wrote:
> > > > > +bradnelson
> > > > > 
> > > > > Bradley, can you review the change to the buildbot run script? We need
> to
> > > > bring
> > > > > up an android emulator so we can actually run the test binaries on the
> > > > > gyp-android bot.
> > > > > 
> > > > > John, can you send this through the gyp trybots with "git try" or "gcl
> > try"?
> > > > The
> > > > > Rietveld-based tryjob triggering doesn't work for gyp. I want to see
> that
> > > > > gyp-android definitely is green ;)
> > > > > 
> > > > > The latest version of the change LGTM.
> > > > 
> > > > Had to make a few minor changes to get the trybot working with the
> emulator,
> > > but
> > > > you can see the results here:
> > > > 
> > > >
http://build.chromium.org/p/tryserver.nacl/builders/gyp-android/builds/952
> > > > 
> > > > As you can see, there's still the six failing tests. What should I do
with
> > > > those? Disable them for android for now?
> > > 
> > > Yeah, file a gyp bug for me to investigate/fix them and then disable them
> with
> > a
> > > comment pointing at the bug. Thanks a lot!
> > 
> > Filed https://code.google.com/p/gyp/issues/detail?id=436 and commented
> > accordingly.
> 
> Thanks a lot John; I'm going to go ahead and land this. I will investigate the
> disabled tests soon and try and fix them. There's one or two more bits of test
> system work it'd be good to get done; we can chat about this offline?

sgtm