Move sources lists to gyp include, add libskia target

Move sources lists to gyp include, add libskia target

[EricB, 2012-09-04 21:08:06]

This works as expected on my linux machine.  Given how much has changed, I want
to make sure that this works on all platforms before committing.

The point of making this change is to be able to build a standalone static
library for Skia to be used in other projects (eg NaCl).  Unfortunately, gyp is
hard-coded to use the -T flag to ar which builds a thin archive, meaning that
the static library we build doesn't actually contain any symbols.

See:
https://groups.google.com/forum/?fromgroups=#!searchin/gyp-developer/thin$20a...
https://groups.google.com/forum/?fromgroups=#!searchin/gyp-developer/thin$20a...
https://groups.google.com/forum/?fromgroups=#!searchin/gyp-developer/thin$20a...

I'm going to look into changing this.

[EricB, 2012-09-05 14:15:40]

Verified on Mac.

[EricB, 2012-09-05 14:20:26]

http://codereview.appspot.com/6498090/diff/7004/gyp/skia.gyp
File gyp/skia.gyp (right):

http://codereview.appspot.com/6498090/diff/7004/gyp/skia.gyp#newcode4
gyp/skia.gyp:4: 'target_name': 'skia',
Should this name be more descriptive?  It might be more clear to run "make
libskia" but keeping the name as just "skia" causes us to generate the desired
"libskia.a."

[EricB, 2012-09-05 14:31:22]

Verified on Windows.

[epoger, 2012-09-05 14:53:18]

Brian- would these changes interfere with Chrome's use of our .gypi files?

[epoger, 2012-09-05 14:56:11]

http://codereview.appspot.com/6498090/diff/7004/gyp/skia.gyp
File gyp/skia.gyp (right):

http://codereview.appspot.com/6498090/diff/7004/gyp/skia.gyp#newcode4
gyp/skia.gyp:4: 'target_name': 'skia',
On 2012/09/05 14:20:26, EricB wrote:
> Should this name be more descriptive?  It might be more clear to run "make
> libskia" but keeping the name as just "skia" causes us to generate the desired
> "libskia.a."

I think it's fine.  Maybe add a comment in here indicating that this does indeed
build the core library, and why it has this name.

If you want, I guess you could put an alias in the Makefile...

[bsalomon, 2012-09-05 15:28:48]

On 2012/09/05 14:53:18, epoger wrote:
> Brian- would these changes interfere with Chrome's use of our .gypi files?

