GN: Use implicit dependency for binary input deps

GN: Use implicit dependency for binary input deps

This is to ensure that the input gets rebuilt when the depencies change
even when they are not included in the depfile generated by the
compiler.

BUG=612623
Committed: https://crrev.com/57ac2eae0f67ef9310e2fad40258b1d45ac82435
Cr-Commit-Position: refs/heads/master@{#400515}

[Petr Hosek, 2016-06-15 17:26:54]

Description was changed from

==========
[gn] Use implicit dependency for binary input deps

This is to ensure that the input gets rebuilt when the depencies change
even when they are not included in the depfile generated by the
compiler.
==========

to

==========
[gn] Use implicit dependency for binary input deps

This is to ensure that the input gets rebuilt when the depencies change
even when they are not included in the depfile generated by the
compiler.
==========

[Petr Hosek, 2016-06-15 17:26:55]

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

[Dirk Pranke, 2016-06-15 22:48:57]

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

[Dirk Pranke, 2016-06-15 22:48:58]

Am I correct in thinking that this should address crbug.com/612623 ? If so,
that'd be cool ...

[Petr Hosek, 2016-06-15 23:13:15]

Description was changed from

==========
[gn] Use implicit dependency for binary input deps

This is to ensure that the input gets rebuilt when the depencies change
even when they are not included in the depfile generated by the
compiler.
==========

to

==========
[gn] Use implicit dependency for binary input deps

This is to ensure that the input gets rebuilt when the depencies change
even when they are not included in the depfile generated by the
compiler.

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=612623
==========

[Petr Hosek, 2016-06-15 23:17:04]

On 2016/06/15 22:48:58, Dirk Pranke wrote:
> Am I correct in thinking that this should address crbug.com/612623 ? If so,
> that'd be cool ...

Yes, I think it should. We run into a similar issue in another project where
we're including a binary blob, that is generated as a dependency, into an
assembly file using the .incbin directive. Since this is not a header
dependency, there is no .d file being generated and as such the assembly file
doesn't get recompiled when the blob is re-generated, which sounds like the same
issue. I chatted to brettw@ about this and he suggested this change.

[Robert Sesek, 2016-06-16 15:10:16]

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

[Robert Sesek, 2016-06-16 15:10:16]

Should we also update the documentation for `gn help inputs`?

[Robert Sesek, 2016-06-16 15:10:24]

Description was changed from

==========
[gn] Use implicit dependency for binary input deps

This is to ensure that the input gets rebuilt when the depencies change
even when they are not included in the depfile generated by the
compiler.

BUG=https://bugs.chromium.org/p/chromium/issues/detail?id=612623
==========

to

==========
[gn] Use implicit dependency for binary input deps

This is to ensure that the input gets rebuilt when the depencies change
even when they are not included in the depfile generated by the
compiler.

BUG=612623
==========

[brettw, 2016-06-16 19:41:34]

Sorry, did these comments a while ago but I forgot to send main.

https://codereview.chromium.org/2071573003/diff/20001/tools/gn/ninja_binary_t...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/2071573003/diff/20001/tools/gn/ninja_binary_t...
tools/gn/ninja_binary_target_writer.cc:524: if (!input_dep.value().empty()) {
No {}, same for the additions below.

https://codereview.chromium.org/2071573003/diff/20001/tools/gn/ninja_binary_t...
File tools/gn/ninja_binary_target_writer_unittest.cc (right):

https://codereview.chromium.org/2071573003/diff/20001/tools/gn/ninja_binary_t...
tools/gn/ninja_binary_target_writer_unittest.cc:297: " | obj/foo/action.stamp\n"
Actually, we specifically don't want to change this case. This is what
order-only deps were designed for.

What I was imagining when I talked about this is how the "inputs" list in a
binary target is treated. Currently they also appear in the order only deps, but
I think we want to split them out to be implicit deps. This should apply only to
the explicit inputs on a target, and not the transitive dependencies of it. So
it would apply to these things:

source_set("foo") {
  sources = [ "foo", "bar" ]
  inputs = [ "baz.dat" ]
}

Changing all actions to be implicit dependencies of binary targets that depend
on them in some way (what it looks like you're doing) will reduce build
parallelism significantly.

[Petr Hosek, 2016-06-16 20:28:16]

On 2016/06/16 19:41:34, brettw wrote:
> Sorry, did these comments a while ago but I forgot to send main.
> 
>
https://codereview.chromium.org/2071573003/diff/20001/tools/gn/ninja_binary_t...
> File tools/gn/ninja_binary_target_writer.cc (right):
> 
>
https://codereview.chromium.org/2071573003/diff/20001/tools/gn/ninja_binary_t...
> tools/gn/ninja_binary_target_writer.cc:524: if (!input_dep.value().empty()) {
> No {}, same for the additions below.
> 
>
https://codereview.chromium.org/2071573003/diff/20001/tools/gn/ninja_binary_t...
> File tools/gn/ninja_binary_target_writer_unittest.cc (right):
> 
>
https://codereview.chromium.org/2071573003/diff/20001/tools/gn/ninja_binary_t...
> tools/gn/ninja_binary_target_writer_unittest.cc:297: " |
obj/foo/action.stamp\n"
> Actually, we specifically don't want to change this case. This is what
> order-only deps were designed for.
> 
> What I was imagining when I talked about this is how the "inputs" list in a
> binary target is treated. Currently they also appear in the order only deps,
but
> I think we want to split them out to be implicit deps. This should apply only
to
> the explicit inputs on a target, and not the transitive dependencies of it. So
> it would apply to these things:
> 
> source_set("foo") {
>   sources = [ "foo", "bar" ]
>   inputs = [ "baz.dat" ]
> }
> 
> Changing all actions to be implicit dependencies of binary targets that depend
> on them in some way (what it looks like you're doing) will reduce build
> parallelism significantly.

I didn't know you can actually specify inputs on other targets, I thought it was
only used for action and action_foreach. I'll update the patch to only add
inputs to implicit dependencies then.

[Petr Hosek, 2016-06-16 23:07:18]

Description was changed from

==========
[gn] Use implicit dependency for binary input deps

This is to ensure that the input gets rebuilt when the depencies change
even when they are not included in the depfile generated by the
compiler.

BUG=612623
==========

to

==========
GN: Use implicit dependency for binary input deps

This is to ensure that the input gets rebuilt when the depencies change
even when they are not included in the depfile generated by the
compiler.

BUG=612623
==========

[Petr Hosek, 2016-06-16 23:11:33]

I've updated the patch to only add implicit dependencies for inputs of binary
targets and I've updated the documentation to reflect that change.

[Petr Hosek, 2016-06-16 23:11:47]

https://codereview.chromium.org/2071573003/diff/20001/tools/gn/ninja_binary_t...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/2071573003/diff/20001/tools/gn/ninja_binary_t...
tools/gn/ninja_binary_target_writer.cc:524: if (!input_dep.value().empty()) {
On 2016/06/16 19:41:34, brettw wrote:
> No {}, same for the additions below.

Done.

https://codereview.chromium.org/2071573003/diff/20001/tools/gn/ninja_binary_t...
File tools/gn/ninja_binary_target_writer_unittest.cc (right):

https://codereview.chromium.org/2071573003/diff/20001/tools/gn/ninja_binary_t...
tools/gn/ninja_binary_target_writer_unittest.cc:297: " | obj/foo/action.stamp\n"
On 2016/06/16 19:41:34, brettw wrote:
> Actually, we specifically don't want to change this case. This is what
> order-only deps were designed for.
> 
> What I was imagining when I talked about this is how the "inputs" list in a
> binary target is treated. Currently they also appear in the order only deps,
but
> I think we want to split them out to be implicit deps. This should apply only
to
> the explicit inputs on a target, and not the transitive dependencies of it. So
> it would apply to these things:
> 
> source_set("foo") {
>   sources = [ "foo", "bar" ]
>   inputs = [ "baz.dat" ]
> }
> 
> Changing all actions to be implicit dependencies of binary targets that depend
> on them in some way (what it looks like you're doing) will reduce build
> parallelism significantly.

Acknowledged.

[brettw, 2016-06-17 17:54:20]

LGTM, thanks!

https://codereview.chromium.org/2071573003/diff/40001/tools/gn/ninja_target_w...
File tools/gn/ninja_target_writer.cc (right):

https://codereview.chromium.org/2071573003/diff/40001/tools/gn/ninja_target_w...
tools/gn/ninja_target_writer.cc:169: // implicit dependency instead.
Can you mentioned that the implicit dep in this case is handled separately by
the binary target writer?

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

https://codereview.chromium.org/2071573003/diff/40001/tools/gn/variables.cc#n...
tools/gn/variables.cc:1052: "  meaning that changes in the inputs will force the
target to compile.\n"
How about:

...changes in any of the inputs will force all sources in the target to be
recompiled. If an input only applies to a subset of source files, you may want
to split those into a separate target to avoid unnecessary recompiles.

[Petr Hosek, 2016-06-17 18:59:30]

https://codereview.chromium.org/2071573003/diff/40001/tools/gn/ninja_target_w...
File tools/gn/ninja_target_writer.cc (right):

https://codereview.chromium.org/2071573003/diff/40001/tools/gn/ninja_target_w...
tools/gn/ninja_target_writer.cc:169: // implicit dependency instead.
On 2016/06/17 17:54:20, brettw wrote:
> Can you mentioned that the implicit dep in this case is handled separately by
> the binary target writer?

Done.

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

https://codereview.chromium.org/2071573003/diff/40001/tools/gn/variables.cc#n...
tools/gn/variables.cc:1052: "  meaning that changes in the inputs will force the
target to compile.\n"
On 2016/06/17 17:54:20, brettw wrote:
> How about:
> 
> ...changes in any of the inputs will force all sources in the target to be
> recompiled. If an input only applies to a subset of source files, you may want
> to split those into a separate target to avoid unnecessary recompiles.

Done.

[Petr Hosek, 2016-06-17 21:43:15]

The CQ bit was checked by phosek@chromium.org

[Petr Hosek, 2016-06-17 21:43:15]

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/2071573003/#ps60001
(title: "Code review changes")

[commit-bot: I haz the power, 2016-06-17 21:43:48]

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

[commit-bot: I haz the power, 2016-06-17 21:49:13]

Description was changed from

==========
GN: Use implicit dependency for binary input deps

This is to ensure that the input gets rebuilt when the depencies change
even when they are not included in the depfile generated by the
compiler.

BUG=612623
==========

to

==========
GN: Use implicit dependency for binary input deps

This is to ensure that the input gets rebuilt when the depencies change
even when they are not included in the depfile generated by the
compiler.

BUG=612623
==========

[commit-bot: I haz the power, 2016-06-17 21:49:15]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-17 21:50:35]

Description was changed from

==========
GN: Use implicit dependency for binary input deps

This is to ensure that the input gets rebuilt when the depencies change
even when they are not included in the depfile generated by the
compiler.

BUG=612623
==========

to

==========
GN: Use implicit dependency for binary input deps

This is to ensure that the input gets rebuilt when the depencies change
even when they are not included in the depfile generated by the
compiler.

BUG=612623

Committed: https://crrev.com/57ac2eae0f67ef9310e2fad40258b1d45ac82435
Cr-Commit-Position: refs/heads/master@{#400515}
==========

[commit-bot: I haz the power, 2016-06-17 21:50:37]

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