🐛 Make the Android test apks depend on their run_ scripts

Make the Android test apks depend on their run_ scripts

The dependency should actually be the other way around, but
for compatibility with GYP, it has to go this way.

BUG=599919
Committed: https://crrev.com/b355ad15304acb0a769d76bd634cb5167e62e96f
Cr-Commit-Position: refs/heads/master@{#388126}

[agrieve, 2016-04-16 00:30:50]

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

[agrieve, 2016-04-18 18:53:04]

Description was changed from

==========
Make Add a data_dep on the wrapper scripts from "test"_apk targets

Required for bots.

BUG=599919
==========

to

==========
Make the Android run_$test scripts depend on their test apks

If you build the script, it should work without having to
separately build the test apk.

BUG=599919
==========

[agrieve, 2016-04-18 18:53:58]

On 2016/04/16 00:30:51, agrieve wrote:

updated to new plan discussed on gn-dev.

[Dirk Pranke, 2016-04-18 19:23:16]

I'm not sure that I understand what this is doing.

assuming we have test("base_unittests") build into out/Default,

it seems like this creates a script at out/Default/base_unittests that then
invokes base_unittests_apk.

I thought we were talking about creating out/Default/bin/run_base_unittests
still?

[agrieve, 2016-04-18 19:51:35]

On 2016/04/18 19:23:16, Dirk Pranke wrote:
> I'm not sure that I understand what this is doing.
> 
> assuming we have test("base_unittests") build into out/Default,
> 
> it seems like this creates a script at out/Default/base_unittests that then
> invokes base_unittests_apk.
> 
> I thought we were talking about creating out/Default/bin/run_base_unittests
> still?

We currently have:
group(target_name)
 -> test_runner_srcipt("some long name")
 -> unittest_apk("${target_name}_apk")

The fix is to make
group(target_name)
-> test_runner_srcipt("some long name")
     -> unittest_apk("${target_name}_apk")

But there's no advantage to having the top-level group() at this point, so I
renamed "some long name" to target_name.

In both before and after, the scripts are generated to bin/run_$_output_name

[Dirk Pranke, 2016-04-18 19:59:45]

On 2016/04/18 19:51:35, agrieve wrote:
> On 2016/04/18 19:23:16, Dirk Pranke wrote:
> > I'm not sure that I understand what this is doing.
> > 
> > assuming we have test("base_unittests") build into out/Default,
> > 
> > it seems like this creates a script at out/Default/base_unittests that then
> > invokes base_unittests_apk.
> > 
> > I thought we were talking about creating out/Default/bin/run_base_unittests
> > still?
> 
> We currently have:
> group(target_name)
>  -> test_runner_srcipt("some long name")
>  -> unittest_apk("${target_name}_apk")
> 
> The fix is to make
> group(target_name)
> -> test_runner_srcipt("some long name")
>      -> unittest_apk("${target_name}_apk")
> 
> But there's no advantage to having the top-level group() at this point, so I
> renamed "some long name" to target_name.
> 
> In both before and after, the scripts are generated to bin/run_$_output_name

Hm. I can see this argument, but it also seems a bit confusing to me to have
"base_unittests" actually generate "bin/run_base_unittests", since on desktop
platforms this wouldn't happen, unless we created the cyclic dependency
I mentioned in the other email.

Let me think about this a bit more.

[agrieve, 2016-04-18 20:15:12]

On 2016/04/18 19:59:45, Dirk Pranke wrote:
> On 2016/04/18 19:51:35, agrieve wrote:
> > On 2016/04/18 19:23:16, Dirk Pranke wrote:
> > > I'm not sure that I understand what this is doing.
> > > 
> > > assuming we have test("base_unittests") build into out/Default,
> > > 
> > > it seems like this creates a script at out/Default/base_unittests that
then
> > > invokes base_unittests_apk.
> > > 
> > > I thought we were talking about creating
out/Default/bin/run_base_unittests
> > > still?
> > 
> > We currently have:
> > group(target_name)
> >  -> test_runner_srcipt("some long name")
> >  -> unittest_apk("${target_name}_apk")
> > 
> > The fix is to make
> > group(target_name)
> > -> test_runner_srcipt("some long name")
> >      -> unittest_apk("${target_name}_apk")
> > 
> > But there's no advantage to having the top-level group() at this point, so I
> > renamed "some long name" to target_name.
> > 
> > In both before and after, the scripts are generated to bin/run_$_output_name
> 
> Hm. I can see this argument, but it also seems a bit confusing to me to have
> "base_unittests" actually generate "bin/run_base_unittests", since on desktop
> platforms this wouldn't happen, unless we created the cyclic dependency
> I mentioned in the other email.
> 
> Let me think about this a bit more.

Android:
":unit_tests" -> wrapper script
":unit_tests_apk" -> just the _apk
"unit_tests" -> maps to ":unit_tests" (except actually maps to nothing for this
target)

Linux:
":unit_tests" -> wrapper script
":unit_tests_bin" -> just the binary
"unit_tests" -> maps to ":unit_tests_bin"

I certainly see the confusion :(. I don't see a better solution than circular
dependencies :(.

Happy to think on it as well. This doesn't block moving the bots to scripts.

[Dirk Pranke, 2016-04-18 22:28:20]

brettw@ and I talked about this some more.

1) There's no way to create a true circular dependency either in Ninja or in GN
(the allow_circular flag in GN is just for header checking), so that takes some
things off the table.

2) Given that it seems like you really want 'ninja bin/run_foo' to also build
'foo', I think that means that we want run_foo to have a dependency on foo. In
addition, since on Android IWRC you actually need the runtime_deps of foo in the
run_foo script, run_foo *has* to depend on foo (by the design we came up with
earlier).

So, this means that run_foo needs to depend on foo.

On desktop, however, that means that 'foo' won't build run_foo. We'll have to
live with that.

On Android, 'foo' can still refer to a group that depends on run_foo and
foo_apk. I would probably still create the dummy group, rather than having the
thing that generates run_foo *actually have the label foo*, because that's just
confusing. 

Lastly, I think (again, IIUC), that if you have a raw android executable test,
you also can't create the dummy group, because you'd get a name conflict, right?
Or do the raw executables get built someplace other than the root_build_dir ?

[agrieve, 2016-04-19 01:24:04]

On 2016/04/18 22:28:20, Dirk Pranke wrote:
> brettw@ and I talked about this some more.
> 
> 1) There's no way to create a true circular dependency either in Ninja or in
GN
> (the allow_circular flag in GN is just for header checking), so that takes
some
> things off the table.

