Revert "Build non-linkable deps async with Ninja's validaitons"
This reverts commit 90478db6b59b9bebf7ca4cf912d860cf868e724c.
Reason for revert: The idea of using validations for data_deps was
rejected by Chromium and Fuchsia folks.
Original change's description:
> Build non-linkable deps async with Ninja's validaitons
>
> This CL introduces a build setting flag `async_non_linkable_deps` that
> write non-linkable deps as Ninja's `validations` instead of order-only
> deps.
>
> I've tested locally that it can build Chromium without errors.
> Having the feature flag allows me to try the feature in Chromium's CQ
> with various build configs on multiple platforms.
>
> Bug: 413507213
> Change-Id: Ic527feebfc64323c5136c83f83264eb577a9560f
> Reviewed-on: https://gn-review.googlesource.com/c/gn/+/18681
> Reviewed-by: Takuto Ikuta <tikuta@google.com>
> Commit-Queue: Takuto Ikuta <tikuta@google.com>
Bug: 413507213
Change-Id: Ia1a31fb1b2ad7a1a28b1d98e954b2df062bfba4a
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/20300
Commit-Queue: Junji Watanabe <jwata@google.com>
Reviewed-by: Takuto Ikuta <tikuta@google.com>
diff --git a/src/gn/build_settings.h b/src/gn/build_settings.h
index 6cf05e0..e21851a 100644
--- a/src/gn/build_settings.h
+++ b/src/gn/build_settings.h
@@ -77,16 +77,6 @@
no_stamp_files_ = no_stamp_files;
}
- // The 'async_non_linkable_deps' boolean flag can be set to generate Ninja
- // files that build non-linkable deps asynchronously using Ninja validations
- // instead of order-only dependencies.
- // This speeds up build by increasing the parallerism of the build graph.
- // This requires Ninja 1.11 for the `validations` feature.
- bool async_non_linkable_deps() const { return async_non_linkable_deps_; }
- void set_async_non_linkable_deps(bool async_non_linkable_deps) {
- async_non_linkable_deps_ = async_non_linkable_deps;
- }
-
const SourceFile& build_config_file() const { return build_config_file_; }
void set_build_config_file(const SourceFile& f) { build_config_file_ = f; }
@@ -163,7 +153,6 @@
// See 40045b9 for the reason behind using 1.7.2 as the default version.
Version ninja_required_version_{1, 7, 2};
bool no_stamp_files_ = true;
- bool async_non_linkable_deps_ = false;
SourceFile build_config_file_;
SourceFile arg_file_template_path_;
diff --git a/src/gn/ninja_c_binary_target_writer.cc b/src/gn/ninja_c_binary_target_writer.cc
index 94580e9..cc98af4 100644
--- a/src/gn/ninja_c_binary_target_writer.cc
+++ b/src/gn/ninja_c_binary_target_writer.cc
@@ -611,8 +611,6 @@
}
// Append data dependencies as order-only dependencies.
- // If `async_non_linkable_deps` flag is set, it uses
- // validations instead.
//
// This will include data dependencies and input dependencies (like when
// this target depends on an action). Having the data dependencies in this
@@ -625,11 +623,7 @@
// on the sources, there is already an implicit order-only dependency.
// However, it's extra work to separate these out and there's no disadvantage
// to listing them again.
- if (settings_->build_settings()->async_non_linkable_deps()) {
- WriteValidations(classified_deps.non_linkable_deps);
- } else {
- WriteOrderOnlyDependencies(classified_deps.non_linkable_deps);
- }
+ WriteOrderOnlyDependencies(classified_deps.non_linkable_deps);
// End of the link "build" line.
out_ << std::endl;
@@ -698,32 +692,15 @@
void NinjaCBinaryTargetWriter::WriteOrderOnlyDependencies(
const UniqueVector<const Target*>& non_linkable_deps) {
- if (non_linkable_deps.empty())
- return;
+ if (!non_linkable_deps.empty()) {
+ out_ << " ||";
- out_ << " ||";
-
- // Non-linkable targets.
- for (auto* non_linkable_dep : non_linkable_deps) {
- if (non_linkable_dep->has_dependency_output()) {
- out_ << " ";
- path_output_.WriteFile(out_, non_linkable_dep->dependency_output());
- }
- }
-}
-
-void NinjaCBinaryTargetWriter::WriteValidations(
- const UniqueVector<const Target*>& non_linkable_deps) {
- if (non_linkable_deps.empty())
- return;
-
- out_ << " |@";
-
- // Non-linkable targets.
- for (auto* non_linkable_dep : non_linkable_deps) {
- if (non_linkable_dep->has_dependency_output()) {
- out_ << " ";
- path_output_.WriteFile(out_, non_linkable_dep->dependency_output());
+ // Non-linkable targets.
+ for (auto* non_linkable_dep : non_linkable_deps) {
+ if (non_linkable_dep->has_dependency_output()) {
+ out_ << " ";
+ path_output_.WriteFile(out_, non_linkable_dep->dependency_output());
+ }
}
}
}
diff --git a/src/gn/ninja_c_binary_target_writer.h b/src/gn/ninja_c_binary_target_writer.h
index 8baa434..39e8320 100644
--- a/src/gn/ninja_c_binary_target_writer.h
+++ b/src/gn/ninja_c_binary_target_writer.h
@@ -103,13 +103,6 @@
void WriteOrderOnlyDependencies(
const UniqueVector<const Target*>& non_linkable_deps);
- // Writes the validations for the link or stamp line. This is
- // the "|@" and everything following it on the ninja line.
- //
- // The validations are the non-linkable deps passed in as an argument, plus
- // the data file dependencies in the target.
- void WriteValidations(const UniqueVector<const Target*>& non_linkable_deps);
-
// Checks for duplicates in the given list of output files. If any duplicates
// are found, throws an error and return false.
bool CheckForDuplicateObjectFiles(const std::vector<OutputFile>& files) const;
diff --git a/src/gn/ninja_c_binary_target_writer_unittest.cc b/src/gn/ninja_c_binary_target_writer_unittest.cc
index dfaa5ca..9789327 100644
--- a/src/gn/ninja_c_binary_target_writer_unittest.cc
+++ b/src/gn/ninja_c_binary_target_writer_unittest.cc
@@ -2846,51 +2846,3 @@
std::string out_str = out.str();
EXPECT_EQ(expected, out_str) << expected << "\n" << out_str;
}
-
-TEST_F(NinjaCBinaryTargetWriterTest, AsyncNonLinkableDeps) {
- Err err;
- TestWithScope setup;
- BuildSettings* settings = setup.build_settings();
- settings->set_async_non_linkable_deps(true);
-
- Target datadep(setup.settings(), Label(SourceDir("//foo/"), "datadep"));
- datadep.set_output_type(Target::ACTION);
- datadep.visibility().SetPublic();
- datadep.SetToolchain(setup.toolchain());
- ASSERT_TRUE(datadep.OnResolved(&err));
-
- Target target(setup.settings(), Label(SourceDir("//foo/"), "bar"));
- target.sources().push_back(SourceFile("//foo/source.cc"));
- target.set_output_type(Target::EXECUTABLE);
- target.visibility().SetPublic();
- target.SetToolchain(setup.toolchain());
- target.data_deps().push_back(LabelTargetPair(&datadep));
- ASSERT_TRUE(target.OnResolved(&err));
-
- std::ostringstream out;
- NinjaBinaryTargetWriter writer(&target, out);
- writer.Run();
-
- const char expected[] =
- "defines =\n"
- "include_dirs =\n"
- "root_out_dir = .\n"
- "target_gen_dir = gen/foo\n"
- "target_out_dir = obj/foo\n"
- "target_output_name = bar\n"
- "\n"
- "build obj/foo/bar.source.o: cxx ../../foo/source.cc\n"
- " source_file_part = source.cc\n"
- " source_name_part = source\n"
- "\n"
- "build ./bar: link obj/foo/bar.source.o |@ phony/foo/datadep\n"
- " ldflags =\n"
- " libs =\n"
- " frameworks =\n"
- " swiftmodules =\n"
- " output_extension =\n"
- " output_dir =\n";
-
- std::string out_str = out.str();
- EXPECT_EQ(expected, out_str) << expected << "\n" << out_str;
-}
diff --git a/src/gn/setup.cc b/src/gn/setup.cc
index f11dfce..49830ae 100644
--- a/src/gn/setup.cc
+++ b/src/gn/setup.cc
@@ -1184,16 +1184,5 @@
export_compile_commands_.push_back(std::move(pat));
}
- // Async non-linkable deps.
- const Value* async_non_linkable_deps =
- dotfile_scope_.GetValue("async_non_linkable_deps", true);
- if (async_non_linkable_deps) {
- if (!async_non_linkable_deps->VerifyTypeIs(Value::BOOLEAN, err)) {
- return false;
- }
- build_settings_.set_async_non_linkable_deps(
- async_non_linkable_deps->boolean_value());
- }
-
return true;
}