I'm not sure. Chrome has it's own complex set of conditions
(http://src.chromium.org/viewvc/chrome/trunk/src/skia/skia.gyp?revision=153958...)
and it's not obvious to me whether they'd conflict with what has been added to
the gypis here. I don't think they define skia_os so that would be a problem.

[bsalomon, 2012-09-05 15:30:02]

On 2012/09/05 15:28:48, bsalomon wrote:
> On 2012/09/05 14:53:18, epoger wrote:
> > Brian- would these changes interfere with Chrome's use of our .gypi files?
> 
> I'm not sure. Chrome has it's own complex set of conditions
>
(http://src.chromium.org/viewvc/chrome/trunk/src/skia/skia.gyp?revision=153958...)
> and it's not obvious to me whether they'd conflict with what has been added to
> the gypis here. I don't think they define skia_os so that would be a problem.

Before we commit any version of this it'd be good to run a chromium try against
the changes so that we can verify on all the various platforms.

[epoger, 2012-09-05 17:07:45]

On 2012/09/05 15:30:02, bsalomon wrote:
> On 2012/09/05 15:28:48, bsalomon wrote:
> > On 2012/09/05 14:53:18, epoger wrote:
> > > Brian- would these changes interfere with Chrome's use of our .gypi files?
> > 
> > I'm not sure. Chrome has it's own complex set of conditions
> >
>
(http://src.chromium.org/viewvc/chrome/trunk/src/skia/skia.gyp?revision=153958...)
> > and it's not obvious to me whether they'd conflict with what has been added
to
> > the gypis here. I don't think they define skia_os so that would be a
problem.
> 
> Before we commit any version of this it'd be good to run a chromium try
against
> the changes so that we can verify on all the various platforms.

Good idea.  Eric, please run it through the trybots, and once those tries come
back clean please let me know (and I will review the change then).  OK?

See the "Trying out a change, created using SVN, that is under Rietveld code
review" section of
https://sites.google.com/site/skiadocs/developer-documentation/managing-chrom...

[epoger, 2012-09-05 18:06:21]

> Good idea.  Eric, please run it through the trybots, and once those tries come
> back clean please let me know (and I will review the change then).  OK?

Eric wasn't able to run this change through the Chromium trybots, because only
Chromium committers can do that.

So I ran it for him (just through the linux trybot, because we're pretty sure it
will fail anyway).  Here's the command I ran, based on
https://sites.google.com/site/skiadocs/developer-documentation/managing-chrom...
:

trychange.py --verbose --no_search \
  --use_svn --svn_repo=svn://svn.chromium.org/chrome-try/try \
  --root=src/third_party/skia \
  --bot=linux \
  --rietveld_url=https://codereview.appspot.com/6498090 \
  --email=borenet@google.com

I was prompted for a password on authentication realm svn.chromium.org, so I
entered a username of epoger@google.com and the password shown at
https://chromium-access.appspot.com/ .

Then I got this message:
Patch 'Issue 6498090' sent to try server: linux

[EricB, 2012-09-05 19:36:46]

Looks like this failed on the "patch" step.  I have to assume that this will
break all kinds of stuff in Chrome, so I'm going to move the platform-specific
portions out of the gypi's, leaving just the sources list.

[EricB, 2012-09-05 21:37:06]

Uploaded patch set 7.  I'm using a new, multiple-include-level strategy, in
which only sources are listed in the deepest-level gypi, platform-specific build
flags, source excludes, include directories, etc go in a middle-level gypi, and
the top-level gyp file has target-level dependency information.  This may be
more indirection than is necessary, but it avoids duplication of the
platform-specific stuff when we need it again for libskia.  I've verified that
this works properly on my linux machine.  I will verify on Mac and Windows
tomorrow.  We'll want to run Chromium tries again.

[epoger, 2012-09-06 14:37:28]

On 2012/09/05 21:37:06, EricB wrote:
> Uploaded patch set 7.  I'm using a new, multiple-include-level strategy, in
> which only sources are listed in the deepest-level gypi, platform-specific
build
> flags, source excludes, include directories, etc go in a middle-level gypi,
and
> the top-level gyp file has target-level dependency information.  This may be
> more indirection than is necessary, but it avoids duplication of the
> platform-specific stuff when we need it again for libskia.  I've verified that
> this works properly on my linux machine.  I will verify on Mac and Windows
> tomorrow.  We'll want to run Chromium tries again.

I just sent this off to the "linux" Chrome trybot at patchset 7, using the same
command line as last time.

Eric, when you get the trybot result email, please paste a link to the trybot
results into this codereview.  I would be curious to know more about the errors
it encountered.  (Once you become a Chromium committer and can run your own
trybots, links to the trybot results will automatically appear in the
codereview.)

The multiple-include-level strategy seems like the "least bad" option given our
constraints at present, but I sure would like to find a cleaner way going
forward.  (I can imagine this triggering despair and rage as people come across
it for the first time.)  There really really ought to be a way that we could
have gyp's in one package depend on gyp's in another.

In the meanwhile, I suggest that you back up a bit and first commit a CL that
just renames the existing .gypi's to *_source.gypi (and when we DEPS-roll that
into Chrome, we'll have to change the Chrome .gyp's to use those new names).  At
least that will make it clearer that those .gypi's only include a list of the
source files.

Brian, what do you think of all this?

[EricB, 2012-09-06 14:45:01]

Here's the trybot failure link:
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/3057

It's failing on the patch step, before it even gets to the compile.  I'm
guessing that Chrome is using a different version of the top-level Skia Makefile
and skia.gyp and so those can't be patched since there are conflicts?

I agree that this solution is messy; I think that starting with renaming all of
the existing gypi's to *_source.gypi makes sense, and I think that if this is
the direction we're choosing we need to enforce that NO source files are
included outside of a file named anything other than *_source.gypi.

Then maybe the second revision could separate the middle layer out and call the
includes *_platform.gypi, or something similar, and we'd make sure to only place
source *excludes* along with platform-specific include directories, flags, etc,
leaving only target-level dependency information in the .gyp files.  Again, if
this is the direction we choose then we need to do this for ALL of our gyp/gypi
files and find a way to enforce it in order to keep things as clean as possible.

[bsalomon, 2012-09-06 14:47:19]

On 2012/09/06 14:37:28, epoger wrote:
> On 2012/09/05 21:37:06, EricB wrote:
> > Uploaded patch set 7.  I'm using a new, multiple-include-level strategy, in
> > which only sources are listed in the deepest-level gypi, platform-specific
> build
> > flags, source excludes, include directories, etc go in a middle-level gypi,
> and
> > the top-level gyp file has target-level dependency information.  This may be
> > more indirection than is necessary, but it avoids duplication of the
> > platform-specific stuff when we need it again for libskia.  I've verified
that
> > this works properly on my linux machine.  I will verify on Mac and Windows
> > tomorrow.  We'll want to run Chromium tries again.
> 
> I just sent this off to the "linux" Chrome trybot at patchset 7, using the
same
> command line as last time.
> 
> Eric, when you get the trybot result email, please paste a link to the trybot
> results into this codereview.  I would be curious to know more about the
errors
> it encountered.  (Once you become a Chromium committer and can run your own
> trybots, links to the trybot results will automatically appear in the
> codereview.)
> 
> The multiple-include-level strategy seems like the "least bad" option given
our
> constraints at present, but I sure would like to find a cleaner way going
> forward.  (I can imagine this triggering despair and rage as people come
across
> it for the first time.)  There really really ought to be a way that we could
> have gyp's in one package depend on gyp's in another.
> 
> In the meanwhile, I suggest that you back up a bit and first commit a CL that
> just renames the existing .gypi's to *_source.gypi (and when we DEPS-roll that
> into Chrome, we'll have to change the Chrome .gyp's to use those new names). 
At
> least that will make it clearer that those .gypi's only include a list of the
> source files.
> 
> Brian, what do you think of all this?

It seems like a fair amount of additional complexity in an already pretty
complex set of build files. However, I don't really grok the issue that
motivated this change (what are archives vs thin archives?). On the groups posts
that Eric linked to Evan says he isn't opposed to a GYP change adding support
regular archives. Would that allow us to avoid doing this? If so, is it
difficult to patch GYP?

[EricB, 2012-09-06 14:54:55]

On 2012/09/06 14:47:19, bsalomon wrote:
> On 2012/09/06 14:37:28, epoger wrote:
> > On 2012/09/05 21:37:06, EricB wrote:
> > > Uploaded patch set 7.  I'm using a new, multiple-include-level strategy,
in
> > > which only sources are listed in the deepest-level gypi, platform-specific
> > build
> > > flags, source excludes, include directories, etc go in a middle-level
gypi,
> > and
> > > the top-level gyp file has target-level dependency information.  This may
be
> > > more indirection than is necessary, but it avoids duplication of the
> > > platform-specific stuff when we need it again for libskia.  I've verified
> that
> > > this works properly on my linux machine.  I will verify on Mac and Windows
> > > tomorrow.  We'll want to run Chromium tries again.
> > 
> > I just sent this off to the "linux" Chrome trybot at patchset 7, using the
> same
> > command line as last time.
> > 
> > Eric, when you get the trybot result email, please paste a link to the
trybot
> > results into this codereview.  I would be curious to know more about the
> errors
> > it encountered.  (Once you become a Chromium committer and can run your own
> > trybots, links to the trybot results will automatically appear in the
> > codereview.)
> > 
> > The multiple-include-level strategy seems like the "least bad" option given
> our
> > constraints at present, but I sure would like to find a cleaner way going
> > forward.  (I can imagine this triggering despair and rage as people come
> across
> > it for the first time.)  There really really ought to be a way that we could
> > have gyp's in one package depend on gyp's in another.
> > 
> > In the meanwhile, I suggest that you back up a bit and first commit a CL
that
> > just renames the existing .gypi's to *_source.gypi (and when we DEPS-roll
that
> > into Chrome, we'll have to change the Chrome .gyp's to use those new names).

