Add gn check --check-generated to check dependencies of generated files
A fair amount of source in Chromium involves generated files now,
through mojo or through the jumbo build system. The current gn check
will not, and can not, check includes in those files since they
can't exist until a build has run.
This adds a gn check flag: --check-generated, which allows gn check
to also look through generated files. For obvious reasons it can only
be used once those generated files have been created.
Bug: gn:57
Change-Id: I3f65b8ca4131931af8906d614334ebce9b006f5d
Reviewed-on: https://gn-review.googlesource.com/c/gn/+/4181
Reviewed-by: Brett Wilson <brettw@chromium.org>
Commit-Queue: Brett Wilson <brettw@chromium.org>
diff --git a/tools/gn/command_check.cc b/tools/gn/command_check.cc
index 9d9dcbd..93099f7 100644
--- a/tools/gn/command_check.cc
+++ b/tools/gn/command_check.cc
@@ -215,9 +215,10 @@
const base::CommandLine* cmdline = base::CommandLine::ForCurrentProcess();
bool force = cmdline->HasSwitch("force");
+ bool check_generated = cmdline->HasSwitch("check-generated");
if (!CheckPublicHeaders(&setup->build_settings(), all_targets,
- targets_to_check, force))
+ targets_to_check, force, check_generated))
return 1;
if (!base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kQuiet)) {
@@ -237,11 +238,11 @@
bool CheckPublicHeaders(const BuildSettings* build_settings,
const std::vector<const Target*>& all_targets,
const std::vector<const Target*>& to_check,
- bool force_check) {
+ bool force_check, bool check_generated) {
ScopedTrace trace(TraceItem::TRACE_CHECK_HEADERS, "Check headers");
scoped_refptr<HeaderChecker> header_checker(
- new HeaderChecker(build_settings, all_targets));
+ new HeaderChecker(build_settings, all_targets, check_generated));
std::vector<Err> header_errors;
header_checker->Run(to_check, force_check, &header_errors);
diff --git a/tools/gn/commands.h b/tools/gn/commands.h
index 71950e6..d6b6728 100644
--- a/tools/gn/commands.h
+++ b/tools/gn/commands.h
@@ -136,12 +136,16 @@
// force_check, if true, will override targets opting out of header checking
// with "check_includes = false" and will check them anyway.
//
+// Generated files are normally not checked since they do not exist
+// unless a build has been run, but passing true for |check_generated|
+// will attempt to check them anyway, assuming they exist.
+//
// On success, returns true. If the check fails, the error(s) will be printed
// to stdout and false will be returned.
bool CheckPublicHeaders(const BuildSettings* build_settings,
const std::vector<const Target*>& all_targets,
const std::vector<const Target*>& to_check,
- bool force_check);
+ bool force_check, bool check_generated);
// Filters the given list of targets by the given pattern list.
void FilterTargetsByPatterns(const std::vector<const Target*>& input,
diff --git a/tools/gn/header_checker.cc b/tools/gn/header_checker.cc
index 03afc46..6f0b9d0 100644
--- a/tools/gn/header_checker.cc
+++ b/tools/gn/header_checker.cc
@@ -119,8 +119,10 @@
} // namespace
HeaderChecker::HeaderChecker(const BuildSettings* build_settings,
- const std::vector<const Target*>& targets)
- : build_settings_(build_settings), lock_(), task_count_cv_() {
+ const std::vector<const Target*>& targets,
+ bool check_generated)
+ : build_settings_(build_settings), check_generated_(check_generated),
+ lock_(), task_count_cv_() {
for (auto* target : targets)
AddTargetToFileMap(target, &file_map_);
}
@@ -155,14 +157,16 @@
type != SOURCE_M && type != SOURCE_MM && type != SOURCE_RC)
continue;
- // If any target marks it as generated, don't check it. We have to check
- // file_map_, which includes all known files; files only includes those
- // being checked.
- bool is_generated = false;
- for (const auto& vect_i : file_map_[file.first])
- is_generated |= vect_i.is_generated;
- if (is_generated)
- continue;
+ if (!check_generated_) {
+ // If any target marks it as generated, don't check it. We have to check
+ // file_map_, which includes all known files; files only includes those
+ // being checked.
+ bool is_generated = false;
+ for (const auto& vect_i : file_map_[file.first])
+ is_generated |= vect_i.is_generated;
+ if (is_generated)
+ continue;
+ }
for (const auto& vect_i : file.second) {
if (vect_i.target->check_includes()) {
@@ -269,12 +273,17 @@
// target. These won't exist at checking time. Since we require all generated
// files to be somewhere in the output tree, we can just check the name to
// see if they should be skipped.
- if (IsFileInOuputDir(file))
+ if (!check_generated_ && IsFileInOuputDir(file))
return true;
base::FilePath path = build_settings_->GetFullPath(file);
std::string contents;
if (!base::ReadFileToString(path, &contents)) {
+ // A missing (not yet) generated file is an acceptable problem
+ // considering this code does not understand conditional includes.
+ if (IsFileInOuputDir(file))
+ return true;
+
errors->emplace_back(from_target->defined_from(), "Source file not found.",
"The target:\n " +
from_target->label().GetUserVisibleName(false) +
@@ -295,7 +304,7 @@
target_include_dirs.end());
}
- bool has_errors = false;
+ size_t error_count_before = errors->size();
CIncludeIterator iter(&input_file);
base::StringPiece current_include;
LocationRange range;
@@ -303,15 +312,11 @@
Err err;
SourceFile include = SourceFileForInclude(current_include, include_dirs,
input_file, range, &err);
- if (!include.is_null()) {
- if (!CheckInclude(from_target, input_file, include, range, &err)) {
- errors->emplace_back(std::move(err));
- has_errors = true;
- }
- }
+ if (!include.is_null())
+ CheckInclude(from_target, input_file, include, range, errors);
}
- return !has_errors;
+ return errors->size() == error_count_before;
}
// If the file exists:
@@ -320,11 +325,11 @@
// - The dependency path to the included target must follow only public_deps.
// - If there are multiple targets with the header in it, only one need be
// valid for the check to pass.
-bool HeaderChecker::CheckInclude(const Target* from_target,
+void HeaderChecker::CheckInclude(const Target* from_target,
const InputFile& source_file,
const SourceFile& include_file,
const LocationRange& range,
- Err* err) const {
+ std::vector<Err>* errors) const {
// Assume if the file isn't declared in our sources that we don't need to
// check it. It would be nice if we could give an error if this happens, but
// our include finder is too primitive and returns all includes, even if
@@ -332,7 +337,7 @@
// not unusual for the buildfiles to not specify that header at all.
FileMap::const_iterator found = file_map_.find(include_file);
if (found == file_map_.end())
- return true;
+ return;
const TargetVector& targets = found->second;
Chain chain; // Prevent reallocating in the loop.
@@ -362,7 +367,7 @@
}
}
if (!present_in_current_toolchain)
- return true;
+ return;
// For all targets containing this file, we require that at least one be
// a direct or public dependency of the current target, and either (1) the
@@ -380,7 +385,7 @@
// target.
const Target* to_target = target.target;
if (to_target == from_target)
- return true;
+ return;
bool is_permitted_chain = false;
if (IsDependencyOf(to_target, from_target, &chain, &is_permitted_chain)) {
@@ -423,15 +428,16 @@
}
}
- if (!found_dependency) {
- DCHECK(!last_error.has_error());
- *err = MakeUnreachableError(source_file, range, from_target, targets);
- return false;
- }
- if (last_error.has_error()) {
- // Found at least one dependency chain above, but it had an error.
- *err = last_error;
- return false;
+ if (!found_dependency || last_error.has_error()) {
+ if (!found_dependency) {
+ DCHECK(!last_error.has_error());
+ Err err = MakeUnreachableError(source_file, range, from_target, targets);
+ errors->push_back(std::move(err));
+ } else {
+ // Found at least one dependency chain above, but it had an error.
+ errors->push_back(std::move(last_error));
+ }
+ return;
}
// One thing we didn't check for is targets that expose their dependents
@@ -451,8 +457,6 @@
// - Save the includes found in each file and actually compute the graph of
// includes to detect when A implicitly includes C's header. This will not
// have the annoying false positive problem, but is complex to write.
-
- return true;
}
bool HeaderChecker::IsDependencyOf(const Target* search_for,
diff --git a/tools/gn/header_checker.h b/tools/gn/header_checker.h
index ef14729..46a47b9 100644
--- a/tools/gn/header_checker.h
+++ b/tools/gn/header_checker.h
@@ -47,8 +47,12 @@
};
typedef std::vector<ChainLink> Chain;
+ // check_generated, if true, will also check generated
+ // files. Something that can only be done after running a build that
+ // has generated them.
HeaderChecker(const BuildSettings* build_settings,
- const std::vector<const Target*>& targets);
+ const std::vector<const Target*>& targets,
+ bool check_generated = false);
// Runs the check. The targets in to_check will be checked.
//
@@ -119,15 +123,15 @@
const SourceFile& file,
std::vector<Err>* err) const;
- // Checks that the given file in the given target can include the given
- // include file. If disallowed, returns false and sets the error. The
- // range indicates the location of the include in the file for error
- // reporting.
- bool CheckInclude(const Target* from_target,
+ // Checks that the given file in the given target can include the
+ // given include file. If disallowed, adds the error or errors to
+ // the errors array. The range indicates the location of the
+ // include in the file for error reporting.
+ void CheckInclude(const Target* from_target,
const InputFile& source_file,
const SourceFile& include_file,
const LocationRange& range,
- Err* err) const;
+ std::vector<Err>* errors) const;
// Returns true if the given search_for target is a dependency of
// search_from.
@@ -171,6 +175,8 @@
const BuildSettings* build_settings_;
+ bool check_generated_;
+
// Maps source files to targets it appears in (usually just one target).
FileMap file_map_;
diff --git a/tools/gn/header_checker_unittest.cc b/tools/gn/header_checker_unittest.cc
index 6519871..ab0a6c8 100644
--- a/tools/gn/header_checker_unittest.cc
+++ b/tools/gn/header_checker_unittest.cc
@@ -195,32 +195,34 @@
// A file in target A can't include a header from D because A has no
// dependency on D.
- EXPECT_FALSE(checker->CheckInclude(&a_, input_file, d_header, range, &err));
- EXPECT_TRUE(err.has_error());
+ std::vector<Err> errors;
+ checker->CheckInclude(&a_, input_file, d_header, range, &errors);
+ EXPECT_GT(errors.size(), 0);
// A can include the public header in B.
- err = Err();
- EXPECT_TRUE(checker->CheckInclude(&a_, input_file, b_public, range, &err));
- EXPECT_FALSE(err.has_error());
+ errors.clear();
+ checker->CheckInclude(&a_, input_file, b_public, range, &errors);
+ EXPECT_EQ(errors.size(), 0);
// Check A depending on the public and private headers in C.
- err = Err();
- EXPECT_TRUE(checker->CheckInclude(&a_, input_file, c_public, range, &err));
- EXPECT_FALSE(err.has_error());
- EXPECT_FALSE(checker->CheckInclude(&a_, input_file, c_private, range, &err));
- EXPECT_TRUE(err.has_error());
+ errors.clear();
+ checker->CheckInclude(&a_, input_file, c_public, range, &errors);
+ EXPECT_EQ(errors.size(), 0);
+ errors.clear();
+ checker->CheckInclude(&a_, input_file, c_private, range, &errors);
+ EXPECT_GT(errors.size(), 0);
// A can depend on a random file unknown to the build.
- err = Err();
- EXPECT_TRUE(checker->CheckInclude(&a_, input_file, SourceFile("//random.h"),
- range, &err));
- EXPECT_FALSE(err.has_error());
+ errors.clear();
+ checker->CheckInclude(&a_, input_file, SourceFile("//random.h"),
+ range, &errors);
+ EXPECT_EQ(errors.size(), 0);
// A can depend on a file present only in another toolchain even with no
// dependency path.
- err = Err();
- EXPECT_TRUE(checker->CheckInclude(&a_, input_file, otc_header, range, &err));
- EXPECT_FALSE(err.has_error());
+ errors.clear();
+ checker->CheckInclude(&a_, input_file, otc_header, range, &errors);
+ EXPECT_EQ(errors.size(), 0);
}
// A public chain of dependencies should always be identified first, even if
@@ -282,17 +284,17 @@
new HeaderChecker(setup_.build_settings(), targets_));
// A depends on B. So B normally can't include headers from A.
- Err err;
- EXPECT_FALSE(checker->CheckInclude(&b_, input_file, a_public, range, &err));
- EXPECT_TRUE(err.has_error());
+ std::vector<Err> errors;
+ checker->CheckInclude(&b_, input_file, a_public, range, &errors);
+ EXPECT_GT(errors.size(), 0);
// Add an allow_circular_includes_from on A that lists B.
a_.allow_circular_includes_from().insert(b_.label());
// Now the include from B to A should be allowed.
- err = Err();
- EXPECT_TRUE(checker->CheckInclude(&b_, input_file, a_public, range, &err));
- EXPECT_FALSE(err.has_error());
+ errors.clear();
+ checker->CheckInclude(&b_, input_file, a_public, range, &errors);
+ EXPECT_EQ(errors.size(), 0);
}
TEST_F(HeaderCheckerTest, SourceFileForInclude) {
@@ -371,12 +373,12 @@
new HeaderChecker(setup_.build_settings(), targets_));
// B should not be allowed to include C's private header.
- err = Err();
- EXPECT_FALSE(checker->CheckInclude(&b_, input_file, c_private, range, &err));
- EXPECT_TRUE(err.has_error());
+ std::vector<Err> errors;
+ checker->CheckInclude(&b_, input_file, c_private, range, &errors);
+ EXPECT_GT(errors.size(), 0);
// A should be able to because of the friend declaration.
- err = Err();
- EXPECT_TRUE(checker->CheckInclude(&a_, input_file, c_private, range, &err));
- EXPECT_FALSE(err.has_error()) << err.message();
+ errors.clear();
+ checker->CheckInclude(&a_, input_file, c_private, range, &errors);
+ EXPECT_EQ(errors.size(), 0);
}
diff --git a/tools/gn/setup.cc b/tools/gn/setup.cc
index b9debab..7b336a6 100644
--- a/tools/gn/setup.cc
+++ b/tools/gn/setup.cc
@@ -418,7 +418,7 @@
}
if (!commands::CheckPublicHeaders(&build_settings_, all_targets, to_check,
- false)) {
+ false, false)) {
return false;
}
}