Hacky idea here (for linux):
1. Have run_FOO depend on FOO executable
2. Have FOO executable depend on a custom action with a dummy output, that also
generates the run_FOO as a side-effect but doesn't tell GN/ninja about it


> 
> 2) Given that it seems like you really want 'ninja bin/run_foo' to also build
> 'foo', I think that means that we want run_foo to have a dependency on foo. In
> addition, since on Android IWRC you actually need the runtime_deps of foo in
the
> run_foo script, run_foo *has* to depend on foo (by the design we came up with
> earlier).
I think this is somewhat work-aroundable if we need to. E.g. can just create a
group() { deps = invoker.deps } in the test macro, then that will have all
runtime_deps except for the main .apk, which is easy to add in. I don't think
this is needed though.

> 
> So, this means that run_foo needs to depend on foo.
> 
> On desktop, however, that means that 'foo' won't build run_foo. We'll have to
> live with that.
> 
> On Android, 'foo' can still refer to a group that depends on run_foo and
> foo_apk. I would probably still create the dummy group, rather than having the
> thing that generates run_foo *actually have the label foo*, because that's
just
> confusing. 

I'll add the dummy group back, but I don't think those outside of test.gni
should care how test.gni is implemented. They just see a target of type
test("foo") that generates an apk and a runner script.



> 
> Lastly, I think (again, IIUC), that if you have a raw android executable test,
> you also can't create the dummy group, because you'd get a name conflict,
right?
> Or do the raw executables get built someplace other than the root_build_dir ?
Exactly. I ran into this problem and solved it by putting the executable in
target_out_dir.

[Dirk Pranke, 2016-04-19 01:39:01]

On 2016/04/19 01:24:04, agrieve wrote:
> On 2016/04/18 22:28:20, Dirk Pranke wrote:
> > brettw@ and I talked about this some more.
> > 
> > 1) There's no way to create a true circular dependency either in Ninja or in
> GN
> > (the allow_circular flag in GN is just for header checking), so that takes
> some
> > things off the table.
> 
> Hacky idea here (for linux):
> 1. Have run_FOO depend on FOO executable
> 2. Have FOO executable depend on a custom action with a dummy output, that
also
> generates the run_FOO as a side-effect but doesn't tell GN/ninja about it

