gn: Fix build issues blocking gfx from being built

gn: Fix build issues blocking gfx from being built

BUG=354261
R=brettw@chromium.org, brettw

Committed: https://chromium.googlesource.com/chromium/src/+/b83a0bd983bdcdcbc14559f263e7e7d8145c976c

[ckocagil, 2014-09-21 02:05:06]

https://codereview.chromium.org/589753005/diff/1/build/toolchain/win/BUILD.gn
File build/toolchain/win/BUILD.gn (right):

https://codereview.chromium.org/589753005/diff/1/build/toolchain/win/BUILD.gn...
build/toolchain/win/BUILD.gn:114: link_output = dllname
These should be explicitly stated, otherwise it uses outputs[0] (dllname) for
both outputs, so it will try linking base.dll instead of base.dll.lib.

https://codereview.chromium.org/589753005/diff/1/tools/gn/ninja_binary_target...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/589753005/diff/1/tools/gn/ninja_binary_target...
tools/gn/ninja_binary_target_writer.cc:239:
implicit_deps.push_back(cur->link_output_file());
We should implicitly depend on both the dll and the lib.

https://codereview.chromium.org/589753005/diff/1/tools/gn/ninja_binary_target...
tools/gn/ninja_binary_target_writer.cc:241:
solibs.push_back(cur->dependency_output_file());
We should output the .lib here, so it can be linked.

https://codereview.chromium.org/589753005/diff/1/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/589753005/diff/1/tools/gn/target.cc#newcode325
tools/gn/target.cc:325: this, tool, tool->depend_output());
I assume this was a copy/paste error.

[brettw, 2014-09-21 02:55:01]

https://codereview.chromium.org/589753005/diff/1/build/toolchain/win/BUILD.gn
File build/toolchain/win/BUILD.gn (right):

https://codereview.chromium.org/589753005/diff/1/build/toolchain/win/BUILD.gn...
build/toolchain/win/BUILD.gn:114: link_output = dllname
Actually, I think both of these should be the .lib. This trick with doing a
different dependency and link output file doesn't work on Windows. Our link
lines should basically match the GYP ones.

https://codereview.chromium.org/589753005/diff/1/third_party/libpng/BUILD.gn
File third_party/libpng/BUILD.gn (right):

https://codereview.chromium.org/589753005/diff/1/third_party/libpng/BUILD.gn#...
third_party/libpng/BUILD.gn:80: forward_dependent_configs_from = [
":libpng_sources" ]
I'm trying to get rid of this. Replace "deps" with "public_deps" and it means
the same things as deps + forward. Same below.

https://codereview.chromium.org/589753005/diff/1/tools/gn/ninja_binary_target...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/589753005/diff/1/tools/gn/ninja_binary_target...
tools/gn/ninja_binary_target_writer.cc:241:
solibs.push_back(cur->dependency_output_file());
I *think* both of these lines were correct before. Did this change a test?

On Linux, it should look like:
  foo.exe: ... | mylib.so.TOC
and this shouldn't be hit on Windows (since the link & depend output files
should be the same).

https://codereview.chromium.org/589753005/diff/1/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/589753005/diff/1/tools/gn/target.cc#newcode325
tools/gn/target.cc:325: this, tool, tool->depend_output());
On 2014/09/21 02:05:05, ckocagil wrote:
> I assume this was a copy/paste error.

This should be fixed if you merge.

[ckocagil, 2014-09-21 11:47:27]

https://codereview.chromium.org/589753005/diff/1/build/toolchain/win/BUILD.gn
File build/toolchain/win/BUILD.gn (right):

https://codereview.chromium.org/589753005/diff/1/build/toolchain/win/BUILD.gn...
build/toolchain/win/BUILD.gn:114: link_output = dllname
On 2014/09/21 02:55:00, brettw wrote:
> Actually, I think both of these should be the .lib. This trick with doing a
> different dependency and link output file doesn't work on Windows. Our link
> lines should basically match the GYP ones.

Done. But could you elaborate on how it doesn't work on Win? Does it not
correctly rebuild if either/both linker outputs are deleted?

https://codereview.chromium.org/589753005/diff/1/third_party/libpng/BUILD.gn
File third_party/libpng/BUILD.gn (right):

