Extract use_system_foo logic out of gyp files, patch 2

Extract use_system_foo logic out of gyp files, patch 2

This removes use_system_foo logic completely from:
 * jsoncpp.gyp
 * libevent.gyp
 * libpng.gyp
 * libusb.gyp
 * libwebp.gyp
 * opus.gyp
 * re2.gyp
 * zlib.gyp

R=rsleevi
TBR=darin

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

[Lei Zhang, 2013-04-25 22:06:49]

rsleevi's probably more familiar, but I like this approach in general.

[Ryan Sleevi, 2013-04-25 22:19:19]

The third_party/ code appears to just be de-indenting, so I've ignored focusing
on that. Please let me know if there is something special I should pay attention
to.

Mostly LG, but a few comments for you while doing your porting.

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libevent...
File build/linux/unbundle/libevent.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libevent...
build/linux/unbundle/libevent.gyp:10: 'toolsets': ['host', 'target'],
I'm surprised to see this is necessary. Is this perhaps accidental/unnecessary
porting?

Having trouble seeing how the same link settings would work correctly for both
host and target. It's a different story when we're compiling, because we
generate different .o's and different libraries.

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libpng.gyp
File build/linux/unbundle/libpng.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libpng.g...
build/linux/unbundle/libpng.gyp:11: '../zlib/zlib.gyp:zlib',
Are you sure this is correct? This is stating that if you use libpng, you MUST
use system zlib.

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libwebp.gyp
File build/linux/unbundle/libwebp.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libwebp....
build/linux/unbundle/libwebp.gyp:17: # Check for presence of webpdemux library,
use it if present.
I don't suppose you can just treat this as a variable?

I thought you were among the (many) who hate automagic dependencies, which this
definitely seems to be.

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libxml.gyp
File build/linux/unbundle/libxml.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libxml.g...
build/linux/unbundle/libxml.gyp:8: 'target_name': 'libxml',
Pretty sure you're going to want to name this separately, to avoid any naming
conflicts with the system library

[Paweł Hajdan Jr., 2013-04-25 22:41:50]

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libevent...
File build/linux/unbundle/libevent.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libevent...
build/linux/unbundle/libevent.gyp:10: 'toolsets': ['host', 'target'],
On 2013/04/25 22:19:19, Ryan Sleevi wrote:
> I'm surprised to see this is necessary. Is this perhaps accidental/unnecessary
> porting?
> 
> Having trouble seeing how the same link settings would work correctly for both
> host and target. It's a different story when we're compiling, because we
> generate different .o's and different libraries.

This is just what _was_ in use_system_libevent branch of the gyp file.

I'd strongly prefer to keep this CL just an extraction, not actual changes to
the code.

This option works as is. I can take a look later whether this is really
necessary.

Note that even when not cross-compiling, the build system will "instantiate"
build rules for host/target, where host==target anyway, but things will be built
twice. That's the most likely reason this code is here.

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libpng.gyp
File build/linux/unbundle/libpng.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libpng.g...
build/linux/unbundle/libpng.gyp:11: '../zlib/zlib.gyp:zlib',
On 2013/04/25 22:19:19, Ryan Sleevi wrote:
> Are you sure this is correct? This is stating that if you use libpng, you MUST
> use system zlib.

This is not the case. What makes you think so?

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libwebp.gyp
File build/linux/unbundle/libwebp.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libwebp....
build/linux/unbundle/libwebp.gyp:17: # Check for presence of webpdemux library,
use it if present.
On 2013/04/25 22:19:19, Ryan Sleevi wrote:
> I don't suppose you can just treat this as a variable?

I could make it require libwebp-0.3...

> I thought you were among the (many) who hate automagic dependencies, which
this
> definitely seems to be.

I think it's not. We always depend on libwebp (as compared to detecting its
absence and silently disabling libwebp support). When system version of libwebp
changes, SONAME will make it do the right thing. I have tested various
scenarios, and I think this works.

Eventually we'll be able to drop this compatibility code and always require
-lwebpdemux.

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libxml.gyp
File build/linux/unbundle/libxml.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libxml.g...
build/linux/unbundle/libxml.gyp:8: 'target_name': 'libxml',
On 2013/04/25 22:19:19, Ryan Sleevi wrote:
> Pretty sure you're going to want to name this separately, to avoid any naming
> conflicts with the system library

