🍬 GN: Make breakpad_unittests & sandbox_linux_unittests use test()

GN: Make breakpad_unittests & sandbox_linux_unittests use test()

This simplifies build rules for native tests, and allows us to get rid
of ${target}_deps targets (once recipes are updated).

This change fixes the generated wrapper scripts, which didn't work.

BUG=589318
Committed: https://crrev.com/638c8c12c9cd5086d4ba8704852cb1167e7b4ae9
Cr-Commit-Position: refs/heads/master@{#384348}

[agrieve, 2016-03-29 20:35:41]

agrieve@chromium.org changed reviewers:
+ dpranke@chromium.org, mark@chromium.org

[agrieve, 2016-03-29 20:35:41]

mark@chromium.org: Please review changes in //breakpad

dpranke@chromium.org: Please review changes in //everywhere else

[Dirk Pranke, 2016-03-29 20:57:13]

lgtm w/ a couple of nits.

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

https://codereview.chromium.org/1841193002/diff/1/testing/test.gni#newcode114
testing/test.gni:114: # pacakges to still produce unique runner scripts.
nit: typo: s/pacakges/packages/.

https://codereview.chromium.org/1841193002/diff/1/testing/test.gni#newcode144
testing/test.gni:144: if (!_use_raw_android_executable) {
Can we combine this block w/ the else block on lines 45-110 (i..e, move this
after line 110)?

[agrieve, 2016-03-30 14:17:25]

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

https://codereview.chromium.org/1841193002/diff/1/testing/test.gni#newcode114
testing/test.gni:114: # pacakges to still produce unique runner scripts.
On 2016/03/29 20:57:13, Dirk Pranke wrote:
> nit: typo: s/pacakges/packages/.

Done.

https://codereview.chromium.org/1841193002/diff/1/testing/test.gni#newcode144
testing/test.gni:144: if (!_use_raw_android_executable) {
On 2016/03/29 20:57:13, Dirk Pranke wrote:
> Can we combine this block w/ the else block on lines 45-110 (i..e, move this
> after line 110)?

Done.

[agrieve, 2016-03-30 15:22:19]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-30 15:22:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841193002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841193002/40001

[agrieve, 2016-03-30 15:52:10]

On 2016/03/30 15:22:45, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1841193002/40001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1841193002/40001

Just realized we do actually have recipes that refer to this _deps target. Going
to have to look to see if I can land this without breaking them intermittently.

[commit-bot: I haz the power, 2016-03-30 15:53:00]

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

[commit-bot: I haz the power, 2016-03-30 15:53:01]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[agrieve, 2016-03-30 19:18:31]

agrieve@chromium.org changed reviewers:
+ jbudorick@chromium.org, jorgelo@chromium.org

[agrieve, 2016-03-30 19:18:32]

jorgelo@chromium.org: Please review changes in //sandbox

jbudorick@chromium.org: Please review changes to //build

[agrieve, 2016-03-30 19:24:34]

Description was changed from

==========
GN: Make breakpad_unittests use test() template

It should work that you can build "breakpad_unittests", and have it work
on Android. Right now you need to build "breakpad_unittests_deps" and
"breakpad_unittests__test_runner_script", which the bots don't know to
do.

In a follow-up change, I'll  add support to test_runner.py so that it
knows how to run a native test without needing to special-case
breakpad_unittests (which it does right now).

BUG=589318
==========

to

==========
GN: Make breakpad_unittests & sandbox_linux_unittests use test()

This simplifies build rules for native tests, and allows us to get rid
of ${target}_deps targets (once recipes are updated).

This change fixes the generated wrapper scripts, which didn't work.

BUG=589318
==========

[jbudorick, 2016-03-30 19:33:38]

https://codereview.chromium.org/1841193002/diff/60001/build/android/pylib/gte...
File build/android/pylib/gtest/gtest_test_instance.py (right):

https://codereview.chromium.org/1841193002/diff/60001/build/android/pylib/gte...
build/android/pylib/gtest/gtest_test_instance.py:153: # TODO(agrieve): Remove
hardcoding after recipes are updated.
:( :( :(

I would prefer that we add --executable-path here and use it if present or the
current default value if not, then switch the bots over, then remove the
default.

https://codereview.chromium.org/1841193002/diff/60001/build/config/android/in...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1841193002/diff/60001/build/config/android/in...
build/config/android/internal_rules.gni:2095: "Must defined either apk_target or
executable for test_runner_script()")
nit: s/defined/define/

[Mark Mentovai, 2016-03-30 19:45:24]

This is a stamp LGTM. I don’t actually know what any of this means.

[agrieve, 2016-03-30 19:52:57]

https://codereview.chromium.org/1841193002/diff/60001/build/android/pylib/gte...
File build/android/pylib/gtest/gtest_test_instance.py (right):

https://codereview.chromium.org/1841193002/diff/60001/build/android/pylib/gte...
build/android/pylib/gtest/gtest_test_instance.py:153: # TODO(agrieve): Remove
hardcoding after recipes are updated.
On 2016/03/30 19:33:38, jbudorick wrote:
> :( :( :(
> 
> I would prefer that we add --executable-path here and use it if present or the
> current default value if not, then switch the bots over, then remove the
> default.

I did add --executable-path, and do use it when present. Can't use the old
default value since it now points to the wrong place.

The reason it points to the wrong place, is that having the executable at the
root of the build means that if you "ninja breakpad_unittests", it will build
the executable rather than the test group() below.

https://codereview.chromium.org/1841193002/diff/60001/build/config/android/in...
File build/config/android/internal_rules.gni (right):

https://codereview.chromium.org/1841193002/diff/60001/build/config/android/in...
build/config/android/internal_rules.gni:2095: "Must defined either apk_target or
executable for test_runner_script()")
On 2016/03/30 19:33:38, jbudorick wrote:
> nit: s/defined/define/

Done.

[jbudorick, 2016-03-30 19:55:22]

https://codereview.chromium.org/1841193002/diff/60001/build/android/pylib/gte...
File build/android/pylib/gtest/gtest_test_instance.py (right):

https://codereview.chromium.org/1841193002/diff/60001/build/android/pylib/gte...
build/android/pylib/gtest/gtest_test_instance.py:153: # TODO(agrieve): Remove
hardcoding after recipes are updated.
On 2016/03/30 19:52:57, agrieve wrote:
> On 2016/03/30 19:33:38, jbudorick wrote:
> > :( :( :(
> > 
> > I would prefer that we add --executable-path here and use it if present or
the
> > current default value if not, then switch the bots over, then remove the
> > default.
> 
> I did add --executable-path, and do use it when present. Can't use the old
> default value since it now points to the wrong place.
> 
> The reason it points to the wrong place, is that having the executable at the
> root of the build means that if you "ninja breakpad_unittests", it will build
> the executable rather than the test group() below.

I mean that I'd prefer adding it separately from this CL, landing beforehand,
switching the bots, and then landing the rest of this CL.

[Jorge Lucangeli Obes, 2016-03-30 20:05:52]

Not quite sure what this is doing, but assuming this doesn't change behaviour,
and all tests are still run the same way, lgtm.

[jbudorick, 2016-03-31 01:00:31]

after discussion offline, build/ lgtm w/ question

https://codereview.chromium.org/1841193002/diff/80001/build/android/pylib/gte...
File build/android/pylib/gtest/gtest_test_instance.py (right):

https://codereview.chromium.org/1841193002/diff/80001/build/android/pylib/gte...
build/android/pylib/gtest/gtest_test_instance.py:147: self._exe_path =
args.executable_path
What's with this line given the following if/else?

https://codereview.chromium.org/1841193002/diff/80001/build/android/pylib/gte...
build/android/pylib/gtest/gtest_test_instance.py:153: # TODO(agrieve): Remove
hardcoding after recipes are updated.
this needs to disappear ASAP

[agrieve, 2016-03-31 01:34:04]

https://codereview.chromium.org/1841193002/diff/80001/build/android/pylib/gte...
File build/android/pylib/gtest/gtest_test_instance.py (right):

https://codereview.chromium.org/1841193002/diff/80001/build/android/pylib/gte...
build/android/pylib/gtest/gtest_test_instance.py:147: self._exe_path =
args.executable_path
On 2016/03/31 01:00:31, jbudorick wrote:
> What's with this line given the following if/else?

Done.

https://codereview.chromium.org/1841193002/diff/80001/build/android/pylib/gte...
build/android/pylib/gtest/gtest_test_instance.py:153: # TODO(agrieve): Remove
hardcoding after recipes are updated.
On 2016/03/31 01:00:31, jbudorick wrote:
> this needs to disappear ASAP

Acknowledged.

[agrieve, 2016-03-31 15:40:33]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-03-31 15:40:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, mark@chromium.org, jorgelo@chromium.org,
jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1841193002/#ps120001
(title: "fix bot failure")

[commit-bot: I haz the power, 2016-03-31 15:41:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841193002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841193002/120001

[commit-bot: I haz the power, 2016-03-31 15:52:47]

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

[commit-bot: I haz the power, 2016-03-31 15:52:48]

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

[agrieve, 2016-03-31 17:05:23]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-03-31 17:05:23]

The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org,
dpranke@chromium.org, jorgelo@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1841193002/#ps140001
(title: "Remove duplicate _run target")

[commit-bot: I haz the power, 2016-03-31 17:05:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841193002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841193002/140001

[commit-bot: I haz the power, 2016-03-31 17:17:31]

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

[commit-bot: I haz the power, 2016-03-31 17:17:32]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)

[agrieve, 2016-03-31 18:26:49]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-03-31 18:27:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1841193002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1841193002/140001

[commit-bot: I haz the power, 2016-03-31 18:53:00]

Description was changed from

==========
GN: Make breakpad_unittests & sandbox_linux_unittests use test()

This simplifies build rules for native tests, and allows us to get rid
of ${target}_deps targets (once recipes are updated).

This change fixes the generated wrapper scripts, which didn't work.

BUG=589318
==========

to

==========
GN: Make breakpad_unittests & sandbox_linux_unittests use test()

This simplifies build rules for native tests, and allows us to get rid
of ${target}_deps targets (once recipes are updated).

This change fixes the generated wrapper scripts, which didn't work.

BUG=589318
==========

[commit-bot: I haz the power, 2016-03-31 18:53:02]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-03-31 18:54:05]

Description was changed from

==========
GN: Make breakpad_unittests & sandbox_linux_unittests use test()

This simplifies build rules for native tests, and allows us to get rid
of ${target}_deps targets (once recipes are updated).

This change fixes the generated wrapper scripts, which didn't work.

BUG=589318
==========

to

==========
GN: Make breakpad_unittests & sandbox_linux_unittests use test()

This simplifies build rules for native tests, and allows us to get rid
of ${target}_deps targets (once recipes are updated).

This change fixes the generated wrapper scripts, which didn't work.

BUG=589318

Committed: https://crrev.com/638c8c12c9cd5086d4ba8704852cb1167e7b4ae9
Cr-Commit-Position: refs/heads/master@{#384348}
==========

[commit-bot: I haz the power, 2016-03-31 18:54:06]

Patchset 8 (id:??) landed as
https://crrev.com/638c8c12c9cd5086d4ba8704852cb1167e7b4ae9
Cr-Commit-Position: refs/heads/master@{#384348}

[jbudorick, 2016-04-01 13:36:26]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1848033005/ by jbudorick@chromium.org.

The reason for reverting is: breaks both suites on GN, e.g.
https://build.chromium.org/p/chromium.android/builders/Lollipop%20Phone%20Tester.