Reland "Propagate public_deps outputs via dependency_output for non-binary targets" This relands the change with a fix to prevent propagating dependency outputs of data-only targets (like bundle_data) via public_deps. Data-only targets are runtime dependencies and should not be propagated as implicit (build-time) dependencies, to avoid having unnecessary inputs. Also I sent deps reduction to chromium as https://crrev.com/c/8023463 to avoid the limit of maximum allowed number of files in remote link. Original change's description: > Revert "Propagate public_deps outputs via dependency_output for non-binary targets" > > This reverts commit 3bccda11b7bc16d89994f34da1e6604889f88f86. > > Reason for revert: This doesn't pass chromium's CQ. > > Failure Link: > https://ci.chromium.org/ui/p/chromium/builders/try/linux-chromeos-rel/2928808? > > Original change's description: > > 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> > > Bug: 513105742 > Change-Id: I35977d90e1f59bbd37fdd3d3c1dc5deedbb3d417 > Reviewed-on: https://gn-review.googlesource.com/c/gn/+/23541 > Reviewed-by: Junji Watanabe <jwata@google.com> > Commit-Queue: Takuto Ikuta <tikuta@google.com> # Not skipping CQ checks because this is a reland. Bug: 513105742 Change-Id: I1599385b8902a3fe0a471afd30975a85f4520670 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/23560 Reviewed-by: Junji Watanabe <jwata@google.com> Commit-Queue: Takuto Ikuta <tikuta@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_target_writer.cc b/src/gn/ninja_target_writer.cc index 825439a..6753701 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() && !pair.ptr->IsDataOnly()) { + 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..9cdfaaa 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,209 @@ // 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)); + + // Set up B_data (action, data_dep A) + Target b_data(setup.settings(), Label(SourceDir("//foo/"), "b_data")); + b_data.set_output_type(Target::ACTION); + b_data.visibility().SetPublic(); + b_data.SetToolchain(setup.toolchain()); + b_data.action_values().set_script(SourceFile("//foo/script.py")); + b_data.action_values().outputs() = + SubstitutionList::MakeForTest("//out/Debug/b_data.out"); + b_data.data_deps().push_back(LabelTargetPair(&a)); + + // Set up C_data (action, public_dep B_data) + Target c_data(setup.settings(), Label(SourceDir("//foo/"), "c_data")); + c_data.set_output_type(Target::ACTION); + c_data.visibility().SetPublic(); + c_data.SetToolchain(setup.toolchain()); + c_data.action_values().set_script(SourceFile("//foo/script.py")); + c_data.action_values().outputs() = + SubstitutionList::MakeForTest("//out/Debug/c_data.out"); + c_data.public_deps().push_back(LabelTargetPair(&b_data)); + + // Set up D_data (action, private_dep C_data) + Target d_data(setup.settings(), Label(SourceDir("//foo/"), "d_data")); + d_data.set_output_type(Target::ACTION); + d_data.visibility().SetPublic(); + d_data.SetToolchain(setup.toolchain()); + d_data.action_values().set_script(SourceFile("//foo/script.py")); + d_data.private_deps().push_back(LabelTargetPair(&c_data)); + + 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)); + ASSERT_TRUE(b_data.OnResolved(&err)); + ASSERT_TRUE(c_data.OnResolved(&err)); + ASSERT_TRUE(d_data.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()); + } + + // 5. Verify B_data's stamp/phony target. It should NOT contain A's output + // in implicit deps (since A is a data_dep), but in order_only_deps. + { + std::ostringstream stream; + NinjaActionTargetWriter writer(&b_data, stream); + writer.Run(); + EXPECT_EQ( + "rule __foo_b_data___rule\n" + " command = ../../foo/script.py\n" + " description = ACTION //foo:b_data()\n" + " restat = 1\n" + "\n" + "build b_data.out: __foo_b_data___rule | ../../foo/script.py || " + "phony/foo/a\n" + "\n" + "build phony/foo/b_data: phony b_data.out\n", + stream.str()); + } + + // 6. Verify C_data's stamp/phony target. It should contain B_data, but NOT A. + { + std::ostringstream stream; + NinjaActionTargetWriter writer(&c_data, stream); + writer.Run(); + EXPECT_EQ( + "rule __foo_c_data___rule\n" + " command = ../../foo/script.py\n" + " description = ACTION //foo:c_data()\n" + " restat = 1\n" + "\n" + "build c_data.out: __foo_c_data___rule | ../../foo/script.py " + "phony/foo/b_data\n" + "\n" + "build phony/foo/c_data: phony c_data.out phony/foo/b_data\n", + stream.str()); + } + + // 7. Verify D_data's inputdeps contains C_data, but NOT A. + { + std::ostringstream stream; + TestingNinjaTargetWriter writer(&d_data, setup.toolchain(), stream); + auto dep = writer.WriteInputDepsStampOrPhonyAndGetDep( + std::vector<const Target*>(), 10u); + + ASSERT_EQ(1u, dep.implicit.size()); + EXPECT_EQ("phony/foo/d_data.inputdeps", dep.implicit[0].value()); + EXPECT_EQ( + "build phony/foo/d_data.inputdeps: phony ../../foo/script.py " + "phony/foo/c_data\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