This could potentially work, apart from the issue in (2), below.

> > 2) Given that it seems like you really want 'ninja bin/run_foo' to also
build
> > 'foo', I think that means that we want run_foo to have a dependency on foo.
In
> > addition, since on Android IWRC you actually need the runtime_deps of foo in
> the
> > run_foo script, run_foo *has* to depend on foo (by the design we came up
with
> > earlier).
> I think this is somewhat work-aroundable if we need to. E.g. can just create a
> group() { deps = invoker.deps } in the test macro, then that will have all
> runtime_deps except for the main .apk, which is easy to add in. I don't think
> this is needed though.

Perhaps. This feels increasingly hacky as we go down this path, though.

> 
> > 
> > So, this means that run_foo needs to depend on foo.
> > 
> > On desktop, however, that means that 'foo' won't build run_foo. We'll have
to
> > live with that.
> > 
> > On Android, 'foo' can still refer to a group that depends on run_foo and
> > foo_apk. I would probably still create the dummy group, rather than having
the
> > thing that generates run_foo *actually have the label foo*, because that's
> just
> > confusing. 
> 
> I'll add the dummy group back, but I don't think those outside of test.gni
> should care how test.gni is implemented. They just see a target of type
> test("foo") that generates an apk and a runner script.

I agree, but even internally (in the implementation of test()) it might be good
to keep things as consistent between desktop and android as possible.

Or, maybe not, I think it is kind of a top-level 
if (is_android) { ... } else if (is_ios) { ... } else { ... } already, so maybe
there's not much to keep consistent.
 
> 
> 
> > 
> > Lastly, I think (again, IIUC), that if you have a raw android executable
test,
> > you also can't create the dummy group, because you'd get a name conflict,
> right?
> > Or do the raw executables get built someplace other than the root_build_dir
?
> Exactly. I ran into this problem and solved it by putting the executable in
> target_out_dir.

[Dirk Pranke, 2016-04-19 01:39:22]

lgtm

[agrieve, 2016-04-19 01:43:23]

On 2016/04/19 01:39:01, Dirk Pranke wrote:
> On 2016/04/19 01:24:04, agrieve wrote:
> > On 2016/04/18 22:28:20, Dirk Pranke wrote:
> > > brettw@ and I talked about this some more.
> > > 
> > > 1) There's no way to create a true circular dependency either in Ninja or
in
> > GN
> > > (the allow_circular flag in GN is just for header checking), so that takes
> > some
> > > things off the table.
> > 
> > Hacky idea here (for linux):
> > 1. Have run_FOO depend on FOO executable
> > 2. Have FOO executable depend on a custom action with a dummy output, that
> also
> > generates the run_FOO as a side-effect but doesn't tell GN/ninja about it
> 
> This could potentially work, apart from the issue in (2), below.
I think it works for 2) as well. There is still a target called run_foo that
depends on foo. There is just *additionally* a target that foo depends on that
silently creates run_foo. 
If you build "foo", then run_foo is created by the dep that doesn't declare
run_foo as an output but creates it anyways.
If you builD "run_foo", then run_foo would actually be created twice. Quite
hacky.


