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,