Propagate module dependencies transitively via public_deps
When using C++ modules, dependencies must be treated as transitive. If a
target `A` has a `public_deps` on module `B`, and `B` in turn has a
`public_deps` on module `C`, any target that depends on `A` must also be
provided with the dependency on `C`.
Previously, GN did not propagate these module dependencies recursively.
This meant the dependency on `C` was not passed to `A`'s dependents,
which could lead to compilation failures. This behavior is incorrect, as
dependencies between C++ modules are analogous to transitive header
inclusions.
This change fixes this by recursively traversing the `public_deps` graph
in `ResolvedTargetData` to collect the complete set of transitive module
dependencies. This ensures that all module dependencies are correctly
passed to the compiler.
Bug: 40440396
Change-Id: Ib7b0e09477705708c92e6981b651c0eb9d6617e3
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/19100
Reviewed-by: David Turner <digit@google.com>
Commit-Queue: Takuto Ikuta <tikuta@google.com>
diff --git a/src/gn/ninja_c_binary_target_writer.cc b/src/gn/ninja_c_binary_target_writer.cc
index 6e27c68..cdfce61 100644
--- a/src/gn/ninja_c_binary_target_writer.cc
+++ b/src/gn/ninja_c_binary_target_writer.cc
@@ -87,10 +87,13 @@
const Target* target,
const ResolvedTargetData& resolved) {
std::vector<ModuleDep> ret;
+ // Use a set to keep track of added PCM files to ensure uniqueness.
+ std::set<OutputFile> added_pcms;
- auto add = [&ret](const Target* t, bool is_self) {
+ auto add_if_new = [&added_pcms, &ret](const Target* t, bool is_self) {
const SourceFile* modulemap = GetModuleMapFromTargetSources(t);
- CHECK(modulemap);
+ if (!modulemap) // Not a module or no .modulemap file.
+ return;
std::string label;
CHECK(SubstitutionWriter::GetTargetSubstitution(
@@ -102,20 +105,30 @@
t->GetOutputFilesForSource(*modulemap, &tool_type, &modulemap_outputs));
// Must be only one .pcm from .modulemap.
CHECK(modulemap_outputs.size() == 1u);
- ret.emplace_back(modulemap, label, modulemap_outputs[0], is_self);
+ const OutputFile& pcm_file = modulemap_outputs[0];
+
+ if (added_pcms.insert(pcm_file).second) {
+ ret.emplace_back(modulemap, label, pcm_file, is_self);
+ }
};
if (target->source_types_used().Get(SourceFile::SOURCE_MODULEMAP)) {
- add(target, true);
+ add_if_new(target, true);
}
- for (const Target* dep : resolved.GetLinkedDeps(target)) {
- // Having a .modulemap source means that the dependency is modularized.
+ // Process direct dependencies and their publicly inherited modules.
+ for (const auto& pairs : resolved.GetModuleDepsInformation(target)) {
+ const Target* dep = pairs.target();
if (dep->source_types_used().Get(SourceFile::SOURCE_MODULEMAP)) {
- add(dep, false);
+ add_if_new(dep, false);
}
}
+ // Sort by pcm path for deterministic output.
+ std::sort(ret.begin(), ret.end(), [](const ModuleDep& a, const ModuleDep& b) {
+ return a.pcm < b.pcm;
+ });
+
return ret;
}
diff --git a/src/gn/ninja_c_binary_target_writer_unittest.cc b/src/gn/ninja_c_binary_target_writer_unittest.cc
index ce6d72b..80f453d 100644
--- a/src/gn/ninja_c_binary_target_writer_unittest.cc
+++ b/src/gn/ninja_c_binary_target_writer_unittest.cc
@@ -2616,6 +2616,7 @@
target2.sources().push_back(SourceFile("//stuff/b.h"));
target2.source_types_used().Set(SourceFile::SOURCE_CPP);
target2.source_types_used().Set(SourceFile::SOURCE_MODULEMAP);
+ target2.public_deps().push_back(LabelTargetPair(&target));
target2.SetToolchain(&module_toolchain);
ASSERT_TRUE(target2.OnResolved(&err));
@@ -2629,21 +2630,21 @@
include_dirs =
cflags =
cflags_cc =
-module_deps = -Xclang -fmodules-embed-all-files -fmodule-file=obj/stuff/libb.b.pcm
-module_deps_no_self = -Xclang -fmodules-embed-all-files
+module_deps = -Xclang -fmodules-embed-all-files -fmodule-file=obj/blah/liba.a.pcm -fmodule-file=obj/stuff/libb.b.pcm
+module_deps_no_self = -Xclang -fmodules-embed-all-files -fmodule-file=obj/blah/liba.a.pcm
label = //stuff$:b
root_out_dir = withmodules
target_out_dir = obj/stuff
target_output_name = libb
-build obj/stuff/libb.b.pcm: cxx_module ../../stuff/b.modulemap
+build obj/stuff/libb.b.pcm: cxx_module ../../stuff/b.modulemap | obj/blah/liba.a.pcm
source_file_part = b.modulemap
source_name_part = b
-build obj/stuff/libb.b.o: cxx ../../stuff/b.cc | obj/stuff/libb.b.pcm
+build obj/stuff/libb.b.o: cxx ../../stuff/b.cc | obj/blah/liba.a.pcm obj/stuff/libb.b.pcm
source_file_part = b.cc
source_name_part = b
-build obj/stuff/libb.a: alink obj/stuff/libb.b.o
+build obj/stuff/libb.a: alink obj/stuff/libb.b.o || obj/blah/liba.a
arflags =
output_extension =
output_dir =
@@ -2658,7 +2659,7 @@
target3.visibility().SetPublic();
target3.sources().push_back(SourceFile("//stuff/c.modulemap"));
target3.source_types_used().Set(SourceFile::SOURCE_MODULEMAP);
- target3.private_deps().push_back(LabelTargetPair(&target));
+ target3.public_deps().push_back(LabelTargetPair(&target2));
target3.SetToolchain(&module_toolchain);
ASSERT_TRUE(target3.OnResolved(&err));
@@ -2673,18 +2674,18 @@
include_dirs =
cflags =
cflags_cc =
-module_deps = -Xclang -fmodules-embed-all-files -fmodule-file=obj/stuff/libc.c.pcm -fmodule-file=obj/blah/liba.a.pcm
-module_deps_no_self = -Xclang -fmodules-embed-all-files -fmodule-file=obj/blah/liba.a.pcm
+module_deps = -Xclang -fmodules-embed-all-files -fmodule-file=obj/blah/liba.a.pcm -fmodule-file=obj/stuff/libb.b.pcm -fmodule-file=obj/stuff/libc.c.pcm
+module_deps_no_self = -Xclang -fmodules-embed-all-files -fmodule-file=obj/blah/liba.a.pcm -fmodule-file=obj/stuff/libb.b.pcm
label = //things$:c
root_out_dir = withmodules
target_out_dir = obj/things
target_output_name = libc
-build obj/stuff/libc.c.pcm: cxx_module ../../stuff/c.modulemap | obj/blah/liba.a.pcm
+build obj/stuff/libc.c.pcm: cxx_module ../../stuff/c.modulemap | obj/blah/liba.a.pcm obj/stuff/libb.b.pcm
source_file_part = c.modulemap
source_name_part = c
-build obj/things/libc.a: alink || obj/blah/liba.a
+build obj/things/libc.a: alink || obj/stuff/libb.a obj/blah/liba.a
arflags =
output_extension =
output_dir =
@@ -2699,7 +2700,6 @@
depender.sources().push_back(SourceFile("//zap/x.cc"));
depender.sources().push_back(SourceFile("//zap/y.cc"));
depender.source_types_used().Set(SourceFile::SOURCE_CPP);
- depender.private_deps().push_back(LabelTargetPair(&target));
depender.private_deps().push_back(LabelTargetPair(&target2));
depender.SetToolchain(&module_toolchain);
ASSERT_TRUE(depender.OnResolved(&err));
@@ -2728,7 +2728,7 @@
source_file_part = y.cc
source_name_part = y
-build withmodules/c: link obj/zap/c.x.o obj/zap/c.y.o obj/blah/liba.a obj/stuff/libb.a
+build withmodules/c: link obj/zap/c.x.o obj/zap/c.y.o obj/stuff/libb.a obj/blah/liba.a
ldflags =
libs =
frameworks =
diff --git a/src/gn/resolved_target_data.cc b/src/gn/resolved_target_data.cc
index 7c3e79f..30433b0 100644
--- a/src/gn/resolved_target_data.cc
+++ b/src/gn/resolved_target_data.cc
@@ -172,6 +172,39 @@
}
}
+void ResolvedTargetData::ComputeModuleDepsInformation(TargetInfo* info) const {
+ TargetPublicPairListBuilder module_deps_information;
+
+ ComputeModuleDepsInformationFor(info->deps.public_deps(), true,
+ &module_deps_information);
+ ComputeModuleDepsInformationFor(info->deps.private_deps(), false,
+ &module_deps_information);
+
+ info->module_deps_information = module_deps_information.Build();
+ info->has_module_deps_information = true;
+}
+
+void ResolvedTargetData::ComputeModuleDepsInformationFor(
+ base::span<const Target*> deps,
+ bool is_public,
+ TargetPublicPairListBuilder* module_deps_information) const {
+ for (const Target* dep : deps) {
+ if (dep->output_type() != Target::STATIC_LIBRARY &&
+ dep->output_type() != Target::SHARED_LIBRARY &&
+ dep->output_type() != Target::SOURCE_SET) {
+ continue;
+ }
+
+ module_deps_information->Append(dep, is_public);
+ const TargetInfo* dep_info = GetTargetModuleDepsInformation(dep);
+ for (const auto& pair : dep_info->module_deps_information) {
+ if (pair.is_public()) {
+ module_deps_information->Append(pair.target(), is_public);
+ }
+ }
+ }
+}
+
void ResolvedTargetData::ComputeRustLibs(TargetInfo* info) const {
RustLibsBuilder rust_libs;
diff --git a/src/gn/resolved_target_data.h b/src/gn/resolved_target_data.h
index 61483bf..a2743fc 100644
--- a/src/gn/resolved_target_data.h
+++ b/src/gn/resolved_target_data.h
@@ -110,6 +110,13 @@
return GetTargetInheritedLibs(target)->inherited_libs;
}
+ // Retrieves an ordered list of (target, is_public) pairs for all module
+ // dependencies inherited by this target.
+ const std::vector<TargetPublicPair>& GetModuleDepsInformation(
+ const Target* target) const {
+ return GetTargetModuleDepsInformation(target)->module_deps_information;
+ }
+
// Retrieves an ordered list of (target, is_public) paris for all link-time
// libraries for Rust-specific binary targets.
const std::vector<TargetPublicPair>& GetRustInheritedLibraries(
@@ -146,6 +153,7 @@
bool has_framework_info = false;
bool has_hard_deps = false;
bool has_inherited_libs = false;
+ bool has_module_deps_information = false;
bool has_rust_libs = false;
bool has_swift_values = false;
@@ -164,6 +172,9 @@
// Only valid if |has_inherited_libs| is true.
std::vector<TargetPublicPair> inherited_libs;
+ // Only valid if |has_module_deps_information| is true.
+ std::vector<TargetPublicPair> module_deps_information;
+
// Only valid if |has_rust_libs| is true.
std::vector<TargetPublicPair> rust_inherited_libs;
std::vector<TargetPublicPair> rust_inheritable_libs;
@@ -224,6 +235,15 @@
return info;
}
+ const TargetInfo* GetTargetModuleDepsInformation(const Target* target) const {
+ TargetInfo* info = GetTargetInfo(target);
+ if (!info->has_module_deps_information) {
+ ComputeModuleDepsInformation(info);
+ DCHECK(info->has_module_deps_information);
+ }
+ return info;
+ }
+
const TargetInfo* GetTargetRustLibs(const Target* target) const {
TargetInfo* info = GetTargetInfo(target);
if (!info->has_rust_libs) {
@@ -249,6 +269,7 @@
void ComputeFrameworkInfo(TargetInfo* info) const;
void ComputeHardDeps(TargetInfo* info) const;
void ComputeInheritedLibs(TargetInfo* info) const;
+ void ComputeModuleDepsInformation(TargetInfo* info) const;
void ComputeRustLibs(TargetInfo* info) const;
void ComputeSwiftValues(TargetInfo* info) const;
@@ -258,6 +279,12 @@
bool is_public,
TargetPublicPairListBuilder* inherited_libraries) const;
+ // Helper function used by ComputeModuleDepsInformation().
+ void ComputeModuleDepsInformationFor(
+ base::span<const Target*> deps,
+ bool is_public,
+ TargetPublicPairListBuilder* module_deps_information) const;
+
// Helper data structure and function used by ComputeRustLibs().
struct RustLibsBuilder {
TargetPublicPairListBuilder inherited;
diff --git a/src/gn/swift_values.h b/src/gn/swift_values.h
index 1dde1c2..c7f4f01 100644
--- a/src/gn/swift_values.h
+++ b/src/gn/swift_values.h
@@ -56,7 +56,7 @@
std::vector<SourceFile>* result) const;
private:
- // Fill informations about .swiftmodule generated by this target.
+ // Fill information about .swiftmodule generated by this target.
bool FillModuleOutputFile(Target* target, Err* err);
// Name of the optional bridge header used to import Objective-C classes.
diff --git a/src/gn/target.cc b/src/gn/target.cc
index 53ed8ad..6cb5572 100644
--- a/src/gn/target.cc
+++ b/src/gn/target.cc
@@ -881,7 +881,7 @@
bundle_data().AddBundleData(pair.ptr, is_create_bundle);
}
- // Recursive bundle_data informations from all dependencies.
+ // Recursive bundle_data information from all dependencies.
if (pair.ptr->has_bundle_data()) {
for (const auto* target : pair.ptr->bundle_data().forwarded_bundle_deps())
bundle_data().AddBundleData(target, is_create_bundle);
diff --git a/src/gn/xcode_writer.h b/src/gn/xcode_writer.h
index 9c960e5..4a75cb3 100644
--- a/src/gn/xcode_writer.h
+++ b/src/gn/xcode_writer.h
@@ -92,7 +92,7 @@
// significant performance improvement).
//
// Extra behaviour is controlled by the |options| parameter. See comments
- // of the Options type for more informations.
+ // of the Options type for more information.
//
// Returns true on success, fails on failure. |err| is set in that case.
static bool RunAndWriteFiles(const BuildSettings* build_settings,
