Reland "GN: do not make indirect dependency to direct dependency" This is a reland of e8a3ce9119189ad6ba5ecaa62bb0b53b07c895f4 Fix for breakage crbug.com/843351 is below. https://chromium-review.googlesource.com/c/chromium/src/+/1065591 Original change's description: > GN: do not make indirect dependency to direct dependency > > If a target is hard dep, there is no need to have the action targets's > recursive deps as direct dependency, because such dependency is > transitive. > > This is found by pcc in gn-dev > https://groups.google.com/a/chromium.org/forum/#!topic/gn-dev/BcrSfPQE84E > > With this change, generated android's toolchain.ninja size is reduced > from 262MB to 31MB. And ninja's startup time reduced from 4.9~5.1s to > 2.6s. > I used args.gn same with android_n5x_swarming_rel bot. > > Also this patch reduced the time of `gn gen` from 9.5~10.3s to 6.8~7.2s > on my machine. > > Change-Id: I0f0214d3abe74143516b263da839e98b3987fb64 > Reviewed-on: https://chromium-review.googlesource.com/1041506 > Reviewed-by: Brett Wilson <brettw@chromium.org> > Reviewed-by: Dirk Pranke <dpranke@chromium.org> > Commit-Queue: Takuto Ikuta <tikuta@chromium.org> > Cr-Commit-Position: refs/heads/master@{#557365} Bug: 843470 Change-Id: I4d3662613b21a1e520517d152865529aa05e813f Reviewed-on: https://chromium-review.googlesource.com/1065690 Commit-Queue: Takuto Ikuta <tikuta@chromium.org> Reviewed-by: Dirk Pranke <dpranke@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#560520} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 3ae94eab7c33428af8e5dcccf85a90975368fa54
diff --git a/tools/gn/ninja_action_target_writer_unittest.cc b/tools/gn/ninja_action_target_writer_unittest.cc index 1b2ddc8..445a31a 100644 --- a/tools/gn/ninja_action_target_writer_unittest.cc +++ b/tools/gn/ninja_action_target_writer_unittest.cc
@@ -409,3 +409,77 @@ "build obj/foo/bar.stamp: stamp input1.out\n"; EXPECT_EQ(expected_linux, out.str()); } + +TEST(NinjaActionTargetWriter, NoTransitiveHardDeps) { + Err err; + TestWithScope setup; + + setup.build_settings()->set_python_path( + base::FilePath(FILE_PATH_LITERAL("/usr/bin/python"))); + + Target dep(setup.settings(), Label(SourceDir("//foo/"), "dep")); + dep.set_output_type(Target::ACTION); + dep.visibility().SetPublic(); + dep.SetToolchain(setup.toolchain()); + ASSERT_TRUE(dep.OnResolved(&err)); + + Target foo(setup.settings(), Label(SourceDir("//foo/"), "foo")); + foo.set_output_type(Target::ACTION); + foo.visibility().SetPublic(); + foo.sources().push_back(SourceFile("//foo/input1.txt")); + foo.action_values().set_script(SourceFile("//foo/script.py")); + foo.private_deps().push_back(LabelTargetPair(&dep)); + foo.SetToolchain(setup.toolchain()); + foo.action_values().outputs() = + SubstitutionList::MakeForTest("//out/Debug/foo.out"); + ASSERT_TRUE(foo.OnResolved(&err)); + + { + std::ostringstream out; + NinjaActionTargetWriter writer(&foo, out); + writer.Run(); + + const char expected_linux[] = + "rule __foo_foo___rule\n" + // These come from the args. + " command = /usr/bin/python ../../foo/script.py\n" + " description = ACTION //foo:foo()\n" + " restat = 1\n" + "\n" + "build foo.out: __foo_foo___rule | ../../foo/script.py" + " ../../foo/input1.txt obj/foo/dep.stamp\n" + "\n" + "build obj/foo/foo.stamp: stamp foo.out\n"; + EXPECT_EQ(expected_linux, out.str()); + } + + Target bar(setup.settings(), Label(SourceDir("//bar/"), "bar")); + bar.set_output_type(Target::ACTION); + bar.sources().push_back(SourceFile("//bar/input1.txt")); + bar.action_values().set_script(SourceFile("//bar/script.py")); + bar.private_deps().push_back(LabelTargetPair(&foo)); + bar.SetToolchain(setup.toolchain()); + bar.action_values().outputs() = + SubstitutionList::MakeForTest("//out/Debug/bar.out"); + ASSERT_TRUE(bar.OnResolved(&err)) << err.message(); + + { + std::ostringstream out; + NinjaActionTargetWriter writer(&bar, out); + writer.Run(); + + const char expected_linux[] = + "rule __bar_bar___rule\n" + // These come from the args. + " command = /usr/bin/python ../../bar/script.py\n" + " description = ACTION //bar:bar()\n" + " restat = 1\n" + "\n" + // Do not have obj/foo/dep.stamp as dependency. + "build bar.out: __bar_bar___rule | ../../bar/script.py" + " ../../bar/input1.txt obj/foo/foo.stamp\n" + "\n" + "build obj/bar/bar.stamp: stamp bar.out\n"; + EXPECT_EQ(expected_linux, out.str()); + } +}
diff --git a/tools/gn/ninja_create_bundle_target_writer_unittest.cc b/tools/gn/ninja_create_bundle_target_writer_unittest.cc index ff6feaf..c745939 100644 --- a/tools/gn/ninja_create_bundle_target_writer_unittest.cc +++ b/tools/gn/ninja_create_bundle_target_writer_unittest.cc
@@ -77,14 +77,16 @@ writer.Run(); const char expected[] = + "build obj/baz/bar.inputdeps.stamp: stamp obj/foo/bar.stamp " + "obj/foo/data.stamp\n" "build bar.bundle/Contents/Resources/input1.txt: copy_bundle_data " - "../../foo/input1.txt || obj/foo/bar.stamp\n" + "../../foo/input1.txt || obj/baz/bar.inputdeps.stamp\n" "build bar.bundle/Contents/Resources/input2.txt: copy_bundle_data " - "../../foo/input2.txt || obj/foo/bar.stamp\n" + "../../foo/input2.txt || obj/baz/bar.inputdeps.stamp\n" "build obj/baz/bar.stamp: stamp " "bar.bundle/Contents/Resources/input1.txt " "bar.bundle/Contents/Resources/input2.txt" - " || obj/foo/bar.stamp\n" + " || obj/baz/bar.inputdeps.stamp\n" "build bar.bundle: phony obj/baz/bar.stamp\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -124,13 +126,16 @@ writer.Run(); const char expected[] = + "build obj/baz/bar.inputdeps.stamp: stamp obj/foo/bar.stamp " + "obj/foo/data.stamp\n" "build gen/bar.bundle/Contents/Resources/input1.txt: copy_bundle_data " - "../../foo/input1.txt || obj/foo/bar.stamp\n" + "../../foo/input1.txt || obj/baz/bar.inputdeps.stamp\n" "build gen/bar.bundle/Contents/Resources/input2.txt: copy_bundle_data " - "../../foo/input2.txt || obj/foo/bar.stamp\n" + "../../foo/input2.txt || obj/baz/bar.inputdeps.stamp\n" "build obj/baz/bar.stamp: stamp " "gen/bar.bundle/Contents/Resources/input1.txt " - "gen/bar.bundle/Contents/Resources/input2.txt || obj/foo/bar.stamp\n" + "gen/bar.bundle/Contents/Resources/input2.txt || " + "obj/baz/bar.inputdeps.stamp\n" "build gen/bar.bundle: phony obj/baz/bar.stamp\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -214,11 +219,15 @@ writer.Run(); const char expected[] = + "build obj/baz/bar.inputdeps.stamp: stamp obj/foo/bar.stamp " + "obj/foo/data.stamp\n" "build bar.bundle/Contents/Resources/Assets.car: compile_xcassets " - "../../foo/Foo.xcassets | obj/foo/data.stamp || obj/foo/bar.stamp\n" + "../../foo/Foo.xcassets | obj/foo/data.stamp || " + "obj/baz/bar.inputdeps.stamp\n" " product_type = com.apple.product-type\n" "build obj/baz/bar.stamp: stamp " - "bar.bundle/Contents/Resources/Assets.car || obj/foo/bar.stamp\n" + "bar.bundle/Contents/Resources/Assets.car || " + "obj/baz/bar.inputdeps.stamp\n" "build bar.bundle: phony obj/baz/bar.stamp\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -337,12 +346,15 @@ writer.Run(); const char expected[] = + "build obj/baz/bar.inputdeps.stamp: stamp obj/foo/assets.stamp " + "obj/foo/bar.stamp obj/foo/data.stamp obj/qux/info_plist.stamp " + "obj/quz/assets.stamp\n" "build bar.bundle/Contents/Info.plist: copy_bundle_data " - "../../qux/qux-Info.plist || obj/foo/bar.stamp\n" + "../../qux/qux-Info.plist || obj/baz/bar.inputdeps.stamp\n" "build bar.bundle/Contents/Resources/input1.txt: copy_bundle_data " - "../../foo/input1.txt || obj/foo/bar.stamp\n" + "../../foo/input1.txt || obj/baz/bar.inputdeps.stamp\n" "build bar.bundle/Contents/Resources/input2.txt: copy_bundle_data " - "../../foo/input2.txt || obj/foo/bar.stamp\n" + "../../foo/input2.txt || obj/baz/bar.inputdeps.stamp\n" "build obj/baz/bar.xcassets.inputdeps.stamp: stamp " "obj/foo/assets.stamp " "obj/quz/assets.stamp\n" @@ -350,7 +362,7 @@ "baz/bar/bar_partial_info.plist: compile_xcassets " "../../foo/Foo.xcassets " "../../quz/Quz.xcassets | obj/baz/bar.xcassets.inputdeps.stamp || " - "obj/foo/bar.stamp\n" + "obj/baz/bar.inputdeps.stamp\n" " product_type = com.apple.product-type\n" " partial_info_plist = baz/bar/bar_partial_info.plist\n" "build obj/baz/bar.stamp: stamp " @@ -358,7 +370,7 @@ "bar.bundle/Contents/Resources/input1.txt " "bar.bundle/Contents/Resources/input2.txt " "bar.bundle/Contents/Resources/Assets.car " - "baz/bar/bar_partial_info.plist || obj/foo/bar.stamp\n" + "baz/bar/bar_partial_info.plist || obj/baz/bar.inputdeps.stamp\n" "build bar.bundle: phony obj/baz/bar.stamp\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str); @@ -416,26 +428,29 @@ writer.Run(); const char expected[] = + "build obj/baz/bar.inputdeps.stamp: stamp ./quz obj/foo/bar.stamp " + "obj/foo/data.stamp\n" "rule __baz_bar___toolchain_default__code_signing_rule\n" " command = ../../build/codesign.py -b=quz bar.bundle\n" " description = CODE SIGNING //baz:bar(//toolchain:default)\n" " restat = 1\n" "\n" "build bar.bundle/Contents/Resources/input1.txt: copy_bundle_data " - "../../foo/input1.txt || obj/foo/bar.stamp\n" + "../../foo/input1.txt || obj/baz/bar.inputdeps.stamp\n" "build bar.bundle/Contents/Resources/input2.txt: copy_bundle_data " - "../../foo/input2.txt || obj/foo/bar.stamp\n" + "../../foo/input2.txt || obj/baz/bar.inputdeps.stamp\n" "build obj/baz/bar.codesigning.inputdeps.stamp: stamp " "../../build/codesign.py " "quz " "bar.bundle/Contents/Resources/input1.txt " - "bar.bundle/Contents/Resources/input2.txt || obj/foo/bar.stamp\n" + "bar.bundle/Contents/Resources/input2.txt || " + "obj/baz/bar.inputdeps.stamp\n" "build bar.bundle/Contents/quz bar.bundle/_CodeSignature/CodeResources: " "__baz_bar___toolchain_default__code_signing_rule " "| obj/baz/bar.codesigning.inputdeps.stamp\n" "build obj/baz/bar.stamp: stamp " "bar.bundle/Contents/quz " - "bar.bundle/_CodeSignature/CodeResources || obj/foo/bar.stamp\n" + "bar.bundle/_CodeSignature/CodeResources || obj/baz/bar.inputdeps.stamp\n" "build bar.bundle: phony obj/baz/bar.stamp\n"; std::string out_str = out.str(); EXPECT_EQ(expected, out_str);
diff --git a/tools/gn/ninja_target_writer_unittest.cc b/tools/gn/ninja_target_writer_unittest.cc index 68a59c8..38dd8de 100644 --- a/tools/gn/ninja_target_writer_unittest.cc +++ b/tools/gn/ninja_target_writer_unittest.cc
@@ -5,6 +5,7 @@ #include <sstream> #include "testing/gtest/include/gtest/gtest.h" +#include "tools/gn/ninja_action_target_writer.h" #include "tools/gn/ninja_target_writer.h" #include "tools/gn/target.h" #include "tools/gn/test_with_scope.h" @@ -94,6 +95,23 @@ EXPECT_EQ("obj/foo/base.stamp", dep[0].value()); } + { + std::ostringstream stream; + NinjaActionTargetWriter writer(&action, stream); + writer.Run(); + EXPECT_EQ( + "rule __foo_action___rule\n" + " command = ../../foo/script.py\n" + " description = ACTION //foo:action()\n" + " restat = 1\n" + "\n" + "build: __foo_action___rule | ../../foo/script.py" + " ../../foo/action_source.txt ./target\n" + "\n" + "build obj/foo/action.stamp: stamp\n", + stream.str()); + } + // Input deps for action which should depend on the base since its a hard dep // that is a (indirect) dependency, as well as the the action source. { @@ -104,9 +122,10 @@ ASSERT_EQ(1u, dep.size()); EXPECT_EQ("obj/foo/action.inputdeps.stamp", dep[0].value()); - EXPECT_EQ("build obj/foo/action.inputdeps.stamp: stamp ../../foo/script.py " - "../../foo/action_source.txt obj/foo/base.stamp\n", - stream.str()); + EXPECT_EQ( + "build obj/foo/action.inputdeps.stamp: stamp ../../foo/script.py " + "../../foo/action_source.txt ./target\n", + stream.str()); } }
diff --git a/tools/gn/target.cc b/tools/gn/target.cc index 15df27f..d0714a8 100644 --- a/tools/gn/target.cc +++ b/tools/gn/target.cc
@@ -581,8 +581,10 @@ void Target::PullRecursiveHardDeps() { for (const auto& pair : GetDeps(DEPS_LINKED)) { // Direct hard dependencies. - if (pair.ptr->hard_dep()) + if (hard_dep() || pair.ptr->hard_dep()) { recursive_hard_deps_.insert(pair.ptr); + continue; + } // If |pair.ptr| is binary target and |pair.ptr| has no public header, // |this| target does not need to have |pair.ptr|'s hard_deps as its
diff --git a/tools/gn/target.h b/tools/gn/target.h index 568bea5..ed8abd5 100644 --- a/tools/gn/target.h +++ b/tools/gn/target.h
@@ -170,10 +170,9 @@ // Returns true if targets depending on this one should have an order // dependency. bool hard_dep() const { - return output_type_ == ACTION || - output_type_ == ACTION_FOREACH || - output_type_ == COPY_FILES || - output_type_ == CREATE_BUNDLE; + return output_type_ == ACTION || output_type_ == ACTION_FOREACH || + output_type_ == COPY_FILES || output_type_ == CREATE_BUNDLE || + output_type_ == BUNDLE_DATA; } // Returns the iterator range which can be used in range-based for loops