GN: Add -Wl,-z defs on linux and fix errors

GN: Add -Wl,-z defs on linux and fix errors

This tells the linker to resolve symbols for shared libraries at build
time, not run time. This alerts developers much earlier that their
dependencies are underspecified.

BUG=371125
Committed: https://crrev.com/4359db72cde4fe4eeb4125d9a88af765fcdf39f1
Cr-Commit-Position: refs/heads/master@{#312317}

[jamesr, 2015-01-15 23:25:00]

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

[jamesr, 2015-01-15 23:29:46]

D'oh, this requires a change in ffmpeg too.

[Nico, 2015-01-15 23:35:07]

lgtm, looks plausible

Sorry, forgot about gn when I did the ffmpeg change :-(

I can do the ffmpeg change if you want.

I needed to add 1 dep to 'wtf' too (in gyp), is that not needed in gn?

https://codereview.chromium.org/843583006/diff/1/build/config/compiler/BUILD.gn
File build/config/compiler/BUILD.gn (right):

https://codereview.chromium.org/843583006/diff/1/build/config/compiler/BUILD....
build/config/compiler/BUILD.gn:304: "-Wl,-z,defs",
Did you try an android build?

https://codereview.chromium.org/843583006/diff/1/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/843583006/diff/1/chrome/renderer/BUILD.gn#new...
chrome/renderer/BUILD.gn:122: if (!enable_webrtc) {
…looks like you did.

https://codereview.chromium.org/843583006/diff/1/content/renderer/BUILD.gn
File content/renderer/BUILD.gn (right):

https://codereview.chromium.org/843583006/diff/1/content/renderer/BUILD.gn#ne...
content/renderer/BUILD.gn:121: print("webrtc enabled")
print("remove this")

https://codereview.chromium.org/843583006/diff/1/gpu/config/BUILD.gn
File gpu/config/BUILD.gn (right):

https://codereview.chromium.org/843583006/diff/1/gpu/config/BUILD.gn#newcode89
gpu/config/BUILD.gn:89: "//ui/gfx/x/",
no trailing / (?)

https://codereview.chromium.org/843583006/diff/1/ui/aura/BUILD.gn
File ui/aura/BUILD.gn (right):

https://codereview.chromium.org/843583006/diff/1/ui/aura/BUILD.gn#newcode115
ui/aura/BUILD.gn:115: "//ui/events/devices",
Are both of these only needed if use_x11? (not sure if there are non-x11 linux
builds on gn yet)

https://codereview.chromium.org/843583006/diff/1/ui/keyboard/BUILD.gn
File ui/keyboard/BUILD.gn (right):

https://codereview.chromium.org/843583006/diff/1/ui/keyboard/BUILD.gn#newcode48
ui/keyboard/BUILD.gn:48: "//skia",
What's this for?

[jamesr, 2015-01-15 23:40:26]

ffmpeg patch up here: https://gerrit.chromium.org/gerrit/#/c/73516/

[jamesr, 2015-01-15 23:41:24]

On 2015/01/15 23:35:07, Nico wrote:
> lgtm, looks plausible
> 
> Sorry, forgot about gn when I did the ffmpeg change :-(
> 
> I can do the ffmpeg change if you want.
> 
> I needed to add 1 dep to 'wtf' too (in gyp), is that not needed in gn?
> 
>
https://codereview.chromium.org/843583006/diff/1/build/config/compiler/BUILD.gn
> File build/config/compiler/BUILD.gn (right):
> 
>
https://codereview.chromium.org/843583006/diff/1/build/config/compiler/BUILD....
> build/config/compiler/BUILD.gn:304: "-Wl,-z,defs",
> Did you try an android build?

No, and the trybots tell me that the android build doesn't work.

> 
> https://codereview.chromium.org/843583006/diff/1/chrome/renderer/BUILD.gn
> File chrome/renderer/BUILD.gn (right):
> 
>
https://codereview.chromium.org/843583006/diff/1/chrome/renderer/BUILD.gn#new...
> chrome/renderer/BUILD.gn:122: if (!enable_webrtc) {
> …looks like you did.

In GN webrtc is disabled on linux (it's disabled everywhere, afaict)

[jamesr, 2015-01-16 01:53:45]

https://codereview.chromium.org/843583006/diff/1/content/renderer/BUILD.gn
File content/renderer/BUILD.gn (right):

https://codereview.chromium.org/843583006/diff/1/content/renderer/BUILD.gn#ne...
content/renderer/BUILD.gn:121: print("webrtc enabled")
On 2015/01/15 23:35:07, Nico wrote:
> print("remove this")

Done.

[Nico, 2015-01-16 01:57:50]

https://codereview.chromium.org/843583006/diff/20001/device/vibration/android...
File device/vibration/android/BUILD.gn (right):

https://codereview.chromium.org/843583006/diff/20001/device/vibration/android...
device/vibration/android/BUILD.gn:16: defines = [
"DEVICE_VIBRATION_IMPLEMENTATION" ]
For a source_set? Shouldn't this be where the component for this is?

[jamesr, 2015-01-16 02:00:45]

PTAL. ffmpeg roll is pending, will submit new batch of tryjobs once it lands

https://codereview.chromium.org/843583006/diff/1/gpu/config/BUILD.gn
File gpu/config/BUILD.gn (right):

https://codereview.chromium.org/843583006/diff/1/gpu/config/BUILD.gn#newcode89
gpu/config/BUILD.gn:89: "//ui/gfx/x/",
On 2015/01/15 23:35:07, Nico wrote:
> no trailing / (?)

Done.

https://codereview.chromium.org/843583006/diff/1/ui/aura/BUILD.gn
File ui/aura/BUILD.gn (right):

https://codereview.chromium.org/843583006/diff/1/ui/aura/BUILD.gn#newcode115
ui/aura/BUILD.gn:115: "//ui/events/devices",
On 2015/01/15 23:35:07, Nico wrote:
> Are both of these only needed if use_x11? (not sure if there are non-x11 linux
> builds on gn yet)

The references are in window_tree_host_x11.cc so I believe the dependency is
X11-only

https://codereview.chromium.org/843583006/diff/1/ui/keyboard/BUILD.gn
File ui/keyboard/BUILD.gn (right):

https://codereview.chromium.org/843583006/diff/1/ui/keyboard/BUILD.gn#newcode48
ui/keyboard/BUILD.gn:48: "//skia",
On 2015/01/15 23:35:07, Nico wrote:
> What's this for? 

https://code.google.com/p/chromium/codesearch#chromium/src/ui/keyboard/keyboa...

this does manipulations with SkRect and SkPath

(this file should IWYU and #include the SkRect/SkPath headers, but that's kind
of outside scope)

[Nico, 2015-01-16 02:11:24]

Did you see the vibration comment above?

[jamesr, 2015-01-16 02:13:18]

On 2015/01/16 01:57:50, Nico wrote:
>
https://codereview.chromium.org/843583006/diff/20001/device/vibration/android...
> File device/vibration/android/BUILD.gn (right):
> 
>
https://codereview.chromium.org/843583006/diff/20001/device/vibration/android...
> device/vibration/android/BUILD.gn:16: defines = [
> "DEVICE_VIBRATION_IMPLEMENTATION" ]
> For a source_set? Shouldn't this be where the component for this is?

Sorry, it is set where the component is.  This source_set defines a symbol that
should be exported:

https://code.google.com/p/chromium/codesearch#chromium/src/device/vibration/a...

and this source_set is linked into the component.  The definition of this symbol
needs to see the export annotation, so it needs the _IMPL macro set.

[Nico, 2015-01-16 02:17:42]

On 2015/01/16 02:13:18, jamesr wrote:
> On 2015/01/16 01:57:50, Nico wrote:
> >
>
https://codereview.chromium.org/843583006/diff/20001/device/vibration/android...
> > File device/vibration/android/BUILD.gn (right):
> > 
> >
>
https://codereview.chromium.org/843583006/diff/20001/device/vibration/android...
> > device/vibration/android/BUILD.gn:16: defines = [
> > "DEVICE_VIBRATION_IMPLEMENTATION" ]
> > For a source_set? Shouldn't this be where the component for this is?
> 
> Sorry, it is set where the component is.  This source_set defines a symbol
that
> should be exported:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/device/vibration/a...
> 
> and this source_set is linked into the component.  The definition of this
symbol
> needs to see the export annotation, so it needs the _IMPL macro set.

Hm, does gn do the Right Thing for this? Is this define set for all compilations
in the component, or only for the compilation of the cc file in the source_set?

I kind of feel that files that need a IMPL define must not be a source_set, they
must be a component (like it is in device/vibration/vibration.gyp). Else linking
will likely behave weirdly on at least one platform.

[jamesr, 2015-01-16 06:07:21]

source_set is designed specifically for this use case (primarily the mess in
//content). Best practice is to visibility-restrict the source_set to the
component to prevent people accidentally lining in only part of a component. I
could do that here although it's kinda beside the point of this patch.

[Nico, 2015-01-16 17:48:35]

On 2015/01/16 06:07:21, jamesr wrote:
> source_set is designed specifically for this use case (primarily the mess in
> //content). Best practice is to visibility-restrict the source_set to the
> component to prevent people accidentally lining in only part of a component. I
> could do that here although it's kinda beside the point of this patch.

Thanks for explaining! lgtm then.

[jamesr, 2015-01-16 20:55:46]

Brett - please review for build/config/ and top-level OWNERS.

[jamesr, 2015-01-16 21:18:31]

One more ffmpeg patch for precise here:
https://gerrit.chromium.org/gerrit/#/c/73528/

[jamesr, 2015-01-16 22:55:19]

Another ffmpeg roll here that should green up the gn linux bots:
https://codereview.chromium.org/851323003/

[Nico, 2015-01-20 18:54:59]

land land land

[jamesr, 2015-01-20 18:59:04]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2015-01-20 19:00:18]

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

[commit-bot: I haz the power, 2015-01-20 19:09:22]

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

[commit-bot: I haz the power, 2015-01-20 19:09:24]

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

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

[Nico, 2015-01-20 19:46:00]

On 2015/01/20 19:09:24, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

(looks like the patch just needs rebasing)

[brettw, 2015-01-20 20:22:33]

lgtm

[jamesr, 2015-01-20 21:03:38]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2015-01-20 21:04:40]

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

[commit-bot: I haz the power, 2015-01-20 21:12:18]

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

[commit-bot: I haz the power, 2015-01-20 21:12:20]

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

[jamesr, 2015-01-20 21:36:50]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2015-01-20 21:38:06]

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

[commit-bot: I haz the power, 2015-01-21 00:08:15]

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

[commit-bot: I haz the power, 2015-01-21 00:08:17]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)

[jamesr, 2015-01-21 00:30:30]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2015-01-21 00:30:59]

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

[commit-bot: I haz the power, 2015-01-21 00:32:21]

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

[commit-bot: I haz the power, 2015-01-21 00:32:24]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...)
  ios_rel_device_ninja_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[jamesr, 2015-01-21 00:48:59]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2015-01-21 00:50:33]

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

[commit-bot: I haz the power, 2015-01-21 02:46:07]

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

[commit-bot: I haz the power, 2015-01-21 02:46:09]

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

[jamesr, 2015-01-21 03:12:59]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2015-01-21 03:14:30]

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

[commit-bot: I haz the power, 2015-01-21 04:14:27]

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

[commit-bot: I haz the power, 2015-01-21 04:14:29]

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

[jamesr, 2015-01-21 04:26:03]

The CQ bit was checked by jamesr@chromium.org

[commit-bot: I haz the power, 2015-01-21 04:27:34]

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

[commit-bot: I haz the power, 2015-01-21 12:40:33]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2015-01-21 12:41:25]

Patchset 6 (id:??) landed as
https://crrev.com/4359db72cde4fe4eeb4125d9a88af765fcdf39f1
Cr-Commit-Position: refs/heads/master@{#312317}