GN: Refine 'complete' static library handling

GN: Refine 'complete' static library handling

Allow static libraries to be dependencies of complete static libraries.
Propagate library settings (lib_dirs/libs) through complete static libraries.

BUG=413776
TEST=gn_unittests
R=brettw@chromium.org

Committed: https://crrev.com/33d9b3bf67d9e64cb476784a0f120a81e546eba5
Cr-Commit-Position: refs/heads/master@{#389041}

[brettw, 2014-09-16 16:46:02]

https://codereview.chromium.org/572893002/diff/1/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/572893002/diff/1/tools/gn/target.cc#newcode247
tools/gn/target.cc:247: } else if (dep->complete_static_lib()) {
I don't think there should have to be a separate step to pull libs across
complete static libraries beyond additional conditions. It seems like everything
should be to do something when a target isn't final or it's an incomplete static
lib (as you do below). If the inherited libs aren't getting propagated, then I
think we're missing some other place.

[kal, 2014-09-16 19:01:00]

Normally inherited libraries don't pass through "final" targets. Since we are
considering complete static libraries as "final" then we need extra logic to
allow dependent targets that won't be directly linked into the static lib to
propagate through. Indeed without that step things aren't linking properly.

[brettw, 2014-09-16 20:02:00]

On 2014/09/16 19:01:00, kal wrote:
> Normally inherited libraries don't pass through "final" targets. Since we are
> considering complete static libraries as "final" then we need extra logic to
> allow dependent targets that won't be directly linked into the static lib to
> propagate through. Indeed without that step things aren't linking properly.

Yes, but that's why I was expecting changes to look like
if (!IsFinal || is_complete_static_lib)...
This doesn't explain why you needed to add new code to reach into a dependency's
dependencies in a particular case. For everything, the old static library code
path should be sufficient, you just need to take it.

[kal, 2014-09-16 20:54:46]

Won't doing that cause all dependent source sets and incomplete static libs in
the dependency chain to float through complete static libraries? Like if you
have targets:

Exe -> CompleteStatic -> SourceSet

The object files from SourceSet would be linked into Exe and into
CompleteStatic. But don't we only want to link them into CompleteStatic?

[brettw, 2014-09-16 23:56:12]

I understand now, thanks.

https://codereview.chromium.org/572893002/diff/1/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/572893002/diff/1/tools/gn/target.cc#newcode247
tools/gn/target.cc:247: } else if (dep->complete_static_lib()) {
I get what's happening here. Can you add a comment about inheriting across the
boundary only things that weren't linked into the complete static library?

Also, I think complete static libraries should be excluded, since they'll
themselves be linked into the current complete static library.

We should get tests for this.

[kal, 2014-09-17 05:03:55]

https://codereview.chromium.org/572893002/diff/1/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/572893002/diff/1/tools/gn/target.cc#newcode247
tools/gn/target.cc:247: } else if (dep->complete_static_lib()) {
OK I will extend the comment here.

The way its implemented now, complete static libraries don't include other
complete static libraries. I think this is the best behavior. You may have a
suite of libraries you are distributing that have dependencies on each other so
if you include complete static libraries in each other you end up with
unnecessary code duplication. Take chromium for example, if you made the
components into static libraries for embedding, basically every static lib would
include most of "base" but "base" is also distributed, so just let the user link
in base themselves. Furthermore, not linking them in is more flexible because if
you really do want to link the dependent complete static library's code you can
just create a source set for each static library and link in the appropriate
source sets you want to have linked in.

[brettw, 2014-09-18 23:05:35]

SO I think the net result is that this with the other patch the only things that
will actually get forwarded are shared libraries. Theoretically we could forward
other static libs, but I think it's much more clear to just disallow this case
for simplicity and clarity. Hopefully this still works for your case.

So LGTM

[kal, 2014-09-19 04:49:24]

I actually need to distribute static libs that have dependencies on each other.
With this patch my use case is working like I would expect. So hopefully we can
at least keep support for that? :-) I can provide tests if that is needed.

[kal, 2014-09-19 05:04:59]

I mean can we at least keep support for complete static libs to depend on each
other. The case of complete static libs depending on _incomplete_ static libs is
less useful because you could probably just always use a source set in that
case.

I think the way its working now is clear and makes sense. Basically I expect
complete static libraries with dependencies to produce more or less the same
result as dynamic libs (except for being static of course). So I should be able
to use the "component" template to produce either _complete_ static libs or
dynamic libs and produce distributable outputs in each case without each
complete static library containing any duplicate code from the others just like
in the dynamic case.

[kal, 2014-09-19 05:11:14]

If you think its more consistent I can update this patch to also fix _complete_
static libraries that depend on _incomplete_ static libraries. I think it only
requires a minor change in NinjaBinaryTargetWriter::ClassifyDependency.

[kal, 2015-01-14 23:30:36]

Patch Set 3 allows complete static libraries to depend on incomplete static
libraries again and works around related linking problems with tools like AR by
making complete static libs link incomplete static libs as if they were source
sets.

[kal, 2015-03-15 16:39:35]

Can we get this merged?

[brettw, 2015-04-06 22:33:46]

Sorry this fell off my list.

We should have a test for this, doubly so since this is not something likely to
be used every day in Chrome.

