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>
diff --git a/docs/reference.md b/docs/reference.md index a8d370a..75b75a0 100644 --- a/docs/reference.md +++ b/docs/reference.md
@@ -6838,12 +6838,6 @@ 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 2d23969..7aa1313 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 ./quz\n" + "bar.bundle/_CodeSignature/CodeResources\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 ./quz\n" + "bar.bundle/_CodeSignature/CodeResources\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 a5b84e9..481db80 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/bundle_data_dep || " + "phony/foo/dep2 || " "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 39a42c8..ec9f72d 100644 --- a/src/gn/ninja_group_target_writer_unittest.cc +++ b/src/gn/ninja_group_target_writer_unittest.cc
@@ -54,8 +54,7 @@ writer.Run(); const char expected[] = - "build phony/foo/bar: phony phony/foo/dep phony/foo/dep2 " - "phony/foo/bundle_data_dep || " + "build phony/foo/bar: phony phony/foo/dep phony/foo/dep2 || " "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 b321457..825439a 100644 --- a/src/gn/ninja_target_writer.cc +++ b/src/gn/ninja_target_writer.cc
@@ -4,7 +4,6 @@ #include "gn/ninja_target_writer.h" -#include <algorithm> #include <sstream> #include "base/files/file_util.h" @@ -620,22 +619,6 @@ 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). @@ -668,12 +651,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(all_files.empty()); + CHECK(files.empty()); CHECK(order_only_deps.empty()); return; } - path_output_.WriteFiles(out_, all_files); + path_output_.WriteFiles(out_, 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 6ba1a27..cd4a5e4 100644 --- a/src/gn/ninja_target_writer_unittest.cc +++ b/src/gn/ninja_target_writer_unittest.cc
@@ -5,7 +5,6 @@ #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" @@ -158,7 +157,7 @@ "build: __foo_action___rule | ../../foo/script.py" " ../../foo/action_source.txt ./target\n" "\n" - "build phony/foo/action: phony ./target\n", + "build phony/foo/action: phony\n", stream.str()); } @@ -258,7 +257,7 @@ "build: __foo_action___rule | ../../foo/script.py" " ../../foo/action_source.txt ./target\n" "\n" - "build obj/foo/action.stamp: stamp ./target\n", + "build obj/foo/action.stamp: stamp\n", stream.str()); } @@ -441,126 +440,3 @@ // 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 dcb46e8..960689b 100644 --- a/src/gn/variables.cc +++ b/src/gn/variables.cc
@@ -2025,12 +2025,6 @@ 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