[Chromecast] Dump the runtime deps for all test targets.

[Chromecast] Dump the runtime deps for all test targets.

For each foo_unittest binary in cast_test_lists, create a
foo_unittest.runtime_deps file that lists all of the runtime
dependencies (data files, data directories, shared libs, etc.) that a
test needs at runtime.

BUG= internal b/33682037
TEST=gn gen

Review-Url: https://codereview.chromium.org/2629573008
Cr-Commit-Position: refs/heads/master@{#444883}
Committed: https://chromium.googlesource.com/chromium/src/+/3ca202de6da5f75fa9bbe79b41b03ed2205b1dea

[mbjorge, 2017-01-14 00:53:01]

mbjorge@chromium.org changed reviewers:
+ alokp@chromium.org, slan@chromium.org

[alokp, 2017-01-17 21:30:25]

Just a few nits - deferring to slan@ for final approval.

https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
File chromecast/build/tests/cast_test.gni (right):

https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
chromecast/build/tests/cast_test.gni:209: group(_test_name +
"_cast_runtime_deps") {
"_cast" prefix seems unnecessary

https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
chromecast/build/tests/cast_test.gni:214: write_runtime_deps =
"$root_out_dir/${_test_name}.runtime_deps"
I would suggest using a proper file type (txt, json, etc) instead of a
runtime_deps. You should probably use "_runtime_deps" suffix in the filename.

[slan, 2017-01-18 17:49:21]

https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
File chromecast/build/tests/cast_test.gni (right):

https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
chromecast/build/tests/cast_test.gni:200: foreach(_test, invoker.tests) {
nit: This for-loop could be combined with the one below. Your call if that makes
it cleaner or nah.

https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
chromecast/build/tests/cast_test.gni:209: group(_test_name +
"_cast_runtime_deps") {
On 2017/01/17 21:30:25, alokp wrote:
> "_cast" prefix seems unnecessary

Actually, I think having "cast" in the name is useful, so that somebody upstream
creating net_unittests_runtime_deps doesn't accidentally collide with our
target. This is just an internal detail, after all.

https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
chromecast/build/tests/cast_test.gni:214: write_runtime_deps =
"$root_out_dir/${_test_name}.runtime_deps"
On 2017/01/17 21:30:25, alokp wrote:
> I would suggest using a proper file type (txt, json, etc) instead of a
> runtime_deps. You should probably use "_runtime_deps" suffix in the filename.

I assume this convention was chosen to match the exsiting GN default convention
(gn help --runtime-deps-list-file). But I agree, I think that I prefer normal
extensions for files that we're generating.

I am more concerned that this clutters the root_out_dir substantially. Could we
put these next to the other files generated by this target?

write_runtime_deps = 
    "$_shared_dir/runtime_deps/${_test_name}.txt

You could also then update the python script to compile all of these files into
a single key-value file (like the run list), leaving you with exactly three
output files from cast_test_lists:
 - build list 
 - run list
 - runtime_deps

That would obscure some of the internal details of this from the test infra
scripts, meaning that we won't have to keep anything from this file in sync with
that code (only the paths specified in chromecast/BUILD.gn). Thoughts?

Thoughts?

[mbjorge, 2017-01-18 23:30:22]

On 2017/01/18 at 17:49:21, slan wrote:
>
https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
> File chromecast/build/tests/cast_test.gni (right):
> 
>
https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
> chromecast/build/tests/cast_test.gni:200: foreach(_test, invoker.tests) {
> nit: This for-loop could be combined with the one below. Your call if that
makes it cleaner or nah.
> 
>
https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
> chromecast/build/tests/cast_test.gni:209: group(_test_name +
"_cast_runtime_deps") {
> On 2017/01/17 21:30:25, alokp wrote:
> > "_cast" prefix seems unnecessary
> 
> Actually, I think having "cast" in the name is useful, so that somebody
upstream creating net_unittests_runtime_deps doesn't accidentally collide with
our target. This is just an internal detail, after all.
> 
>
https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
> chromecast/build/tests/cast_test.gni:214: write_runtime_deps =
"$root_out_dir/${_test_name}.runtime_deps"
> On 2017/01/17 21:30:25, alokp wrote:
> > I would suggest using a proper file type (txt, json, etc) instead of a
> > runtime_deps. You should probably use "_runtime_deps" suffix in the
filename.
> 
> I assume this convention was chosen to match the exsiting GN default
convention (gn help --runtime-deps-list-file). But I agree, I think that I
prefer normal extensions for files that we're generating.
> 
> I am more concerned that this clutters the root_out_dir substantially. Could
we put these next to the other files generated by this target?
> 
> write_runtime_deps = 
>     "$_shared_dir/runtime_deps/${_test_name}.txt
> 
> You could also then update the python script to compile all of these files
into a single key-value file (like the run list), leaving you with exactly three
output files from cast_test_lists:
>  - build list 
>  - run list
>  - runtime_deps
> 
> That would obscure some of the internal details of this from the test infra
scripts, meaning that we won't have to keep anything from this file in sync with
that code (only the paths specified in chromecast/BUILD.gn). Thoughts?
> 
> Thoughts?

Ok, I've kept the _cast prefix in the target name, changed the filenames to
output to _shared_dir and renamed to use a .txt extension. I then updated the
generate script so it combines all of the runtime deps into a single .json dict
file. I think this will make it simple to either gather all of the runtime_deps
at once, while also making it simple to get the runtime deps for a single
unittest, all without cluttering the output directory.

The runtime_deps.json file looks like:

  $ cat out_chromecast_salami/release/tests/runtime_deps.json 
  {
    "base_unittests": [
      "./base_unittests",
      "../../base/test/data/",
      "./libtest_shared_library.so",
      "icudtl.dat",
      "../../testing/test_env.py",
      "./libmalloc_wrapper.so"
    ],
    "cacheinvalidation_unittests": [
      "./cacheinvalidation_unittests",
      "../../testing/test_env.py",
      "icudtl.dat"
    ],
    ...
  }

[alokp, 2017-01-18 23:35:36]

https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
File chromecast/build/tests/cast_test.gni (right):

https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
chromecast/build/tests/cast_test.gni:209: group(_test_name +
"_cast_runtime_deps") {
On 2017/01/18 17:49:21, slan wrote:
> On 2017/01/17 21:30:25, alokp wrote:
> > "_cast" prefix seems unnecessary
> 
> Actually, I think having "cast" in the name is useful, so that somebody
upstream
> creating net_unittests_runtime_deps doesn't accidentally collide with our
> target. This is just an internal detail, after all.

Don't we already have test_name in the group target-name, which should be unique
to start with?

[slan, 2017-01-19 00:15:04]

lgtm - looks great!

https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
File chromecast/build/tests/cast_test.gni (right):

https://codereview.chromium.org/2629573008/diff/1/chromecast/build/tests/cast...
chromecast/build/tests/cast_test.gni:209: group(_test_name +
"_cast_runtime_deps") {
On 2017/01/18 23:35:36, alokp wrote:
> On 2017/01/18 17:49:21, slan wrote:
> > On 2017/01/17 21:30:25, alokp wrote:
> > > "_cast" prefix seems unnecessary
> > 
> > Actually, I think having "cast" in the name is useful, so that somebody
> upstream
> > creating net_unittests_runtime_deps doesn't accidentally collide with our
> > target. This is just an internal detail, after all.
> 
> Don't we already have test_name in the group target-name, which should be
unique
> to start with?

Yes, but since we build tests from other parts of Chrome, those names could
create a chance of collision. For example, we include in our test list
//net:net_unittests. _test_name is "net_unittests", and the name of this target
then becomes "net_unittests_cast_runtime_deps". If we were to remove "cast" from
the suffix, this target would be named "net_unittests_runtime_deps", which has a
much higher chance of future collision with something in //net. 

...but as I type this, I realize that that isn't really a problem: it would
simply necessitate typing the fully-qualified path on the command-line (which we
will never do). So, I don't really care whether or not we leave "cast" in or
out. It is an internal detail, so I don't think it matters all that much.

[mbjorge, 2017-01-19 22:09:44]

The CQ bit was checked by mbjorge@chromium.org

[commit-bot: I haz the power, 2017-01-19 22:10:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 23:19:35]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484863784090450,
"parent_rev": "1d1af48d62555904d4d14dab42ed1798fdbd1e09", "commit_rev":
"3ca202de6da5f75fa9bbe79b41b03ed2205b1dea"}

[commit-bot: I haz the power, 2017-01-19 23:20:13]

Description was changed from

==========
[Chromecast] Dump the runtime deps for all test targets.

For each foo_unittest binary in cast_test_lists, create a
foo_unittest.runtime_deps file that lists all of the runtime
dependencies (data files, data directories, shared libs, etc.) that a
test needs at runtime.

BUG= internal b/33682037
TEST=gn gen
==========

to

==========
[Chromecast] Dump the runtime deps for all test targets.

For each foo_unittest binary in cast_test_lists, create a
foo_unittest.runtime_deps file that lists all of the runtime
dependencies (data files, data directories, shared libs, etc.) that a
test needs at runtime.

BUG= internal b/33682037
TEST=gn gen

Review-Url: https://codereview.chromium.org/2629573008
Cr-Commit-Position: refs/heads/master@{#444883}
Committed:
https://chromium.googlesource.com/chromium/src/+/3ca202de6da5f75fa9bbe79b41b0...
==========

[commit-bot: I haz the power, 2017-01-19 23:20:14]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/3ca202de6da5f75fa9bbe79b41b0...