> At
> > least that will make it clearer that those .gypi's only include a list of
the
> > source files.
> > 
> > Brian, what do you think of all this?
> 
> It seems like a fair amount of additional complexity in an already pretty
> complex set of build files. However, I don't really grok the issue that
> motivated this change (what are archives vs thin archives?). On the groups
posts
> that Eric linked to Evan says he isn't opposed to a GYP change adding support
> regular archives. Would that allow us to avoid doing this? If so, is it
> difficult to patch GYP?

In order to build a static library for Skia, we need *both* changes.  This build
system alteration allows us to build libskia.a, but gyp by default builds a
"thin archive," meaning that the static library does not actually contain any
symbols but only forwards the linker to the object files which contain those
symbols.  This speeds up the build significantly for Chrome since it drastically
reduces the amount of copying, but it means that we can't produce a
*stand-alone* static library to be linked by applications which aren't using
gyp.

As an alternative to this rather big and messy change, we could simply add
gyp/skia.gyp which would basically consist of the contents of core.gyp,
ports.gyp, utils.gyp, and opts.gyp, copy-pasted.  That would be hard to
maintain.

[EricB, 2012-09-06 15:18:33]

I just had a live discussion with Brian in which we decided that, rather than
overhauling our gyp system to work around the fact that gyp does not support
building a static library as a "final" target, it would be better to try to
change gyp itself, so that we can add the libskia target as a static library
which is marked as a top-level, "final" target so that all of its dependencies
are linked into libskia.a and a "fat archive" is built.  In other words, we want
to modify gyp so that we can build a static_library in the same way that we
build a shared_library, allowing us to do something like:
{
'target_name': 'skia',
'type': 'static_library_as_final_target',
'dependencies': [
  'core.gyp:core',
  'ports.gyp:ports',
  'utils.gyp:utils',
  'opts.gyp:opts',
],
}
And all of the symbols from all of the dependencies get dumped into libskia.a,
just as would be done with a shared_library target, and libskia.a is a "fat"
archive.  I'm going to investigate this briefly to determine how feasible it
would be.  Since I agree with Elliot that, if we're going to overhaul our gyp
system, it would be easier to do this incrementally, I'm going to abandon this
change.  If we decide that we do need to modify our gyp system, I'll upload new
changes that do it incrementally.

