Refactor Skia's GYP to separate Chrome additions from the core lib.

Refactor Skia's GYP to separate Chrome additions from the core lib.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211639

R=robertphillips@google.com, torne@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213080

[djsollen, 2013-07-11 17:31:51]

This CL uses the pattern found in chrome/content/content.gyp to avoid linker
issues.

[djsollen, 2013-07-12 13:47:54]

now that this seems to build correctly on all platforms I think it is ready for
review.

[robertphillips, 2013-07-12 14:01:40]

lgtm + some suggestions (mainly comments)

Will it be possible to (in the future) move more of this directly into Skia (I'm
particular I thinking of the opts setup.

https://codereview.chromium.org/18177021/diff/4001/skia/skia_chrome.gypi
File skia/skia_chrome.gypi (right):

https://codereview.chromium.org/18177021/diff/4001/skia/skia_chrome.gypi#newc...
skia/skia_chrome.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
2013?

https://codereview.chromium.org/18177021/diff/4001/skia/skia_chrome.gypi#newc...
skia/skia_chrome.gypi:4: 
# This gyp file contains all the Chrome-specific enhancements to Skia.
# In the shared library it is folded together with the Skia files but in
# the static library build it is its own library.

https://codereview.chromium.org/18177021/diff/4001/skia/skia_common.gypi
File skia/skia_common.gypi (right):

https://codereview.chromium.org/18177021/diff/4001/skia/skia_common.gypi#newc...
skia/skia_common.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
2013?

https://codereview.chromium.org/18177021/diff/4001/skia/skia_common.gypi#newc...
skia/skia_common.gypi:5: {
# This file handles the removal of platform-specific files from
# the Skia build

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library.gypi
File skia/skia_library.gypi (right):

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library.gypi#new...
skia/skia_library.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
2013

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library.gypi#new...
skia/skia_library.gypi:4: 
# This file builds the Skia library. In shared library mode it is folded
# together with the Chrome-specific extensions. In static library mode it is
# its own library.

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library.gypi#new...
skia/skia_library.gypi:19: ['OS=="ios" or OS=="android"', {
# IOS and Android have their own PDF solutions

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library.gypi#new...
skia/skia_library.gypi:37: 'SK_SUPPORT_GPU=<(skia_support_gpu)',
Hasn't SK_DEBUG_PATH_REF been removed?

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library.gypi#new...
skia/skia_library.gypi:56: 'SKIA_DLL',
# TODO: fold these two defines together in Skia and remove 1 here

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library.gypi#new...
skia/skia_library.gypi:76:
'../third_party/skia/src/ports/SkImageDecoder_empty.cpp',
Remove all commented out files?

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library_opts.gyp
File skia/skia_library_opts.gyp (right):

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library_opts.gyp...
skia/skia_library_opts.gyp:1: # Copyright (c) 2012 The Chromium Authors. All
rights reserved.
2013?

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library_opts.gyp...
skia/skia_library_opts.gyp:5: {
# This file manages the platform-specific optimizations for Skia

[djsollen, 2013-07-12 14:43:26]

https://codereview.chromium.org/18177021/diff/4001/skia/skia_chrome.gypi
File skia/skia_chrome.gypi (right):

https://codereview.chromium.org/18177021/diff/4001/skia/skia_chrome.gypi#newc...
skia/skia_chrome.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
On 2013/07/12 14:01:40, robertphillips wrote:
> 2013?

Done.

https://codereview.chromium.org/18177021/diff/4001/skia/skia_chrome.gypi#newc...
skia/skia_chrome.gypi:4: 
On 2013/07/12 14:01:40, robertphillips wrote:
> # This gyp file contains all the Chrome-specific enhancements to Skia.
> # In the shared library it is folded together with the Skia files but in
> # the static library build it is its own library.

Done.

https://codereview.chromium.org/18177021/diff/4001/skia/skia_common.gypi
File skia/skia_common.gypi (right):

https://codereview.chromium.org/18177021/diff/4001/skia/skia_common.gypi#newc...
skia/skia_common.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
On 2013/07/12 14:01:40, robertphillips wrote:
> 2013?

Done.

https://codereview.chromium.org/18177021/diff/4001/skia/skia_common.gypi#newc...
skia/skia_common.gypi:5: {
On 2013/07/12 14:01:40, robertphillips wrote:
> # This file handles the removal of platform-specific files from
> # the Skia build

Done.

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library.gypi
File skia/skia_library.gypi (right):

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library.gypi#new...
skia/skia_library.gypi:1: # Copyright (c) 2012 The Chromium Authors. All rights
reserved.
On 2013/07/12 14:01:40, robertphillips wrote:
> 2013

Done.

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library.gypi#new...
skia/skia_library.gypi:4: 
On 2013/07/12 14:01:40, robertphillips wrote:
> # This file builds the Skia library. In shared library mode it is folded
> # together with the Chrome-specific extensions. In static library mode it is
> # its own library.

Done.

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library.gypi#new...
skia/skia_library.gypi:37: 'SK_SUPPORT_GPU=<(skia_support_gpu)',
On 2013/07/12 14:01:40, robertphillips wrote:
> Hasn't SK_DEBUG_PATH_REF been removed?

Done.

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library.gypi#new...
skia/skia_library.gypi:56: 'SKIA_DLL',
On 2013/07/12 14:01:40, robertphillips wrote:
> # TODO: fold these two defines together in Skia and remove 1 here

Done.

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library.gypi#new...
skia/skia_library.gypi:76:
'../third_party/skia/src/ports/SkImageDecoder_empty.cpp',
On 2013/07/12 14:01:40, robertphillips wrote:
> Remove all commented out files?

Done.

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library_opts.gyp
File skia/skia_library_opts.gyp (right):

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library_opts.gyp...
skia/skia_library_opts.gyp:1: # Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2013/07/12 14:01:40, robertphillips wrote:
> 2013?

Done.

https://codereview.chromium.org/18177021/diff/4001/skia/skia_library_opts.gyp...
skia/skia_library_opts.gyp:5: {
On 2013/07/12 14:01:40, robertphillips wrote:
> # This file manages the platform-specific optimizations for Skia

Done.

[robertphillips, 2013-07-12 17:56:53]

lgtm

[Torne, 2013-07-15 11:11:53]

LGTM - I kicked off an android_aosp run on the latest CL (which tests the
webview build; not the most descriptive name) and that was green too, so this
appears to be pretty much good to go :)

[commit-bot: I haz the power, 2013-07-15 11:41:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/djsollen@google.com/18177021/10003

[commit-bot: I haz the power, 2013-07-15 14:54:39]

Change committed as 211639

[djsollen, 2013-07-15 15:18:39]

On 2013/07/15 14:54:39, I haz the power (commit-bot) wrote:
> Change committed as 211639

The change was reverted due to a failure with the mac_asan build.

https://codereview.chromium.org/19166003/

[djsollen, 2013-07-15 17:45:55]

So after some testing it appears that it really is ASAN that is causing
something to go wrong with the ninja generator. The following GYP commands
produces the error, but removing asan fixes it.

GYP_GENERATORS="ninja" GYP_DEFINES="asan=1
release_extra_cflags=-gline-tables-only component=static_library"
./build/gyp_chromium

The big difference that I see is that asan=1 adds the following to skia.ninja
file...

ratchet:src djsollen$ cat out/Debug/obj/skia/skia.ninja
build libskia.a: alink || libclang_rt.asan_osx_dynamic.dylib

[Torne, 2013-07-16 10:16:35]

On 2013/07/15 17:45:55, djsollen wrote:
> So after some testing it appears that it really is ASAN that is causing
> something to go wrong with the ninja generator. The following GYP commands
> produces the error, but removing asan fixes it.
> 
> GYP_GENERATORS="ninja" GYP_DEFINES="asan=1
> release_extra_cflags=-gline-tables-only component=static_library"
> ./build/gyp_chromium
> 
> The big difference that I see is that asan=1 adds the following to skia.ninja
> file...
> 
> ratchet:src djsollen$ cat out/Debug/obj/skia/skia.ninja
> build libskia.a: alink || libclang_rt.asan_osx_dynamic.dylib

I don't have any ideas for how to fix this specific issue, but one thing that I
was doing on my change that you may find useful is if you do "git cl try -t
compile" it will submit compile-only (no test) tryjobs, and this uses a *larger*
range of bots than the default set (it includes mac_asan, for example). Before
relanding you may want to try with both the default set and the compile-only
set, just to be doubly sure?

[Torne, 2013-07-16 10:16:59]

On 2013/07/16 10:16:35, Torne wrote:
> On 2013/07/15 17:45:55, djsollen wrote:
> > So after some testing it appears that it really is ASAN that is causing
> > something to go wrong with the ninja generator. The following GYP commands
> > produces the error, but removing asan fixes it.
> > 
> > GYP_GENERATORS="ninja" GYP_DEFINES="asan=1
> > release_extra_cflags=-gline-tables-only component=static_library"
> > ./build/gyp_chromium
> > 
> > The big difference that I see is that asan=1 adds the following to
skia.ninja
> > file...
> > 
> > ratchet:src djsollen$ cat out/Debug/obj/skia/skia.ninja
> > build libskia.a: alink || libclang_rt.asan_osx_dynamic.dylib
> 
> I don't have any ideas for how to fix this specific issue, but one thing that
I
> was doing on my change that you may find useful is if you do "git cl try -t
> compile" it will submit compile-only (no test) tryjobs, and this uses a
*larger*
> range of bots than the default set (it includes mac_asan, for example). Before
> relanding you may want to try with both the default set and the compile-only
> set, just to be doubly sure?

Oh, also: please try on android_aosp as well, which isn't included in either set
by default.

[Torne, 2013-07-16 10:17:00]

On 2013/07/16 10:16:35, Torne wrote:
> On 2013/07/15 17:45:55, djsollen wrote:
> > So after some testing it appears that it really is ASAN that is causing
> > something to go wrong with the ninja generator. The following GYP commands
> > produces the error, but removing asan fixes it.
> > 
> > GYP_GENERATORS="ninja" GYP_DEFINES="asan=1
> > release_extra_cflags=-gline-tables-only component=static_library"
> > ./build/gyp_chromium
> > 
> > The big difference that I see is that asan=1 adds the following to
skia.ninja
> > file...
> > 
> > ratchet:src djsollen$ cat out/Debug/obj/skia/skia.ninja
> > build libskia.a: alink || libclang_rt.asan_osx_dynamic.dylib
> 
> I don't have any ideas for how to fix this specific issue, but one thing that
I
> was doing on my change that you may find useful is if you do "git cl try -t
> compile" it will submit compile-only (no test) tryjobs, and this uses a
*larger*
> range of bots than the default set (it includes mac_asan, for example). Before
> relanding you may want to try with both the default set and the compile-only
> set, just to be doubly sure?

Oh, also: please try on android_aosp as well, which isn't included in either set
by default.

[Torne, 2013-07-16 10:17:02]

On 2013/07/16 10:16:35, Torne wrote:
> On 2013/07/15 17:45:55, djsollen wrote:
> > So after some testing it appears that it really is ASAN that is causing
> > something to go wrong with the ninja generator. The following GYP commands
> > produces the error, but removing asan fixes it.
> > 
> > GYP_GENERATORS="ninja" GYP_DEFINES="asan=1
> > release_extra_cflags=-gline-tables-only component=static_library"
> > ./build/gyp_chromium
> > 
> > The big difference that I see is that asan=1 adds the following to
skia.ninja
> > file...
> > 
> > ratchet:src djsollen$ cat out/Debug/obj/skia/skia.ninja
> > build libskia.a: alink || libclang_rt.asan_osx_dynamic.dylib
> 
> I don't have any ideas for how to fix this specific issue, but one thing that
I
> was doing on my change that you may find useful is if you do "git cl try -t
> compile" it will submit compile-only (no test) tryjobs, and this uses a
*larger*
> range of bots than the default set (it includes mac_asan, for example). Before
> relanding you may want to try with both the default set and the compile-only
> set, just to be doubly sure?

Oh, also: please try on android_aosp as well, which isn't included in either set
by default.

[robertphillips, 2013-07-23 12:03:58]

lgtm

[djsollen, 2013-07-23 13:01:23]

Committed patchset #10 manually as r213080 (presubmit successful).

[Torne, 2013-07-23 13:01:59]

On 2013/07/23 13:01:23, djsollen wrote:
> Committed patchset #10 manually as r213080 (presubmit successful).

Woohoo! Let's see if it sticks this time :)