Fix errors found in Mac gn component builds

Fix errors found in Mac gn component builds

Mac gn component build fail because of missing frameworks and missing some other
dependencies, and occasionally file list out of sync with the gyp counterpart:
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_dbg/builds/321/steps/compile/logs/stdio

For instance, the frameworks linked in the components that sync depends on is not
automatically linked into sync, thus revealing the issue.

Committed: https://crrev.com/ae8297db22a6ca96652d42bd0cdc3deff17aba4a
Cr-Commit-Position: refs/heads/master@{#325479}

[Jiang Jiang, 2015-04-15 06:14:25]

jiangj@opera.com changed reviewers:
+ dpranke@google.com

[Jiang Jiang, 2015-04-15 06:16:19]

libblink_platform.dylib linking issue should be fixed by
https://codereview.chromium.org/1056373003/

[Dirk Pranke, 2015-04-15 15:48:00]

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

[Dirk Pranke, 2015-04-15 15:48:02]

I would reword your CL description to be a little clearer.

You got the failure because that try job ran during a brief window when the
debug mac builds were doing component builds; normally they do static builds.

So, you're fixing component build failures, but the debug build is normally
green (though, once your change lands, we can presumably turn component on).

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/BUILD.gn (right):

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/BUILD.gn:172: "OpenGL.framework",
it seems like these two libraries are often needed by multiple targets in
different directories; maybe we should make them a separate target so that they
can be pulled in via deps?

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer_ser...
File gpu/command_buffer_service.gypi (right):

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer_ser...
gpu/command_buffer_service.gypi:150:
'$(SDKROOT)/System/Library/Frameworks/OpenGL.framework',
why is the full path needed here if it's not needed in
gpu/command_buffer/service ?

https://codereview.chromium.org/1087873003/diff/100001/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/1087873003/diff/100001/sync/sync.gyp#newcode468
sync/sync.gyp:468:
'$(SDKROOT)/System/Library/Frameworks/SystemConfiguration.framework',
same question ... why do we need the full path here?

[Jiang Jiang, 2015-04-15 16:17:20]

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/BUILD.gn (right):

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/BUILD.gn:172: "OpenGL.framework",
On 2015/04/15 15:48:01, Dirk Pranke wrote:
> it seems like these two libraries are often needed by multiple targets in
> different directories; maybe we should make them a separate target so that
they
> can be pulled in via deps?

I'm not sure about that, what's needed here is completely based on the sources
of this target itself, for instance here gles2_cmd_decoder.cc is in the sources
list, that happens to use both IOSurface and OpenGL frameworks, thus we are
adding them. For other places maybe only one is used, since using one does not
necessarily mean you have to use the other.

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer_ser...
File gpu/command_buffer_service.gypi (right):

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer_ser...
gpu/command_buffer_service.gypi:150:
'$(SDKROOT)/System/Library/Frameworks/OpenGL.framework',
On 2015/04/15 15:48:01, Dirk Pranke wrote:
> why is the full path needed here if it's not needed in
> gpu/command_buffer/service ?

I'm not sure what you are referring to, from what I can tell it seems that in
gyp we always use the full path with $(SDKROOT) while in gn we always use the
framework name only, would be tricky to see how we link to
$(SDKROOT)/System/Library/PrivateFrameworks/ in gn though.

https://codereview.chromium.org/1087873003/diff/100001/sync/sync.gyp
File sync/sync.gyp (right):

https://codereview.chromium.org/1087873003/diff/100001/sync/sync.gyp#newcode468
sync/sync.gyp:468:
'$(SDKROOT)/System/Library/Frameworks/SystemConfiguration.framework',
On 2015/04/15 15:48:02, Dirk Pranke wrote:
> same question ... why do we need the full path here?

Same answer as
https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer_ser...

I think gyp always use the full path.

[Jiang Jiang, 2015-04-15 16:20:47]

With this, https://codereview.chromium.org/1056373003/ and
https://codereview.chromium.org/1060943003/, gn component "all" builds for me on
Mac locally.

[Dirk Pranke, 2015-04-15 16:21:07]

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer/ser...
File gpu/command_buffer/service/BUILD.gn (right):

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer/ser...
gpu/command_buffer/service/BUILD.gn:172: "OpenGL.framework",
My point was that it seemed like there were > 2 places where we needed
specifically these two frameworks. But, I don't feel strongly about this.

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer_ser...
File gpu/command_buffer_service.gypi (right):

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer_ser...
gpu/command_buffer_service.gypi:150:
'$(SDKROOT)/System/Library/Frameworks/OpenGL.framework',
On 2015/04/15 16:17:20, Jiang Jiang wrote:
> I'm not sure what you are referring to, from what I can tell it seems that in
> gyp we always use the full path with $(SDKROOT) while in gn we always use the
> framework name only, would be tricky to see how we link to
> $(SDKROOT)/System/Library/PrivateFrameworks/ in gn though.

I'm talking about gpu/command_buffer/service/BUILD.gn, where you added the same
two frameworks but only listed the framework names and not the full paths.

[Jiang Jiang, 2015-04-15 16:29:26]

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer_ser...
File gpu/command_buffer_service.gypi (right):

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer_ser...
gpu/command_buffer_service.gypi:150:
'$(SDKROOT)/System/Library/Frameworks/OpenGL.framework',
On 2015/04/15 16:21:07, Dirk Pranke wrote:
> I'm talking about gpu/command_buffer/service/BUILD.gn, where you added the
same
> two frameworks but only listed the framework names and not the full paths.

See https://codereview.chromium.org/88813005, so basically what we do in gn by
saying libs = [ "Foo.framework" ] will result a "-framework Foo" being added to
the final linking command.

As for gyp I don't know, blame history refers to commits such as
https://codereview.chromium.org/118473009/. Nico should know better why SDKROOT
was there.

[Dirk Pranke, 2015-04-15 16:34:09]

lgtm.

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer_ser...
File gpu/command_buffer_service.gypi (right):

https://codereview.chromium.org/1087873003/diff/100001/gpu/command_buffer_ser...
gpu/command_buffer_service.gypi:150:
'$(SDKROOT)/System/Library/Frameworks/OpenGL.framework',
Oh! I see, I was totally confused and didn't realize this was a GYP file :(.

Never mind ...

[Jiang Jiang, 2015-04-15 16:52:24]

The CQ bit was checked by jiangj@opera.com

[commit-bot: I haz the power, 2015-04-15 16:53:04]

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

[commit-bot: I haz the power, 2015-04-15 17:04:00]

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

[commit-bot: I haz the power, 2015-04-15 17:04:01]

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

[Jiang Jiang, 2015-04-15 17:39:50]

jiangj@opera.com changed reviewers:
+ brettw@chromium.org

[Jiang Jiang, 2015-04-15 17:39:51]

Need OWNERS stamping. Brett, can you please take a look?

[brettw, 2015-04-15 19:01:06]

I would like to understand the GYP changes more since I would not expect these
in this type of change.

I also feel like the teams (gpu/cast/etc.) should approve the GYP changes in
their areas since there may be restrictions on linking I don't understand.

I'm happy to rubberstamp the corresponding GN stuff after that if there are
things that still need owner approval.

https://codereview.chromium.org/1087873003/diff/160001/third_party/sudden_mot...
File third_party/sudden_motion_sensor/BUILD.gn (right):

https://codereview.chromium.org/1087873003/diff/160001/third_party/sudden_mot...
third_party/sudden_motion_sensor/BUILD.gn:5:
static_library("sudden_motion_sensor") {
This should be a source_set.

https://codereview.chromium.org/1087873003/diff/160001/third_party/sudden_mot...
third_party/sudden_motion_sensor/BUILD.gn:9: sources = [
GN style is to put the source first.

[Jiang Jiang, 2015-04-15 20:16:37]

On 2015/04/15 19:01:06, brettw wrote:
> I would like to understand the GYP changes more since I would not expect these
> in this type of change.
> 
> I also feel like the teams (gpu/cast/etc.) should approve the GYP changes in
> their areas since there may be restrictions on linking I don't understand.
> 
> I'm happy to rubberstamp the corresponding GN stuff after that if there are
> things that still need owner approval.

The reason for making the gyp changes is to be consistent with gn changes. These
frameworks got carried over by other shared libraries in gyp builds, so we don't
necessarily need to explicitly list them. However I still think it's more
correct to have them listed, since I also give the exact reason for every
framework added.

[Jiang Jiang, 2015-04-15 20:17:15]

jiangj@opera.com changed reviewers:
+ avi@chromium.org

[Jiang Jiang, 2015-04-15 20:17:15]

Avi, can you please take a look at the content/ and ui/ changes?

[Jiang Jiang, 2015-04-15 20:21:26]

https://codereview.chromium.org/1087873003/diff/160001/third_party/sudden_mot...
File third_party/sudden_motion_sensor/BUILD.gn (right):

https://codereview.chromium.org/1087873003/diff/160001/third_party/sudden_mot...
third_party/sudden_motion_sensor/BUILD.gn:5:
static_library("sudden_motion_sensor") {
On 2015/04/15 19:01:06, brettw wrote:
> This should be a source_set.

Done. Is there a rule of thumb for choosing source_set vs. static_library?

https://codereview.chromium.org/1087873003/diff/160001/third_party/sudden_mot...
third_party/sudden_motion_sensor/BUILD.gn:9: sources = [
On 2015/04/15 19:01:06, brettw wrote:
> GN style is to put the source first.

Done.

[Avi (use Gerrit), 2015-04-15 20:22:01]

lgtm

stampity stamp

[Jiang Jiang, 2015-04-15 20:23:15]

jiangj@opera.com changed reviewers:
+ piman@chromium.org, toyoshim@chromium.org

[Jiang Jiang, 2015-04-15 20:23:15]

+toyoshim for gyp changes in media/.
+piman for gyp changes in gnu/.

[Dirk Pranke, 2015-04-15 20:35:41]

On 2015/04/15 20:21:26, Jiang Jiang wrote:
>
https://codereview.chromium.org/1087873003/diff/160001/third_party/sudden_mot...
> File third_party/sudden_motion_sensor/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1087873003/diff/160001/third_party/sudden_mot...
> third_party/sudden_motion_sensor/BUILD.gn:5:
> static_library("sudden_motion_sensor") {
> On 2015/04/15 19:01:06, brettw wrote:
> > This should be a source_set.
> 
> Done. Is there a rule of thumb for choosing source_set vs. static_library?
> 

Always use source_sets instead of static_libraries where you can. The only good
reason to explicitly use a static_library
is if that is the end deliverable (i.e., the product is a static library). There
is also a bad reason, which is that you depend
on the unused-code-stripping aspect of linking against static libraries in order
to not get duplicated symbols, but 
usually that reflects bad structuring in your targets that should be fixed
instead.

[Jiang Jiang, 2015-04-15 20:54:41]

On 2015/04/15 20:35:41, Dirk Pranke wrote:
> Always use source_sets instead of static_libraries where you can. The only
good
> reason to explicitly use a static_library
> is if that is the end deliverable (i.e., the product is a static library).
There
> is also a bad reason, which is that you depend
> on the unused-code-stripping aspect of linking against static libraries in
order
> to not get duplicated symbols, but 
> usually that reflects bad structuring in your targets that should be fixed
> instead.

Thanks, if you have read the other issue I was trying to fix, the libvpx one, do
you think my fix https://codereview.chromium.org/1060943003/ makes sense by
turning static_library into source_set? The BUILD.gn there appears to be for
chromium only so I guess the libvpx built by gn doesn't count as an end
deliverable but rather something to be used by other parts of chromium (media
and ffmpeg).

[piman, 2015-04-15 20:57:45]

lgtm

[Dirk Pranke, 2015-04-15 21:07:07]

On 2015/04/15 20:54:41, Jiang Jiang wrote:
> Thanks, if you have read the other issue I was trying to fix, the libvpx one,
do
> you think my fix https://codereview.chromium.org/1060943003/ makes sense by
> turning static_library into source_set? 

I just replied on the bug, but short answer is no, that fix doesn't (yet) make
sense to me :).

[Takashi Toyoshima, 2015-04-16 05:23:47]

lgtm for CoreMIDI in media/
thank you for catching this up.

[Jiang Jiang, 2015-04-16 06:58:11]

jiangj@opera.com changed reviewers:
+ pavely@chromium.org

[Jiang Jiang, 2015-04-16 06:58:12]

+pavely for //sync/sync.gyp changes.

[Takashi Toyoshima, 2015-04-16 07:33:28]

toyoshim@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[Takashi Toyoshima, 2015-04-16 07:35:52]

+dalecurtis for other media changes except for MIDI because I'm a tentative
OWNER of media's build files.

[pavely, 2015-04-16 16:45:06]

sync/* lgtm

[DaleCurtis, 2015-04-16 16:46:30]

media/ lgtm

[brettw, 2015-04-16 17:04:18]

lgtm

https://codereview.chromium.org/1087873003/diff/160001/third_party/sudden_mot...
File third_party/sudden_motion_sensor/BUILD.gn (right):

https://codereview.chromium.org/1087873003/diff/160001/third_party/sudden_mot...
third_party/sudden_motion_sensor/BUILD.gn:5:
static_library("sudden_motion_sensor") {
Yes, use source_set unless there's a good reason not to. The only real reason
not to is that something depends on the specific static library symbol
resolution rules.

[Jiang Jiang, 2015-04-16 17:31:01]

The CQ bit was checked by jiangj@opera.com

[Jiang Jiang, 2015-04-16 17:31:01]

The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org
Link to the patchset: https://codereview.chromium.org/1087873003/#ps180001
(title: "Fix sudden_motion_sensor")

[commit-bot: I haz the power, 2015-04-16 17:32:06]

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

[commit-bot: I haz the power, 2015-04-16 18:24:57]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-04-16 18:25:47]

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

[maniscalco, 2015-04-16 18:54:05]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/1089773003/ by maniscalco@chromium.org.

The reason for reverting is: Suspected of breaking Mac GN, see
https://codereview.chromium.org/1087873003.

[maniscalco, 2015-04-16 18:58:39]

On 2015/04/16 18:54:05, maniscalco wrote:
> A revert of this CL (patchset #10 id:180001) has been created in
> https://codereview.chromium.org/1089773003/ by mailto:maniscalco@chromium.org.
> 
> The reason for reverting is: Suspected of breaking Mac GN, see
> https://codereview.chromium.org/1087873003.

Err, see https://code.google.com/p/chromium/issues/detail?id=477710