Build Skia as a static library

Build Skia as a static library

- Roll GYP so that we get non-thin archives on Linux
- Add merge_static_libs.py
- Add skia_core_lib target which builds core, ports, opts*, and utils
- Replace dependencies on core/ports/opts/utils with skia_core_libs
- Rename exportable libraries with "skia_"
Committed: https://code.google.com/p/skia/source/detail?r=5889

[EricB, 2012-10-04 21:57:59]

I don't think this is quite ready to go, but I'd like some feedback on the
approach.

https://codereview.appspot.com/6619049/diff/2001/DEPS
File DEPS (right):

https://codereview.appspot.com/6619049/diff/2001/DEPS#newcode12
DEPS:12: "third_party/externals/gyp" :
"http://gyp.googlecode.com/svn/trunk@1512",
This revision will change once https://codereview.chromium.org/11031005/ is
committed.

[EricB, 2012-10-05 12:47:18]

On 2012/10/04 21:57:59, EricB wrote:
> I don't think this is quite ready to go, but I'd like some feedback on the
> approach.
> 
> https://codereview.appspot.com/6619049/diff/2001/DEPS
> File DEPS (right):
> 
> https://codereview.appspot.com/6619049/diff/2001/DEPS#newcode12
> DEPS:12: "third_party/externals/gyp" :
> "http://gyp.googlecode.com/svn/trunk@1512",
> This revision will change once https://codereview.chromium.org/11031005/ is
> committed.

The reason we build separate static libraries and then merge them rather than
using gyp includes to build one library is that opts_* require different cflags
from the other targets (See comment in opts.gyp).  Alternatively, we could build
one big libskia.a with everything except opts_* and then build separate
libskia_opts* libraries which would have to be paired with libskia.  I think
that's okay since we're going to be putting effects, views, etc in separate
libs, and it would save us having to invoke a custom merge_static_libs script.

[DerekS, 2012-10-05 13:52:27]

from the end user perspective I like the idea of being able to call 'make
static_core' or 'make static_libs' to be able to get static libraries for usage
in other apps.

However, the gyp changes seem non-trivial so I'm going to let elliot take a
closer look.

https://codereview.appspot.com/6619049/diff/2001/Makefile
File Makefile (right):

https://codereview.appspot.com/6619049/diff/2001/Makefile#newcode32
Makefile:32: ALL_TARGETS := core SampleApp bench gm libskia tests tools
libskia is a bit ambiguous as to whether it is producing a shared or static
library. Could we be more clear on what type of lib we are producing?

[epoger, 2012-10-05 16:26:01]

On 2012/10/04 21:57:59, EricB wrote:
> I don't think this is quite ready to go, but I'd like some feedback on the
> approach.
> 
> https://codereview.appspot.com/6619049/diff/2001/DEPS
> File DEPS (right):
> 
> https://codereview.appspot.com/6619049/diff/2001/DEPS#newcode12
> DEPS:12: "third_party/externals/gyp" :
> "http://gyp.googlecode.com/svn/trunk@1512",
> This revision will change once https://codereview.chromium.org/11031005/ is
> committed.

Looking now.  BTW, I just took a look at the gyp change you made... nicely done,
kudos to you for wading into that!

[epoger, 2012-10-05 16:26:36]

On 2012/10/05 16:26:01, epoger wrote:
> On 2012/10/04 21:57:59, EricB wrote:
> > I don't think this is quite ready to go, but I'd like some feedback on the
> > approach.
> > 
> > https://codereview.appspot.com/6619049/diff/2001/DEPS
> > File DEPS (right):
> > 
> > https://codereview.appspot.com/6619049/diff/2001/DEPS#newcode12
> > DEPS:12: "third_party/externals/gyp" :
> > "http://gyp.googlecode.com/svn/trunk@1512",
> > This revision will change once https://codereview.chromium.org/11031005/ is
> > committed.
> 
> Looking now.  BTW, I just took a look at the gyp change you made... nicely
done,
> kudos to you for wading into that!

(By "gyp change", I mean https://codereview.chromium.org/11031005/  in the gyp
repo)

[epoger, 2012-10-05 17:12:05]

I think this is pretty much right, ASSUMING that we want this particular set of
targets to be built into the one library.

https://codereview.appspot.com/6619049/diff/2001/DEPS
File DEPS (right):

https://codereview.appspot.com/6619049/diff/2001/DEPS#newcode12
DEPS:12: "third_party/externals/gyp" :
"http://gyp.googlecode.com/svn/trunk@1512",
Should be 1517, not 1512

https://codereview.appspot.com/6619049/diff/2001/Makefile
File Makefile (right):

https://codereview.appspot.com/6619049/diff/2001/Makefile#newcode32
Makefile:32: ALL_TARGETS := core SampleApp bench gm libskia tests tools
On 2012/10/05 13:52:27, DerekS wrote:
> libskia is a bit ambiguous as to whether it is producing a shared or static
> library. Could we be more clear on what type of lib we are producing?

Maybe we could have "libshared" and "libstatic" targets, instead of a single
"libskia" target?  Or is the output library's name forced to something in
particular based on the target name?

