GN: Generate error if multiple rules generate same file

GN: Generate error if multiple rules generate same file

This will pretty much always result in a bad build but is only a warning
in ninja.

Committed: https://crrev.com/598f6a85bb66f2e5ce5c04b43de9786aea55a089
Cr-Commit-Position: refs/heads/master@{#297077}

[jamesr, 2014-09-25 07:12:30]

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

[jamesr, 2014-09-25 07:12:30]

Example of the error this produces with this patch (which results in two
components with the name 'gpu'):

diff --git a/webkit/common/gpu/BUILD.gn b/webkit/common/gpu/BUILD.gn
index cecbd9d..150789c 100644
--- a/webkit/common/gpu/BUILD.gn
+++ b/webkit/common/gpu/BUILD.gn
@@ -4,8 +4,6 @@
 
 # GYP version: webkit/common/gpu/webkit_gpu.gyp:webkit_gpu
 component("gpu") {
-  output_name = "webkit_gpu"
-
   sources = [
     "context_provider_in_process.cc",
     "context_provider_in_process.h",

$ ./out/Release/gn gen out_gn/Component
ERROR Duplicate rules for output file ./libgpu.dylib.TOC

Unfortunately there's no location since the output name isn't available until
after we do expansions, so it's hard to get the original location back.  But
this is still better than getting a warning in ninja that everyone will ignore.

[brettw, 2014-09-25 21:05:50]

Do you have timings? I'm concerned that this will be ~25K more lock calls. One
thing we can do is pass a vector of OutputFiles, since in the binary target
case, we're producing that vector anyway and will only need to lock once for the
entire thing.

[jamesr, 2014-09-26 04:56:58]

Without this patch on my z620:

$ ./out/Release/gn gen out_gn/Component
Done. Wrote 1620 targets from 552 files in 481ms
jamesr@beemo:/hd2/chrome/src$ ./out/Release/gn gen out_gn/Component
Done. Wrote 1620 targets from 552 files in 483ms
jamesr@beemo:/hd2/chrome/src$ ./out/Release/gn gen out_gn/Component
Done. Wrote 1620 targets from 552 files in 474ms
jamesr@beemo:/hd2/chrome/src$ ./out/Release/gn gen out_gn/Component
Done. Wrote 1620 targets from 552 files in 475ms
jamesr@beemo:/hd2/chrome/src$ ./out/Release/gn gen out_gn/Component
Done. Wrote 1620 targets from 552 files in 480ms

with this patch:

$ ./out/Release/gn gen out_gn/Component
Done. Wrote 1620 targets from 552 files in 493ms
jamesr@beemo:/hd2/chrome/src$ ./out/Release/gn gen out_gn/Component
Done. Wrote 1620 targets from 552 files in 506ms
jamesr@beemo:/hd2/chrome/src$ ./out/Release/gn gen out_gn/Component
Done. Wrote 1620 targets from 552 files in 491ms
jamesr@beemo:/hd2/chrome/src$ ./out/Release/gn gen out_gn/Component
Done. Wrote 1620 targets from 552 files in 492ms
jamesr@beemo:/hd2/chrome/src$ ./out/Release/gn gen out_gn/Component
Done. Wrote 1620 targets from 552 files in 498ms

So roughly 4% slower.  That seems like too much, I'll try to speed it up a bit.

[brettw, 2014-09-26 05:01:29]

Also, I'm not actually that concerned about files from different targets mapping
to the same object files. They go in different dirs and are prefixed by the
target name.

The more important things are making sure the same target doesn't have two
object files, and there aren't targets with the same final output names.

The object files are easy to check. We can check the target outputs when writing
the main build.ninja files (we already have the full list of targets there and
can do it without locking).

[jamesr, 2014-09-26 05:21:27]

Ah, that's much better. The error message kind of sucks though:

ERROR Duplicate rules for libgpu.so.TOC
ERROR Couldn't open toolchain buildfile(s) for writing

know how to fix it?  Guess the return value of NinjaBuildWriter could be
richer...

[brettw, 2014-09-26 18:01:20]

Also, this doesn't check the object files within the same target like the
previous version did. Do you still want to do that?

https://codereview.chromium.org/455193003/diff/60001/tools/gn/ninja_build_wri...
File tools/gn/ninja_build_writer.cc (right):

https://codereview.chromium.org/455193003/diff/60001/tools/gn/ninja_build_wri...
tools/gn/ninja_build_writer.cc:95: bool NinjaBuildWriter::Run() {
Yeah, this should probably take an Err* out param.

[jamesr, 2014-09-26 20:55:06]

PTAL.

I think multiple sources in the same target should also be an error but will
address separately.

[brettw, 2014-09-26 23:48:09]

lgtm

[jamesr, 2014-09-26 23:49:44]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2014-09-26 23:51:54]

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

[commit-bot: I haz the power, 2014-09-26 23:57:08]

Committed patchset #5 (id:80001) as d993334bb1d1afa16e4e4218e735652fcff624eb

[commit-bot: I haz the power, 2014-09-26 23:57:54]

Patchset 5 (id:??) landed as
https://crrev.com/598f6a85bb66f2e5ce5c04b43de9786aea55a089
Cr-Commit-Position: refs/heads/master@{#297077}