Add "standalone_static_library" flag

Add "standalone_static_library" target flag

This flag is only valid for targets of type "static_library"

Adding 'standalone_static_library': 1, ensures:
1) "thin" archive optimisation is disabled for this target (it will contain
   symbols)
2) The static_library will be "installed" to PRODUCT_DIR

This change re-lands r1510-1512.

Landed in gyp r1516/r1517

[borenet, 2012-10-01 17:31:43]

There is a good bit of demand in Skia for this feature (see skia-discuss, where
five out of six of the most recent threads ask for it directly or indirectly).

It might be nicer to set a flag on a given target or even to add another target
type such as, "standalone_static_library," which would not only result in a
"fat" static library, but would cause it to be copied to the base of the build
directory, rather than hiding in obj.target.

[Ryan Sleevi, 2012-10-01 17:39:54]

https://codereview.chromium.org/11031005/diff/1/pylib/gyp/generator/make.py
File pylib/gyp/generator/make.py (right):

https://codereview.chromium.org/11031005/diff/1/pylib/gyp/generator/make.py#n...
pylib/gyp/generator/make.py:1993: if generator_flags.get('fat_static_libraries',
False):
random drive by: If you're going to go through the trouble of setting up arflags
per-target (which seems perfectly reasonable), why force this to be managed by a
generator flag?

Why not just let the target (or an include file, whether ~/.gyp/include.gypi,
the equivalent-to-Chromium's build/common.gypi, or a command-line generator
define) determine which targets are fat and which aren't?

[borenet, 2012-10-01 18:07:10]

https://codereview.chromium.org/11031005/diff/1/pylib/gyp/generator/make.py
File pylib/gyp/generator/make.py (right):

https://codereview.chromium.org/11031005/diff/1/pylib/gyp/generator/make.py#n...
pylib/gyp/generator/make.py:1993: if generator_flags.get('fat_static_libraries',
False):
On 2012/10/01 17:39:54, Ryan Sleevi wrote:
> random drive by: If you're going to go through the trouble of setting up
arflags
> per-target (which seems perfectly reasonable), why force this to be managed by
a
> generator flag?
> 
> Why not just let the target (or an include file, whether ~/.gyp/include.gypi,
> the equivalent-to-Chromium's build/common.gypi, or a command-line generator
> define) determine which targets are fat and which aren't?

This is what I was contemplating in my comment; it would be nice to be able to
specify that a static library should be considered a "final" and "standalone"
target, which would imply a fat static library and would cause the library to be
copied to the same top-level build directory where, for example, an executable
target would be copied.  In this case, I think adding a "standalone" flavor of
static library would be the best option, but I hesitate to just start adding
target types.

I guess another option would be to allow the target itself to override arflags.

[Sam Clegg, 2012-10-01 18:36:04]

On 2012/10/01 18:07:10, borenet wrote:
> https://codereview.chromium.org/11031005/diff/1/pylib/gyp/generator/make.py
> File pylib/gyp/generator/make.py (right):
> 
>
https://codereview.chromium.org/11031005/diff/1/pylib/gyp/generator/make.py#n...
> pylib/gyp/generator/make.py:1993: if
generator_flags.get('fat_static_libraries',
> False):
> On 2012/10/01 17:39:54, Ryan Sleevi wrote:
> > random drive by: If you're going to go through the trouble of setting up
> arflags
> > per-target (which seems perfectly reasonable), why force this to be managed
by
> a
> > generator flag?
> > 
> > Why not just let the target (or an include file, whether
~/.gyp/include.gypi,
> > the equivalent-to-Chromium's build/common.gypi, or a command-line generator
> > define) determine which targets are fat and which aren't?
> 
> This is what I was contemplating in my comment; it would be nice to be able to
> specify that a static library should be considered a "final" and "standalone"
> target, which would imply a fat static library and would cause the library to
be
> copied to the same top-level build directory where, for example, an executable
> target would be copied.  In this case, I think adding a "standalone" flavor of
> static library would be the best option, but I hesitate to just start adding
> target types.
> 
> I guess another option would be to allow the target itself to override
arflags.

I agree that we should probably come up with a way to distinguish
between libraries that purely local to the build and what you
call "final" libraries.  I don't know the best way to do that though.

Also, the use of the word 'fat' here is a little confusing as apple
use that word already WRT to libraries and executables with a
completely different meaning.  I would stick to ar's use of the word
"thin" even though it means using "non-thin" to describe normal 
libraries :(

[borenet, 2012-10-01 21:27:48]

Uploaded patch set 2. Since I didn't meet a lot of resistance to the idea of
adding a target type for a "non-thin," "final" static library, I changed this CL
to add a "standalone_static_library" type.  I have only tested this with make.

https://codereview.chromium.org/11031005/diff/6001/pylib/gyp/generator/make.py
File pylib/gyp/generator/make.py (right):

https://codereview.chromium.org/11031005/diff/6001/pylib/gyp/generator/make.p...
pylib/gyp/generator/make.py:1547: postbuilds=postbuilds)
What should be the behavior when a standalone_static_library depends on a
static_library or another standalone_static_library?  I would think that the
symbols from the dependency should get included in the
standalone_static_library.

