Add output reference to gen written files.

Add output reference to gen written files.

By adding the outputs written at gn gen time to the build.ninja representation the build tool (ninja) can know how to recreate them if and when necessary.

For example, if one or more of the <blah>.tmp files is removed, even as part of an entire hierarchy/subtree currently the build fails because ninja does not know how to re-make it.

BUG=469621
TEST=Build time only. See https://code.google.com/p/chromium/issues/detail?id=469621#c3

Committed: https://crrev.com/517ac52f86903beb206b19e6ddeedbc709b253a1
Cr-Commit-Position: refs/heads/master@{#338929}

[Peter Mayo, 2015-07-02 22:32:14]

petermayo@chromium.org changed reviewers:
+ brettw@chromium.org, dpranke@chromium.org

[Peter Mayo, 2015-07-02 22:32:15]

PTAL.
It seems this change doesn't change the existing unittests (they still pass).
It's not immediately obvious how best to add unittests for this, suggestions
welcome.


Contrast with https://codereview.chromium.org/1226933002/

[tfarina, 2015-07-03 00:31:04]

Could you describe in the CL description what problem this solves? And possibly
describe the steps to reproduce the problem, so it can be verified manually?

[Peter Mayo, 2015-07-03 14:08:41]

On 2015/07/03 00:31:04, tfarina wrote:
> Could you describe in the CL description what problem this solves? 
I thought I had, I'll expand and simplify.

> And possibly
> describe the steps to reproduce the problem, so it can be verified manually?

[Peter Mayo, 2015-07-03 14:24:33]

On 2015/07/03 14:08:41, Peter Mayo wrote:
> On 2015/07/03 00:31:04, tfarina wrote:
> > Could you describe in the CL description what problem this solves? 
> I thought I had, I'll expand and simplify.
> 
> > And possibly
> > describe the steps to reproduce the problem, so it can be verified manually?

Updated.

Contrast CL is incorrect in initial email,
https://codereview.chromium.org/1222693002/

(one more 2, one less 3; I miss good cut and paste)

[tfarina, 2015-07-03 16:04:09]

When this (removing <blah>.tmp) occurrs in practice? Is this a bot thing?

[Peter Mayo, 2015-07-03 16:29:03]

On 2015/07/03 16:04:09, tfarina wrote:
> When this (removing <blah>.tmp) occurrs in practice? Is this a bot thing?

I hit it when debugging and understanding some new configurations, in particular
around the processing of blink event interfaces where the transformation between
file/pathnames and symols was going wrong, and I was trying to understand the
entire chain.

I've not seen it on bots, but I can see them getting just as stuck as developers
if they ever do get into the situation.

I'm not sure the answer to this belongs in the CL, so much as the discussion on
the bug.

[Peter Mayo, 2015-07-06 18:40:57]

This change interacts badly with some of the other features of gen:

ninja: Entering directory `out/linked'
ninja explain: output
.../src/out/linked/obj/content/shell/content_shell_resources_grit_expected_outputs.txt
older than most recent input .../src/third_party/icu/BUILD.gn (1436204211 vs
1436205519)
[1/1] .../src/out/local/gn --root=.../src -q --v gen //out/linked/
ninja explain: output
.../src/out/linked/obj/content/shell/content_shell_resources_grit_expected_outputs.txt
older than most recent input .../src/third_party/icu/BUILD.gn (1436204211 vs
1436205519)


In particular because it has a rule that outputs the file, it tries to use it to
bring it up to date, but since the write_file doesn't update the timestamp when
the contents is correct by default it doesn't change the situation and the gen
step is re-run indefinitely.

This happens when some other dependency gets built.  Note that if the change
caused the contents to change then this is not a practical problem, but does get
the correct actions taken.

In the previous situation where there was no rule to generate it, it appears
ninja just ignores the staleness.  This is arguably an error.

[Peter Mayo, 2015-07-06 21:03:24]

https://codereview.chromium.org/1220223002/diff/20001/tools/gn/ninja_build_wr...
File tools/gn/ninja_build_writer.cc (right):

https://codereview.chromium.org/1220223002/diff/20001/tools/gn/ninja_build_wr...
tools/gn/ninja_build_writer.cc:147: out_ << "  restat = 1\n\n";
This takes care of the problem we introduced in having output files on the build
edge below that aren't updated unless modified.

[brettw, 2015-07-06 21:28:19]

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

[brettw, 2015-07-06 21:28:19]

I don't think this is quite right.

Your Ninja file makes semantic sense, but it's not really the way Ninja works.
When Ninja starts, it first checks the dependencies of build.ninja. If it's out
of date, it runs the command and reinitializes itself.

Your patch is instead putting the build regeneration step as part of a general
rule in the tree. This will make GN run again in the middle of your ninja build.
The result will be the ninja files won't match the current build that Ninja is
doing, and it won't detect the changes. We don't want to be in this state. Plus,
Ninja could run GN more than once in parallel and they could actually make a
corrupted build.

[Peter Mayo, 2015-07-06 21:58:05]

On 2015/07/06 21:28:19, brettw wrote:
> I don't think this is quite right.
> 
> Your Ninja file makes semantic sense, but it's not really the way Ninja works.
> When Ninja starts, it first checks the dependencies of build.ninja. If it's
out
> of date, it runs the command and reinitializes itself.
> 
> Your patch is instead putting the build regeneration step as part of a general
> rule in the tree. This will make GN run again in the middle of your ninja
build.
> The result will be the ninja files won't match the current build that Ninja is
> doing, and it won't detect the changes. We don't want to be in this state.

Are you sure?  From the behavior on failure and a minimal repro of the looping
behavior it appears to try to get all of the outputs of the generator = 1
rule(s?).
In particular tacking an orphan output into the same build edge as build.ninja
will cause a loop in the gen stage, even if it is not referenced by any of the
generator = 0 rules/targets.


> Plus, Ninja could run GN more than once in parallel and they could actually
make a
> corrupted build.

If ninja would run multiple copies of an edge (or even just multiple output
edges)
in parallel wouldn't almost all (such) builds be potentially compromised?
Compiles
and links are usually parallel unsafe, and rarely atomic or idempotent for the
same
outputs.

I definitely agree these possibilities should be eliminated before landing such
a change.

[brettw, 2015-07-06 22:02:43]

You're right it won't get run in parallel since there's only one build rule for
all outputs. I'll let Nico comment on the generator stuff, I'm not very familiar
with it.

[Nico, 2015-07-07 22:32:55]

I don't understand what I'm supposed to comment on here.

https://codereview.chromium.org/1220223002/diff/20001/tools/gn/ninja_build_wr...
File tools/gn/ninja_build_writer.cc (right):

https://codereview.chromium.org/1220223002/diff/20001/tools/gn/ninja_build_wr...
tools/gn/ninja_build_writer.cc:156: out_ << " " <<
FilePathToUTF8(build_settings_->GetFullPath(written_file));
These are inputs, shouldn't they be on the rhs of the :? (probably as implicit
dependencies)

[Nico, 2015-07-07 22:34:41]

Ah, peter sent me an off-list email with a short explanation. I think the change
is correct. (generator rules run before the actual build) I don't have an
opinion if it's a good idea.

https://codereview.chromium.org/1220223002/diff/20001/tools/gn/ninja_build_wr...
File tools/gn/ninja_build_writer.cc (right):

https://codereview.chromium.org/1220223002/diff/20001/tools/gn/ninja_build_wr...
tools/gn/ninja_build_writer.cc:156: out_ << " " <<
FilePathToUTF8(build_settings_->GetFullPath(written_file));
On 2015/07/07 22:32:55, Nico wrote:
> These are inputs, shouldn't they be on the rhs of the :? (probably as implicit
> dependencies)

Nevermind.

[Peter Mayo, 2015-07-13 16:03:34]

petermayo@chromium.org changed reviewers:
- thakis@chromium.org

[Peter Mayo, 2015-07-13 16:03:35]

I maintain that this is a more accurate, higher quality fix than the previously
committed work-around - adding it as a dependency.

But since the bug is no-longer active, I'm going to move on to other things.

[Peter Mayo, 2015-07-14 16:08:43]

On 2015/07/13 16:03:35, Peter Mayo wrote:
> I maintain that this is a more accurate, higher quality fix than the
previously
> committed work-around - adding it as a dependency.
> 
> But since the bug is no-longer active, I'm going to move on to other things.

I'm going to reverse myself here given some offline discussion and the bug
reopening.

I've re-opened the issue.

Ping: let's continue the discussion.

Since this only changes a generator rule there is no parallelism difference -
ninja will only run the rule at the start of a build.

One unfortunate effect of either approach is that ninja will re-run the rule to
stat the outputs if a build file is modified even if the gen was run again
manually.

[Peter Mayo, 2015-07-14 19:33:08]

Remove dependency hack at the same time

[brettw, 2015-07-15 20:55:31]

Since Nico says this is OK, LGTM.

[Peter Mayo, 2015-07-15 21:47:27]

The CQ bit was checked by petermayo@chromium.org

[commit-bot: I haz the power, 2015-07-15 21:49:53]

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

[commit-bot: I haz the power, 2015-07-15 22:22:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-15 22:24:32]

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

[Dirk Pranke, 2015-07-27 23:08:34]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1255113004/ by dpranke@chromium.org.

The reason for reverting is: Looks like this doesn't quite work on Windows:

http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/....