Revert "Record generated_file()s as outputs of `gn`" This reverts commit 23ede3b7b53f5bc56d3952c6d727f167cae34ab2. Reason for revert: seeing errors in which `gn gen` dirties build.ninja, which results in an infinite loop. Original change's description: > Record generated_file()s as outputs of `gn` > > This is an accurate relationship and allows for tooling to ensure a > given generated_file() is up-to-date without an unconditional `gn gen`. > > build.ninja was manually verified in a particular GN-based build, > wherein `ninja $SOME_GENERATED_FILE` now works. > > Fixed: 301 > Change-Id: I06c7ec142072d2e73860cdde4898af6f9e65065a > Reviewed-on: https://gn-review.googlesource.com/c/gn/+/14580 > Commit-Queue: Joshua Seaton <joshuaseaton@google.com> > Reviewed-by: Brett Wilson <brettw@chromium.org> TBR=brettw@chromium.org,brettw@google.com,joshuaseaton@google.com,gn-scoped@luci-project-accounts.iam.gserviceaccount.com Change-Id: Iae22482c42eafeff0cb0b54d204be4693f4f3208 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://gn-review.googlesource.com/c/gn/+/14600 Reviewed-by: Brett Wilson <brettw@chromium.org> Commit-Queue: Nico Weber <thakis@chromium.org> Reviewed-by: Joshua Seaton <joshuaseaton@google.com>
diff --git a/src/gn/ninja_build_writer.cc b/src/gn/ninja_build_writer.cc index 5e83bc8..6a5b91d 100644 --- a/src/gn/ninja_build_writer.cc +++ b/src/gn/ninja_build_writer.cc
@@ -23,7 +23,6 @@ #include "gn/input_file_manager.h" #include "gn/loader.h" #include "gn/ninja_utils.h" -#include "gn/pointer_set.h" #include "gn/pool.h" #include "gn/scheduler.h" #include "gn/string_atom.h" @@ -141,10 +140,10 @@ // Given an output that appears more than once, generates an error message // that describes the problem and which targets generate it. -Err GetDuplicateOutputError(const std::vector<const Target*>& targets, +Err GetDuplicateOutputError(const std::vector<const Target*>& all_targets, const OutputFile& bad_output) { std::vector<const Target*> matches; - for (const Target* target : targets) { + for (const Target* target : all_targets) { for (const auto& output : target->computed_outputs()) { if (output == bad_output) { matches.push_back(target); @@ -197,7 +196,7 @@ const BuildSettings* build_settings, const std::unordered_map<const Settings*, const Toolchain*>& used_toolchains, - const PointerSet<const Target>& all_targets, + const std::vector<const Target*>& all_targets, const Toolchain* default_toolchain, const std::vector<const Target*>& default_toolchain_targets, std::ostream& out, @@ -241,11 +240,9 @@ // skip adding it to the list every time in the loop. used_toolchains[default_toolchain_settings] = default_toolchain; - PointerSet<const Target> target_set; std::vector<const Target*> default_toolchain_targets; default_toolchain_targets.reserve(all_targets.size()); for (const Target* target : all_targets) { - target_set.insert(target); if (target->settings() == default_toolchain_settings) { default_toolchain_targets.push_back(target); // The default toolchain will already have been added to the used @@ -259,7 +256,7 @@ std::stringstream file; std::stringstream depfile; - NinjaBuildWriter gen(build_settings, used_toolchains, target_set, + NinjaBuildWriter gen(build_settings, used_toolchains, all_targets, default_toolchain, default_toolchain_targets, file, depfile); if (!gen.Run(err)) @@ -335,27 +332,7 @@ << "# build.ninja edge is separate to prevent ninja from deleting it\n" << "# (due to depfile usage) if interrupted. gn uses atomic writes to\n" << "# ensure that build.ninja is always valid even if interrupted.\n" - << "build build.ninja.stamp"; - - // Record all generated files as outputs of `gn` as well. - // - // TODO(fxbug.dev/303): We currently rely on the sort below to dedupe - // entries. - std::multimap<OutputFile, const Target*> generated_files = g_scheduler->GetGeneratedFiles(); - VectorSetSorter<OutputFile> generated_file_sorter(generated_files.size()); - for (const auto& entry : generated_files) { - // Only includes those actually in the build. - if (all_targets_.contains(entry.second)) { - generated_file_sorter.Add(entry.first); - } - } - generated_file_sorter.IterateOver( - [this](const OutputFile& output_file) { - out_ << " "; - path_output_.WriteFile(out_, output_file); - }); - - out_ << ": gn\n" + << "build build.ninja.stamp: gn\n" << " generator = 1\n" << " depfile = build.ninja.d\n" << "\n" @@ -382,6 +359,7 @@ const base::FilePath build_path = build_settings_->build_dir().Resolve(build_settings_->root_path()); + EscapeOptions depfile_escape; depfile_escape.mode = ESCAPE_DEPFILE; auto item_callback = [this, &depfile_escape,
diff --git a/src/gn/ninja_build_writer.h b/src/gn/ninja_build_writer.h index dbfe891..c21769a 100644 --- a/src/gn/ninja_build_writer.h +++ b/src/gn/ninja_build_writer.h
@@ -12,7 +12,6 @@ #include <vector> #include "gn/path_output.h" -#include "gn/pointer_set.h" class Builder; class BuildSettings; @@ -33,7 +32,7 @@ NinjaBuildWriter(const BuildSettings* settings, const std::unordered_map<const Settings*, const Toolchain*>& used_toolchains, - const PointerSet<const Target>& all_targets, + const std::vector<const Target*>& all_targets, const Toolchain* default_toolchain, const std::vector<const Target*>& default_toolchain_targets, std::ostream& out, @@ -89,7 +88,7 @@ const BuildSettings* build_settings_; const std::unordered_map<const Settings*, const Toolchain*>& used_toolchains_; - const PointerSet<const Target>& all_targets_; + const std::vector<const Target*>& all_targets_; const Toolchain* default_toolchain_; const std::vector<const Target*>& default_toolchain_targets_;
diff --git a/src/gn/ninja_build_writer_unittest.cc b/src/gn/ninja_build_writer_unittest.cc index 8966f59..8ab5e56 100644 --- a/src/gn/ninja_build_writer_unittest.cc +++ b/src/gn/ninja_build_writer_unittest.cc
@@ -134,14 +134,13 @@ used_toolchains[setup.settings()] = setup.toolchain(); used_toolchains[&other_settings] = &other_toolchain; - std::vector<const Target*> target_list = {&target_foo, &target_bar, &target_baz}; - PointerSet<const Target> target_set(target_list.begin(), target_list.end()); + std::vector<const Target*> targets = {&target_foo, &target_bar, &target_baz}; std::ostringstream ninja_out; std::ostringstream depfile_out; - NinjaBuildWriter writer(setup.build_settings(), used_toolchains, target_set, - setup.toolchain(), target_list, ninja_out, depfile_out); + NinjaBuildWriter writer(setup.build_settings(), used_toolchains, targets, + setup.toolchain(), targets, ninja_out, depfile_out); ASSERT_TRUE(writer.Run(&err)); const char expected_rule_gn[] = "rule gn\n"; @@ -190,65 +189,6 @@ EXPECT_EQ(std::string::npos, out_str.find("pool console")); } -TEST_F(NinjaBuildWriterTest, GeneratedFiles) { - TestWithScope setup; - Err err; - - Target target_foo(setup.settings(), Label(SourceDir("//foo/"), "foo")); - target_foo.set_output_type(Target::GENERATED_FILE); - target_foo.action_values().outputs() = SubstitutionList::MakeForTest( - "//out/Debug/foo.json"); - target_foo.SetToolchain(setup.toolchain()); - ASSERT_TRUE(target_foo.OnResolved(&err)); - - Target target_bar(setup.settings(), Label(SourceDir("//bar/"), "bar")); - target_bar.set_output_type(Target::GENERATED_FILE); - target_bar.action_values().outputs() = SubstitutionList::MakeForTest( - "//out/Debug/bar.txt"); - target_bar.SetToolchain(setup.toolchain()); - ASSERT_TRUE(target_bar.OnResolved(&err)); - - // Make a secondary toolchain that references two pools. - Label other_toolchain_label(SourceDir("//other/"), "toolchain"); - Toolchain other_toolchain(setup.settings(), other_toolchain_label); - TestWithScope::SetupToolchain(&other_toolchain); - - std::unordered_map<const Settings*, const Toolchain*> used_toolchains; - used_toolchains[setup.settings()] = setup.toolchain(); - - std::vector<const Target*> target_list = {&target_foo, &target_bar}; - PointerSet<const Target> target_set(target_list.begin(), target_list.end()); - - std::ostringstream ninja_out; - std::ostringstream depfile_out; - - NinjaBuildWriter writer(setup.build_settings(), used_toolchains, target_set, - setup.toolchain(), target_list, ninja_out, depfile_out); - ASSERT_TRUE(writer.Run(&err)); - - const char expected_rule_gn[] = "rule gn\n"; - const char expected_build_ninja_stamp[] = - "build build.ninja.stamp bar.txt foo.json: gn\n" - " generator = 1\n" - " depfile = build.ninja.d\n"; - const char expected_build_ninja[] = - "build build.ninja: phony build.ninja.stamp\n" - " generator = 1\n"; - const char expected_toolchain[] = "subninja toolchain.ninja\n"; - const char expected_default[] = "default all\n"; - std::string out_str = ninja_out.str(); -#define EXPECT_SNIPPET(expected) \ - EXPECT_NE(std::string::npos, out_str.find(expected)) \ - << "Expected to find: " << expected << "\n" \ - << "Within: " << out_str - EXPECT_SNIPPET(expected_rule_gn); - EXPECT_SNIPPET(expected_build_ninja_stamp); - EXPECT_SNIPPET(expected_build_ninja); - EXPECT_SNIPPET(expected_toolchain); - EXPECT_SNIPPET(expected_default); -#undef EXPECT_SNIPPET -} - TEST_F(NinjaBuildWriterTest, ExtractRegenerationCommands) { TestWithScope setup; Err err; @@ -270,14 +210,13 @@ std::unordered_map<const Settings*, const Toolchain*> used_toolchains; used_toolchains[setup.settings()] = setup.toolchain(); - std::vector<const Target*> target_list = {&target_foo}; - PointerSet<const Target> target_set(target_list.begin(), target_list.end()); + std::vector<const Target*> targets = {&target_foo}; std::stringstream ninja_out; std::ostringstream depfile_out; - NinjaBuildWriter writer(setup.build_settings(), used_toolchains, target_set, - setup.toolchain(), target_list, ninja_out, depfile_out); + NinjaBuildWriter writer(setup.build_settings(), used_toolchains, targets, + setup.toolchain(), targets, ninja_out, depfile_out); ASSERT_TRUE(writer.Run(&err)); const char expected_rule_gn[] = "rule gn\n"; @@ -348,10 +287,11 @@ std::unordered_map<const Settings*, const Toolchain*> used_toolchains; used_toolchains[setup.settings()] = setup.toolchain(); + std::vector<const Target*> targets; std::ostringstream ninja_out; std::ostringstream depfile_out; - NinjaBuildWriter writer(setup.build_settings(), used_toolchains, {}, - setup.toolchain(), {}, ninja_out, depfile_out); + NinjaBuildWriter writer(setup.build_settings(), used_toolchains, targets, + setup.toolchain(), targets, ninja_out, depfile_out); ASSERT_TRUE(writer.Run(&err)); EXPECT_EQ(depfile_out.str(), @@ -380,13 +320,11 @@ std::unordered_map<const Settings*, const Toolchain*> used_toolchains; used_toolchains[setup.settings()] = setup.toolchain(); - - std::vector<const Target*> target_list = {&target_foo, &target_bar}; - PointerSet<const Target> target_set(target_list.begin(), target_list.end()); + std::vector<const Target*> targets = {&target_foo, &target_bar}; std::ostringstream ninja_out; std::ostringstream depfile_out; - NinjaBuildWriter writer(setup.build_settings(), used_toolchains, target_set, - setup.toolchain(), target_list, ninja_out, depfile_out); + NinjaBuildWriter writer(setup.build_settings(), used_toolchains, targets, + setup.toolchain(), targets, ninja_out, depfile_out); ASSERT_FALSE(writer.Run(&err)); const char expected_help_test[] =
diff --git a/src/gn/scheduler.cc b/src/gn/scheduler.cc index 35ae097..6525f33 100644 --- a/src/gn/scheduler.cc +++ b/src/gn/scheduler.cc
@@ -111,21 +111,16 @@ return false; } -void Scheduler::AddGeneratedFile(const Target* target, const OutputFile& file) { +void Scheduler::AddGeneratedFile(const SourceFile& entry) { std::lock_guard<std::mutex> lock(lock_); - generated_files_.insert(std::make_pair(file, target)); + generated_files_.insert(std::make_pair(entry, true)); } -bool Scheduler::IsFileGeneratedByTarget(const OutputFile& file) const { +bool Scheduler::IsFileGeneratedByTarget(const SourceFile& file) const { std::lock_guard<std::mutex> lock(lock_); return generated_files_.find(file) != generated_files_.end(); } -std::multimap<OutputFile, const Target*> Scheduler::GetGeneratedFiles() const { - std::lock_guard<std::mutex> lock(lock_); - return generated_files_; -} - std::multimap<SourceFile, const Target*> Scheduler::GetUnknownGeneratedInputs() const { std::lock_guard<std::mutex> lock(lock_);
diff --git a/src/gn/scheduler.h b/src/gn/scheduler.h index 52e0b47..cf29fe8 100644 --- a/src/gn/scheduler.h +++ b/src/gn/scheduler.h
@@ -70,17 +70,8 @@ bool IsFileGeneratedByWriteRuntimeDeps(const OutputFile& file) const; // Tracks generated_file calls. - void AddGeneratedFile(const Target* target, const OutputFile& file); - bool IsFileGeneratedByTarget(const OutputFile& file) const; - - // Returns the collection of generated files. The associated target is - // tracked as well in order to determine whether the generated file actually - // was created (i.e., whether the target was included in the build). This - // target pointer must only be dereferenced after the graph is complete. - // - // TODO(crbug.com/gn/303): This should be a map but currently GN allows - // multiple overlapping generated files. - std::multimap<OutputFile, const Target*> GetGeneratedFiles() const; + void AddGeneratedFile(const SourceFile& entry); + bool IsFileGeneratedByTarget(const SourceFile& file) const; // Unknown generated inputs are files that a target declares as an input // in the output directory, but which aren't generated by any dependency. @@ -154,7 +145,7 @@ std::vector<SourceFile> written_files_; std::vector<const Target*> write_runtime_deps_targets_; std::multimap<SourceFile, const Target*> unknown_generated_inputs_; - std::multimap<OutputFile, const Target*> generated_files_; + std::map<SourceFile, bool> generated_files_; Scheduler(const Scheduler&) = delete; Scheduler& operator=(const Scheduler&) = delete;
diff --git a/src/gn/target.cc b/src/gn/target.cc index ea6277a..7b545b5 100644 --- a/src/gn/target.cc +++ b/src/gn/target.cc
@@ -526,7 +526,8 @@ if (output_type_ == GENERATED_FILE) { DCHECK(!computed_outputs_.empty()); - g_scheduler->AddGeneratedFile(this, computed_outputs_[0]); + g_scheduler->AddGeneratedFile( + computed_outputs_[0].AsSourceFile(settings()->build_settings())); } return true; @@ -1220,7 +1221,7 @@ // Allow dependency to be through data_deps for files generated by gn. check_data_deps = g_scheduler->IsFileGeneratedByWriteRuntimeDeps(out_file) || - g_scheduler->IsFileGeneratedByTarget(out_file); + g_scheduler->IsFileGeneratedByTarget(source); // Check object files (much slower and very rare) only if the "normal" // output check failed. consider_object_files = !check_data_deps;