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: 3ae94eab7c33428af8e5dcccf85a90975368fa54diff --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