This is just extracting existing code that was in use. No such problem has been
observed.

If it ever becomes a problem, it'll be easy to fix in a separate CL.

[Ryan Sleevi, 2013-04-25 23:12:40]

lgtm

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libevent...
File build/linux/unbundle/libevent.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libevent...
build/linux/unbundle/libevent.gyp:10: 'toolsets': ['host', 'target'],
On 2013/04/25 22:41:50, Paweł Hajdan Jr. wrote:
> On 2013/04/25 22:19:19, Ryan Sleevi wrote:
> > I'm surprised to see this is necessary. Is this perhaps
accidental/unnecessary
> > porting?
> > 
> > Having trouble seeing how the same link settings would work correctly for
both
> > host and target. It's a different story when we're compiling, because we
> > generate different .o's and different libraries.
> 
> This is just what _was_ in use_system_libevent branch of the gyp file.
> 
> I'd strongly prefer to keep this CL just an extraction, not actual changes to
> the code.
> 
> This option works as is. I can take a look later whether this is really
> necessary.
> 
> Note that even when not cross-compiling, the build system will "instantiate"
> build rules for host/target, where host==target anyway, but things will be
built
> twice. That's the most likely reason this code is here.

Since it sounds like you're unwilling to try to improve this CL, and that code
in build/linux/unbundle is "unsupported", I'm fine stamping this.

I'm still convinced it's wrong though. You should only need a toolset
declaration if a toolchain built separately for host/target depends on this, and
if it does, you will not get the correct build results here. I think the
original gyp for use_system_libevent was wrong - and it may even be that it's
unnecessary for the non-use_system_libevent.

I would love to think that the Chromium goal of "Leave things better than how
you found them" was practiced here, and this was investigated, but since this
isn't "really" Chromium code, I don't feel like it would be useful to block this
CL on cleaning it up. This will be something for those unbundling to maintain -
which admittedly is the whole goal of splitting this out.

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libpng.gyp
File build/linux/unbundle/libpng.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libpng.g...
build/linux/unbundle/libpng.gyp:11: '../zlib/zlib.gyp:zlib',
On 2013/04/25 22:41:50, Paweł Hajdan Jr. wrote:
> On 2013/04/25 22:19:19, Ryan Sleevi wrote:
> > Are you sure this is correct? This is stating that if you use libpng, you
MUST
> > use system zlib.
> 
> This is not the case. What makes you think so?

Apologies, I misread the path of this file to think it was
build/linux/unbundle/libpng/libpng.gyp (eg: that ../ still kept you in
build/linux/unbundle)

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libwebp.gyp
File build/linux/unbundle/libwebp.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libwebp....
build/linux/unbundle/libwebp.gyp:17: # Check for presence of webpdemux library,
use it if present.
On 2013/04/25 22:41:50, Paweł Hajdan Jr. wrote:
> On 2013/04/25 22:19:19, Ryan Sleevi wrote:
> > I don't suppose you can just treat this as a variable?
> 
> I could make it require libwebp-0.3...
> 
> > I thought you were among the (many) who hate automagic dependencies, which
> this
> > definitely seems to be.
> 
> I think it's not. We always depend on libwebp (as compared to detecting its
> absence and silently disabling libwebp support). When system version of
libwebp
> changes, SONAME will make it do the right thing. I have tested various
> scenarios, and I think this works.

How is this not exactly that - attempting to detect -lwebpdemux support, and
silently disabling it if not present?

I don't feel strongly about this, since this is in the 'unmaintained' area, but
it seems worth calling out.

> 
> Eventually we'll be able to drop this compatibility code and always require
> -lwebpdemux.

[Paweł Hajdan Jr., 2013-04-26 00:20:38]

Committed patchset #1 manually as r196551 (presubmit successful).

[Paweł Hajdan Jr., 2013-04-26 00:55:18]

+Darin TBR third_party OWNERS

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libevent...
File build/linux/unbundle/libevent.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libevent...
build/linux/unbundle/libevent.gyp:10: 'toolsets': ['host', 'target'],
Ryan, don't worry - I do care about quality. I just prefer to split trivial code
moves/extractions from changes which modify semantics.

I checked, and removal of it results in the following error on
build/gyp_chromium:

