GN: Add notion of 'complete' static libraries, akin to GYP.

GN: Add notion of 'complete' static libraries, akin to GYP.

In general, GN assumes that static libraries are intermediate targets and
will forward their child dependencies up the tree until a "final" target
is reached. Until now, this meant only shared_library and executable targets.
This change introduces the notion of a "complete" static_library target
which is linked and, as such, suitable for distribution.

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

Committed: https://crrev.com/a426cf930c6bb1955b2662be12534fc9dce2030e
Cr-Commit-Position: refs/heads/master@{#294709}

[Chris Masone, 2014-09-12 17:32:57]

PLZ advise on what test cases I missed. I'm sure there are more things I should
be checking.

[brettw, 2014-09-12 20:53:34]

Looks pretty good, I noticed only one omission

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

https://codereview.chromium.org/565283002/diff/20001/tools/gn/target.cc#newco...
tools/gn/target.cc:244: if (dep->output_type() != SHARED_LIBRARY &&
I think this condition should be replaced by an IsFinal call. And we should add
a test for this.

This happens when you have

exe -> static lib A (complete) -> static lib or source set B

B should be linked into A which will then be linked into the exe. Without this,
I think there will be a copy of B's code in A, and B itself will also be added
to the inherited libraries of the exe.

See the Target.InheritLibs test.

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

https://codereview.chromium.org/565283002/diff/20001/tools/gn/variables.cc#ne...
tools/gn/variables.cc:377: "  Static libraries are normally treated roughly like
source_sets, in\n"
I don't like defining static libraries in terms of source sets, since nobody
knows what a source set is. Maybe something like (feel free to clarify):


Links all dependencies into a static library.

A static library normally doesn't include code from dependencies, but instead
forward the static libraries and source sets in its deps up the dependency chain
until a linkable target (an executable or shared library) is reached. The final
linkable target only links each static library once, even if it appears more
than once in its dependency graph.

In some cases the static library might be the final desired output. For example,
you may be producing a static library for distribution to third parties. In this
case, the static library should include code from all dependencies in one
complete package.

https://codereview.chromium.org/565283002/diff/20001/tools/gn/variables.cc#ne...
tools/gn/variables.cc:384: "Example:\n"
I've been doing no colon and a blank line after these headings.

https://codereview.chromium.org/565283002/diff/20001/tools/gn/variables.cc#ne...
tools/gn/variables.cc:386: "    complete_static_lib = true  # Force foo to.\n"
Delete comment. And maybe add a line with deps that would get linked.

[Chris Masone, 2014-09-12 22:56:37]

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

https://codereview.chromium.org/565283002/diff/20001/tools/gn/target.cc#newco...
tools/gn/target.cc:244: if (dep->output_type() != SHARED_LIBRARY &&
On 2014/09/12 20:53:33, brettw wrote:
> I think this condition should be replaced by an IsFinal call. And we should
add
> a test for this.
> 
> This happens when you have
> 
> exe -> static lib A (complete) -> static lib or source set B
> 
> B should be linked into A which will then be linked into the exe. Without
this,
> I think there will be a copy of B's code in A, and B itself will also be added
> to the inherited libraries of the exe.
> 
> See the Target.InheritLibs test.

Done.

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

https://codereview.chromium.org/565283002/diff/20001/tools/gn/variables.cc#ne...
tools/gn/variables.cc:377: "  Static libraries are normally treated roughly like
source_sets, in\n"
On 2014/09/12 20:53:33, brettw wrote:
> I don't like defining static libraries in terms of source sets, since nobody
> knows what a source set is. Maybe something like (feel free to clarify):
> 
> 
> Links all dependencies into a static library.
> 
> A static library normally doesn't include code from dependencies, but instead
> forward the static libraries and source sets in its deps up the dependency
chain
> until a linkable target (an executable or shared library) is reached. The
final
> linkable target only links each static library once, even if it appears more
> than once in its dependency graph.
> 
> In some cases the static library might be the final desired output. For
example,
> you may be producing a static library for distribution to third parties. In
this
> case, the static library should include code from all dependencies in one
> complete package.

Done.

https://codereview.chromium.org/565283002/diff/20001/tools/gn/variables.cc#ne...
tools/gn/variables.cc:384: "Example:\n"
On 2014/09/12 20:53:34, brettw wrote:
> I've been doing no colon and a blank line after these headings.

Done.

https://codereview.chromium.org/565283002/diff/20001/tools/gn/variables.cc#ne...
tools/gn/variables.cc:386: "    complete_static_lib = true  # Force foo to.\n"
On 2014/09/12 20:53:33, brettw wrote:
> Delete comment. And maybe add a line with deps that would get linked.

Done.

[brettw, 2014-09-12 23:01:46]

lgtm

https://codereview.chromium.org/565283002/diff/40001/tools/gn/ninja_binary_ta...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/565283002/diff/40001/tools/gn/ninja_binary_ta...
tools/gn/ninja_binary_target_writer.cc:413: if (target_->IsFinal()) {
Can we just re-use can_link_libs here (I realize it was like this before).

https://codereview.chromium.org/565283002/diff/40001/tools/gn/target.h
File tools/gn/target.h (right):

https://codereview.chromium.org/565283002/diff/40001/tools/gn/target.h#newcod...
tools/gn/target.h:105: DCHECK_EQ(output_type_, STATIC_LIBRARY);
Swap these (expected is first for Google logging).

[Chris Masone, 2014-09-12 23:03:08]

On 2014/09/12 22:56:37, Chris Masone wrote:
> https://codereview.chromium.org/565283002/diff/20001/tools/gn/target.cc
> File tools/gn/target.cc (right):
> 
>
https://codereview.chromium.org/565283002/diff/20001/tools/gn/target.cc#newco...
> tools/gn/target.cc:244: if (dep->output_type() != SHARED_LIBRARY &&
> On 2014/09/12 20:53:33, brettw wrote:
> > I think this condition should be replaced by an IsFinal call. And we should
> add
> > a test for this.
> > 
> > This happens when you have
> > 
> > exe -> static lib A (complete) -> static lib or source set B
> > 
> > B should be linked into A which will then be linked into the exe. Without
> this,
> > I think there will be a copy of B's code in A, and B itself will also be
added
> > to the inherited libraries of the exe.
> > 
> > See the Target.InheritLibs test.
> 
> Done.
> 
> https://codereview.chromium.org/565283002/diff/20001/tools/gn/variables.cc
> File tools/gn/variables.cc (right):
> 
>
https://codereview.chromium.org/565283002/diff/20001/tools/gn/variables.cc#ne...
> tools/gn/variables.cc:377: "  Static libraries are normally treated roughly
like
> source_sets, in\n"
> On 2014/09/12 20:53:33, brettw wrote:
> > I don't like defining static libraries in terms of source sets, since nobody
> > knows what a source set is. Maybe something like (feel free to clarify):
> > 
> > 
> > Links all dependencies into a static library.
> > 
> > A static library normally doesn't include code from dependencies, but
instead
> > forward the static libraries and source sets in its deps up the dependency
> chain
> > until a linkable target (an executable or shared library) is reached. The
> final
> > linkable target only links each static library once, even if it appears more
> > than once in its dependency graph.
> > 
> > In some cases the static library might be the final desired output. For
> example,
> > you may be producing a static library for distribution to third parties. In
> this
> > case, the static library should include code from all dependencies in one
> > complete package.
> 
> Done.
> 
>
https://codereview.chromium.org/565283002/diff/20001/tools/gn/variables.cc#ne...
> tools/gn/variables.cc:384: "Example:\n"
> On 2014/09/12 20:53:34, brettw wrote:
> > I've been doing no colon and a blank line after these headings.
> 
> Done.
> 
>
https://codereview.chromium.org/565283002/diff/20001/tools/gn/variables.cc#ne...
> tools/gn/variables.cc:386: "    complete_static_lib = true  # Force foo to.\n"
> On 2014/09/12 20:53:33, brettw wrote:
> > Delete comment. And maybe add a line with deps that would get linked.
> 
> Done.

Hm. When I create a static_library target with complete_static_lib = true, I get

ERROR at //mojo/public/BUILD.gn:6:25: Assignment had no effect.
  complete_static_lib = true
                        ^---
You set the variable "complete_static_lib" here and it was unused before it went
out of scope.



I'm running the gn binary I built, though. gn help complete_static_lib works.
What'd I miss?

[brettw, 2014-09-12 23:05:07]

https://codereview.chromium.org/565283002/diff/40001/tools/gn/binary_target_g...
File tools/gn/binary_target_generator.cc (right):

https://codereview.chromium.org/565283002/diff/40001/tools/gn/binary_target_g...
tools/gn/binary_target_generator.cc:79: void
BinaryTargetGenerator::FillCompleteStaticLib() {
You forgot to call this function, silly-pants!

[Chris Masone, 2014-09-12 23:08:13]

On 2014/09/12 23:05:07, brettw wrote:
>
https://codereview.chromium.org/565283002/diff/40001/tools/gn/binary_target_g...
> File tools/gn/binary_target_generator.cc (right):
> 
>
https://codereview.chromium.org/565283002/diff/40001/tools/gn/binary_target_g...
> tools/gn/binary_target_generator.cc:79: void
> BinaryTargetGenerator::FillCompleteStaticLib() {
> You forgot to call this function, silly-pants!

I am a dumb person, whose dumbness is full of stupidity.

[Chris Masone, 2014-09-12 23:15:00]

https://codereview.chromium.org/565283002/diff/40001/tools/gn/binary_target_g...
File tools/gn/binary_target_generator.cc (right):

https://codereview.chromium.org/565283002/diff/40001/tools/gn/binary_target_g...
tools/gn/binary_target_generator.cc:79: void
BinaryTargetGenerator::FillCompleteStaticLib() {
On 2014/09/12 23:05:06, brettw wrote:
> You forgot to call this function, silly-pants!

Done.

https://codereview.chromium.org/565283002/diff/40001/tools/gn/ninja_binary_ta...
File tools/gn/ninja_binary_target_writer.cc (right):

https://codereview.chromium.org/565283002/diff/40001/tools/gn/ninja_binary_ta...
tools/gn/ninja_binary_target_writer.cc:413: if (target_->IsFinal()) {
On 2014/09/12 23:01:46, brettw wrote:
> Can we just re-use can_link_libs here (I realize it was like this before).

Done.

https://codereview.chromium.org/565283002/diff/40001/tools/gn/target.h
File tools/gn/target.h (right):

https://codereview.chromium.org/565283002/diff/40001/tools/gn/target.h#newcod...
tools/gn/target.h:105: DCHECK_EQ(output_type_, STATIC_LIBRARY);
On 2014/09/12 23:01:46, brettw wrote:
> Swap these (expected is first for Google logging).

Done.

[Chris Masone, 2014-09-12 23:15:09]

The CQ bit was checked by cmasone@chromium.org

[brettw, 2014-09-12 23:17:25]

still lgtm

[commit-bot: I haz the power, 2014-09-12 23:22:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/565283002/60001

[commit-bot: I haz the power, 2014-09-13 00:52:47]

Committed patchset #4 (id:60001) as 998ae50de876b88ce8839a28f2dfa37d505643ed

[commit-bot: I haz the power, 2014-09-13 00:56:55]

Patchset 4 (id:??) landed as
https://crrev.com/a426cf930c6bb1955b2662be12534fc9dce2030e
Cr-Commit-Position: refs/heads/master@{#294709}