Alternatively, I guess we could add a GYP_DEFINES variable that the caller would
set to force static vs shared library (instead of just taking the default
behavior).

https://codereview.appspot.com/6619049/diff/2001/gyp/FileReaderApp.gyp
File gyp/FileReaderApp.gyp (left):

https://codereview.appspot.com/6619049/diff/2001/gyp/FileReaderApp.gyp#oldcode23
gyp/FileReaderApp.gyp:23: 'opts.gyp:opts',
I would like Mike or Brian to weigh in on exactly which targets (opts? ports?
utils?) should be rolled into "libskia".

Maybe libskia should include EVERYTHING, to make it a nice-n-easy dependency for
Skia users, who later on might want to pare down exactly what they get...

https://codereview.appspot.com/6619049/diff/2001/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.appspot.com/6619049/diff/2001/gyp/common_variables.gypi#ne...
gyp/common_variables.gypi:71: ['skia_nacl', {
Is there some reason that this skia_nacl change is part of the same CL?  (Is it
related to the static library issue?)

https://codereview.appspot.com/6619049/diff/2001/gyp/core.gyp
File gyp/core.gyp (right):

https://codereview.appspot.com/6619049/diff/2001/gyp/core.gyp#newcode7
gyp/core.gyp:7: 'standalone_static_library': 1,
If we pulled "opts" out of "libskia", then could we declare
'standalone_static_library' only in skia.gyp?  That would be nice...

https://codereview.appspot.com/6619049/diff/2001/gyp/skia.gyp
File gyp/skia.gyp (right):

https://codereview.appspot.com/6619049/diff/2001/gyp/skia.gyp#newcode38
gyp/skia.gyp:38: 'action_name': 'MergeStaticLibs',
While I'm not excited about this custom action (and from your toplevel comment
it seems you aren't either), it seems like the best way to accomplish our goal,
GIVEN the limitations described in opts.gyp, IF our goal is indeed to include
the platform optimizations within the single Skia library.

In other words... I think we should either do this, or not, depending on whether
we think our users would be better served by the platform optimizations being
built into the single Skia library.

If we want the platform optimizations to be built into the single Skia library
-> open a bug tracking the gyp/gcc limitation that makes us do it this way, and
add a link to that bug here and in opts.gyp

If we don't -> get rid of this mess. :-)

https://codereview.appspot.com/6619049/diff/2001/tools/merge_static_libs.py
File tools/merge_static_libs.py (right):

https://codereview.appspot.com/6619049/diff/2001/tools/merge_static_libs.py#n...
tools/merge_static_libs.py:12: def Usage():
I recommend you prefix all these function names with underscores... nobody else
should be calling them.

https://codereview.appspot.com/6619049/diff/2001/tools/merge_static_libs.py#n...
tools/merge_static_libs.py:20: in_libs[idx] = os.path.abspath(in_lib)
If I understand this part correctly, it's modifying the contents of in_libs so
that each one is an absolute path.  Right?

Seems a bit scary to me that it modifies those in-place.  I don't think the
caller would be expecting that... (not that anyone else should be calling this)

I think it would be better to do this:
in_libs_abspaths = [os.path.abspath(path) for path in in_libs]
... and then use in_libs_abspaths below

(the performance difference won't matter, so I think it's worth doing)

[EricB, 2012-10-05 20:52:23]

This is getting closer.  A couple of things I'm not happy with:

- The merge_static_libs script takes non-thin static libraries which were just
packaged and unpackages them.  It would be more efficient if it could be made to
work with thin archives.  This would have the benefit that the libraries which
make up libskia.a would not have to be declared as standalone_static_libraries
and would therefore not end up in PRODUCT_DIR, leaving only the executables and
libraries we really want to export.

- I'd like to clean up the target heirarchy a bit.  I'm thinking about renaming
"tests" to "unittests" and then making the "tests" target be a meta-target for
"unittests," "gm," and "bench."  SampleApp has a different capitalization scheme
than the rest of the targets.  It's been driving me nuts for a while now.

https://codereview.appspot.com/6619049/diff/2001/DEPS
File DEPS (right):

https://codereview.appspot.com/6619049/diff/2001/DEPS#newcode12
DEPS:12: "third_party/externals/gyp" :
"http://gyp.googlecode.com/svn/trunk@1512",
On 2012/10/05 17:12:05, epoger wrote:
> Should be 1517, not 1512

Done.

https://codereview.appspot.com/6619049/diff/2001/Makefile
File Makefile (right):

https://codereview.appspot.com/6619049/diff/2001/Makefile#newcode32
Makefile:32: ALL_TARGETS := core SampleApp bench gm libskia tests tools
On 2012/10/05 17:12:05, epoger wrote:
> On 2012/10/05 13:52:27, DerekS wrote:
> > libskia is a bit ambiguous as to whether it is producing a shared or static
> > library. Could we be more clear on what type of lib we are producing?
> 
> Maybe we could have "libshared" and "libstatic" targets, instead of a single
> "libskia" target?  Or is the output library's name forced to something in
> particular based on the target name?
> 
> Alternatively, I guess we could add a GYP_DEFINES variable that the caller
would
> set to force static vs shared library (instead of just taking the default
> behavior).

I like the idea of using a static/shared gyp define.  I think there's enough
complexity in this CL that we should just assume everything is a static library
for now.

https://codereview.appspot.com/6619049/diff/2001/gyp/common_variables.gypi
File gyp/common_variables.gypi (right):

https://codereview.appspot.com/6619049/diff/2001/gyp/common_variables.gypi#ne...
gyp/common_variables.gypi:71: ['skia_nacl', {
On 2012/10/05 17:12:05, epoger wrote:
> Is there some reason that this skia_nacl change is part of the same CL?  (Is
it
> related to the static library issue?)

I was working on this in parallel with making Skia compile under nacl.  I
removed that part for now.

https://codereview.appspot.com/6619049/diff/2001/gyp/core.gyp
File gyp/core.gyp (right):

https://codereview.appspot.com/6619049/diff/2001/gyp/core.gyp#newcode7
gyp/core.gyp:7: 'standalone_static_library': 1,
On 2012/10/05 17:12:05, epoger wrote:
> If we pulled "opts" out of "libskia", then could we declare
> 'standalone_static_library' only in skia.gyp?  That would be nice...

I'm not sure what you mean.  If we pulled opts out of libskia, it would *have*
to be a standalone_static_library so that it could be used alongside libskia.

https://codereview.appspot.com/6619049/diff/2001/gyp/skia.gyp
File gyp/skia.gyp (right):

https://codereview.appspot.com/6619049/diff/2001/gyp/skia.gyp#newcode38
gyp/skia.gyp:38: 'action_name': 'MergeStaticLibs',
On 2012/10/05 17:12:05, epoger wrote:
> While I'm not excited about this custom action (and from your toplevel comment
> it seems you aren't either), it seems like the best way to accomplish our
goal,
> GIVEN the limitations described in opts.gyp, IF our goal is indeed to include
> the platform optimizations within the single Skia library.
> 
> In other words... I think we should either do this, or not, depending on
whether
> we think our users would be better served by the platform optimizations being
> built into the single Skia library.
> 
> If we want the platform optimizations to be built into the single Skia library
> -> open a bug tracking the gyp/gcc limitation that makes us do it this way,
and
> add a link to that bug here and in opts.gyp
> 
> If we don't -> get rid of this mess. :-)

I think we really should include opts in libskia.a.  Skia will be compiled
expecting a particular set of opts to exist.  In my opinion, if a given instance
of libskia.a requires a specific instance of libskia_opts.a, then those two
should be the same library.

https://codereview.appspot.com/6619049/diff/2001/tools/merge_static_libs.py
File tools/merge_static_libs.py (right):

https://codereview.appspot.com/6619049/diff/2001/tools/merge_static_libs.py#n...
tools/merge_static_libs.py:12: def Usage():
On 2012/10/05 17:12:05, epoger wrote:
> I recommend you prefix all these function names with underscores... nobody
else
> should be calling them.

Done.

https://codereview.appspot.com/6619049/diff/2001/tools/merge_static_libs.py#n...
tools/merge_static_libs.py:16: def MergeLibs(in_libs, out_lib):
I didn't make this one "private" because it might be nice to be able to call it
from another script.

https://codereview.appspot.com/6619049/diff/2001/tools/merge_static_libs.py#n...
tools/merge_static_libs.py:20: in_libs[idx] = os.path.abspath(in_lib)
On 2012/10/05 17:12:05, epoger wrote:
> If I understand this part correctly, it's modifying the contents of in_libs so
> that each one is an absolute path.  Right?
> 
> Seems a bit scary to me that it modifies those in-place.  I don't think the
> caller would be expecting that... (not that anyone else should be calling
this)
> 
> I think it would be better to do this:
> in_libs_abspaths = [os.path.abspath(path) for path in in_libs]
> ... and then use in_libs_abspaths below
> 
> (the performance difference won't matter, so I think it's worth doing)
> 

Done.

[EricB, 2012-10-08 14:44:48]

Uploaded patch set 12.  I made merge_static_libraries.py work with both thin and
non-thin static libraries.

Verified for Linux, Android, Mac, and Win.

https://codereview.appspot.com/6619049/diff/13064/gyp/bench.gyp
File gyp/bench.gyp (right):

https://codereview.appspot.com/6619049/diff/13064/gyp/bench.gyp#newcode27
gyp/bench.gyp:27: '../src/gpu',
I could've used direct_dependent_settings and export_dependent_settings to
remove the need for the conditional addition of this include_dir, but it seems
bad to always include something inside the src directory, even if we already do
it in lots of places.

[epoger, 2012-10-09 16:14:06]

I'm getting concerned about the unwieldy / potentially difficult-to-maintain
nature of this change.  Details within.

Perhaps some in-person discussion (VC or tomorrow) would help...

https://codereview.appspot.com/6619049/diff/13064/Makefile
File Makefile (right):

https://codereview.appspot.com/6619049/diff/13064/Makefile#newcode33
Makefile:33: bench \
I'm not clear why you added these additional targets (and why they are in this
particular order).

But hopefully https://codereview.appspot.com/6637045/ ('Rename 'all' to 'most',
split out debugger build.') will make this moot anyway.

https://codereview.appspot.com/6619049/diff/13064/gyp/bench.gyp
File gyp/bench.gyp (right):

https://codereview.appspot.com/6619049/diff/13064/gyp/bench.gyp#newcode27
gyp/bench.gyp:27: '../src/gpu',
On 2012/10/08 14:44:48, EricB wrote:
> I could've used direct_dependent_settings and export_dependent_settings to
> remove the need for the conditional addition of this include_dir, but it seems
> bad to always include something inside the src directory, even if we already
do
> it in lots of places.

Is it true that "bench" needs visibility into some GPU routines that most
library-users do not?

If so, this seems like the right approach.

But if ALL users of the library need access to headers in src/gpu, it seems like
this SHOULD be done by *_dependent_settings, or (more likely) those header files
should be moved into includes/ .

https://codereview.appspot.com/6619049/diff/13064/gyp/core.gyp
File gyp/core.gyp (right):

https://codereview.appspot.com/6619049/diff/13064/gyp/core.gyp#newcode103
gyp/core.gyp:103: 'conditions': [
Why did these conditions get added?

https://codereview.appspot.com/6619049/diff/13064/gyp/skia.gyp
File gyp/skia.gyp (right):

https://codereview.appspot.com/6619049/diff/13064/gyp/skia.gyp#newcode10
gyp/skia.gyp:10: 'action_output':
'<(PRODUCT_DIR)/<(static_lib_prefix)skia<(static_lib_suffix)',
Ugh.  This is so hard to follow.  Variables set in common_variables.gypi, which
are used to set variables here, which are used to run a custom MergeStaticLibs
below.

In Patchset 2, when the madness was confined to this file, I felt OK about it. 
Now I'm not so sure.

https://codereview.appspot.com/6619049/diff/13064/tools/merge_static_libs.py
File tools/merge_static_libs.py (right):

https://codereview.appspot.com/6619049/diff/13064/tools/merge_static_libs.py#...
tools/merge_static_libs.py:46: objects.append(object)
Why not just use os.path.abspath() in both cases, so we don't need parallel code
paths?  Would that not work?

Again, I feel like this workaround for what is basically a gyp shortcoming is
getting pretty unwieldy.

[EricB, 2012-10-09 17:51:57]

https://codereview.appspot.com/6619049/diff/13064/Makefile
File Makefile (right):

https://codereview.appspot.com/6619049/diff/13064/Makefile#newcode33
Makefile:33: bench \
On 2012/10/09 16:14:07, epoger wrote:
> I'm not clear why you added these additional targets (and why they are in this
> particular order).
> 
> But hopefully https://codereview.appspot.com/6637045/ ('Rename 'all' to
'most',
> split out debugger build.') will make this moot anyway.

Adding these here allows us to actually build any of them, where before we could
only build the executables which use them.  The idea is to decide on a subset of
our libraries which will be "exported" for use by third-party developers and
allow those to be built at the top level.

https://codereview.appspot.com/6619049/diff/13064/gyp/bench.gyp
File gyp/bench.gyp (right):

https://codereview.appspot.com/6619049/diff/13064/gyp/bench.gyp#newcode27
gyp/bench.gyp:27: '../src/gpu',
On 2012/10/09 16:14:07, epoger wrote:
> On 2012/10/08 14:44:48, EricB wrote:
> > I could've used direct_dependent_settings and export_dependent_settings to
> > remove the need for the conditional addition of this include_dir, but it
seems
> > bad to always include something inside the src directory, even if we already
> do
> > it in lots of places.
> 
> Is it true that "bench" needs visibility into some GPU routines that most
> library-users do not?
> 
> If so, this seems like the right approach.
> 
> But if ALL users of the library need access to headers in src/gpu, it seems
like
> this SHOULD be done by *_dependent_settings, or (more likely) those header
files
> should be moved into includes/ .

Without knowing the details of why we need to add src/gpu to our include dirs,
I'd say that those things should be moved into include/gpu.

https://codereview.appspot.com/6619049/diff/13064/gyp/core.gyp
File gyp/core.gyp (right):

https://codereview.appspot.com/6619049/diff/13064/gyp/core.gyp#newcode103
gyp/core.gyp:103: 'conditions': [
On 2012/10/09 16:14:07, epoger wrote:
> Why did these conditions get added?

We want these include dirs to be propagated to all of the targets who depend on
core, ie the new "skia" target, which then propagates these include dirs to
targets which depend on it, via export_dependent_settings.  This is better than
just declaring these include dirs in the targets which depend on "skia," since
it's assumed that we'll need these headers for any such target.

https://codereview.appspot.com/6619049/diff/13064/gyp/skia.gyp
File gyp/skia.gyp (right):

https://codereview.appspot.com/6619049/diff/13064/gyp/skia.gyp#newcode10
gyp/skia.gyp:10: 'action_output':
'<(PRODUCT_DIR)/<(static_lib_prefix)skia<(static_lib_suffix)',
On 2012/10/09 16:14:07, epoger wrote:
> Ugh.  This is so hard to follow.  Variables set in common_variables.gypi,
which
> are used to set variables here, which are used to run a custom MergeStaticLibs
> below.
> 
> In Patchset 2, when the madness was confined to this file, I felt OK about it.

> Now I'm not so sure.

What I really wanted was to have a single list:

['core', 'opts', 'ports', 'utils', extra_opts]

And then construct the dependencies, export_dependent_settings, inputs to the
action, and the action command line itself from that list.  Unfortunately,
there's no way to say, "this list variable is formed by taking another list
variable and pre/appending this string."

I can place the static_lib_* variable definitions inside this file if that makes
it any better, but it seemed useful to have those available to any file.

Alternatively, I can do the dumb thing and just list everything outright,
although I'd still need platform-dependent path/prefix/suffix.

https://codereview.appspot.com/6619049/diff/13064/tools/merge_static_libs.py
File tools/merge_static_libs.py (right):

https://codereview.appspot.com/6619049/diff/13064/tools/merge_static_libs.py#...
tools/merge_static_libs.py:46: objects.append(object)
On 2012/10/09 16:14:07, epoger wrote:
> Why not just use os.path.abspath() in both cases, so we don't need parallel
code
> paths?  Would that not work?
> 
> Again, I feel like this workaround for what is basically a gyp shortcoming is
> getting pretty unwieldy.

It's more heavy-handed than I would like.  The alternatives as I see them:

1. Make a change in GYP which allows file-specific settings, including cflags. 
This is probably going to be difficult, time-consuming, and they may not even
accept that kind of change.

2. Use gypi's to build libskia.a with everything but opts_*, which would become
libskia_opts.a.  My complaint with this is that the two libraries would be
tightly coupled and it might not be obvious to a third party developer that both
are needed, or that they must be compiled together with exactly the same
settings.  It seems fragile.

[bsalomon, 2012-10-09 18:35:56]

https://codereview.appspot.com/6619049/diff/13064/gyp/bench.gyp
File gyp/bench.gyp (right):

https://codereview.appspot.com/6619049/diff/13064/gyp/bench.gyp#newcode27
gyp/bench.gyp:27: '../src/gpu',
On 2012/10/09 17:51:57, EricB wrote:
> On 2012/10/09 16:14:07, epoger wrote:
> > On 2012/10/08 14:44:48, EricB wrote:
> > > I could've used direct_dependent_settings and export_dependent_settings to
> > > remove the need for the conditional addition of this include_dir, but it
> seems
> > > bad to always include something inside the src directory, even if we
already
> > do
> > > it in lots of places.
> > 
> > Is it true that "bench" needs visibility into some GPU routines that most
> > library-users do not?
> > 
> > If so, this seems like the right approach.
> > 
> > But if ALL users of the library need access to headers in src/gpu, it seems
> like
> > this SHOULD be done by *_dependent_settings, or (more likely) those header
> files
> > should be moved into includes/ .
> 
> Without knowing the details of why we need to add src/gpu to our include dirs,
> I'd say that those things should be moved into include/gpu.

We want to directly bench classes in src/gpu that no Skia client should ever see
directly. I think bench, tests, and maybe gm will need to peer into src/gpu.

[epoger, 2012-10-09 18:58:27]

https://codereview.appspot.com/6619049/diff/13064/Makefile
File Makefile (right):

https://codereview.appspot.com/6619049/diff/13064/Makefile#newcode33
Makefile:33: bench \
On 2012/10/09 17:51:57, EricB wrote:
> On 2012/10/09 16:14:07, epoger wrote:
> > I'm not clear why you added these additional targets (and why they are in
this
> > particular order).
> > 
> > But hopefully https://codereview.appspot.com/6637045/ ('Rename 'all' to
> 'most',
> > split out debugger build.') will make this moot anyway.
> 
> Adding these here allows us to actually build any of them, where before we
could
> only build the executables which use them.  The idea is to decide on a subset
of
> our libraries which will be "exported" for use by third-party developers and
> allow those to be built at the top level.

OK.  I would think the "top level" targets would be:

static_lib (builds a library of ALL Skia functionality, including
animator/effects/images/pdf/etc, for a particular OS/platform combination). 
Third-party products that use Skia could just depend on this target.

dynamic_lib (maybe, in the future)

tests (unit tests, gm, bench, SampleApp).  Under the covers, this depends on
static_lib.  Alternatively, these could be part of "tools".

tools (skdiff and its ilk)


I think a simple set like that would be simpler for third-party developers to
understand.  I guess we need to also provide more granular pieces (e.g. some
embedded developer wants to incorporate Skia's core but not animator), so maybe
there are TWO sets:

1. The easy list: static_lib, tests, tools
2. The advanced list: (what you have already written here)


But for now, this is fine.  Maybe we can create that "easier" top-level target
list in a separate CL.

https://codereview.appspot.com/6619049/diff/13064/gyp/skia.gyp
File gyp/skia.gyp (right):

https://codereview.appspot.com/6619049/diff/13064/gyp/skia.gyp#newcode10
gyp/skia.gyp:10: 'action_output':
'<(PRODUCT_DIR)/<(static_lib_prefix)skia<(static_lib_suffix)',
On 2012/10/09 17:51:57, EricB wrote:
> On 2012/10/09 16:14:07, epoger wrote:
> > Ugh.  This is so hard to follow.  Variables set in common_variables.gypi,
> which
> > are used to set variables here, which are used to run a custom
MergeStaticLibs
> > below.
> > 
> > In Patchset 2, when the madness was confined to this file, I felt OK about
it.
> 
> > Now I'm not so sure.
> 
> What I really wanted was to have a single list:
> 
> ['core', 'opts', 'ports', 'utils', extra_opts]
> 
> And then construct the dependencies, export_dependent_settings, inputs to the
> action, and the action command line itself from that list.  Unfortunately,
> there's no way to say, "this list variable is formed by taking another list
> variable and pre/appending this string."
> 
> I can place the static_lib_* variable definitions inside this file if that
makes
> it any better, but it seemed useful to have those available to any file.
> 
> Alternatively, I can do the dumb thing and just list everything outright,
> although I'd still need platform-dependent path/prefix/suffix.

IIRC, most of this complication would go away if gyp would allow us to pass
different cflags for the opts_* targets than the other targets.  Right?

If so, I'd really like to quarantine all of the ugliness within one file,
commented to indicate our distaste at its existence.  This will make it easier
to remove when (hopefully) we find a better way, such as a change to GYP.

https://codereview.appspot.com/6619049/diff/13064/tools/merge_static_libs.py
File tools/merge_static_libs.py (right):

https://codereview.appspot.com/6619049/diff/13064/tools/merge_static_libs.py#...
tools/merge_static_libs.py:46: objects.append(object)
On 2012/10/09 17:51:57, EricB wrote:
> On 2012/10/09 16:14:07, epoger wrote:
> > Why not just use os.path.abspath() in both cases, so we don't need parallel
> code
> > paths?  Would that not work?
> > 
> > Again, I feel like this workaround for what is basically a gyp shortcoming
is
> > getting pretty unwieldy.
> 
> It's more heavy-handed than I would like.  The alternatives as I see them:
> 
> 1. Make a change in GYP which allows file-specific settings, including cflags.

> This is probably going to be difficult, time-consuming, and they may not even
> accept that kind of change.
> 
> 2. Use gypi's to build libskia.a with everything but opts_*, which would
become
> libskia_opts.a.  My complaint with this is that the two libraries would be
> tightly coupled and it might not be obvious to a third party developer that
both
> are needed, or that they must be compiled together with exactly the same
> settings.  It seems fragile.

I agree that #2 would be fragile.  Thanks for the explanation.

Would the hack be any smaller/simpler if we built it in TWO chunks (the
granularity at which we need to use different cflags: everything-but-opts and
opts) and then combined those, rather than building it in 4 chunks (core, opts,
ports, utils) and combining those?

Maybe if there were just two libraries to merge, just writing them out inline
for the various cases (and deleting the static_lib_* variables) would be more
reasonable?

[EricB, 2012-10-09 19:18:09]

https://codereview.appspot.com/6619049/diff/13064/Makefile
File Makefile (right):

https://codereview.appspot.com/6619049/diff/13064/Makefile#newcode33
Makefile:33: bench \
On 2012/10/09 18:58:27, epoger wrote:
> On 2012/10/09 17:51:57, EricB wrote:
> > On 2012/10/09 16:14:07, epoger wrote:
> > > I'm not clear why you added these additional targets (and why they are in
> this
> > > particular order).
> > > 
> > > But hopefully https://codereview.appspot.com/6637045/ ('Rename 'all' to
> > 'most',
> > > split out debugger build.') will make this moot anyway.
> > 
> > Adding these here allows us to actually build any of them, where before we
> could
> > only build the executables which use them.  The idea is to decide on a
subset
> of
> > our libraries which will be "exported" for use by third-party developers and
> > allow those to be built at the top level.
> 
> OK.  I would think the "top level" targets would be:
> 
> static_lib (builds a library of ALL Skia functionality, including
> animator/effects/images/pdf/etc, for a particular OS/platform combination). 
> Third-party products that use Skia could just depend on this target.
> 
> dynamic_lib (maybe, in the future)
> 
> tests (unit tests, gm, bench, SampleApp).  Under the covers, this depends on
> static_lib.  Alternatively, these could be part of "tools".
> 
> tools (skdiff and its ilk)
> 
> 
> I think a simple set like that would be simpler for third-party developers to
> understand.  I guess we need to also provide more granular pieces (e.g. some
> embedded developer wants to incorporate Skia's core but not animator), so
maybe
> there are TWO sets:
> 
> 1. The easy list: static_lib, tests, tools
> 2. The advanced list: (what you have already written here)
> 
> 
> But for now, this is fine.  Maybe we can create that "easier" top-level target
> list in a separate CL.

I guess my opinion is that third party developers might want to build the pdf
backend specifically but not svg, for example.  Since pdf isn't packaged with
the "core" Skia library, we need a way to do it separately.

I definitely agree about merging tests (which becomes unittests), gm, and bench
under a "tests" umbrella, so that our top-level target lists becomes something
like:

- skia (the "core" library, currently in static form, but maybe appearing as
shared later)
- tests (unit tests, gm, bench)
- tools
- SampleApp
- Some set of "extra" libraries, such as animator, effects, pdf, etc etc.

https://codereview.appspot.com/6619049/diff/13064/gyp/bench.gyp
File gyp/bench.gyp (right):

https://codereview.appspot.com/6619049/diff/13064/gyp/bench.gyp#newcode27
gyp/bench.gyp:27: '../src/gpu',
On 2012/10/09 18:35:56, bsalomon wrote:
> On 2012/10/09 17:51:57, EricB wrote:
> > On 2012/10/09 16:14:07, epoger wrote:
> > > On 2012/10/08 14:44:48, EricB wrote:
> > > > I could've used direct_dependent_settings and export_dependent_settings
to
> > > > remove the need for the conditional addition of this include_dir, but it
> > seems
> > > > bad to always include something inside the src directory, even if we
> already
> > > do
> > > > it in lots of places.
> > > 
> > > Is it true that "bench" needs visibility into some GPU routines that most
> > > library-users do not?
> > > 
> > > If so, this seems like the right approach.
> > > 
> > > But if ALL users of the library need access to headers in src/gpu, it
seems
> > like
> > > this SHOULD be done by *_dependent_settings, or (more likely) those header
> > files
> > > should be moved into includes/ .
> > 
> > Without knowing the details of why we need to add src/gpu to our include
dirs,
> > I'd say that those things should be moved into include/gpu.
> 
> We want to directly bench classes in src/gpu that no Skia client should ever
see
> directly. I think bench, tests, and maybe gm will need to peer into src/gpu.

Then I'll leave this as-is, since we don't want to make that visible by default.

https://codereview.appspot.com/6619049/diff/13064/gyp/skia.gyp
File gyp/skia.gyp (right):

https://codereview.appspot.com/6619049/diff/13064/gyp/skia.gyp#newcode10
gyp/skia.gyp:10: 'action_output':
'<(PRODUCT_DIR)/<(static_lib_prefix)skia<(static_lib_suffix)',
On 2012/10/09 18:58:27, epoger wrote:
> On 2012/10/09 17:51:57, EricB wrote:
> > On 2012/10/09 16:14:07, epoger wrote:
> > > Ugh.  This is so hard to follow.  Variables set in common_variables.gypi,
> > which
> > > are used to set variables here, which are used to run a custom
> MergeStaticLibs
> > > below.
> > > 
> > > In Patchset 2, when the madness was confined to this file, I felt OK about
> it.
> > 
> > > Now I'm not so sure.
> > 
> > What I really wanted was to have a single list:
> > 
> > ['core', 'opts', 'ports', 'utils', extra_opts]
> > 
> > And then construct the dependencies, export_dependent_settings, inputs to
the
> > action, and the action command line itself from that list.  Unfortunately,
> > there's no way to say, "this list variable is formed by taking another list
> > variable and pre/appending this string."
> > 
> > I can place the static_lib_* variable definitions inside this file if that
> makes
> > it any better, but it seemed useful to have those available to any file.
> > 
> > Alternatively, I can do the dumb thing and just list everything outright,
> > although I'd still need platform-dependent path/prefix/suffix.
> 
> IIRC, most of this complication would go away if gyp would allow us to pass
> different cflags for the opts_* targets than the other targets.  Right?
> 
> If so, I'd really like to quarantine all of the ugliness within one file,
> commented to indicate our distaste at its existence.  This will make it easier
> to remove when (hopefully) we find a better way, such as a change to GYP.

Yes, but the problem is that we need cflags to be defined for a subset of
*files*.  GYP doesn't support file-specific settings, so the best we can do is
put those files in a separate target, which induces the need for "linking" them
together after the fact.

https://codereview.appspot.com/6619049/diff/13064/tools/merge_static_libs.py
File tools/merge_static_libs.py (right):

https://codereview.appspot.com/6619049/diff/13064/tools/merge_static_libs.py#...
tools/merge_static_libs.py:46: objects.append(object)
On 2012/10/09 18:58:27, epoger wrote:
> On 2012/10/09 17:51:57, EricB wrote:
> > On 2012/10/09 16:14:07, epoger wrote:
> > > Why not just use os.path.abspath() in both cases, so we don't need
parallel
> > code
> > > paths?  Would that not work?
> > > 
> > > Again, I feel like this workaround for what is basically a gyp shortcoming
> is
> > > getting pretty unwieldy.
> > 
> > It's more heavy-handed than I would like.  The alternatives as I see them:
> > 
> > 1. Make a change in GYP which allows file-specific settings, including
cflags.
> 
> > This is probably going to be difficult, time-consuming, and they may not
even
> > accept that kind of change.
> > 
> > 2. Use gypi's to build libskia.a with everything but opts_*, which would
> become
> > libskia_opts.a.  My complaint with this is that the two libraries would be
> > tightly coupled and it might not be obvious to a third party developer that
> both
> > are needed, or that they must be compiled together with exactly the same
> > settings.  It seems fragile.
> 
> I agree that #2 would be fragile.  Thanks for the explanation.
> 
> Would the hack be any smaller/simpler if we built it in TWO chunks (the
> granularity at which we need to use different cflags: everything-but-opts and
> opts) and then combined those, rather than building it in 4 chunks (core,
opts,
> ports, utils) and combining those?
> 
> Maybe if there were just two libraries to merge, just writing them out inline
> for the various cases (and deleting the static_lib_* variables) would be more
> reasonable?

I don't think the code would look any better, because that nasty set of
conditionals would occur in twice as many places.  I like the idea of using
gypi's for everything other than opts, but I don't think it reduces the
nastiness or necessity of the code in this file.

[epoger, 2012-10-09 19:35:05]

My brain runneth over.

If there is consensus among Eric/Brian/Derek that this is the right approach,
I'm all for it.

Otherwise, let's talk it over tomorrow.  Maybe it will make more sense when I'm
wearing pants.

[DerekS, 2012-10-09 20:00:54]

On 2012/10/09 19:35:05, epoger wrote:
> My brain runneth over.
> 
> If there is consensus among Eric/Brian/Derek that this is the right approach,
> I'm all for it.
> 
> Otherwise, let's talk it over tomorrow.  Maybe it will make more sense when
I'm
> wearing pants.

Let's setup a brief meeting when Elliot is in tomorrow to talk this through in
person as I've kinda lost track of the state of things as this CL has
progressed.

[bsalomon, 2012-10-09 20:58:53]

On 2012/10/09 20:00:54, DerekS wrote:
> On 2012/10/09 19:35:05, epoger wrote:
> > My brain runneth over.
> > 
> > If there is consensus among Eric/Brian/Derek that this is the right
approach,
> > I'm all for it.
> > 
> > Otherwise, let's talk it over tomorrow.  Maybe it will make more sense when
> I'm
> > wearing pants.
> 
> Let's setup a brief meeting when Elliot is in tomorrow to talk this through in
> person as I've kinda lost track of the state of things as this CL has
> progressed.

Sounds good. I'd like everyone to feel comfortable and welcome to express their
feelings about this CL in a safe environment so I propose a pants-free meeting.

[EricB, 2012-10-10 15:54:49]

Per today's discussion, I have modified the CL:
- No longer "merge" multiple static libraries into one
- Use a "skia_core_libs" meta-target for the basic set of required Skia
libraries
- Still include merge_static_libs.py

Basically, we get "non-thin" static libraries but we don't have a single,
primary library.  This is why merge_static_libs.py is included; in case someone
wants to glob them into one monster library.

I have made every library that I thought we might want to make available to
developers a standalone_static_library.  I may have gotten that set wrong;
please take a close look at which ones should and shouldn't be exported. 
Additionally, I believe that those libraries should be listed as part of "all"
in the top-level Makefile, so that they can be built directly.  Right now we'd
have to build one of the targets that depends on them in order to have them
built.

I'd like to be able to run "make skia_core_libs effects", which we can't
currently do since effects isn't listed at the top level.

[epoger, 2012-10-10 16:17:59]

LGTM

I think you have walked this tightrope just right.  I still have one question,
though...

https://codereview.appspot.com/6619049/diff/25006/gyp/core.gyp
File gyp/core.gyp (right):

https://codereview.appspot.com/6619049/diff/25006/gyp/core.gyp#newcode6
gyp/core.gyp:6: 'product_name': 'skia_core',
I'm confused as to why target 'core' in gyp/core.gyp has product_name
'skia_core' (as opposed to the new gyp/skia_core.gyp file)

Maybe you could add a bit of documentation at the top of these two files
explaining the difference?

[EricB, 2012-10-10 17:51:05]

Uploaded patch set 15.

https://codereview.appspot.com/6619049/diff/25006/gyp/core.gyp
File gyp/core.gyp (right):

https://codereview.appspot.com/6619049/diff/25006/gyp/core.gyp#newcode6
gyp/core.gyp:6: 'product_name': 'skia_core',
On 2012/10/10 16:17:59, epoger wrote:
> I'm confused as to why target 'core' in gyp/core.gyp has product_name
> 'skia_core' (as opposed to the new gyp/skia_core.gyp file)
> 
> Maybe you could add a bit of documentation at the top of these two files
> explaining the difference?

Renamed skia_core_libs to skia_base_libs and and added a slightly more
descriptive comment.

[EricB, 2012-10-10 18:19:17]

Uploaded patch set 15.

[epoger, 2012-10-10 19:09:17]

LGTM