Fix GN build errors when splits are enabled.

Fix GN build errors when splits are enabled.

They all looked like: ERROR Input to target not generated by a dependency.

BUG=504971
Committed: https://crrev.com/06c6aab53db5a3f7c17b3b75ffbde75217abc96b
Cr-Commit-Position: refs/heads/master@{#336719}

[agrieve, 2015-06-26 18:38:00]

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

[agrieve, 2015-06-26 18:38:00]

Using a foreach() to get last element of the array. Please advice if there's a
better way

[agrieve, 2015-06-26 18:59:32]

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

[commit-bot: I haz the power, 2015-06-26 19:00:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1217583002/1

[commit-bot: I haz the power, 2015-06-26 19:00:11]

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

[commit-bot: I haz the power, 2015-06-26 19:00:12]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[brettw, 2015-06-26 19:48:52]

Adding these deps is the correct solution

https://codereview.chromium.org/1217583002/diff/1/build/config/android/rules.gni
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1217583002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1458: _android_manifest_deps = []
It's not clear to me what this block is doing. Can you put a higher-level
comment here about what this is trying to accomplish?

[agrieve, 2015-06-26 20:43:55]

https://codereview.chromium.org/1217583002/diff/1/build/config/android/rules.gni
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1217583002/diff/1/build/config/android/rules....
build/config/android/rules.gni:1458: _android_manifest_deps = []
On 2015/06/26 19:48:52, brettw wrote:
> It's not clear to me what this block is doing. Can you put a higher-level
> comment here about what this is trying to accomplish?

Done.

[cjhopman, 2015-06-26 22:38:49]

https://codereview.chromium.org/1217583002/diff/20001/build/config/android/ru...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1217583002/diff/20001/build/config/android/ru...
build/config/android/rules.gni:1471: # Use the last output, since depfile
outputs always need to come first.
This is strange and makes hard to find assumptions about the structure of the
target generating the manifest. Can't we require that the invoker specifies the
manifest path?

[cjhopman, 2015-06-26 23:03:11]

Or how about just adding all the specified deps to the targets that use the
manifest. Making users specify different types of deps in different variables
makes things rather complicated.

[agrieve, 2015-06-27 00:35:39]

On 2015/06/26 23:03:11, cjhopman wrote:
> Or how about just adding all the specified deps to the targets that use the
> manifest. Making users specify different types of deps in different variables
> makes things rather complicated.

Seems strange to me to double-specify the manifest (both as a file and as a
rule). I do see the oddness of assuming it's the last output of the rule though.

I've changed it so that you need to specify them both. Adding it as a dep to all
targets doesn't accurately capture dependencies (e.g. why does generating a
split APK manifest depend on jar files?)

Blaze automatically creates deps when it notices generated files used as inputs.
GN now has an error for this, so it clearly can recognize this same case. Do you
know what the rationale behind this was? Without this behaviour, any rule that
takes inputs won't work with generated inputs by default (extra work is required
on the rules part to allow generated inputs).

[brettw, 2015-06-28 19:55:44]

On 2015/06/27 00:35:39, agrieve wrote:
> Blaze automatically creates deps when it notices generated files used as
inputs.
> GN now has an error for this, so it clearly can recognize this same case. Do
you
> know what the rationale behind this was? Without this behaviour, any rule that
> takes inputs won't work with generated inputs by default (extra work is
required
> on the rules part to allow generated inputs).

GN has different types of dependencies and things that happen when you add
dependencies. If you did this, do you want public or private dependency? Should
all_dependent_configs and public_configs apply to you when you do this or not?
It's better to be explicit.

From an implementation perspective, GN works incrementally, so when the deps of
a target are know, the target is marked "resolved" and we're done with it. But
it could be we haven't read the target that generates the given input file yet,
so don't know that such a dependency should be created. Waiting until the entire
build is read in before writing would limit parallelism and slow things down.

[agrieve, 2015-06-29 14:33:40]

On 2015/06/28 19:55:44, brettw wrote:
> On 2015/06/27 00:35:39, agrieve wrote:
> > Blaze automatically creates deps when it notices generated files used as
> inputs.
> > GN now has an error for this, so it clearly can recognize this same case. Do
> you
> > know what the rationale behind this was? Without this behaviour, any rule
that
> > takes inputs won't work with generated inputs by default (extra work is
> required
> > on the rules part to allow generated inputs).
> 
> GN has different types of dependencies and things that happen when you add
> dependencies. If you did this, do you want public or private dependency?
Should
> all_dependent_configs and public_configs apply to you when you do this or not?
> It's better to be explicit.

Good reasoning. Now I agree :)

> 
> From an implementation perspective, GN works incrementally, so when the deps
of
> a target are know, the target is marked "resolved" and we're done with it. But
> it could be we haven't read the target that generates the given input file
yet,
> so don't know that such a dependency should be created. Waiting until the
entire
> build is read in before writing would limit parallelism and slow things down.

[cjhopman, 2015-06-29 22:55:17]

lgtm

https://codereview.chromium.org/1217583002/diff/40001/build/config/android/ru...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1217583002/diff/40001/build/config/android/ru...
build/config/android/rules.gni:1459: # generated it.
This comment should probably go in with the target-level docstring (around
#1296).

[agrieve, 2015-06-30 02:04:35]

https://codereview.chromium.org/1217583002/diff/40001/build/config/android/ru...
File build/config/android/rules.gni (right):

https://codereview.chromium.org/1217583002/diff/40001/build/config/android/ru...
build/config/android/rules.gni:1459: # generated it.
On 2015/06/29 22:55:17, cjhopman wrote:
> This comment should probably go in with the target-level docstring (around
> #1296).

Done.

[agrieve, 2015-06-30 02:12:32]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-06-30 02:12:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
cjhopman@chromium.org
Link to the patchset: https://codereview.chromium.org/1217583002/#ps60001
(title: "Add parameter docs")

[commit-bot: I haz the power, 2015-06-30 02:12:37]

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

[commit-bot: I haz the power, 2015-06-30 02:58:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-06-30 03:00:28]

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