https://codereview.chromium.org/589753005/diff/1/third_party/libpng/BUILD.gn#...
third_party/libpng/BUILD.gn:80: forward_dependent_configs_from = [
":libpng_sources" ]
On 2014/09/21 02:55:00, brettw wrote:
> I'm trying to get rid of this. Replace "deps" with "public_deps" and it means
> the same things as deps + forward. Same below.

Done.

https://codereview.chromium.org/589753005/diff/1/tools/gn/ninja_binary_target...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/589753005/diff/1/tools/gn/ninja_binary_target...
tools/gn/ninja_binary_target_writer.cc:241:
solibs.push_back(cur->dependency_output_file());
On 2014/09/21 02:55:00, brettw wrote:
> I *think* both of these lines were correct before. Did this change a test?
> 
> On Linux, it should look like:
>   foo.exe: ... | mylib.so.TOC
> and this shouldn't be hit on Windows (since the link & depend output files
> should be the same).

Done.

https://codereview.chromium.org/589753005/diff/1/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/589753005/diff/1/tools/gn/target.cc#newcode325
tools/gn/target.cc:325: this, tool, tool->depend_output());
On 2014/09/21 02:55:01, brettw wrote:
> On 2014/09/21 02:05:05, ckocagil wrote:
> > I assume this was a copy/paste error.
> 
> This should be fixed if you merge.

I don't see a change on ToT:
https://chromium.googlesource.com/chromium/src/+/master/tools/gn/target.cc

[brettw, 2014-09-22 04:45:56]

https://codereview.chromium.org/589753005/diff/1/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/589753005/diff/1/tools/gn/target.cc#newcode325
tools/gn/target.cc:325: this, tool, tool->depend_output());
Oh whoops, haven't checked it in yet. See
https://codereview.chromium.org/583363003/ Since mine has a unit test, I'd
prefer it.

[ckocagil, 2014-09-22 16:39:33]

https://codereview.chromium.org/589753005/diff/1/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/589753005/diff/1/tools/gn/target.cc#newcode325
tools/gn/target.cc:325: this, tool, tool->depend_output());
Okay, I removed the change here.

[brettw, 2014-09-22 16:47:58]

lgtm

[ckocagil, 2014-09-22 16:49:10]

The CQ bit was checked by ckocagil@chromium.org

[commit-bot: I haz the power, 2014-09-22 16:50:20]

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

[commit-bot: I haz the power, 2014-09-22 17:00:51]

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

[commit-bot: I haz the power, 2014-09-22 17:00:52]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[ckocagil, 2014-09-22 17:06:15]

Do you globally own BUILD.gn files? If so I'll manually land after the try bots
turn green.

[ckocagil, 2014-09-22 19:33:13]

(assuming yes)

[commit-bot: I haz the power, 2014-09-22 19:45:43]

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

[ckocagil, 2014-09-22 19:45:55]

Committed patchset #3 (id:40001) manually as b83a0bd.

[Ilya Sherman, 2014-09-23 05:39:20]

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

[Ilya Sherman, 2014-09-23 05:39:21]

FYI, this CL seems to have broken libpng compilation on Mac.  Here's the output
I get:

22:38 isherman@isherman-macpro /Volumes/Work/chrome/src
$ gn gen out/gn
Done. Wrote 1522 targets from 522 files in 1727ms
22:38 isherman@isherman-macpro /Volumes/Work/chrome/src
$ ninja -C out/gn libpng
ninja: Entering directory `out/gn'
[17/17] LIBTOOL-STATIC obj/third_party/libpng/libpng.a
FAILED: rm -f obj/third_party/libpng/libpng.a && ./gyp-mac-tool filter-libtool
libtool -static -o obj/third_party/libpng/libpng.a 
error:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool:
no files specified
Usage:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool
-static [-] file [...] [-filelist listfile[,dirname]] [-arch_only arch] [-sacLT]
Usage:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/libtool
-dynamic [-] file [...] [-filelist listfile[,dirname]] [-arch_only arch] [-o
output] [-install_name name] [-compatibility_version #] [-current_version #]
[-seg1addr 0x#] [-segs_read_only_addr 0x#] [-segs_read_write_addr 0x#]
[-seg_addr_table <filename>] [-seg_addr_table_filename <file_system_path>]
[-all_load] [-noall_load]
ninja: build stopped: subcommand failed.