GN: Always check for bad items.
Previous checking for most types of errors in the GN introspection commands was
disabled. The reason was that it can be convenient to ask for certain types
of information which GN can provide even if there are errors elsewhere in the
build.
Unfortunately, not checking for bad items can leave the build graph in an
inconsistent state. In the case of missing dependencies, a target will never
be resolved which means most of the information the tools will return about it
will be missing or incorrect which is very surprising and confusing.
As a result, it seems best to always ensure things are in a consistent state
before running any introspection calculations.
Review-Url: https://codereview.chromium.org/2492273002
Cr-Original-Commit-Position: refs/heads/master@{#431667}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: f77e13a490ea98bed6f136661fa778ba3972b8e1
diff --git a/tools/gn/build_settings.cc b/tools/gn/build_settings.cc
index 14f1aa8..3fac976 100644
--- a/tools/gn/build_settings.cc
+++ b/tools/gn/build_settings.cc
@@ -9,8 +9,7 @@
#include "base/files/file_util.h"
#include "tools/gn/filesystem_utils.h"
-BuildSettings::BuildSettings()
- : check_for_bad_items_(true) {
+BuildSettings::BuildSettings() {
}
BuildSettings::BuildSettings(const BuildSettings& other)
@@ -20,8 +19,7 @@
python_path_(other.python_path_),
build_config_file_(other.build_config_file_),
build_dir_(other.build_dir_),
- build_args_(other.build_args_),
- check_for_bad_items_(true) {
+ build_args_(other.build_args_) {
}
BuildSettings::~BuildSettings() {
diff --git a/tools/gn/build_settings.h b/tools/gn/build_settings.h
index 5424cf9..0b986cd 100644
--- a/tools/gn/build_settings.h
+++ b/tools/gn/build_settings.h
@@ -95,18 +95,6 @@
exec_script_whitelist_ = std::move(list);
}
- // When set (the default), code should perform normal validation of inputs
- // and structures, like undefined or possibly incorrectly used things. For
- // some interrogation commands, we don't care about this and actually want
- // to allow the user to check the structure of the build to solve their
- // problem, and these checks are undesirable.
- bool check_for_bad_items() const {
- return check_for_bad_items_;
- }
- void set_check_for_bad_items(bool c) {
- check_for_bad_items_ = c;
- }
-
private:
base::FilePath root_path_;
std::string root_path_utf8_;
@@ -122,8 +110,6 @@
std::unique_ptr<std::set<SourceFile>> exec_script_whitelist_;
- bool check_for_bad_items_;
-
BuildSettings& operator=(const BuildSettings& other); // Disallow.
};
diff --git a/tools/gn/command_analyze.cc b/tools/gn/command_analyze.cc
index ee3774c..6fd5823 100644
--- a/tools/gn/command_analyze.cc
+++ b/tools/gn/command_analyze.cc
@@ -107,7 +107,6 @@
}
Setup* setup = new Setup;
- setup->build_settings().set_check_for_bad_items(false);
if (!setup->DoSetup(args[0], false) || !setup->Run())
return 1;
diff --git a/tools/gn/command_args.cc b/tools/gn/command_args.cc
index 374a274..1b81135 100644
--- a/tools/gn/command_args.cc
+++ b/tools/gn/command_args.cc
@@ -118,7 +118,6 @@
int ListArgs(const std::string& build_dir) {
Setup* setup = new Setup;
- setup->build_settings().set_check_for_bad_items(false);
if (!setup->DoSetup(build_dir, false) || !setup->Run())
return 1;
@@ -231,7 +230,6 @@
// Scope the setup. We only use it for some basic state. We'll do the
// "real" build below in the gen command.
Setup setup;
- setup.build_settings().set_check_for_bad_items(false);
// Don't fill build arguments. We're about to edit the file which supplies
// these in the first place.
setup.set_fill_arguments(false);
diff --git a/tools/gn/command_desc.cc b/tools/gn/command_desc.cc
index 2257221..9f7f903 100644
--- a/tools/gn/command_desc.cc
+++ b/tools/gn/command_desc.cc
@@ -430,7 +430,6 @@
// Deliberately leaked to avoid expensive process teardown.
Setup* setup = new Setup;
- setup->build_settings().set_check_for_bad_items(false);
if (!setup->DoSetup(args[0], false))
return 1;
if (!setup->Run())
diff --git a/tools/gn/command_ls.cc b/tools/gn/command_ls.cc
index 676ff57..58b9c87 100644
--- a/tools/gn/command_ls.cc
+++ b/tools/gn/command_ls.cc
@@ -74,7 +74,6 @@
}
Setup* setup = new Setup;
- setup->build_settings().set_check_for_bad_items(false);
if (!setup->DoSetup(args[0], false) || !setup->Run())
return 1;
diff --git a/tools/gn/command_refs.cc b/tools/gn/command_refs.cc
index 6fa0146..4d7ad7b 100644
--- a/tools/gn/command_refs.cc
+++ b/tools/gn/command_refs.cc
@@ -403,7 +403,6 @@
bool all_toolchains = cmdline->HasSwitch(switches::kAllToolchains);
Setup* setup = new Setup;
- setup->build_settings().set_check_for_bad_items(false);
if (!setup->DoSetup(args[0], false) || !setup->Run())
return 1;
diff --git a/tools/gn/setup.cc b/tools/gn/setup.cc
index c5eace2..8a433bc 100644
--- a/tools/gn/setup.cc
+++ b/tools/gn/setup.cc
@@ -330,22 +330,20 @@
bool Setup::RunPostMessageLoop() {
Err err;
- if (build_settings_.check_for_bad_items()) {
- if (!builder_.CheckForBadItems(&err)) {
- err.PrintToStdout();
- return false;
- }
+ if (!builder_.CheckForBadItems(&err)) {
+ err.PrintToStdout();
+ return false;
+ }
- if (!build_settings_.build_args().VerifyAllOverridesUsed(&err)) {
- // TODO(brettw) implement a system to have a different marker for
- // warnings. Until we have a better system, print the error but don't
- // return failure unless requested on the command line.
- err.PrintToStdout();
- if (base::CommandLine::ForCurrentProcess()->HasSwitch(
- switches::kFailOnUnusedArgs))
- return false;
- return true;
- }
+ if (!build_settings_.build_args().VerifyAllOverridesUsed(&err)) {
+ // TODO(brettw) implement a system to have a different marker for
+ // warnings. Until we have a better system, print the error but don't
+ // return failure unless requested on the command line.
+ err.PrintToStdout();
+ if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+ switches::kFailOnUnusedArgs))
+ return false;
+ return true;
}
if (check_public_headers_) {
diff --git a/tools/gn/target.cc b/tools/gn/target.cc
index aafdc4f..49de35d 100644
--- a/tools/gn/target.cc
+++ b/tools/gn/target.cc
@@ -300,15 +300,13 @@
FillOutputFiles();
- if (settings()->build_settings()->check_for_bad_items()) {
- if (!CheckVisibility(err))
- return false;
- if (!CheckTestonly(err))
- return false;
- if (!CheckAssertNoDeps(err))
- return false;
- CheckSourcesGenerated();
- }
+ if (!CheckVisibility(err))
+ return false;
+ if (!CheckTestonly(err))
+ return false;
+ if (!CheckAssertNoDeps(err))
+ return false;
+ CheckSourcesGenerated();
if (!write_runtime_deps_output_.value().empty())
g_scheduler->AddWriteRuntimeDepsTarget(this);