[Nico, 2012-10-01 21:34:17]

This is to disable the thin archive optimization on a per-target level, right? I
thoguht about doing this a few times, and I always thought I'd add a new
attribute, say 'force_standalone_static_library' or 'is_distributable', which
people would add to the targets that should become fat static libraries.

That way, you only have to change the generators that have a thin static library
optimization, and more importantly, no new target type appears (which
potentially has new dependency rules, etc).

Does that make sense?

[Mark Mentovai, 2012-10-01 21:35:46]

As written, I don’t think this will work properly. static_library has special
meanings in input.py too, and I’d hate to have to maintain another static
library type.

[borenet, 2012-10-02 12:09:23]

I agree that it doesn't make a lot of sense to have two almost-identical target
types.  I'm not sure I like the addition of another attribute to force behavior
that intuitively should be the default.  Would it make more sense (with the
obvious objection that it would require changes in Chromium) to instead have a
"thin_archive_optimization" flag which defaults to zero but can be set to one?

[Nico, 2012-10-02 12:41:26]

On 2012/10/02 12:09:23, borenet wrote:
> I agree that it doesn't make a lot of sense to have two almost-identical
target
> types.  I'm not sure I like the addition of another attribute to force
behavior
> that intuitively should be the default.  Would it make more sense (with the
> obvious objection that it would require changes in Chromium) to instead have a
> "thin_archive_optimization" flag which defaults to zero but can be set to one?

At least in Chromium, we'd want to set this on every static library. On the
other hand, there's only one target (libskia.a) that wants a non-thin library,
so I'd say the fast path is the usually desired path.

[borenet, 2012-10-02 12:47:45]

On 2012/10/02 12:41:26, Nico wrote:
> On 2012/10/02 12:09:23, borenet wrote:
> > I agree that it doesn't make a lot of sense to have two almost-identical
> target
> > types.  I'm not sure I like the addition of another attribute to force
> behavior
> > that intuitively should be the default.  Would it make more sense (with the
> > obvious objection that it would require changes in Chromium) to instead have
a
> > "thin_archive_optimization" flag which defaults to zero but can be set to
one?
> 
> At least in Chromium, we'd want to set this on every static library. On the
> other hand, there's only one target (libskia.a) that wants a non-thin library,
> so I'd say the fast path is the usually desired path.

Fair enough.  I'll modify this to add a "standalone_static_library" flag.

[borenet, 2012-10-02 15:39:21]

Uploaded patch set 3, which instead adds a "standalone_static_library" flag.

[Nico, 2012-10-02 15:52:04]

Thanks, I like this much better.

. Can you update the CL description with the exact semantics of this attribute?
I suppose it's a) non-thin standalone library b) library appears in
<(PRODUCT_DIR) c) library is called
<(STATIC_LIBRARY_PREFIX)targetname<(STATIC_LIBRARY_SUFFIX)?

. Can you add a test that checks the semantics described above? (see the test/
directory; `python gyptest.py -a` will run all tests, `python -f ninja test/mac`
will run only the tests below test/mac, and only for ninja)

Thanks for working on this!

[borenet, 2012-10-02 19:50:37]

Uploaded patch set 4, which adds a test.

[Nico, 2012-10-03 03:42:14]

LGTM!

Let me know if you want me to land this. (In that case, I can address the nit
below myself before committing.)

https://codereview.chromium.org/11031005/diff/11001/test/standalone-static-li...
File test/standalone-static-library/gyptest-standalone-static-library.py
(right):

https://codereview.chromium.org/11031005/diff/11001/test/standalone-static-li...
test/standalone-static-library/gyptest-standalone-static-library.py:21: # Verify
that the static library is copied to PRODUCT_DIR
nit:Style guide says to have trailing '.'s on all comments

[borenet, 2012-10-03 12:04:06]

On 2012/10/03 03:42:14, Nico wrote:
> LGTM!
> 
> Let me know if you want me to land this. (In that case, I can address the nit
> below myself before committing.)
> 
>
https://codereview.chromium.org/11031005/diff/11001/test/standalone-static-li...
> File test/standalone-static-library/gyptest-standalone-static-library.py
> (right):
> 
>
https://codereview.chromium.org/11031005/diff/11001/test/standalone-static-li...
> test/standalone-static-library/gyptest-standalone-static-library.py:21: #
Verify
> that the static library is copied to PRODUCT_DIR
> nit:Style guide says to have trailing '.'s on all comments

That would be great.  Thanks!

[Nico, 2012-10-03 14:06:13]

