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;
