Don't propagate rlibs through shared libraries for linking purposes.
If a C++ target depends on an rlib R, the rlibs are treated as libraries
for linking purposes, and all transitive dependencies of R are required
at linking time.
The above holds true within a single final linking target, such as an
executable or shared library. However currently the rlibs inside a
shared library get linked again into anything that depends on the
shared library. This results in linking issues, such as missing
symbols.
executable(A) -> dylib(B) -> rlib(C) -> rlib(D)
If A wants to use C or D, and they are public dependencies, it does
not need to link them into A. They are already linked into B and A
will be linked against B.
I believe this becomes a more visible problem when C or D depends on a
C++ static library which has non-public symbols, as then A ends up
incorrectly linking C without linking its non-Rust dependencies too.
Bug: 276
Change-Id: I8ee2c1d89190920cb5a9eee0e0b6da8a18d4a235
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/13080
Reviewed-by: Tyler Mandry <tmandry@google.com>
Reviewed-by: Brett Wilson <brettw@google.com>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Commit-Queue: Brett Wilson <brettw@chromium.org>
diff --git a/src/gn/ninja_c_binary_target_writer.cc b/src/gn/ninja_c_binary_target_writer.cc
index 3321191..438e185 100644
--- a/src/gn/ninja_c_binary_target_writer.cc
+++ b/src/gn/ninja_c_binary_target_writer.cc
@@ -775,12 +775,13 @@
std::copy(input_deps.begin(), input_deps.end(),
std::back_inserter(implicit_deps));
- // Any C++ target which depends on a Rust .rlib has to depend on its
- // entire tree of transitive rlibs.
+ // Any C++ target which depends on a Rust .rlib has to depend on its entire
+ // tree of transitive rlibs found inside the linking target (which excludes
+ // rlibs only depended on inside a shared library dependency).
std::vector<OutputFile> transitive_rustlibs;
if (target_->IsFinal()) {
for (const auto* dep :
- target_->rust_transitive_inherited_libs().GetOrdered()) {
+ target_->rust_linkable_inherited_libs().GetOrdered()) {
if (dep->output_type() == Target::RUST_LIBRARY) {
transitive_rustlibs.push_back(dep->dependency_output_file());
implicit_deps.push_back(dep->dependency_output_file());
diff --git a/src/gn/ninja_c_binary_target_writer_unittest.cc b/src/gn/ninja_c_binary_target_writer_unittest.cc
index 61bc517..e1bdbcc 100644
--- a/src/gn/ninja_c_binary_target_writer_unittest.cc
+++ b/src/gn/ninja_c_binary_target_writer_unittest.cc
@@ -1500,6 +1500,110 @@
}
}
+TEST_F(NinjaCBinaryTargetWriterTest, RustDepsOverDynamicLinking) {
+ Err err;
+ TestWithScope setup;
+
+ Target rlib3(setup.settings(), Label(SourceDir("//baz/"), "baz"));
+ rlib3.set_output_type(Target::RUST_LIBRARY);
+ rlib3.visibility().SetPublic();
+ SourceFile lib3("//baz/lib.rs");
+ rlib3.sources().push_back(lib3);
+ rlib3.source_types_used().Set(SourceFile::SOURCE_RS);
+ rlib3.rust_values().set_crate_root(lib3);
+ rlib3.rust_values().crate_name() = "baz";
+ rlib3.public_deps().push_back(LabelTargetPair(&rlib3));
+ rlib3.SetToolchain(setup.toolchain());
+ ASSERT_TRUE(rlib3.OnResolved(&err));
+
+ Target rlib2(setup.settings(), Label(SourceDir("//bar/"), "bar"));
+ rlib2.set_output_type(Target::RUST_LIBRARY);
+ rlib2.visibility().SetPublic();
+ SourceFile lib2("//bar/lib.rs");
+ rlib2.sources().push_back(lib2);
+ rlib2.source_types_used().Set(SourceFile::SOURCE_RS);
+ rlib2.rust_values().set_crate_root(lib2);
+ rlib2.rust_values().crate_name() = "bar";
+ rlib2.public_deps().push_back(LabelTargetPair(&rlib2));
+ rlib2.SetToolchain(setup.toolchain());
+ ASSERT_TRUE(rlib2.OnResolved(&err));
+
+ Target rlib(setup.settings(), Label(SourceDir("//foo/"), "foo"));
+ rlib.set_output_type(Target::RUST_LIBRARY);
+ rlib.visibility().SetPublic();
+ SourceFile lib("//foo/lib.rs");
+ rlib.sources().push_back(lib);
+ rlib.source_types_used().Set(SourceFile::SOURCE_RS);
+ rlib.rust_values().set_crate_root(lib);
+ rlib.rust_values().crate_name() = "foo";
+ rlib.public_deps().push_back(LabelTargetPair(&rlib2));
+ rlib.SetToolchain(setup.toolchain());
+ ASSERT_TRUE(rlib.OnResolved(&err));
+
+ Target cdylib(setup.settings(), Label(SourceDir("//sh/"), "mylib"));
+ cdylib.set_output_type(Target::SHARED_LIBRARY);
+ cdylib.visibility().SetPublic();
+ SourceFile barlib("//sh/lib.rs");
+ cdylib.sources().push_back(barlib);
+ cdylib.source_types_used().Set(SourceFile::SOURCE_RS);
+ cdylib.rust_values().set_crate_type(RustValues::CRATE_CDYLIB);
+ cdylib.rust_values().set_crate_root(barlib);
+ cdylib.rust_values().crate_name() = "mylib";
+ cdylib.private_deps().push_back(LabelTargetPair(&rlib));
+ cdylib.public_deps().push_back(LabelTargetPair(&rlib3));
+ cdylib.SetToolchain(setup.toolchain());
+ ASSERT_TRUE(cdylib.OnResolved(&err));
+
+ Target nearrlib(setup.settings(), Label(SourceDir("//near/"), "near"));
+ nearrlib.set_output_type(Target::RUST_LIBRARY);
+ nearrlib.visibility().SetPublic();
+ SourceFile nearlib("//near/lib.rs");
+ nearrlib.sources().push_back(nearlib);
+ nearrlib.source_types_used().Set(SourceFile::SOURCE_RS);
+ nearrlib.rust_values().set_crate_root(nearlib);
+ nearrlib.rust_values().crate_name() = "near";
+ nearrlib.public_deps().push_back(LabelTargetPair(&cdylib));
+ nearrlib.SetToolchain(setup.toolchain());
+ ASSERT_TRUE(nearrlib.OnResolved(&err));
+
+ Target target(setup.settings(), Label(SourceDir("//exe/"), "binary"));
+ target.set_output_type(Target::EXECUTABLE);
+ target.visibility().SetPublic();
+ target.sources().push_back(SourceFile("//exe/main.cc"));
+ target.source_types_used().Set(SourceFile::SOURCE_CPP);
+ target.private_deps().push_back(LabelTargetPair(&nearrlib));
+ target.SetToolchain(setup.toolchain());
+ ASSERT_TRUE(target.OnResolved(&err));
+
+ std::ostringstream out;
+ NinjaCBinaryTargetWriter 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/exe\n"
+ "target_output_name = binary\n"
+ "\n"
+ "build obj/exe/binary.main.o: cxx ../../exe/main.cc\n"
+ "\n"
+ "build ./binary: link obj/exe/binary.main.o obj/sh/libmylib.so | "
+ "obj/near/libnear.rlib\n"
+ " ldflags =\n"
+ " libs =\n"
+ " frameworks =\n"
+ " swiftmodules =\n"
+ " output_extension = \n"
+ " output_dir = \n"
+ " rlibs = obj/near/libnear.rlib\n";
+
+ std::string out_str = out.str();
+ EXPECT_EQ(expected, out_str) << expected << "\n" << out_str;
+}
+
TEST_F(NinjaCBinaryTargetWriterTest, ModuleMapInStaticLibrary) {
TestWithScope setup;
Err err;
diff --git a/src/gn/target.cc b/src/gn/target.cc
index c491897..53c6510 100644
--- a/src/gn/target.cc
+++ b/src/gn/target.cc
@@ -769,8 +769,7 @@
if (dep->output_type() == STATIC_LIBRARY ||
dep->output_type() == SHARED_LIBRARY ||
- dep->output_type() == RUST_LIBRARY ||
- dep->output_type() == GROUP) {
+ dep->output_type() == RUST_LIBRARY || dep->output_type() == GROUP) {
// Here we have: `this` --[depends-on]--> `dep`
//
// The `this` target has direct access to `dep` since its a direct
@@ -792,6 +791,20 @@
rust_transitive_inherited_libs_.Append(dep, true);
rust_transitive_inheritable_libs_.Append(dep, is_public);
}
+ if (!dep->IsFinal()) {
+ // A "final" dependency is not an rlib, so while a final dependency would be
+ // part of another linking target, we don't inherit it here. Example:
+ //
+ // [bin E] -> [rlib A] -> [so D] -> [rlib B]
+ //
+ // The [so D] is linked into [bin E] along with [rlib A]. But we didn't add
+ // the edge to [so D] as an inherited rlib.
+ rust_linkable_inherited_libs_.Append(dep, true);
+ // Gathers the set of rlibs that are part of the current target's final
+ // linking target.
+ rust_linkable_inherited_libs_.AppendInherited(
+ dep->rust_linkable_inherited_libs(), true);
+ }
if (dep->output_type() == RUST_LIBRARY ||
RustValues::InferredCrateType(dep) == RustValues::CRATE_DYLIB) {
diff --git a/src/gn/target.h b/src/gn/target.h
index de076a8..31d4e3a 100644
--- a/src/gn/target.h
+++ b/src/gn/target.h
@@ -322,6 +322,12 @@
const InheritedLibraries& rust_transitive_inheritable_libs() const {
return rust_transitive_inheritable_libs_;
}
+ // The transitive closure of libraries that are depended on by this target
+ // and are part of the current linking step. Previously-linked libraries are
+ // not included.
+ const InheritedLibraries& rust_linkable_inherited_libs() const {
+ return rust_linkable_inherited_libs_;
+ }
const UniqueVector<SourceDir>& all_lib_dirs() const { return all_lib_dirs_; }
const UniqueVector<LibFile>& all_libs() const { return all_libs_; }
@@ -534,6 +540,10 @@
// For each library marked public: "If you depend on me, you get access to
// these targets."
InheritedLibraries rust_transitive_inheritable_libs_;
+ // Lists all transitive libraries in the current linking target. Unlike the
+ // two sets above, this does not include libraries that have already been
+ // linked into a dependency. The public bit is ignored.
+ InheritedLibraries rust_linkable_inherited_libs_;
// User for Swift targets.
std::unique_ptr<SwiftValues> swift_values_;