> 
> > > 2) Given that it seems like you really want 'ninja bin/run_foo' to also
> build
> > > 'foo', I think that means that we want run_foo to have a dependency on
foo.
> In
> > > addition, since on Android IWRC you actually need the runtime_deps of foo
in
> > the
> > > run_foo script, run_foo *has* to depend on foo (by the design we came up
> with
> > > earlier).
> > I think this is somewhat work-aroundable if we need to. E.g. can just create
a
> > group() { deps = invoker.deps } in the test macro, then that will have all
> > runtime_deps except for the main .apk, which is easy to add in. I don't
think
> > this is needed though.
> 
> Perhaps. This feels increasingly hacky as we go down this path, though.
> 
> > 
> > > 
> > > So, this means that run_foo needs to depend on foo.
> > > 
> > > On desktop, however, that means that 'foo' won't build run_foo. We'll have
> to
> > > live with that.
> > > 
> > > On Android, 'foo' can still refer to a group that depends on run_foo and
> > > foo_apk. I would probably still create the dummy group, rather than having
> the
> > > thing that generates run_foo *actually have the label foo*, because that's
> > just
> > > confusing. 
> > 
> > I'll add the dummy group back, but I don't think those outside of test.gni
> > should care how test.gni is implemented. They just see a target of type
> > test("foo") that generates an apk and a runner script.
> 
> I agree, but even internally (in the implementation of test()) it might be
good
> to keep things as consistent between desktop and android as possible.
> 
> Or, maybe not, I think it is kind of a top-level 
> if (is_android) { ... } else if (is_ios) { ... } else { ... } already, so
maybe
> there's not much to keep consistent.
>  
> > 
> > 
> > > 
> > > Lastly, I think (again, IIUC), that if you have a raw android executable
> test,
> > > you also can't create the dummy group, because you'd get a name conflict,
> > right?
> > > Or do the raw executables get built someplace other than the
root_build_dir
> ?
> > Exactly. I ran into this problem and solved it by putting the executable in
> > target_out_dir.

[agrieve, 2016-04-19 01:43:29]

The CQ bit was checked by agrieve@chromium.org

[commit-bot: I haz the power, 2016-04-19 01:43:51]

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

[agrieve, 2016-04-19 01:48:06]

On 2016/04/19 01:43:23, agrieve wrote:
> On 2016/04/19 01:39:01, Dirk Pranke wrote:
> > On 2016/04/19 01:24:04, agrieve wrote:
> > > On 2016/04/18 22:28:20, Dirk Pranke wrote:
> > > > brettw@ and I talked about this some more.
> > > > 
> > > > 1) There's no way to create a true circular dependency either in Ninja
or
> in
> > > GN
> > > > (the allow_circular flag in GN is just for header checking), so that
takes
> > > some
> > > > things off the table.
> > > 
> > > Hacky idea here (for linux):
> > > 1. Have run_FOO depend on FOO executable
> > > 2. Have FOO executable depend on a custom action with a dummy output, that
> > also
> > > generates the run_FOO as a side-effect but doesn't tell GN/ninja about it
> > 
> > This could potentially work, apart from the issue in (2), below.
> I think it works for 2) as well. There is still a target called run_foo that
> depends on foo. There is just *additionally* a target that foo depends on that
> silently creates run_foo. 
> If you build "foo", then run_foo is created by the dep that doesn't declare
> run_foo as an output but creates it anyways.
> If you builD "run_foo", then run_foo would actually be created twice. Quite
> hacky.

Another idea - maybe less hacky - put the linux test binaries in target_out_dir
like is done on Android. That way building the label will build the group().



> 
> 
> > 
> > > > 2) Given that it seems like you really want 'ninja bin/run_foo' to also
> > build
> > > > 'foo', I think that means that we want run_foo to have a dependency on
> foo.
> > In
> > > > addition, since on Android IWRC you actually need the runtime_deps of
foo
> in
> > > the
> > > > run_foo script, run_foo *has* to depend on foo (by the design we came up
> > with
> > > > earlier).
> > > I think this is somewhat work-aroundable if we need to. E.g. can just
create
> a
> > > group() { deps = invoker.deps } in the test macro, then that will have all
> > > runtime_deps except for the main .apk, which is easy to add in. I don't
> think
> > > this is needed though.
> > 
> > Perhaps. This feels increasingly hacky as we go down this path, though.
> > 
> > > 
> > > > 
> > > > So, this means that run_foo needs to depend on foo.
> > > > 
> > > > On desktop, however, that means that 'foo' won't build run_foo. We'll
have
> > to
> > > > live with that.
> > > > 
> > > > On Android, 'foo' can still refer to a group that depends on run_foo and
> > > > foo_apk. I would probably still create the dummy group, rather than
having
> > the
> > > > thing that generates run_foo *actually have the label foo*, because
that's
> > > just
> > > > confusing. 
> > > 
> > > I'll add the dummy group back, but I don't think those outside of test.gni
> > > should care how test.gni is implemented. They just see a target of type
> > > test("foo") that generates an apk and a runner script.
> > 
> > I agree, but even internally (in the implementation of test()) it might be
> good
> > to keep things as consistent between desktop and android as possible.
> > 
> > Or, maybe not, I think it is kind of a top-level 
> > if (is_android) { ... } else if (is_ios) { ... } else { ... } already, so
> maybe
> > there's not much to keep consistent.
> >  
> > > 
> > > 
> > > > 
> > > > Lastly, I think (again, IIUC), that if you have a raw android executable
> > test,
> > > > you also can't create the dummy group, because you'd get a name
conflict,
> > > right?
> > > > Or do the raw executables get built someplace other than the
> root_build_dir
> > ?
> > > Exactly. I ran into this problem and solved it by putting the executable
in
> > > target_out_dir.

