Make the gn copy() tool honor dependencies.

Make the gn copy() tool honor dependencies.

Previously it didn't, and perhaps there was no need for
it to, but it seems like NaCl has a need. Specifically,
NaCl has two targets A and B with both run fine on their
own, but A can delete the output of B, and so needs to run
before B.

R=brettw@chromium.org
BUG=428576
Committed: https://crrev.com/44128e3c9ce524ae93343fcef7c875c32783e8e3
Cr-Commit-Position: refs/heads/master@{#302342}

[Dirk Pranke, 2014-10-30 02:22:19]

https://codereview.chromium.org/693443002/diff/1/tools/gn/ninja_copy_target_w...
File tools/gn/ninja_copy_target_writer.cc (right):

https://codereview.chromium.org/693443002/diff/1/tools/gn/ninja_copy_target_w...
tools/gn/ninja_copy_target_writer.cc:75:
WriteInputDepsStampAndGetDep(std::vector<const Target*>());
I just stole this from ninja_binary_target_writer; it's not clear to me if using
a stamp file for this is the right thing to do, though (vs. just writing out the
dependencies directly).

Also, it's not clear to me if this'll cause other dependencies to get written?
(I didn't write any tests for this yet or look at any existing tests).

https://codereview.chromium.org/693443002/diff/1/tools/gn/ninja_copy_target_w...
tools/gn/ninja_copy_target_writer.cc:97: // locally run command, so there is no
need for a toolchain dependency.
The comment above doesn't talk about order-only dependencies. Perhaps it should?

[cjhopman, 2014-10-30 16:52:54]

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

[cjhopman, 2014-10-30 16:52:54]

I'd be careful doing this. The inputs to a copy step are almost required to be
either non-build-generated or specified as the outputs of some other step. If
this isn't the case, in a clean build, ninja will complain that it doesn't know
how to create the input. So, whatever thing creates the files that are to be
copied, must list those files in its outputs. At that point, adding deps to the
copy target shouldn't actually be able to have any affect.

I'd rather copy throw a warning when someone does add deps to it.

[cjhopman, 2014-10-30 16:54:02]

As for the "almost required": there is a very hacky way around the requirement
that I don't think should ever be used.

[Nick Bray (chromium), 2014-10-30 17:18:00]

On 2014/10/30 16:54:02, cjhopman wrote:
> As for the "almost required": there is a very hacky way around the requirement
> that I don't think should ever be used.

deps for a copy already occurs in practice.
https://code.google.com/p/chromium/codesearch#chromium/src/native_client/buil...
The step it depends on, due to how the untar tool works, will delete everything
in the directory it is being copied to.  The dependency ensures the copy will
happen after the step that could destroy the output.  Sadly, not every tool
adheres to a strict input => output mapping, and in those cases dependancies are
needed to enforce sequencing.

Out of curiosity, does ninja must check for the existence of the output
immediately before the copy operation occurs, or just once in the beginning?

[cjhopman, 2014-10-30 17:19:43]

On 2014/10/30 17:18:00, Nick Bray (chromium) wrote:
> On 2014/10/30 16:54:02, cjhopman wrote:
> > As for the "almost required": there is a very hacky way around the
requirement
> > that I don't think should ever be used.
> 
> deps for a copy already occurs in practice.
>
https://code.google.com/p/chromium/codesearch#chromium/src/native_client/buil...
> The step it depends on, due to how the untar tool works, will delete
everything
> in the directory it is being copied to.  The dependency ensures the copy will
> happen after the step that could destroy the output.  Sadly, not every tool
> adheres to a strict input => output mapping, and in those cases dependancies
are
> needed to enforce sequencing.
> 
> Out of curiosity, does ninja must check for the existence of the output
> immediately before the copy operation occurs, or just once in the beginning?

For ninja, on startup, any file that appears as an input to some build action
must either already exist or appear as the output of some other build action.

[Dirk Pranke, 2014-10-30 17:26:47]

This isn't about the inputs to the copy; I agree with concerns there. 

This is about order-only dependencies. Namely, we need to be sure that some
other command runs (and completes) before the copy command, because the effects
of the other command stomp on this one.

Caveats:

(a) It's quite possible that the patch in question is the *wrong* way to fix the
problem I'm trying to address.

(b) It's possible that NaCl could be doing things in a smarter way. For example,
 it could have a single script that untars and then overlays files in what
appears to be a single step to GN and Ninja. 

However, it's not clear to me that the current NaCl approach is obviously bad,
and it seems like a logical use case that should be supported. Hence the bug ...

[brettw, 2014-10-30 17:32:49]

I think we should do this. It's not much different than allowing deps for
scripts, even though the script's "inputs"/"sources" should really express all
of the necessary dependencies.

Instead of disallowing deps, I think we should actually add a "gn check" check
that verifies dependencies that generate the given input files are listed
(assuming the input files are in the output directory, otherwise they're
probably checked-in sources). The reason for this is that the deps will cause a
visibility check. Otherwise, you can make copy steps dependent on other targets
that you theoretically shouldn't be able to depend on.

We should get a test for this change.

[Dirk Pranke, 2014-10-30 19:05:40]

On 2014/10/30 17:32:49, brettw wrote:
> I think we should do this. It's not much different than allowing deps for
> scripts, even though the script's "inputs"/"sources" should really express all
> of the necessary dependencies.

Just in case we're talking past each other here ... the specific thing I'm
trying to fix
cannot be fixed by listing the 'untar' as an input or a source to the copy. The
input
to the copy is a file that is checked in. 

The copy will succeed if run before or after the untar. The problem is that the
untar, 
if run after the copy, will undo the work of the copy, causing problems down the
road.

> Instead of disallowing deps, I think we should actually add a "gn check" check
> that verifies dependencies that generate the given input files are listed
> (assuming the input files are in the output directory, otherwise they're
> probably checked-in sources). The reason for this is that the deps will cause
a
> visibility check. Otherwise, you can make copy steps dependent on other
targets
> that you theoretically shouldn't be able to depend on.

I'll probably come talk to you in person about this suggestion; I have some
questions
that probably arise from me not understanding the implementation well enough.

> We should get a test for this change.

Of course.

[cjhopman, 2014-10-30 20:21:01]

On 2014/10/30 19:05:40, Dirk Pranke wrote:
> On 2014/10/30 17:32:49, brettw wrote:
> > I think we should do this. It's not much different than allowing deps for
> > scripts, even though the script's "inputs"/"sources" should really express
all
> > of the necessary dependencies.
> 
> Just in case we're talking past each other here ... the specific thing I'm
> trying to fix
> cannot be fixed by listing the 'untar' as an input or a source to the copy.
The
> input
> to the copy is a file that is checked in. 
> 
> The copy will succeed if run before or after the untar. The problem is that
the
> untar, 
> if run after the copy, will undo the work of the copy, causing problems down
the
> road.

Ah, got it. I strongly believe that build rules should all completely list their
outputs and that not doing that (and particularly having multiple actions modify
the same output file) is not a good idea (it's just really easy to get things
wrong, especially in ways that will non-deterministically work). That being
said, this does seem like a valid use of having deps in a copy action. The 'gn
check' check suggested by brett is also a good reason to support deps on copies.

> 
> > Instead of disallowing deps, I think we should actually add a "gn check"
check
> > that verifies dependencies that generate the given input files are listed
> > (assuming the input files are in the output directory, otherwise they're
> > probably checked-in sources). The reason for this is that the deps will
cause
> a
> > visibility check. Otherwise, you can make copy steps dependent on other
> targets
> > that you theoretically shouldn't be able to depend on.
> 
> I'll probably come talk to you in person about this suggestion; I have some
> questions
> that probably arise from me not understanding the implementation well enough.
> 
> > We should get a test for this change.
> 
> Of course.

[Dirk Pranke, 2014-10-30 21:25:42]

Tests and comment added, and description improved. 

I've filed crbug.com/428979 to add copy to the things 'gn check' checks.

Please take another look?

[brettw, 2014-10-31 19:56:57]

lgtm

https://codereview.chromium.org/693443002/diff/20001/tools/gn/ninja_copy_targ...
File tools/gn/ninja_copy_target_writer.cc (left):

https://codereview.chromium.org/693443002/diff/20001/tools/gn/ninja_copy_targ...
tools/gn/ninja_copy_target_writer.cc:83: //   output: copy input | foo.stamp
Seems like this was auto-rewrapping that messed up the formatting? We should
keep this on its own line indented.

[Dirk Pranke, 2014-10-31 20:02:33]

https://codereview.chromium.org/693443002/diff/20001/tools/gn/ninja_copy_targ...
File tools/gn/ninja_copy_target_writer.cc (left):

https://codereview.chromium.org/693443002/diff/20001/tools/gn/ninja_copy_targ...
tools/gn/ninja_copy_target_writer.cc:83: //   output: copy input | foo.stamp
On 2014/10/31 19:56:57, brettw wrote:
> Seems like this was auto-rewrapping that messed up the formatting? We should
> keep this on its own line indented.

Hm. I don't know what caused this, or the change on lines 101-102. Maybe I
auto-formatted something by accident. Will fix.

[Dirk Pranke, 2014-10-31 22:12:59]

The CQ bit was checked by dpranke@chromium.org

[commit-bot: I haz the power, 2014-10-31 22:13:43]

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

[commit-bot: I haz the power, 2014-10-31 23:01:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-31 23:02:14]

Patchset 3 (id:??) landed as
https://crrev.com/44128e3c9ce524ae93343fcef7c875c32783e8e3
Cr-Commit-Position: refs/heads/master@{#302342}