Propagate public_deps outputs via dependency_output for non-binary targets For non-binary targets (like actions, groups, etc.), their dependency outputs (stamp or phony targets) now transitively depend on the dependency outputs of their public_deps. This ensures that dependents of the current target will also implicitly depend on the outputs of its public_deps, preventing the inflation of implicit inputs on each build line. Also updated various unit tests and added a new complex test case to verify this behavior. This removes the necessity of some redundant BUILD.gn config changes like https://crrev.com/c/8003676/8/front_end/models/ai_assistance/skills/BUILD.gn to propagate outputs of actions to indirect dependents. Bug: 513105742 Change-Id: I14de8cea4e4add977cacdc88386737aedda16f86 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/23520 Commit-Queue: Takuto Ikuta <tikuta@google.com> Reviewed-by: Matt Stark <msta@google.com>
diff --git a/docs/reference.md b/docs/reference.md index 75b75a0..a8d370a 100644 --- a/docs/reference.md +++ b/docs/reference.md
@@ -6838,6 +6838,12 @@ publicly depends on (directly or indirectly) are propagated up the dependency tree to dependents for linking. + - For non-binary targets (like actions, groups, etc.), their dependency + outputs (stamp or phony targets) will transitively depend on the + dependency outputs of their public_deps. This ensures that dependents + of the current target will also implicitly depend on the outputs of the + public_deps. + See also "gn help public_configs". ```
diff --git a/src/gn/ninja_create_bundle_target_writer_unittest.cc b/src/gn/ninja_create_bundle_target_writer_unittest.cc index 7aa1313..2d23969 100644 --- a/src/gn/ninja_create_bundle_target_writer_unittest.cc +++ b/src/gn/ninja_create_bundle_target_writer_unittest.cc
@@ -493,7 +493,7 @@ "| phony/baz/bar.postprocessing.inputdeps\n" "build phony/baz/bar: phony " "bar.bundle/Contents/quz " - "bar.bundle/_CodeSignature/CodeResources\n" + "bar.bundle/_CodeSignature/CodeResources ./quz\n" "build bar.bundle: phony phony/baz/bar\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -579,7 +579,7 @@ "| toolchain/phony/baz/bar.postprocessing.inputdeps\n" "build toolchain/phony/baz/bar: phony " "bar.bundle/Contents/quz " - "bar.bundle/_CodeSignature/CodeResources\n" + "bar.bundle/_CodeSignature/CodeResources ./quz\n" "build bar.bundle: phony toolchain/phony/baz/bar\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str);
diff --git a/src/gn/ninja_generated_file_target_writer_unittest.cc b/src/gn/ninja_generated_file_target_writer_unittest.cc index 481db80..a5b84e9 100644 --- a/src/gn/ninja_generated_file_target_writer_unittest.cc +++ b/src/gn/ninja_generated_file_target_writer_unittest.cc
@@ -64,7 +64,7 @@ const char expected[] = "build phony/foo/bar: phony foo.json phony/foo/dep " - "phony/foo/dep2 || " + "phony/foo/dep2 phony/foo/bundle_data_dep || " "phony/foo/bundle_data_dep phony/foo/datadep\n"; EXPECT_EQ(expected, out.str()); }
diff --git a/src/gn/ninja_group_target_writer_unittest.cc b/src/gn/ninja_group_target_writer_unittest.cc index ec9f72d..39a42c8 100644 --- a/src/gn/ninja_group_target_writer_unittest.cc +++ b/src/gn/ninja_group_target_writer_unittest.cc
@@ -54,7 +54,8 @@ writer.Run(); const char expected[] = - "build phony/foo/bar: phony phony/foo/dep phony/foo/dep2 || " + "build phony/foo/bar: phony phony/foo/dep phony/foo/dep2 " + "phony/foo/bundle_data_dep || " "phony/foo/bundle_data_dep phony/foo/datadep\n"; EXPECT_EQ(expected, out.str()); }
diff --git a/src/gn/ninja_target_writer.cc b/src/gn/ninja_target_writer.cc index 825439a..b321457 100644 --- a/src/gn/ninja_target_writer.cc +++ b/src/gn/ninja_target_writer.cc
@@ -4,6 +4,7 @@ #include "gn/ninja_target_writer.h" +#include <algorithm> #include <sstream> #include "base/files/file_util.h" @@ -619,6 +620,22 @@ void NinjaTargetWriter::WriteStampOrPhonyForTarget( const std::vector<OutputFile>& files, const std::vector<OutputFile>& order_only_deps) { + // Add dependency outputs of public_deps to this target's stamp or phony + // target to propagate public dependencies transitively. This allows + // dependents of this target to implicitly depend on the public_deps without + // listing them all directly on their build lines, preventing inflation of + // implicit inputs. + std::vector<OutputFile> all_files = files; + for (const auto& pair : target_->public_deps()) { + if (pair.ptr->has_dependency_output()) { + OutputFile dep_out = pair.ptr->dependency_output(); + if (std::find(all_files.begin(), all_files.end(), dep_out) == + all_files.end()) { + all_files.push_back(dep_out); + } + } + } + // We should have already discerned whether this target is a stamp or a phony. // If there's a dependency_output_file, it should be a stamp. Else is a phony // or omitted phony (in which case, we don't write it). @@ -651,12 +668,12 @@ } else { // This is the omitted phony case. We should not get here if there were any // dependencies, so ensure that none got added. - CHECK(files.empty()); + CHECK(all_files.empty()); CHECK(order_only_deps.empty()); return; } - path_output_.WriteFiles(out_, files); + path_output_.WriteFiles(out_, all_files); if (!order_only_deps.empty()) { out_ << " ||";
diff --git a/src/gn/ninja_target_writer_unittest.cc b/src/gn/ninja_target_writer_unittest.cc index cd4a5e4..6ba1a27 100644 --- a/src/gn/ninja_target_writer_unittest.cc +++ b/src/gn/ninja_target_writer_unittest.cc
@@ -5,6 +5,7 @@ #include <sstream> #include "gn/ninja_action_target_writer.h" +#include "gn/ninja_group_target_writer.h" #include "gn/ninja_target_writer.h" #include "gn/target.h" #include "gn/test_with_scope.h" @@ -157,7 +158,7 @@ "build: __foo_action___rule | ../../foo/script.py" " ../../foo/action_source.txt ./target\n" "\n" - "build phony/foo/action: phony\n", + "build phony/foo/action: phony ./target\n", stream.str()); } @@ -257,7 +258,7 @@ "build: __foo_action___rule | ../../foo/script.py" " ../../foo/action_source.txt ./target\n" "\n" - "build obj/foo/action.stamp: stamp\n", + "build obj/foo/action.stamp: stamp ./target\n", stream.str()); } @@ -440,3 +441,126 @@ // output. EXPECT_EQ("build phony/foo/target: phony phony/foo/dep\n", out); } + +TEST(NinjaTargetWriter, PhonyPropagation) { + TestWithScope setup; + Err err; + + // Set up common target A (action) + Target a(setup.settings(), Label(SourceDir("//foo/"), "a")); + a.set_output_type(Target::ACTION); + a.visibility().SetPublic(); + a.SetToolchain(setup.toolchain()); + a.action_values().set_script(SourceFile("//foo/script.py")); + a.action_values().outputs() = + SubstitutionList::MakeForTest("//out/Debug/a.out"); + + // Set up B_pub (action, public_dep A) + Target b_pub(setup.settings(), Label(SourceDir("//foo/"), "b_pub")); + b_pub.set_output_type(Target::ACTION); + b_pub.visibility().SetPublic(); + b_pub.SetToolchain(setup.toolchain()); + b_pub.action_values().set_script(SourceFile("//foo/script.py")); + b_pub.action_values().outputs() = + SubstitutionList::MakeForTest("//out/Debug/b_pub.out"); + b_pub.public_deps().push_back(LabelTargetPair(&a)); + + // Set up B_priv (action, private_dep A) + Target b_priv(setup.settings(), Label(SourceDir("//foo/"), "b_priv")); + b_priv.set_output_type(Target::ACTION); + b_priv.visibility().SetPublic(); + b_priv.SetToolchain(setup.toolchain()); + b_priv.action_values().set_script(SourceFile("//foo/script.py")); + b_priv.action_values().outputs() = + SubstitutionList::MakeForTest("//out/Debug/b_priv.out"); + b_priv.private_deps().push_back(LabelTargetPair(&a)); + + // Set up C_pub (action, private_dep B_pub) + Target c_pub(setup.settings(), Label(SourceDir("//foo/"), "c_pub")); + c_pub.set_output_type(Target::ACTION); + c_pub.visibility().SetPublic(); + c_pub.SetToolchain(setup.toolchain()); + c_pub.action_values().set_script(SourceFile("//foo/script.py")); + c_pub.private_deps().push_back(LabelTargetPair(&b_pub)); + + // Set up C_priv (action, private_dep B_priv) + Target c_priv(setup.settings(), Label(SourceDir("//foo/"), "c_priv")); + c_priv.set_output_type(Target::ACTION); + c_priv.visibility().SetPublic(); + c_priv.SetToolchain(setup.toolchain()); + c_priv.action_values().set_script(SourceFile("//foo/script.py")); + c_priv.private_deps().push_back(LabelTargetPair(&b_priv)); + + ASSERT_TRUE(a.OnResolved(&err)); + ASSERT_TRUE(b_pub.OnResolved(&err)); + ASSERT_TRUE(b_priv.OnResolved(&err)); + ASSERT_TRUE(c_pub.OnResolved(&err)); + ASSERT_TRUE(c_priv.OnResolved(&err)); + + // 1. Verify B_pub's stamp/phony target. It should contain A's output + // (phony/foo/a). + { + std::ostringstream stream; + NinjaActionTargetWriter writer(&b_pub, stream); + writer.Run(); + EXPECT_EQ( + "rule __foo_b_pub___rule\n" + " command = ../../foo/script.py\n" + " description = ACTION //foo:b_pub()\n" + " restat = 1\n" + "\n" + "build b_pub.out: __foo_b_pub___rule | ../../foo/script.py " + "phony/foo/a\n" + "\n" + "build phony/foo/b_pub: phony b_pub.out phony/foo/a\n", + stream.str()); + } + + // 2. Verify B_priv's stamp/phony target. It should NOT contain A's output. + { + std::ostringstream stream; + NinjaActionTargetWriter writer(&b_priv, stream); + writer.Run(); + EXPECT_EQ( + "rule __foo_b_priv___rule\n" + " command = ../../foo/script.py\n" + " description = ACTION //foo:b_priv()\n" + " restat = 1\n" + "\n" + "build b_priv.out: __foo_b_priv___rule | ../../foo/script.py " + "phony/foo/a\n" + "\n" + "build phony/foo/b_priv: phony b_priv.out\n", + stream.str()); + } + + // 3. Verify C_pub's inputdeps contains B_pub's stamp/phony. + { + std::ostringstream stream; + TestingNinjaTargetWriter writer(&c_pub, setup.toolchain(), stream); + auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + std::vector<const Target*>(), 10u); + + ASSERT_EQ(1u, dep.implicit.size()); + EXPECT_EQ("phony/foo/c_pub.inputdeps", dep.implicit[0].value()); + EXPECT_EQ( + "build phony/foo/c_pub.inputdeps: phony ../../foo/script.py " + "phony/foo/b_pub\n", + stream.str()); + } + + // 4. Verify C_priv's inputdeps contains B_priv's stamp/phony. + { + std::ostringstream stream; + TestingNinjaTargetWriter writer(&c_priv, setup.toolchain(), stream); + auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + std::vector<const Target*>(), 10u); + + ASSERT_EQ(1u, dep.implicit.size()); + EXPECT_EQ("phony/foo/c_priv.inputdeps", dep.implicit[0].value()); + EXPECT_EQ( + "build phony/foo/c_priv.inputdeps: phony ../../foo/script.py " + "phony/foo/b_priv\n", + stream.str()); + } +}
diff --git a/src/gn/variables.cc b/src/gn/variables.cc index 960689b..dcb46e8 100644 --- a/src/gn/variables.cc +++ b/src/gn/variables.cc
@@ -2025,6 +2025,12 @@ publicly depends on (directly or indirectly) are propagated up the dependency tree to dependents for linking. + - For non-binary targets (like actions, groups, etc.), their dependency + outputs (stamp or phony targets) will transitively depend on the + dependency outputs of their public_deps. This ensures that dependents + of the current target will also implicitly depend on the outputs of the + public_deps. + See also "gn help public_configs". Discussion