[commit-bot: I haz the power, 2016-04-19 01:52:07]

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

[commit-bot: I haz the power, 2016-04-19 01:52:07]

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

[Dirk Pranke, 2016-04-19 01:53:35]

On Mon, Apr 18, 2016 at 6:48 PM, <agrieve@chromium.org> wrote:

> Another idea - maybe less hacky - put the linux test binaries in
> target_out_dir
> like is done on Android. That way building the label will build the
> group().
>

Yeah, I thought about that, but that would be really disruptive to people
used
to running them directly, and to the N different scripts we would need to
fix as well.

I think it's also less clear how useful the wrapper scripts will be for
devs on
the desktop platforms. Maybe if we get to a place where everyone always
uses the wrappers, it would make more sense to do something like this.

-- Dirk

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[agrieve, 2016-04-19 02:49:10]

On 2016/04/19 01:53:35, Dirk Pranke wrote:
> On Mon, Apr 18, 2016 at 6:48 PM, <mailto:agrieve@chromium.org> wrote:
> 
> > Another idea - maybe less hacky - put the linux test binaries in
> > target_out_dir
> > like is done on Android. That way building the label will build the
> > group().
> >
> 
> Yeah, I thought about that, but that would be really disruptive to people
> used
> to running them directly, and to the N different scripts we would need to
> fix as well.
> 
> I think it's also less clear how useful the wrapper scripts will be for
> devs on
> the desktop platforms. Maybe if we get to a place where everyone always
> uses the wrappers, it would make more sense to do something like this.
> 
> -- Dirk
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Trybot failed due to a related recipe change that I reverted. However, to fix
the recipe change, this needs to go back to PS1 until we're off of GYP.

[agrieve, 2016-04-19 02:50:30]

Description was changed from

==========
Make the Android run_$test scripts depend on their test apks

If you build the script, it should work without having to
separately build the test apk.

BUG=599919
==========

to

==========
Make the Android test apks depend on their run_ scripts

The dependency should actually be the other way around, but
for compatibility with GYP, it has to go this way.

BUG=599919
==========

[agrieve, 2016-04-19 02:53:18]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-04-19 02:53:18]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1893063002/#ps60001
(title: "revert to PS1")

[commit-bot: I haz the power, 2016-04-19 02:53:37]

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

[Dirk Pranke, 2016-04-19 02:54:06]

patchset #4 / patchset #1 lgtm as per our discussion.

[commit-bot: I haz the power, 2016-04-19 03:45:34]

Description was changed from

==========
Make the Android test apks depend on their run_ scripts

The dependency should actually be the other way around, but
for compatibility with GYP, it has to go this way.

BUG=599919
==========

to

==========
Make the Android test apks depend on their run_ scripts

The dependency should actually be the other way around, but
for compatibility with GYP, it has to go this way.

BUG=599919
==========

[commit-bot: I haz the power, 2016-04-19 03:45:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-19 03:47:36]

Description was changed from

==========
Make the Android test apks depend on their run_ scripts

The dependency should actually be the other way around, but
for compatibility with GYP, it has to go this way.

BUG=599919
==========

to

==========
Make the Android test apks depend on their run_ scripts

The dependency should actually be the other way around, but
for compatibility with GYP, it has to go this way.

BUG=599919

Committed: https://crrev.com/b355ad15304acb0a769d76bd634cb5167e62e96f
Cr-Commit-Position: refs/heads/master@{#388126}
==========

[commit-bot: I haz the power, 2016-04-19 03:47:37]

Patchset 4 (id:??) landed as
https://crrev.com/b355ad15304acb0a769d76bd634cb5167e62e96f
Cr-Commit-Position: refs/heads/master@{#388126}