[gn] Bypass groups in order-only deps to use source set linkdeps When a binary target depends on a group target, and that group target depends on a source set with additional outputs (e.g., .dwo files), the binary target previously ended up depending on the group target's phony output. This caused the binary target to include the source set's additional outputs in its order-only dependencies. As established in commit a9f1a50154c7873b8152842dfd1392badeee55d6, .linkdeps targets are used to avoid propagating non-object files like .dwo to order-only dependencies. While .dwo files are generated simultaneously with .o files (so they don't cause build delays), having them in order-only dependencies causes problems for remote linking. Specifically, it forces the upload of .dwo files to remote linkers, leading to increased data transfer and potentially hitting file count limits. However, this isolation of .linkdeps was broken when a group target acted as an intermediate dependency. To fix this and respect the intent of a9f1a50154c7873b8152842dfd1392badeee55d6, this commit modifies NinjaCBinaryTargetWriter and NinjaRustBinaryTargetWriter to recursively expand dependencies of group targets. If a group depends on a source set, we now depend directly on its .linkdeps instead of the group itself. Bug: 502431091 Change-Id: Ia133f2048493a199ba8996332b9cbc3bb1591ca2 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/22400 Commit-Queue: Takuto Ikuta <tikuta@google.com> Reviewed-by: Junji Watanabe <jwata@google.com>
diff --git a/src/gn/ninja_binary_target_writer.cc b/src/gn/ninja_binary_target_writer.cc index 163141b..aad2eb2 100644 --- a/src/gn/ninja_binary_target_writer.cc +++ b/src/gn/ninja_binary_target_writer.cc
@@ -547,3 +547,60 @@ out << std::endl; } } + +std::vector<OutputFile> +NinjaBinaryTargetWriter::GetOrderOnlyDepsFromNonLinkableDeps( + const UniqueVector<const Target*>& non_linkable_deps) const { + std::vector<const Target*> group_stack; + std::vector<OutputFile> outputs_to_write; + std::set<std::string> seen_outputs; + + auto add_output = [&](const OutputFile& output) { + if (seen_outputs.insert(output.value()).second) { + outputs_to_write.push_back(output); + } + }; + + auto process_dep = [&](const Target* dep) { + if (dep->output_type() == Target::GROUP) { + group_stack.push_back(dep); + } else if (dep->has_dependency_output()) { + OutputFile dep_output = dep->dependency_output(); + if (dep->output_type() == Target::SOURCE_SET) { + dep_output.value().append(".linkdeps"); + } + add_output(dep_output); + } + }; + + for (auto* dep : non_linkable_deps) { + process_dep(dep); + } + + // Recursively expand dependencies of groups to avoid unnecessary + // dependencies. If a group depends on a source set, we depend on its + // .linkdeps instead of the group itself. This prevents including non-object + // files (like .dwo files) in order-only dependencies. This is crucial for + // remote linking to avoid uploading unnecessary files, which increases data + // transfer and could hit file count limits. + std::set<const Target*> visited_groups; + while (!group_stack.empty()) { + const Target* current = group_stack.back(); + group_stack.pop_back(); + + if (!visited_groups.insert(current).second) + continue; + + auto add_deps = [&](const LabelTargetVector& deps) { + for (const auto& pair : deps) { + process_dep(pair.ptr); + } + }; + + add_deps(current->public_deps()); + add_deps(current->private_deps()); + add_deps(current->data_deps()); + } + + return outputs_to_write; +}
diff --git a/src/gn/ninja_binary_target_writer.h b/src/gn/ninja_binary_target_writer.h index ef91127..99b2b72 100644 --- a/src/gn/ninja_binary_target_writer.h +++ b/src/gn/ninja_binary_target_writer.h
@@ -48,6 +48,10 @@ // object files from source sets we need to link. ClassifiedDeps GetClassifiedDeps() const; + // Expands group dependencies and returns order-only dependencies. + std::vector<OutputFile> GetOrderOnlyDepsFromNonLinkableDeps( + const UniqueVector<const Target*>& non_linkable_deps) const; + // Classifies the dependency as linkable or nonlinkable with the current // target, adding it to the appropriate vector of |classified_deps|. If the // dependency is a source set we should link in, the source set's object
diff --git a/src/gn/ninja_c_binary_target_writer.cc b/src/gn/ninja_c_binary_target_writer.cc index 3121718..5346e06 100644 --- a/src/gn/ninja_c_binary_target_writer.cc +++ b/src/gn/ninja_c_binary_target_writer.cc
@@ -805,19 +805,14 @@ void NinjaCBinaryTargetWriter::WriteOrderOnlyDependencies( const UniqueVector<const Target*>& non_linkable_deps) { - if (!non_linkable_deps.empty()) { - out_ << " ||"; + std::vector<OutputFile> outputs_to_write = + GetOrderOnlyDepsFromNonLinkableDeps(non_linkable_deps); - // Non-linkable targets. - for (auto* non_linkable_dep : non_linkable_deps) { - if (non_linkable_dep->has_dependency_output()) { - out_ << " "; - OutputFile dep_output = non_linkable_dep->dependency_output(); - if (non_linkable_dep->output_type() == Target::SOURCE_SET) { - dep_output.value().append(".linkdeps"); - } - path_output_.WriteFile(out_, dep_output); - } + if (!outputs_to_write.empty()) { + out_ << " ||"; + for (const auto& output : outputs_to_write) { + out_ << " "; + path_output_.WriteFile(out_, output); } } }
diff --git a/src/gn/ninja_c_binary_target_writer_unittest.cc b/src/gn/ninja_c_binary_target_writer_unittest.cc index 4b0219b..161845c 100644 --- a/src/gn/ninja_c_binary_target_writer_unittest.cc +++ b/src/gn/ninja_c_binary_target_writer_unittest.cc
@@ -9,6 +9,7 @@ #include <utility> #include "gn/config.h" +#include "gn/ninja_group_target_writer.h" #include "gn/ninja_target_command_util.h" #include "gn/pool.h" #include "gn/scheduler.h" @@ -379,6 +380,98 @@ } } +// Tests that order-only dependencies bypass group targets and use .linkdeps +// for transitive source set dependencies to avoid unnecessary dependencies +// on non-object files (like .dwo files). +TEST_F(NinjaCBinaryTargetWriterTest, GroupOrderOnlyDeps) { + Err err; + TestWithScope setup; + + Value outputs_value(nullptr, Value::LIST); + outputs_value.list_value().push_back( + Value(nullptr, "{{target_out_dir}}/{{source_name_part}}.dwo")); + + Config config(setup.settings(), Label(SourceDir("//foo/"), "split_dwarf")); + config.visibility().SetPublic(); + + std::vector<SubstitutionPattern> patterns; + for (const auto& v : outputs_value.list_value()) { + SubstitutionPattern pattern; + ASSERT_TRUE(pattern.Parse(v, &err)); + patterns.push_back(std::move(pattern)); + } + config.own_values().c_additional_outputs() = std::move(patterns); + ASSERT_TRUE(config.OnResolved(&err)); + + // Source set B (dependee) + Target target_b(setup.settings(), Label(SourceDir("//foo/"), "b")); + target_b.set_output_type(Target::SOURCE_SET); + target_b.sources().push_back(SourceFile("//foo/b.cc")); + target_b.sources().push_back(SourceFile("//foo/b2.cc")); + target_b.source_types_used().Set(SourceFile::SOURCE_CPP); + target_b.visibility().SetPublic(); + target_b.configs().push_back(LabelConfigPair(&config)); + target_b.SetToolchain(setup.toolchain()); + ASSERT_TRUE(target_b.OnResolved(&err)) << err.message(); + + // Group E (depender) + Target target_e(setup.settings(), Label(SourceDir("//foo/"), "e")); + target_e.set_output_type(Target::GROUP); + target_e.visibility().SetPublic(); + target_e.private_deps().push_back(LabelTargetPair(&target_b)); + target_e.SetToolchain(setup.toolchain()); + ASSERT_TRUE(target_e.OnResolved(&err)) << err.message(); + + // Verify E's output + { + std::ostringstream out; + NinjaGroupTargetWriter writer(&target_e, out); + writer.Run(); + + const char expected[] = "build phony/foo/e: phony phony/foo/b\n"; + + EXPECT_EQ(expected, out.str()); + } + + // Static library F (depender on group E) + Target target_f(setup.settings(), Label(SourceDir("//foo/"), "f")); + target_f.set_output_type(Target::STATIC_LIBRARY); + target_f.sources().push_back(SourceFile("//foo/f.cc")); + target_f.source_types_used().Set(SourceFile::SOURCE_CPP); + target_f.private_deps().push_back(LabelTargetPair(&target_e)); + target_f.configs().push_back(LabelConfigPair(&config)); + target_f.SetToolchain(setup.toolchain()); + ASSERT_TRUE(target_f.OnResolved(&err)) << err.message(); + + // Verify F's output + { + std::ostringstream out; + NinjaCBinaryTargetWriter writer(&target_f, out); + writer.Run(); + + const char expected[] = + "defines =\n" + "include_dirs =\n" + "cflags =\n" + "cflags_cc =\n" + "root_out_dir = .\n" + "target_gen_dir = gen/foo\n" + "target_out_dir = obj/foo\n" + "target_output_name = libf\n" + "\n" + "build obj/foo/libf.f.o obj/foo/f.dwo: cxx ../../foo/f.cc\n" + " source_file_part = f.cc\n" + " source_name_part = f\n" + "\n" + "build obj/foo/libf.a: alink obj/foo/libf.f.o || phony/foo/b.linkdeps\n" + " arflags =\n" + " output_extension =\n" + " output_dir =\n"; + + EXPECT_EQ(expected, out.str()); + } +} + TEST_F(NinjaCBinaryTargetWriterTest, EscapeDefines) { TestWithScope setup; Err err;
diff --git a/src/gn/ninja_rust_binary_target_writer.cc b/src/gn/ninja_rust_binary_target_writer.cc index cbb5e4f..e9b44e8 100644 --- a/src/gn/ninja_rust_binary_target_writer.cc +++ b/src/gn/ninja_rust_binary_target_writer.cc
@@ -168,9 +168,13 @@ non_linkable_dep->output_type() != Target::SOURCE_SET) { rustdeps.push_back(non_linkable_dep->dependency_output()); } - order_only_deps.push_back(non_linkable_dep->dependency_output()); } } + + std::vector<OutputFile> group_order_only_deps = + GetOrderOnlyDepsFromNonLinkableDeps(classified_deps.non_linkable_deps); + order_only_deps.insert(order_only_deps.end(), group_order_only_deps.begin(), + group_order_only_deps.end()); for (const auto* linkable_dep : classified_deps.linkable_deps) { // Rust cdylibs are treated as non-Rust dependencies for linking purposes. if (linkable_dep->source_types_used().RustSourceUsed() &&
diff --git a/src/gn/ninja_rust_binary_target_writer_unittest.cc b/src/gn/ninja_rust_binary_target_writer_unittest.cc index 680652c..f38b892 100644 --- a/src/gn/ninja_rust_binary_target_writer_unittest.cc +++ b/src/gn/ninja_rust_binary_target_writer_unittest.cc
@@ -585,7 +585,7 @@ "\n" "build obj/bar/libmylib.rlib: rust_rlib ../../bar/lib.rs | " "../../bar/mylib.rs ../../bar/lib.rs obj/bar/libmymacro.so || " - "phony/baz/group\n" + "obj/bar/libmymacro.so\n" " source_file_part = lib.rs\n" " source_name_part = lib\n" " externs = --extern mymacro=obj/bar/libmymacro.so\n" @@ -641,6 +641,89 @@ } } +// Tests that order-only dependencies bypass group targets and use .linkdeps +// for transitive source set dependencies in Rust targets. +TEST_F(NinjaRustBinaryTargetWriterTest, GroupOrderOnlyDeps) { + Err err; + TestWithScope setup; + + Value outputs_value(nullptr, Value::LIST); + outputs_value.list_value().push_back( + Value(nullptr, "{{target_out_dir}}/{{source_name_part}}.dwo")); + + Config config(setup.settings(), Label(SourceDir("//foo/"), "split_dwarf")); + config.visibility().SetPublic(); + + std::vector<SubstitutionPattern> patterns; + for (const auto& v : outputs_value.list_value()) { + SubstitutionPattern pattern; + ASSERT_TRUE(pattern.Parse(v, &err)); + patterns.push_back(std::move(pattern)); + } + config.own_values().c_additional_outputs() = std::move(patterns); + ASSERT_TRUE(config.OnResolved(&err)); + + // Source set B (dependee, C++) + Target target_b(setup.settings(), Label(SourceDir("//foo/"), "b")); + target_b.set_output_type(Target::SOURCE_SET); + target_b.sources().push_back(SourceFile("//foo/b.cc")); + target_b.source_types_used().Set(SourceFile::SOURCE_CPP); + target_b.visibility().SetPublic(); + target_b.configs().push_back(LabelConfigPair(&config)); + target_b.SetToolchain(setup.toolchain()); + ASSERT_TRUE(target_b.OnResolved(&err)) << err.message(); + + // Group E (depender) + Target target_e(setup.settings(), Label(SourceDir("//foo/"), "e")); + target_e.set_output_type(Target::GROUP); + target_e.visibility().SetPublic(); + target_e.private_deps().push_back(LabelTargetPair(&target_b)); + target_e.SetToolchain(setup.toolchain()); + ASSERT_TRUE(target_e.OnResolved(&err)) << err.message(); + + // Rust Executable F (depender on group E) + Target target_f(setup.settings(), Label(SourceDir("//foo/"), "f")); + target_f.set_output_type(Target::EXECUTABLE); + SourceFile main("//foo/main.rs"); + target_f.sources().push_back(main); + target_f.source_types_used().Set(SourceFile::SOURCE_RS); + target_f.rust_values().set_crate_root(main); + target_f.rust_values().crate_name() = "f_crate"; + target_f.private_deps().push_back(LabelTargetPair(&target_e)); + target_f.SetToolchain(setup.toolchain()); + ASSERT_TRUE(target_f.OnResolved(&err)) << err.message(); + + // Verify F's output + { + std::ostringstream out; + NinjaRustBinaryTargetWriter writer(&target_f, out); + writer.Run(); + + const char expected[] = + "crate_name = f_crate\n" + "crate_type = bin\n" + "output_extension = \n" + "output_dir = \n" + "rustflags =\n" + "rustenv =\n" + "root_out_dir = .\n" + "target_gen_dir = gen/foo\n" + "target_out_dir = obj/foo\n" + "target_output_name = f\n" + "\n" + "build ./f_crate: rust_bin ../../foo/main.rs | ../../foo/main.rs " + "obj/foo/b.b.o || phony/foo/b.linkdeps\n" + " source_file_part = main.rs\n" + " source_name_part = main\n" + " externs =\n" + " rustdeps = -Clink-arg=-Bdynamic -Clink-arg=obj/foo/b.b.o\n" + " ldflags =\n" + " sources = ../../foo/main.rs\n"; + + EXPECT_EQ(expected, out.str()); + } +} + TEST_F(NinjaRustBinaryTargetWriterTest, RenamedDeps) { Err err; TestWithScope setup; @@ -825,7 +908,7 @@ "../../foo/main.rs obj/baz/sourceset.csourceset.o " "obj/bar/libmylib.rlib " "obj/foo/libstatic.a ./libshared.so ./libshared_with_toc.so.TOC " - "|| phony/baz/sourceset\n" + "|| phony/baz/sourceset.linkdeps\n" " source_file_part = main.rs\n" " source_name_part = main\n" " externs = --extern mylib=obj/bar/libmylib.rlib\n" @@ -1090,8 +1173,8 @@ "obj/pub_in_staticlib/libpub_in_staticlib.rlib " "obj/priv_in_staticlib/libpriv_in_staticlib.rlib " "obj/pub_in_dylib/libpub_in_dylib.rlib || " - "phony/pub_sset_in_staticlib/pub_sset_in_staticlib " - "phony/priv_sset_in_staticlib/priv_sset_in_staticlib\n" + "phony/pub_sset_in_staticlib/pub_sset_in_staticlib.linkdeps " + "phony/priv_sset_in_staticlib/priv_sset_in_staticlib.linkdeps\n" " source_file_part = main.rs\n" " source_name_part = main\n" " externs = " @@ -1542,7 +1625,7 @@ "\n" "build ./foo_bar: rust_bin ../../foo/main.rs | " "../../foo/source.rs ../../foo/main.rs obj/bar/libmylib.rlib || " - "phony/baz/group\n" + "obj/bar/libmylib.rlib\n" " source_file_part = main.rs\n" " source_name_part = main\n" " externs = --extern mylib=obj/bar/libmylib.rlib\n" @@ -1905,8 +1988,9 @@ "\n" "build ./exe: rust_bin ../../linked/exe.rs | ../../linked/exe.rs " "obj/sset/bar.input1.o obj/public/libbehind_sourceset_public.rlib " - "obj/private/libbehind_sourceset_private.rlib || phony/sset/bar " - "phony/sset/module\n" + "obj/private/libbehind_sourceset_private.rlib || " + "phony/sset/bar.linkdeps " + "phony/sset/module.linkdeps\n" " source_file_part = exe.rs\n" " source_name_part = exe\n" " externs = --extern " @@ -2091,7 +2175,7 @@ "\n" "build ./exe: rust_bin ../../linked/exe.rs | ../../linked/exe.rs " "obj/foo/file1.o obj/foo/file2.o || " - "phony/foo/foo obj/foo/Foo.swiftmodule phony/foo/foo\n" + "phony/foo/foo obj/foo/Foo.swiftmodule phony/foo/foo.linkdeps\n" " source_file_part = exe.rs\n" " source_name_part = exe\n" " externs =\n"