[epoger, 2012-09-06 15:34:38]

On 2012/09/06 15:18:33, EricB wrote:
> I just had a live discussion with Brian

I think your plan of action sounds good.

Ideally, we could modify gyp to allow something like you describe.  If that
DOESN'T work out, perhaps we could harness the fact that, in
https://code.google.com/p/skia/source/browse/trunk/gyp_skia , we can force
additional settings that will affect only OUR gyp runs (as opposed to gyp runs
in other projects that depend on our .gyp/.gypi files).  So maybe gyp_skia could
set a variable that switches things on and off in the rest of the .gyp/.gypi
files when we're building as a static library...

As for the failing patch (in Makefile and skia.gyp), I see what the problem
is... http://src.chromium.org/viewvc/chrome/trunk/src/DEPS?view=markup imports
Skia's gyp/ subdirectory, but not Skia's toplevel Makefile and skia.gyp files. 
So it has nothing to patch those against.  In this unusual case, our best bet to
run the trybots would be to hand-edit the patch file we send to the trybots and
remove those two files.

[EricB, 2012-09-06 15:36:38]

On 2012/09/06 15:34:38, epoger wrote:
> On 2012/09/06 15:18:33, EricB wrote:
> > I just had a live discussion with Brian
> 
> I think your plan of action sounds good.
> 
> Ideally, we could modify gyp to allow something like you describe.  If that
> DOESN'T work out, perhaps we could harness the fact that, in
> https://code.google.com/p/skia/source/browse/trunk/gyp_skia , we can force
> additional settings that will affect only OUR gyp runs (as opposed to gyp runs
> in other projects that depend on our .gyp/.gypi files).  So maybe gyp_skia
could
> set a variable that switches things on and off in the rest of the .gyp/.gypi
> files when we're building as a static library...
> 
> As for the failing patch (in Makefile and skia.gyp), I see what the problem
> is... http://src.chromium.org/viewvc/chrome/trunk/src/DEPS?view=markup imports
> Skia's gyp/ subdirectory, but not Skia's toplevel Makefile and skia.gyp files.

> So it has nothing to patch those against.  In this unusual case, our best bet
to
> run the trybots would be to hand-edit the patch file we send to the trybots
and
> remove those two files.

Sounds good.  I'm closing this issue.