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
Review URL: https://codereview.chromium.org/572893002
Cr-Original-Commit-Position: refs/heads/master@{#389041}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 33d9b3bf67d9e64cb476784a0f120a81e546eba5
diff --git a/tools/gn/ninja_binary_target_writer.cc b/tools/gn/ninja_binary_target_writer.cc
index cb48688..30658f2 100644
--- a/tools/gn/ninja_binary_target_writer.cc
+++ b/tools/gn/ninja_binary_target_writer.cc
@@ -938,7 +938,14 @@
// don't link at all.
bool can_link_libs = target_->IsFinal();
- if (dep->output_type() == Target::SOURCE_SET) {
+ if (dep->output_type() == Target::SOURCE_SET ||
+ // If a complete static library depends on an incomplete static library,
+ // manually link in the object files of the dependent library as if it
+ // were a source set. This avoids problems with braindead tools such as
+ // ar which don't properly link dependent static libraries.
+ (target_->complete_static_lib() &&
+ dep->output_type() == Target::STATIC_LIBRARY &&
+ !dep->complete_static_lib())) {
// Source sets have their object files linked into final targets
// (shared libraries, executables, loadable modules, and complete static
// libraries). Intermediate static libraries and other source sets
@@ -954,6 +961,8 @@
// can be complete. Otherwise, these will be skipped since this target
// will depend only on the source set's object files.
non_linkable_deps->push_back(dep);
+ } else if (target_->complete_static_lib() && dep->IsFinal()) {
+ non_linkable_deps->push_back(dep);
} else if (can_link_libs && dep->IsLinkable()) {
linkable_deps->push_back(dep);
} else {
diff --git a/tools/gn/ninja_binary_target_writer_unittest.cc b/tools/gn/ninja_binary_target_writer_unittest.cc
index ab8b641..f2297fb 100644
--- a/tools/gn/ninja_binary_target_writer_unittest.cc
+++ b/tools/gn/ninja_binary_target_writer_unittest.cc
@@ -179,6 +179,81 @@
EXPECT_EQ(expected, out_str);
}
+TEST(NinjaBinaryTargetWriter, CompleteStaticLibrary) {
+ TestWithScope setup;
+ Err err;
+
+ TestTarget target(setup, "//foo:bar", Target::STATIC_LIBRARY);
+ target.sources().push_back(SourceFile("//foo/input1.cc"));
+ target.config_values().arflags().push_back("--asdf");
+ target.set_complete_static_lib(true);
+
+ TestTarget baz(setup, "//foo:baz", Target::STATIC_LIBRARY);
+ baz.sources().push_back(SourceFile("//foo/input2.cc"));
+
+ target.public_deps().push_back(LabelTargetPair(&baz));
+
+ ASSERT_TRUE(target.OnResolved(&err));
+ ASSERT_TRUE(baz.OnResolved(&err));
+
+ // A complete static library that depends on an incomplete static library
+ // should link in the dependent object files as if the dependent target
+ // were a source set.
+ {
+ std::ostringstream out;
+ NinjaBinaryTargetWriter writer(&target, out);
+ writer.Run();
+
+ const char expected[] =
+ "defines =\n"
+ "include_dirs =\n"
+ "cflags =\n"
+ "cflags_cc =\n"
+ "root_out_dir = .\n"
+ "target_out_dir = obj/foo\n"
+ "target_output_name = libbar\n"
+ "\n"
+ "build obj/foo/libbar.input1.o: cxx ../../foo/input1.cc\n"
+ "\n"
+ "build obj/foo/libbar.a: alink obj/foo/libbar.input1.o "
+ "obj/foo/libbaz.input2.o || obj/foo/libbaz.a\n"
+ " arflags = --asdf\n"
+ " output_extension = \n"
+ " output_dir = \n";
+ std::string out_str = out.str();
+ EXPECT_EQ(expected, out_str);
+ }
+
+ // Make the dependent static library complete.
+ baz.set_complete_static_lib(true);
+
+ // Dependent complete static libraries should not be linked directly.
+ {
+ std::ostringstream out;
+ NinjaBinaryTargetWriter writer(&target, out);
+ writer.Run();
+
+ const char expected[] =
+ "defines =\n"
+ "include_dirs =\n"
+ "cflags =\n"
+ "cflags_cc =\n"
+ "root_out_dir = .\n"
+ "target_out_dir = obj/foo\n"
+ "target_output_name = libbar\n"
+ "\n"
+ "build obj/foo/libbar.input1.o: cxx ../../foo/input1.cc\n"
+ "\n"
+ "build obj/foo/libbar.a: alink obj/foo/libbar.input1.o "
+ "|| obj/foo/libbaz.a\n"
+ " arflags = --asdf\n"
+ " output_extension = \n"
+ " output_dir = \n";
+ std::string out_str = out.str();
+ EXPECT_EQ(expected, out_str);
+ }
+}
+
// This tests that output extension and output dir overrides apply, and input
// dependencies are applied.
TEST(NinjaBinaryTargetWriter, OutputExtensionAndInputDeps) {
diff --git a/tools/gn/target.cc b/tools/gn/target.cc
index e88c82f..ddc8ad5 100644
--- a/tools/gn/target.cc
+++ b/tools/gn/target.cc
@@ -55,19 +55,6 @@
"Either mark it test-only or don't do this dependency.");
}
-Err MakeStaticLibDepsError(const Target* from, const Target* to) {
- return Err(from->defined_from(),
- "Complete static libraries can't depend on static libraries.",
- from->label().GetUserVisibleName(false) +
- "\n"
- "which is a complete static library can't depend on\n" +
- to->label().GetUserVisibleName(false) +
- "\n"
- "which is a static library.\n"
- "\n"
- "Use source sets for intermediate targets instead.");
-}
-
// Set check_private_deps to true for the first invocation since a target
// can see all of its dependencies. For recursive invocations this will be set
// to false to follow only public dependency paths.
@@ -317,8 +304,6 @@
return false;
if (!CheckTestonly(err))
return false;
- if (!CheckNoNestedStaticLibs(err))
- return false;
if (!CheckAssertNoDeps(err))
return false;
CheckSourcesGenerated();
@@ -482,8 +467,24 @@
// The current target isn't linked, so propogate linked deps and
// libraries up the dependency tree.
inherited_libraries_.AppendInherited(dep->inherited_libraries(), is_public);
+ } else if (dep->complete_static_lib()) {
+ // Inherit only final targets through _complete_ static libraries.
+ //
+ // Inherited final libraries aren't linked into complete static libraries.
+ // They are forwarded here so that targets that depend on complete
+ // static libraries can link them in. Conversely, since complete static
+ // libraries link in non-final targets they shouldn't be inherited.
+ for (const auto& inherited :
+ dep->inherited_libraries().GetOrderedAndPublicFlag()) {
+ if (inherited.first->IsFinal()) {
+ inherited_libraries_.Append(inherited.first,
+ is_public && inherited.second);
+ }
+ }
+ }
- // Inherited library settings.
+ // Library settings are always inherited across static library boundaries.
+ if (!dep->IsFinal() || dep->output_type() == STATIC_LIBRARY) {
all_lib_dirs_.append(dep->all_lib_dirs());
all_libs_.append(dep->all_libs());
}
@@ -702,30 +703,6 @@
return true;
}
-bool Target::CheckNoNestedStaticLibs(Err* err) const {
- // If the current target is not a complete static library, it can depend on
- // static library targets with no problem.
- if (!(output_type() == Target::STATIC_LIBRARY && complete_static_lib()))
- return true;
-
- // Verify no deps are static libraries.
- for (const auto& pair : GetDeps(DEPS_ALL)) {
- if (pair.ptr->output_type() == Target::STATIC_LIBRARY) {
- *err = MakeStaticLibDepsError(this, pair.ptr);
- return false;
- }
- }
-
- // Verify no inherited libraries are static libraries.
- for (const auto& lib : inherited_libraries().GetOrdered()) {
- if (lib->output_type() == Target::STATIC_LIBRARY) {
- *err = MakeStaticLibDepsError(this, lib);
- return false;
- }
- }
- return true;
-}
-
bool Target::CheckAssertNoDeps(Err* err) const {
if (assert_no_deps_.empty())
return true;
diff --git a/tools/gn/target.h b/tools/gn/target.h
index ac159e1..495a9c1 100644
--- a/tools/gn/target.h
+++ b/tools/gn/target.h
@@ -330,7 +330,6 @@
// Validates the given thing when a target is resolved.
bool CheckVisibility(Err* err) const;
bool CheckTestonly(Err* err) const;
- bool CheckNoNestedStaticLibs(Err* err) const;
bool CheckAssertNoDeps(Err* err) const;
void CheckSourcesGenerated() const;
void CheckSourceGenerated(const SourceFile& source) const;
diff --git a/tools/gn/target_unittest.cc b/tools/gn/target_unittest.cc
index e2e41a8..622dd54 100644
--- a/tools/gn/target_unittest.cc
+++ b/tools/gn/target_unittest.cc
@@ -183,7 +183,13 @@
TestTarget a(setup, "//foo:a", Target::EXECUTABLE);
TestTarget b(setup, "//foo:b", Target::STATIC_LIBRARY);
b.set_complete_static_lib(true);
+
+ const LibFile lib("foo");
+ const SourceDir lib_dir("/foo_dir/");
TestTarget c(setup, "//foo:c", Target::SOURCE_SET);
+ c.config_values().libs().push_back(lib);
+ c.config_values().lib_dirs().push_back(lib_dir);
+
a.public_deps().push_back(LabelTargetPair(&b));
b.public_deps().push_back(LabelTargetPair(&c));
@@ -199,42 +205,74 @@
// A should have B in its inherited libs, but not any others (the complete
// static library will include the source set).
std::vector<const Target*> a_inherited = a.inherited_libraries().GetOrdered();
- EXPECT_EQ(1u, a_inherited.size());
+ ASSERT_EQ(1u, a_inherited.size());
EXPECT_EQ(&b, a_inherited[0]);
+
+ // A should inherit the libs and lib_dirs from the C.
+ ASSERT_EQ(1u, a.all_libs().size());
+ EXPECT_EQ(lib, a.all_libs()[0]);
+ ASSERT_EQ(1u, a.all_lib_dirs().size());
+ EXPECT_EQ(lib_dir, a.all_lib_dirs()[0]);
}
-TEST(Target, InheritCompleteStaticLibNoDirectStaticLibDeps) {
+TEST(Target, InheritCompleteStaticLibStaticLibDeps) {
TestWithScope setup;
Err err;
// Create a dependency chain:
- // A (complete static lib) -> B (static lib)
- TestTarget a(setup, "//foo:a", Target::STATIC_LIBRARY);
- a.set_complete_static_lib(true);
+ // A (executable) -> B (complete static lib) -> C (static lib)
+ TestTarget a(setup, "//foo:a", Target::EXECUTABLE);
TestTarget b(setup, "//foo:b", Target::STATIC_LIBRARY);
-
- a.public_deps().push_back(LabelTargetPair(&b));
- ASSERT_TRUE(b.OnResolved(&err));
- ASSERT_FALSE(a.OnResolved(&err));
-}
-
-TEST(Target, InheritCompleteStaticLibNoIheritedStaticLibDeps) {
- TestWithScope setup;
- Err err;
-
- // Create a dependency chain:
- // A (complete static lib) -> B (source set) -> C (static lib)
- TestTarget a(setup, "//foo:a", Target::STATIC_LIBRARY);
- a.set_complete_static_lib(true);
- TestTarget b(setup, "//foo:b", Target::SOURCE_SET);
+ b.set_complete_static_lib(true);
TestTarget c(setup, "//foo:c", Target::STATIC_LIBRARY);
-
a.public_deps().push_back(LabelTargetPair(&b));
b.public_deps().push_back(LabelTargetPair(&c));
ASSERT_TRUE(c.OnResolved(&err));
ASSERT_TRUE(b.OnResolved(&err));
- ASSERT_FALSE(a.OnResolved(&err));
+ ASSERT_TRUE(a.OnResolved(&err));
+
+ // B should have C in its inherited libs.
+ std::vector<const Target*> b_inherited = b.inherited_libraries().GetOrdered();
+ ASSERT_EQ(1u, b_inherited.size());
+ EXPECT_EQ(&c, b_inherited[0]);
+
+ // A should have B in its inherited libs, but not any others (the complete
+ // static library will include the static library).
+ std::vector<const Target*> a_inherited = a.inherited_libraries().GetOrdered();
+ ASSERT_EQ(1u, a_inherited.size());
+ EXPECT_EQ(&b, a_inherited[0]);
+}
+
+TEST(Target, InheritCompleteStaticLibInheritedCompleteStaticLibDeps) {
+ TestWithScope setup;
+ Err err;
+
+ // Create a dependency chain:
+ // A (executable) -> B (complete static lib) -> C (complete static lib)
+ TestTarget a(setup, "//foo:a", Target::EXECUTABLE);
+ TestTarget b(setup, "//foo:b", Target::STATIC_LIBRARY);
+ b.set_complete_static_lib(true);
+ TestTarget c(setup, "//foo:c", Target::STATIC_LIBRARY);
+ c.set_complete_static_lib(true);
+
+ a.private_deps().push_back(LabelTargetPair(&b));
+ b.private_deps().push_back(LabelTargetPair(&c));
+
+ ASSERT_TRUE(c.OnResolved(&err));
+ ASSERT_TRUE(b.OnResolved(&err));
+ ASSERT_TRUE(a.OnResolved(&err));
+
+ // B should have C in its inherited libs.
+ std::vector<const Target*> b_inherited = b.inherited_libraries().GetOrdered();
+ ASSERT_EQ(1u, b_inherited.size());
+ EXPECT_EQ(&c, b_inherited[0]);
+
+ // A should have B and C in its inherited libs.
+ std::vector<const Target*> a_inherited = a.inherited_libraries().GetOrdered();
+ ASSERT_EQ(2u, a_inherited.size());
+ EXPECT_EQ(&b, a_inherited[0]);
+ EXPECT_EQ(&c, a_inherited[1]);
}
TEST(Target, NoActionDepPropgation) {
diff --git a/tools/gn/variables.cc b/tools/gn/variables.cc
index e9006a2..eea5bca 100644
--- a/tools/gn/variables.cc
+++ b/tools/gn/variables.cc
@@ -664,9 +664,16 @@
" In some cases the static library might be the final desired output.\n"
" For example, you may be producing a static library for distribution to\n"
" third parties. In this case, the static library should include code\n"
- " for all dependencies in one complete package. Since GN does not unpack\n"
- " static libraries to forward their contents up the dependency chain,\n"
- " it is an error for complete static libraries to depend on other static\n"
+ " for all dependencies in one complete package. However, complete static\n"
+ " libraries themselves are never linked into other complete static\n"
+ " libraries. All complete static libraries are for distribution and\n"
+ " linking them in would cause code duplication in this case. If the\n"
+ " static library is not for distribution, it should not be complete.\n"
+ "\n"
+ " GN treats non-complete static libraries as source sets when they are\n"
+ " linked into complete static libraries. This is done because some tools\n"
+ " like AR do not handle dependent static libraries properly. This makes\n"
+ " it easier to write \"alink\" rules.\n"
"\n"
" In rare cases it makes sense to list a header in more than one\n"
" target if it could be considered conceptually a member of both.\n"
@@ -756,7 +763,7 @@
const char kConsole[] = "console";
const char kConsole_HelpShort[] =
- "console [boolean]: Run this action in the console pool.";
+ "console: [boolean] Run this action in the console pool.";
const char kConsole_Help[] =
"console: Run this action in the console pool.\n"
"\n"