gyp: Dependency
'/home/phajdan/chromium/src/third_party/libevent/libevent.gyp:libevent#host' not
found while trying to load target
/home/phajdan/chromium/src/base/base.gyp:base#host

If there is something to be fixed there, I can do that - just in a separate CL,
this could get a bit larger as a cleanup.

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libwebp.gyp
File build/linux/unbundle/libwebp.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libwebp....
build/linux/unbundle/libwebp.gyp:17: # Check for presence of webpdemux library,
use it if present.
On 2013/04/25 23:12:40, Ryan Sleevi wrote:
> How is this not exactly that - attempting to detect -lwebpdemux support, and
> silently disabling it if not present?

I'll take a closer look at how libwebp handles SONAME bump there. libwebp.so's
SONAME is the same between libwebp-0.2 and libwebp-0.3, which is a bit
suspicious - but this will require some more looking and testing before I say
something more.

> I don't feel strongly about this, since this is in the 'unmaintained' area,
but
> it seems worth calling out.

Thank you for being persistent in bringing this to my attention - it could be an
issue I have overlooked.

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libxml.gyp
File build/linux/unbundle/libxml.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libxml.g...
build/linux/unbundle/libxml.gyp:8: 'target_name': 'libxml',
On 2013/04/25 22:19:19, Ryan Sleevi wrote:
> Pretty sure you're going to want to name this separately, to avoid any naming
> conflicts with the system library

I tested it and it works. Here's a possible explanation: the library's name is
libxml2, while the target name is libxml.

[Paweł Hajdan Jr., 2013-04-26 18:21:40]

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libwebp.gyp
File build/linux/unbundle/libwebp.gyp (right):

https://codereview.chromium.org/14203006/diff/1/build/linux/unbundle/libwebp....
build/linux/unbundle/libwebp.gyp:17: # Check for presence of webpdemux library,
use it if present.
On 2013/04/26 00:55:18, Paweł Hajdan Jr. wrote:
> On 2013/04/25 23:12:40, Ryan Sleevi wrote:
> > How is this not exactly that - attempting to detect -lwebpdemux support, and
> > silently disabling it if not present?
> 
> I'll take a closer look at how libwebp handles SONAME bump there. libwebp.so's
> SONAME is the same between libwebp-0.2 and libwebp-0.3, which is a bit
> suspicious - but this will require some more looking and testing before I say
> something more.
> 
> > I don't feel strongly about this, since this is in the 'unmaintained' area,
> but
> > it seems worth calling out.
> 
> Thank you for being persistent in bringing this to my attention - it could be
an
> issue I have overlooked.

I have further investigated this. Chrome binary built against libwebp-0.2 works
when linked against libwebp-0.3 . Backward compatibility of libwebp library is
preserved. Binary linked against libwebp-0.3's libwebpdemux will not run against
libwebp-0.2, but I'm not aware of expectations of such forward compatibility in
the Linux world.

As far as I can tell things are OK here.

[Noel Gordon, 2013-05-10 04:28:35]

Well making changes to webp image decoder in Blink is a real pain given all
these webp versions. Since we don't need to support WebKit ports anymore, I'd
really like to simplify and require libwebp/mux 0.3.

Also, it's getting very hard to work out just reading .gypi files to determine
who/what is turning on a use_system_foo, at least for me.  So for libpng, noting
https://chromiumcodereview.appspot.com/9365007 I was curious to know if the
changes here undid that?

[Paweł Hajdan Jr., 2013-05-10 17:14:25]

On 2013/05/10 04:28:35, Noel Gordon (Google) wrote:
> Well making changes to webp image decoder in Blink is a real pain given all
> these webp versions. Since we don't need to support WebKit ports anymore, I'd
> really like to simplify and require libwebp/mux 0.3.

Don't worry about bits in build/linux/unbundle - they are not used for Chrome
build.

> Also, it's getting very hard to work out just reading .gypi files to determine
> who/what is turning on a use_system_foo, at least for me.

This is exactly what the linked bug is about. :) Feel free to star it and check
out other changes there.

> So for libpng, noting
> https://chromiumcodereview.appspot.com/9365007 I was curious to know if the
> changes here undid that?

Yes, this patch removes use_system_foo bits from libpng.gyp completely.