🚙 GN: Add write_runtime_deps variable

GN: Add write_runtime_deps variable

Allows targets to use runtime_deps at build-time.

BUG=593416
Committed: https://crrev.com/0f8ad42ac1a6b159655c51bf7095ff2d21ae540f
Cr-Commit-Position: refs/heads/master@{#384411}

[agrieve, 2016-03-16 18:50:44]

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

[agrieve, 2016-03-16 18:50:44]

ptal

[brettw, 2016-03-16 23:03:36]

When I was thinking about this, I was thinking this would be an attribute on a
target, like:
  executable("foo") {
    runtime_deps_file = "$root_out_dir/foo.deps"
  }
Your implementation seems like it might be a bit simpler, since my design
requires that the file name get saved on the target and then we have to remember
to schedule a task when the target is resolved.

I was unsure which way we should go but Dirk pointed out that we will likely
want to treat the runtime deps file as an output of a target, so that if you
want to list a runtime deps file as an input to an action, you depend on the
corresponding target to make sure that stuff is actually built before your step
runs.

If we want this to work, I think we have to make the target aware of what its
corresponding runtime deps file is for the computate in
target.cc:EnsureFileIsGeneratedByDependency.

What do you think? Sorry, I didn't realize you had already started working on
this or I would have wanted to discuss more in advance.

[agrieve, 2016-03-17 00:51:46]

On 2016/03/16 23:03:36, brettw wrote:
> When I was thinking about this, I was thinking this would be an attribute on a
> target, like:
>   executable("foo") {
>     runtime_deps_file = "$root_out_dir/foo.deps"
>   }
> Your implementation seems like it might be a bit simpler, since my design
> requires that the file name get saved on the target and then we have to
remember
> to schedule a task when the target is resolved.
> 
> I was unsure which way we should go but Dirk pointed out that we will likely
> want to treat the runtime deps file as an output of a target, so that if you
> want to list a runtime deps file as an input to an action, you depend on the
> corresponding target to make sure that stuff is actually built before your
step
> runs.

Making sure that the data files are built before the dependent target certainly
would increase complexity. I don't think we need these semantics though. If we
did, I think it'd be better to have a way to specify that you want data_deps to
be built before your action runs, rather than needing to write a file to get
these semantics.

> 
> If we want this to work, I think we have to make the target aware of what its
> corresponding runtime deps file is for the computate in
> target.cc:EnsureFileIsGeneratedByDependency.
> 
> What do you think? Sorry, I didn't realize you had already started working on
> this or I would have wanted to discuss more in advance.

No worries, just started on it this morning.

I thought of putting it on the target as well, but then thought it shouldn't
really be the target's responsibility to write the file. E.g. a test has
runtime_deps, but it's only the test runner that requires them to be written to
a file. So, it should be the test runner that requests them to be written. True,
you could declare a dummy group() for the express purpose of setting
runtime_deps_file, but since the files are written at gn gen time, all that does
is touch a stamp file.

[agrieve, 2016-03-17 13:56:34]

On 2016/03/17 00:51:46, agrieve wrote:
> On 2016/03/16 23:03:36, brettw wrote:
> > When I was thinking about this, I was thinking this would be an attribute on
a
> > target, like:
> >   executable("foo") {
> >     runtime_deps_file = "$root_out_dir/foo.deps"
> >   }
> > Your implementation seems like it might be a bit simpler, since my design
> > requires that the file name get saved on the target and then we have to
> remember
> > to schedule a task when the target is resolved.
> > 
> > I was unsure which way we should go but Dirk pointed out that we will likely
> > want to treat the runtime deps file as an output of a target, so that if you
> > want to list a runtime deps file as an input to an action, you depend on the
> > corresponding target to make sure that stuff is actually built before your
> step
> > runs.
> 
> Making sure that the data files are built before the dependent target
certainly
> would increase complexity. I don't think we need these semantics though. If we
> did, I think it'd be better to have a way to specify that you want data_deps
to
> be built before your action runs, rather than needing to write a file to get
> these semantics.
> 
> > 
> > If we want this to work, I think we have to make the target aware of what
its
> > corresponding runtime deps file is for the computate in
> > target.cc:EnsureFileIsGeneratedByDependency.
> > 
> > What do you think? Sorry, I didn't realize you had already started working
on
> > this or I would have wanted to discuss more in advance.
> 
> No worries, just started on it this morning.
> 
> I thought of putting it on the target as well, but then thought it shouldn't
> really be the target's responsibility to write the file. E.g. a test has
> runtime_deps, but it's only the test runner that requires them to be written
to
> a file. So, it should be the test runner that requests them to be written.
True,
> you could declare a dummy group() for the express purpose of setting
> runtime_deps_file, but since the files are written at gn gen time, all that
does
> is touch a stamp file.

After sleeping on it, I feel even more strongly that creating the file should
not be tied to building its data_deps:
- "gn desc //out/Debug runtime_deps target" does not cause the target's
dependencies to be built
- A rule might depend on the .runtime_deps file, but not depend on the file
paths that are contained within it (e.g. I could write a rule that just asserts
there are no *.dll files in it)

[brettw, 2016-03-17 20:39:44]

On 2016/03/17 13:56:34, agrieve wrote:
> After sleeping on it, I feel even more strongly that creating the file should
> not be tied to building its data_deps:
> - "gn desc //out/Debug runtime_deps target" does not cause the target's
> dependencies to be built
> - A rule might depend on the .runtime_deps file, but not depend on the file
> paths that are contained within it (e.g. I could write a rule that just
asserts
> there are no *.dll files in it)

Your example is theoretically correct, but in all practical cases you need the
runtime deps file because you want to do something with the files. And
forgetting to include the dependency will lead to hard-to-diagnose flaky
compiles. Preventing flaky compiles is the entire goal of extremely detailed
inputs/outputs checking that GN does. And in any ambiguous cases, GN
overconstrains the build graph to prevent flaky compiles (e.g. treating all
actions as "hard" deps for all transitive dependants).

If the external build environment needs runtime deps for its use the
--runtime-deps-list-file would be sufficient. I think this is what you're
getting at with the "desc" example, but I'm not 100% sure. In your first email
you discuss the test runner needing these files. If the test runner is external
to the build, --runtime-deps-list-file is the correct solution (the build
shouldn't make assumptions about the enclosing test environment).

But we're discussing adding this feature precisely because the runtime deps are
needed as part of the build, right? Anything that's part of the build needs to
participate in the build dependency graph.

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

On 2016/03/17 20:39:44, brettw wrote:
> On 2016/03/17 13:56:34, agrieve wrote:
> > After sleeping on it, I feel even more strongly that creating the file
should
> > not be tied to building its data_deps:
> > - "gn desc //out/Debug runtime_deps target" does not cause the target's
> > dependencies to be built
> > - A rule might depend on the .runtime_deps file, but not depend on the file
> > paths that are contained within it (e.g. I could write a rule that just
> asserts
> > there are no *.dll files in it)
> 
> Your example is theoretically correct, but in all practical cases you need the
> runtime deps file because you want to do something with the files. And
> forgetting to include the dependency will lead to hard-to-diagnose flaky
> compiles. Preventing flaky compiles is the entire goal of extremely detailed
> inputs/outputs checking that GN does. And in any ambiguous cases, GN
> overconstrains the build graph to prevent flaky compiles (e.g. treating all
> actions as "hard" deps for all transitive dependants).

There's only one in-practice use for this atm that I'm aware of: Android unit
tests that push files to the device.

How it will be used is shown here: https://codereview.chromium.org/1805643002/
(it just runs gn desc atm)

In that change, I'm creating a new target: "_${target_name}__apk_isolate", which
creates the .isolate. It does not need any of the files to be built (and so I
don't list the lib target as a data_dep).

I think your concern is that someone could write a rule that lists the
write_runtime_deps() as an input, and then forget to list the actual target
under data_deps(), but in practice I can't see this ever happening. If you
expect things to be built, then you list them in something that ends with
"deps".

In the example of test.gni, we're defining the shared_library(), and so would be
able to add a variable to it. However, I could also envision a case like
android_apk, where a shared_library is passed to it. What if I want to write the
runtime_deps() of the shared_library for use in the .apk? I could declare a
group() that writes its runtime_deps, and then have it depend on the
shared_library(), but I think that causes the shared_library() itself to be
listed as a runtime_dep now.

> 
> If the external build environment needs runtime deps for its use the
> --runtime-deps-list-file would be sufficient. I think this is what you're
> getting at with the "desc" example, but I'm not 100% sure. In your first email
> you discuss the test runner needing these files. If the test runner is
external
> to the build, --runtime-deps-list-file is the correct solution (the build
> shouldn't make assumptions about the enclosing test environment).

The flag is sufficient, but telling everyone to include it for all tests they
want to run is super annoying. Having the test runner run "gn deps" to generate
the file list is also workable, but also annoying since it adds >2 seconds to
every invokation.

> 
> But we're discussing adding this feature precisely because the runtime deps
are
> needed as part of the build, right? Anything that's part of the build needs to
> participate in the build dependency graph.

It participates in the same way that write_file() does. And I think that makes
sense since it's also writing a file at gen-time.

How about I add a note to the help message that calls out the fact that usages
probably also want to include the target in their data_deps?

[agrieve, 2016-03-18 23:56:40]

On 2016/03/18 02:19:31, agrieve wrote:
> On 2016/03/17 20:39:44, brettw wrote:
> > On 2016/03/17 13:56:34, agrieve wrote:
> > > After sleeping on it, I feel even more strongly that creating the file
> should
> > > not be tied to building its data_deps:
> > > - "gn desc //out/Debug runtime_deps target" does not cause the target's
> > > dependencies to be built
> > > - A rule might depend on the .runtime_deps file, but not depend on the
file
> > > paths that are contained within it (e.g. I could write a rule that just
> > asserts
> > > there are no *.dll files in it)
> > 
> > Your example is theoretically correct, but in all practical cases you need
the
> > runtime deps file because you want to do something with the files. And
> > forgetting to include the dependency will lead to hard-to-diagnose flaky
> > compiles. Preventing flaky compiles is the entire goal of extremely detailed
> > inputs/outputs checking that GN does. And in any ambiguous cases, GN
> > overconstrains the build graph to prevent flaky compiles (e.g. treating all
> > actions as "hard" deps for all transitive dependants).
> 
> There's only one in-practice use for this atm that I'm aware of: Android unit
> tests that push files to the device.
> 
> How it will be used is shown here: https://codereview.chromium.org/1805643002/
> (it just runs gn desc atm)
> 
> In that change, I'm creating a new target: "_${target_name}__apk_isolate",
which
> creates the .isolate. It does not need any of the files to be built (and so I
> don't list the lib target as a data_dep).
> 
> I think your concern is that someone could write a rule that lists the
> write_runtime_deps() as an input, and then forget to list the actual target
> under data_deps(), but in practice I can't see this ever happening. If you
> expect things to be built, then you list them in something that ends with
> "deps".
> 
> In the example of test.gni, we're defining the shared_library(), and so would
be
> able to add a variable to it. However, I could also envision a case like
> android_apk, where a shared_library is passed to it. What if I want to write
the
> runtime_deps() of the shared_library for use in the .apk? I could declare a
> group() that writes its runtime_deps, and then have it depend on the
> shared_library(), but I think that causes the shared_library() itself to be
> listed as a runtime_dep now.
> 
> > 
> > If the external build environment needs runtime deps for its use the
> > --runtime-deps-list-file would be sufficient. I think this is what you're
> > getting at with the "desc" example, but I'm not 100% sure. In your first
email
> > you discuss the test runner needing these files. If the test runner is
> external
> > to the build, --runtime-deps-list-file is the correct solution (the build
> > shouldn't make assumptions about the enclosing test environment).
> 
> The flag is sufficient, but telling everyone to include it for all tests they
> want to run is super annoying. Having the test runner run "gn deps" to
generate
> the file list is also workable, but also annoying since it adds >2 seconds to
> every invokation.
> 
> > 
> > But we're discussing adding this feature precisely because the runtime deps
> are
> > needed as part of the build, right? Anything that's part of the build needs
to
> > participate in the build dependency graph.
> 
> It participates in the same way that write_file() does. And I think that makes
> sense since it's also writing a file at gen-time.
> 
> How about I add a note to the help message that calls out the fact that usages
> probably also want to include the target in their data_deps?

Couple more tidbits:
- I'd like to be able to "ninja base_unittests__generate_isolate" without having
to build the native library (which would be the case if we went with
runtime_deps_file=)
- An isolate / list of runtime deps is analogous to an AndroidManifest.xml. We
generate these from jinja templates, and they contain pointers to resources &
java classes, but you don't have to build the resources & java classes to
generate the manifest.

[Dirk Pranke, 2016-03-19 01:27:03]

It seems like there's two main uses for this sort of functionality so far:

1) The android test runner case, where we need a .isolate file to exist as input
to the bin/run_$test_name wrapper script .
In this case, we need the .runtime_deps file to exist in order to generate the
wrapper, but we do *not* need the actual
binary to be built. Since the .runtime_deps file is actually written out during
`gn gen` time, this means that we can generate
the wrapper (and the .isolate file) well in advance of actually needing the
binary.

2) The swarming case, where we need an .isolate file for a given binary.
Currently the .isolate files are generated "outside"
of the build (by MB invoking GN with an explicit list of targets to generate
deps for).

It's not actually obvious to me how we would model (2) as in-gen or in-build
steps. I suppose we could modify the
dummy _run targets to actually write the .runtime_deps file and then invoke a
script to generate the .isolate from it.

If we wanted to support both of these cases -- which we do for running android
tests under swarming -- ideally
we generate the .runtime_deps file *once* , which would suggest either the _run
target depends on the
test_runner_script() target, or vice versa, or they both depend on a common
__runtime_deps target that is a 
no-op at compile time but writes the file at gen time. 

That suggests to me that we don't do this as a runtime_deps_file setting on a
given target, since in no case is
that particularly useful.

I think this means I'm roughly ending up agreeing w/ agrieve@.

@brettw - what do you think?

[brettw, 2016-03-21 23:35:50]

On 2016/03/19 01:27:03, Dirk Pranke wrote:
> It seems like there's two main uses for this sort of functionality so far:
> 
> 1) The android test runner case, where we need a .isolate file to exist as
input
> to the bin/run_$test_name wrapper script .
> In this case, we need the .runtime_deps file to exist in order to generate the
> wrapper, but we do *not* need the actual
> binary to be built. Since the .runtime_deps file is actually written out
during
> `gn gen` time, this means that we can generate
> the wrapper (and the .isolate file) well in advance of actually needing the
> binary.

What happens if I do
  ninja run_browser_tests
(i.e. the wrapper target). In this case, do I get a wrapper script that fails to
run because the dependencies weren't built? If so, that seems like a bug to me.

Brett

[agrieve, 2016-03-22 00:45:46]

On 2016/03/21 23:35:50, brettw wrote:
> On 2016/03/19 01:27:03, Dirk Pranke wrote:
> > It seems like there's two main uses for this sort of functionality so far:
> > 
> > 1) The android test runner case, where we need a .isolate file to exist as
> input
> > to the bin/run_$test_name wrapper script .
> > In this case, we need the .runtime_deps file to exist in order to generate
the
> > wrapper, but we do *not* need the actual
> > binary to be built. Since the .runtime_deps file is actually written out
> during
> > `gn gen` time, this means that we can generate
> > the wrapper (and the .isolate file) well in advance of actually needing the
> > binary.
> 
> What happens if I do
>   ninja run_browser_tests
> (i.e. the wrapper target). In this case, do I get a wrapper script that fails
to
> run because the dependencies weren't built? If so, that seems like a bug to
me.
> 
> Brett

The test templates are defined here:
https://code.google.com/p/chromium/codesearch#chromium/src/testing/test.gni

The _run targets have TODO() saying that they are going away once GYP is no
more, and they are just group()s that alias other targets, so I'm assuming
you're referring to the android-specific wrapper scripts.

These script targets are named "${_test_name}__test_runner_script", so it's
unlikely that you'd build it directly.

Right now, it's intentionally set up so that if you build the wrapper script
directly, it does *not* build the .apk:
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/andro...

This is so that it can be built in parallel with the .apk.

Arguably, we should remove the wrapper group in favour of listing the .apk as a
data_dep of the wrapper script.

Does this answer your question? I think no matter which approach we use for
writing the runtime deps, it'll be the case that the wrapper script will depend
on the .isolate file when there is one.

[brettw, 2016-03-22 18:05:03]

On 2016/03/22 00:45:46, agrieve wrote:
> These script targets are named "${_test_name}__test_runner_script", so it's
> unlikely that you'd build it directly.
> 
> Right now, it's intentionally set up so that if you build the wrapper script
> directly, it does *not* build the .apk:
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/andro...

That's exactly the kind of bug I'm trying to prevent! When you build a target,
its dependencies should be complete so that it works. The fact that the contents
of the file aren't *technically* used at buildtime isn't very important to me
either way.

I'm trying to guarantee the build is correct to the extent GN can know. It can
know about this file, this file can (and is!) used in a manner contrary to the
dependencies such that a broken build will result for certain targets or
dependency paths. I go through great lengths to find all such cases and stop
them.

The complaint is that the build is overconstrained. This is true. The build is
vastly overconstrained in many, many ways because it's difficult-to-impossible
to prove correctness otherwise. GN is much more conservative about correctness
than GYP (like treating all actions as "hard dependencies"). This discussion has
just made me feel even more strongly that the current type of usage needs to be
stopped with an explicit check.

[agrieve, 2016-03-22 19:14:36]

On 2016/03/22 18:05:03, brettw wrote:
> On 2016/03/22 00:45:46, agrieve wrote:
> > These script targets are named "${_test_name}__test_runner_script", so it's
> > unlikely that you'd build it directly.
> > 
> > Right now, it's intentionally set up so that if you build the wrapper script
> > directly, it does *not* build the .apk:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/andro...
> 
> That's exactly the kind of bug I'm trying to prevent! When you build a target,
> its dependencies should be complete so that it works. The fact that the
contents
> of the file aren't *technically* used at buildtime isn't very important to me
> either way.
> 
> I'm trying to guarantee the build is correct to the extent GN can know. It can
> know about this file, this file can (and is!) used in a manner contrary to the
> dependencies such that a broken build will result for certain targets or
> dependency paths. I go through great lengths to find all such cases and stop
> them.
> 
> The complaint is that the build is overconstrained. This is true. The build is
> vastly overconstrained in many, many ways because it's difficult-to-impossible
> to prove correctness otherwise. GN is much more conservative about correctness
> than GYP (like treating all actions as "hard dependencies"). This discussion
has
> just made me feel even more strongly that the current type of usage needs to
be
> stopped with an explicit check.

The android rules are full of example like this, where internal helper-targets
can be built, but they are not actually useful unless you build their
public-facing target. As the person writing these internal targets, I find this
quite useful as it allows me to iterate faster.

At this point though, I really just want to make some progress.

I think I've still given valid reasons to not want a new variable for the target
(since you might not be the owner of it). How about a new target: 

write_runtime_deps("target_name") { 
  data_deps = [...]
  outputs = [ "$root_out_dir/whereever.txt" ]
}


This would be semantically equivalent to a group(), except that you'll need to
depend on it in order to list the output as an input.

[brettw, 2016-03-22 21:05:03]

> The android rules are full of example like this

Just so you know where I'm coming from, my opinion is that your ability to do
this is a bug. I'm not interested in optimizing build times for the one person
writing rules in some internal thing, but for the absolute correctness of the
build in every case. Over time, somebody on our team does every possible thing,
especially with the build where people don't review carefully.


> I think I've still given valid reasons to not want a new variable for the
target
> (since you might not be the owner of it). How about a new target: 

You could already do that with my proposal. Since runtime deps are transitive
you can put them wherever you want as long as you have visibility to the
destination target:

  group("foo") {
    data_deps = [ "//base:base_unittests" ]
    runtime_deps_file = "$root_out_dir/wherever.txt"
  }

[brettw, 2016-03-22 21:25:25]

On 2016/03/22 21:05:03, brettw wrote:
> > The android rules are full of example like this
> 
> Just so you know where I'm coming from, my opinion is that your ability to do
> this is a bug. I'm not interested in optimizing build times for the one person
> writing rules in some internal thing, but for the absolute correctness of the
> build in every case. Over time, somebody on our team does every possible
thing,
> especially with the build where people don't review carefully.
> 
> 
> > I think I've still given valid reasons to not want a new variable for the
> target
> > (since you might not be the owner of it). How about a new target: 
> 
> You could already do that with my proposal. Since runtime deps are transitive
> you can put them wherever you want as long as you have visibility to the
> destination target:
> 
>   group("foo") {
>     data_deps = [ "//base:base_unittests" ]
>     runtime_deps_file = "$root_out_dir/wherever.txt"
>   }

I'm not in principal opposed to the new target type though. They're conceptually
equivalent. My main concern would be doing whatever keeps the "number of things
in GN" lower.

[agrieve, 2016-03-22 21:35:07]

On 2016/03/22 21:25:25, brettw wrote:
> On 2016/03/22 21:05:03, brettw wrote:
> > > The android rules are full of example like this
> > 
> > Just so you know where I'm coming from, my opinion is that your ability to
do
> > this is a bug. I'm not interested in optimizing build times for the one
person
> > writing rules in some internal thing, but for the absolute correctness of
the
> > build in every case. Over time, somebody on our team does every possible
> thing,
> > especially with the build where people don't review carefully.
> > 
> > 
> > > I think I've still given valid reasons to not want a new variable for the
> > target
> > > (since you might not be the owner of it). How about a new target: 
> > 
> > You could already do that with my proposal. Since runtime deps are
transitive
> > you can put them wherever you want as long as you have visibility to the
> > destination target:
> > 
> >   group("foo") {
> >     data_deps = [ "//base:base_unittests" ]
> >     runtime_deps_file = "$root_out_dir/wherever.txt"
> >   }

Heh, just signed on again to point out that I realized this is the case! Seems
like probably the variable is the fewer-ways-to-do-it way then.

One annoying thing is that I don't think any android targets that use .isolate
files to push files to the device depend on any generated files. We're now going
to enforce building a bunch of files that will then be filtered out by
https://codereview.chromium.org/1805643002/. I do realize that that's a
coincidence and different problem, but I find it annoying nonetheless :(.

> 
> I'm not in principal opposed to the new target type though. They're
conceptually
> equivalent. My main concern would be doing whatever keeps the "number of
things
> in GN" lower.

[agrieve, 2016-03-25 01:32:50]

On 2016/03/22 21:35:07, agrieve wrote:
> On 2016/03/22 21:25:25, brettw wrote:
> > On 2016/03/22 21:05:03, brettw wrote:
> > > > The android rules are full of example like this
> > > 
> > > Just so you know where I'm coming from, my opinion is that your ability to
> do
> > > this is a bug. I'm not interested in optimizing build times for the one
> person
> > > writing rules in some internal thing, but for the absolute correctness of
> the
> > > build in every case. Over time, somebody on our team does every possible
> > thing,
> > > especially with the build where people don't review carefully.
> > > 
> > > 
> > > > I think I've still given valid reasons to not want a new variable for
the
> > > target
> > > > (since you might not be the owner of it). How about a new target: 
> > > 
> > > You could already do that with my proposal. Since runtime deps are
> transitive
> > > you can put them wherever you want as long as you have visibility to the
> > > destination target:
> > > 
> > >   group("foo") {
> > >     data_deps = [ "//base:base_unittests" ]
> > >     runtime_deps_file = "$root_out_dir/wherever.txt"
> > >   }
> 
> Heh, just signed on again to point out that I realized this is the case! Seems
> like probably the variable is the fewer-ways-to-do-it way then.
> 
> One annoying thing is that I don't think any android targets that use .isolate
> files to push files to the device depend on any generated files. We're now
going
> to enforce building a bunch of files that will then be filtered out by
> https://codereview.chromium.org/1805643002/. I do realize that that's a
> coincidence and different problem, but I find it annoying nonetheless :(.
> 
> > 
> > I'm not in principal opposed to the new target type though. They're
> conceptually
> > equivalent. My main concern would be doing whatever keeps the "number of
> things
> > in GN" lower.

This is now re-worked to be a variable.

Note: Although I think we can actually get away without using this for android
tests, it will still be useful to replace where we currently use readelf to list
all dependent .so files when packing an .apk.

[agrieve, 2016-03-25 01:33:41]

Description was changed from

==========
Add write_runtime_deps(file, label) builtin

This is required for Android tests, which use .isolate's at runtime
(which in turn must be listed as files within the parent .isolate).

BUG=593416
==========

to

==========
GN: Add write_runtime_deps variable

Allows targets to use runtime_deps at build-time.

BUG=593416
==========

[brettw, 2016-03-28 22:47:28]

Sorry for the delay, I haven't forgotten!

[brettw, 2016-03-28 23:06:35]

https://codereview.chromium.org/1804303004/diff/20001/tools/gn/scheduler.cc
File tools/gn/scheduler.cc (right):

https://codereview.chromium.org/1804303004/diff/20001/tools/gn/scheduler.cc#n...
tools/gn/scheduler.cc:163: const {
const can be on previous line.

https://codereview.chromium.org/1804303004/diff/20001/tools/gn/scheduler.cc#n...
tools/gn/scheduler.cc:171: for (const Target* target :
write_runtime_deps_targets_) {
This will crash because it accesses the list not protected by the lock. I don't
think we want to lock here if we can avoid it anyway since this can be called a
lot.

target.cc is already going through all the deps tree. I didn't entirely consider
the details of the changes you made to the deps walking algorithm there but I
wonder if it can be modified to handle all the cases without doing this code.

https://codereview.chromium.org/1804303004/diff/20001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/1804303004/diff/20001/tools/gn/variables.cc#n...
tools/gn/variables.cc:1532: "  Path must be within the output directory.\n";
Can you add here:

See "gn help runtime_deps" for how the runtime dependencies are computed. 

The format of this file will list one file per line with no escaping. The files
will be relative to the root_build_dir. The first line of the file will be the
main output file of the target itself. The file contents will be the same as
requesting the runtime deps be written on the command line (see "gn help
--runtime-deps-list-file").

[agrieve, 2016-03-29 16:14:01]

Also added a test that somehow got left off the last time.

https://codereview.chromium.org/1804303004/diff/20001/tools/gn/scheduler.cc
File tools/gn/scheduler.cc (right):

https://codereview.chromium.org/1804303004/diff/20001/tools/gn/scheduler.cc#n...
tools/gn/scheduler.cc:163: const {
On 2016/03/28 23:06:35, brettw wrote:
> const can be on previous line.

Done.

https://codereview.chromium.org/1804303004/diff/20001/tools/gn/scheduler.cc#n...
tools/gn/scheduler.cc:171: for (const Target* target :
write_runtime_deps_targets_) {
On 2016/03/28 23:06:35, brettw wrote:
> This will crash because it accesses the list not protected by the lock. I
don't
> think we want to lock here if we can avoid it anyway since this can be called
a
> lot.
> 
> target.cc is already going through all the deps tree. I didn't entirely
consider
> the details of the changes you made to the deps walking algorithm there but I
> wonder if it can be modified to handle all the cases without doing this code.

Added the lock.

This function is called only when the initial
EnsureFileIsGeneratedByDependency() fails, so it shouldn't be called a lot.

https://codereview.chromium.org/1804303004/diff/20001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/1804303004/diff/20001/tools/gn/variables.cc#n...
tools/gn/variables.cc:1532: "  Path must be within the output directory.\n";
On 2016/03/28 23:06:35, brettw wrote:
> Can you add here:
> 
> See "gn help runtime_deps" for how the runtime dependencies are computed. 
> 
> The format of this file will list one file per line with no escaping. The
files
> will be relative to the root_build_dir. The first line of the file will be the
> main output file of the target itself. The file contents will be the same as
> requesting the runtime deps be written on the command line (see "gn help
> --runtime-deps-list-file").

Done.

[Dirk Pranke, 2016-03-29 23:56:03]

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

[brettw, 2016-03-30 00:03:52]

Sorry to come back on this again, I was really hoping to be done (and sorry for
the delay, this week is crazy and I'm also preparing for the GN talk).

The current thing uses the scheduler's list to tell whether it should search
data deps for the list file. I think this is reasonable to the extent we want
data_deps to count. But after thinking about it for a bit and also talking to
Dirk, I think we should be forcing this to be regular deps. I believe this will
work for your use-case just as well, and it's more conservatively correct.

For your use-case I realize that the file is consumed only at runtime so
data_deps is sufficient. But deps should also work at a trivial cost of build
parallelism.

The counter example is //blimp/engine:blimp_engine_bundle. They have
jerry-rigged a thing where they take the output of GN deps, pipe it to a file,
and check it in. This seems like a perfect use-case for your files (with the
caveats that they want to filter a few out and may want to know when the list
changes for release sanity). I know about this target because they were
complaining that their build was flaky and asked me why GN was screwing up (in
so many words). Their problem was that the dependency list was incomplete and
not everything was built by the time they were reading their manifest file. In
this case, they need the list to be real deps for the build to be correct, and
the type of checking for this that GN does is to prevent this type of screw up
(and transitively me getting blamed for every build flake in the system).

I think if we don't need the data deps thing, we can actually remove the
scheduler list check and the new lock, and also slightly simplify the changes in
target.cc.

[agrieve, 2016-03-30 14:48:24]

On 2016/03/30 00:03:52, brettw wrote:
> Sorry to come back on this again, I was really hoping to be done (and sorry
for
> the delay, this week is crazy and I'm also preparing for the GN talk).
> 
> The current thing uses the scheduler's list to tell whether it should search
> data deps for the list file. I think this is reasonable to the extent we want
> data_deps to count. But after thinking about it for a bit and also talking to
> Dirk, I think we should be forcing this to be regular deps. I believe this
will
> work for your use-case just as well, and it's more conservatively correct.
> 
> For your use-case I realize that the file is consumed only at runtime so
> data_deps is sufficient. But deps should also work at a trivial cost of build
> parallelism.
> 
> The counter example is //blimp/engine:blimp_engine_bundle. They have
> jerry-rigged a thing where they take the output of GN deps, pipe it to a file,
> and check it in. This seems like a perfect use-case for your files (with the
> caveats that they want to filter a few out and may want to know when the list
> changes for release sanity). I know about this target because they were
> complaining that their build was flaky and asked me why GN was screwing up (in
> so many words). Their problem was that the dependency list was incomplete and
> not everything was built by the time they were reading their manifest file. In
> this case, they need the list to be real deps for the build to be correct, and
> the type of checking for this that GN does is to prevent this type of screw up
> (and transitively me getting blamed for every build flake in the system).
> 
> I think if we don't need the data deps thing, we can actually remove the
> scheduler list check and the new lock, and also slightly simplify the changes
in
> target.cc.

Would changing it to deps fix their race condition? .runtime_deps lists only
data deps, and I thought data_deps, even when depended on via an intermediate
deps=, were not guaranteed to be built until the end. Unless this is incorrect,
depending on a write_runtime_deps via data_deps would be the more correct way
since its data_deps won't be built beforehand either way.

[brettw, 2016-03-30 23:44:40]

On 2016/03/30 14:48:24, agrieve wrote:
> Would changing it to deps fix their race condition? .runtime_deps lists only
> data deps, and I thought data_deps, even when depended on via an intermediate
> deps=, were not guaranteed to be built until the end. Unless this is
incorrect,
> depending on a write_runtime_deps via data_deps would be the more correct way
> since its data_deps won't be built beforehand either way.

Yes, actions deps are always done completely before the action is run.

[agrieve, 2016-03-31 00:00:18]

On 2016/03/30 23:44:40, brettw wrote:
> On 2016/03/30 14:48:24, agrieve wrote:
> > Would changing it to deps fix their race condition? .runtime_deps lists only
> > data deps, and I thought data_deps, even when depended on via an
intermediate
> > deps=, were not guaranteed to be built until the end. Unless this is
> incorrect,
> > depending on a write_runtime_deps via data_deps would be the more correct
way
> > since its data_deps won't be built beforehand either way.
> 
> Yes, actions deps are always done completely before the action is run.

Deps, are, but are the data_deps of the action? 

What's in a write_runfiles_file is the list of data & data_deps. Will they be
built if you have a dep = the-thing-that-has-write_runtime_deps set? If not,
then I don't see the point of enforcing proper deps (and think it's actually a
bit misleading)

[brettw, 2016-03-31 17:35:14]

On 2016/03/31 00:00:18, agrieve wrote:
> What's in a write_runfiles_file is the list of data & data_deps. Will they be
> built if you have a dep = the-thing-that-has-write_runtime_deps set? If not,
> then I don't see the point of enforcing proper deps (and think it's actually a
> bit misleading)

Ahh, you're right! This Blimp thing confused me and I now realize their bundle
step is more messed up than I thought. They're using data deps and extracting it
via the command line tool, but then using that as part of the actual build,
which is not guaranteed correct. I was thinking it should work for their case
and was trying to make this catch the error they were having.

[brettw, 2016-03-31 17:35:25]

LGTM

https://codereview.chromium.org/1804303004/diff/40001/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/1804303004/diff/40001/tools/gn/target.cc#newc...
tools/gn/target.cc:100: if (file == target->write_runtime_deps_output()) {
No {}

[Dirk Pranke, 2016-03-31 20:58:44]

agrieve@, if you go ahead and land this I'll do a roll to bring it into
chromium.

[agrieve, 2016-03-31 21:08:33]

https://codereview.chromium.org/1804303004/diff/40001/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/1804303004/diff/40001/tools/gn/target.cc#newc...
tools/gn/target.cc:100: if (file == target->write_runtime_deps_output()) {
On 2016/03/31 17:35:25, brettw wrote:
> No {}

Done.

[agrieve, 2016-03-31 21:09:05]

On 2016/03/31 20:58:44, Dirk Pranke wrote:
> agrieve@, if you go ahead and land this I'll do a roll to bring it into
> chromium.

Thanks! I'll push submit now, but will be out for the next 3 hours.

[agrieve, 2016-03-31 21:09:12]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2016-03-31 21:09:13]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1804303004/#ps60001
(title: "remove {}s")

[commit-bot: I haz the power, 2016-03-31 21:10:04]

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

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

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

[commit-bot: I haz the power, 2016-03-31 21:20:04]

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

[Dirk Pranke, 2016-03-31 21:46:37]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2016-03-31 21:46:58]

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

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

Description was changed from

==========
GN: Add write_runtime_deps variable

Allows targets to use runtime_deps at build-time.

BUG=593416
==========

to

==========
GN: Add write_runtime_deps variable

Allows targets to use runtime_deps at build-time.

BUG=593416
==========

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

Committed patchset #4 (id:60001)

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

Description was changed from

==========
GN: Add write_runtime_deps variable

Allows targets to use runtime_deps at build-time.

BUG=593416
==========

to

==========
GN: Add write_runtime_deps variable

Allows targets to use runtime_deps at build-time.

BUG=593416

Committed: https://crrev.com/0f8ad42ac1a6b159655c51bf7095ff2d21ae540f
Cr-Commit-Position: refs/heads/master@{#384411}
==========

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

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