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; }