On 2012/10/03 12:04:06, borenet wrote:
> On 2012/10/03 03:42:14, Nico wrote:
> > LGTM!
> > 
> > Let me know if you want me to land this. (In that case, I can address the
nit
> > below myself before committing.)
> > 
> >
>
https://codereview.chromium.org/11031005/diff/11001/test/standalone-static-li...
> > File test/standalone-static-library/gyptest-standalone-static-library.py
> > (right):
> > 
> >
>
https://codereview.chromium.org/11031005/diff/11001/test/standalone-static-li...
> > test/standalone-static-library/gyptest-standalone-static-library.py:21: #
> Verify
> > that the static library is copied to PRODUCT_DIR
> > nit:Style guide says to have trailing '.'s on all comments
> 
> That would be great.  Thanks!

I landed this in 1510 (and addressed the nit in 1511). Sadly, all bots on
go/gypbot went red, for various reasons. I tried to fix in 1512, but didn't fix
everything, so I reverted it all in 1513. Can you run trybots for this change?
bungeman@ in your office can tell you how to do that I believe.

[borenet, 2012-10-03 23:17:20]

On 2012/10/03 14:06:13, Nico wrote:
> On 2012/10/03 12:04:06, borenet wrote:
> > On 2012/10/03 03:42:14, Nico wrote:
> > > LGTM!
> > > 
> > > Let me know if you want me to land this. (In that case, I can address the
> nit
> > > below myself before committing.)
> > > 
> > >
> >
>
https://codereview.chromium.org/11031005/diff/11001/test/standalone-static-li...
> > > File test/standalone-static-library/gyptest-standalone-static-library.py
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/11031005/diff/11001/test/standalone-static-li...
> > > test/standalone-static-library/gyptest-standalone-static-library.py:21: #
> > Verify
> > > that the static library is copied to PRODUCT_DIR
> > > nit:Style guide says to have trailing '.'s on all comments
> > 
> > That would be great.  Thanks!
> 
> I landed this in 1510 (and addressed the nit in 1511). Sadly, all bots on
> go/gypbot went red, for various reasons. I tried to fix in 1512, but didn't
fix
> everything, so I reverted it all in 1513. Can you run trybots for this change?
> bungeman@ in your office can tell you how to do that I believe.

Looks like the trybots pass with patch set 13.

[Nico, 2012-10-04 02:18:58]

https://codereview.chromium.org/11031005/diff/15016/pylib/gyp/generator/make.py
File pylib/gyp/generator/make.py (right):

https://codereview.chromium.org/11031005/diff/15016/pylib/gyp/generator/make....
pylib/gyp/generator/make.py:1544: if (sys.platform.startswith('linux') and not
Basing this off the host platform doesn't seem right. For example, when building
chrome/android on mac, this is possible too.

The 'alink_thin' rule is based on `self.flavor not in ['mac', 'win']`, so I'd
use that here too.

https://codereview.chromium.org/11031005/diff/15016/test/standalone-static-li...
File test/standalone-static-library/gyptest-standalone-static-library.py
(right):

https://codereview.chromium.org/11031005/diff/15016/test/standalone-static-li...
test/standalone-static-library/gyptest-standalone-static-library.py:32: if
test.format in ('make', 'msvs', 'ninja'):
Why is this necessary? scons? I know that xcode always puts static libraries
into the product dir too.

If this is just for scons, I'd probably rather use `test =
TestGyp.TestGyp(formats='!scons')` to disable the test on scons.

[borenet, 2012-10-04 16:07:47]

Uploaded patch set 14, which succeeds on all trybots.

https://codereview.chromium.org/11031005/diff/15016/pylib/gyp/generator/make.py
File pylib/gyp/generator/make.py (right):

https://codereview.chromium.org/11031005/diff/15016/pylib/gyp/generator/make....
pylib/gyp/generator/make.py:1544: if (sys.platform.startswith('linux') and not
On 2012/10/04 02:18:58, Nico wrote:
> Basing this off the host platform doesn't seem right. For example, when
building
> chrome/android on mac, this is possible too.
> 
> The 'alink_thin' rule is based on `self.flavor not in ['mac', 'win']`, so I'd
> use that here too.

Done.

https://codereview.chromium.org/11031005/diff/15016/test/standalone-static-li...
File test/standalone-static-library/gyptest-standalone-static-library.py
(right):

https://codereview.chromium.org/11031005/diff/15016/test/standalone-static-li...
test/standalone-static-library/gyptest-standalone-static-library.py:32: if
test.format in ('make', 'msvs', 'ninja'):
On 2012/10/04 02:18:58, Nico wrote:
> Why is this necessary? scons? I know that xcode always puts static libraries
> into the product dir too.
> 
> If this is just for scons, I'd probably rather use `test =
> TestGyp.TestGyp(formats='!scons')` to disable the test on scons.

Yep.  I changed the condition; skipping the test seems excessive since it all
works as expected otherwise.

[Nico, 2012-10-05 04:24:01]

Landed in gyp 1516 / 1517. Thanks for the patch!

[borenet, 2012-10-05 11:59:49]

On 2012/10/05 04:24:01, Nico wrote:
> Landed in gyp 1516 / 1517. Thanks for the patch!

Thanks for taking your time to review/commit it!