[brettw, 2015-04-06 22:34:21]

not LGTM without a test <-- to make CQ do the right thing.

[kal, 2015-04-27 21:10:34]

Brett what test is needed to make CQ happy? Or will any test suffice?

https://codereview.chromium.org/572893002/diff/60001/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/572893002/diff/60001/tools/gn/target.cc#newco...
tools/gn/target.cc:243: is_public && inherited.second);
Does the is_public handling here make sense?

[brettw, 2015-04-27 21:30:16]

What I mean there was just to undo the previous good review so CQ wouldn't
commit it. Just ignore it and add a test, and I'll do another review at that
point.

[kal, 2016-04-20 20:12:31]

I made some tests. I think its ready now. Please review.

[kal, 2016-04-20 20:25:56]

Description was changed from

==========
GN: Refine 'complete' static library handling

Don't link "final" dependencies into complete static libraries. Always
propagate library settings (lib_dirs/libs) through static libraries.

BUG=413776
TEST=gn_unittests
R=brettw@chromium.org
==========

to

==========
GN: Refine 'complete' static library handling

Allow static libraries to be dependencies of complete static libraries.
Propagate library settings (lib_dirs/libs) through complete static libraries.

BUG=413776
TEST=gn_unittests
R=brettw@chromium.org
==========

[kal, 2016-04-20 20:26:15]

Description was changed from

==========
GN: Refine 'complete' static library handling

Allow static libraries to be dependencies of complete static libraries.
Propagate library settings (lib_dirs/libs) through complete static libraries.

BUG=413776
TEST=gn_unittests
R=brettw@chromium.org
==========

to

==========
GN: Refine 'complete' static library handling

Allow static libraries to be dependencies of complete static libraries.
Propagate library settings (lib_dirs/libs) through complete static libraries.

BUG=413776
TEST=gn_unittests
R=brettw@chromium.org
==========

[brettw, 2016-04-21 20:47:00]

https://codereview.chromium.org/572893002/diff/80001/tools/gn/ninja_binary_ta...
File tools/gn/ninja_binary_target_writer.cc (left):

https://codereview.chromium.org/572893002/diff/80001/tools/gn/ninja_binary_ta...
tools/gn/ninja_binary_target_writer.cc:950: 
I liked this blank line here as it was before.

https://codereview.chromium.org/572893002/diff/80001/tools/gn/target.cc
File tools/gn/target.cc (right):

https://codereview.chromium.org/572893002/diff/80001/tools/gn/target.cc#newco...
tools/gn/target.cc:479: if (inherited.first->IsFinal())
Need {} since there are >1 lines in here, even though it's just one statement
(as per style guide).

https://codereview.chromium.org/572893002/diff/80001/tools/gn/variables.cc
File tools/gn/variables.cc (right):

https://codereview.chromium.org/572893002/diff/80001/tools/gn/variables.cc#ne...
tools/gn/variables.cc:673: "  In rare cases it makes sense to list a header in
more than one\n"
Can you add a new paragraph here on the "promotion to source set" aspect of
linking a non-complete static library into a complete one?

[brettw, 2016-04-21 20:47:12]

LGTM with my last set of comments addressed.

[kal, 2016-04-22 05:04:01]

The CQ bit was checked by kcconley@gmail.com

[kal, 2016-04-22 05:04:02]

The patchset sent to the CQ was uploaded after l-g-t-m from brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/572893002/#ps100001
(title: "Improve complete_static_lib help; other style nitpicks")

[commit-bot: I haz the power, 2016-04-22 05:04:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/572893002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/572893002/100001

[commit-bot: I haz the power, 2016-04-22 06:18:36]

Description was changed from

==========
GN: Refine 'complete' static library handling

Allow static libraries to be dependencies of complete static libraries.
Propagate library settings (lib_dirs/libs) through complete static libraries.

BUG=413776
TEST=gn_unittests
R=brettw@chromium.org
==========

to

==========
GN: Refine 'complete' static library handling

Allow static libraries to be dependencies of complete static libraries.
Propagate library settings (lib_dirs/libs) through complete static libraries.

BUG=413776
TEST=gn_unittests
R=brettw@chromium.org
==========

[commit-bot: I haz the power, 2016-04-22 06:18:38]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-22 19:44:50]

Description was changed from

==========
GN: Refine 'complete' static library handling

Allow static libraries to be dependencies of complete static libraries.
Propagate library settings (lib_dirs/libs) through complete static libraries.

BUG=413776
TEST=gn_unittests
R=brettw@chromium.org
==========

to

==========
GN: Refine 'complete' static library handling

Allow static libraries to be dependencies of complete static libraries.
Propagate library settings (lib_dirs/libs) through complete static libraries.

BUG=413776
TEST=gn_unittests
R=brettw@chromium.org

Committed: https://crrev.com/33d9b3bf67d9e64cb476784a0f120a81e546eba5
Cr-Commit-Position: refs/heads/master@{#389041}
==========

[commit-bot: I haz the power, 2016-04-22 19:44:52]

Patchset 6 (id:??) landed as
https://crrev.com/33d9b3bf67d9e64cb476784a0f120a81e546eba5
Cr-Commit-Position: refs/heads/master@{#389041}