Add support for include_dirs to gn check.
With this change "gn check" no longer assumes all #include paths are
relative to the source root. Instead it searches for the includes by
checking each of the directories listed in include_dirs for the target.
A number of new issues have been discovered. The trivial ones have been
fixed, the larger and more complex ones have been reported and the related
parts of the project have been excluded from the default gn check run.
BUG=794926
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: I39c17a7ac47dc024dd374c8891ce4911809195cb
Reviewed-on: https://chromium-review.googlesource.com/827014
Commit-Queue: Brett Wilson <brettw@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: David Reveman <reveman@chromium.org>
Reviewed-by: Yuzhu Shen <yzshen@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#531334}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 2f14ea14de1ea4ffe82aaa9c01425e41f3625797diff --git a/tools/gn/command_check.cc b/tools/gn/command_check.cc
index 86caf85..9581c4c 100644
--- a/tools/gn/command_check.cc
+++ b/tools/gn/command_check.cc
@@ -95,10 +95,8 @@
- Only includes using "quotes" are checked. <brackets> are assumed to be
system includes.
- - Include paths are assumed to be relative to either the source root or the
- "root_gen_dir" and must include all the path components. (It might be
- nice in the future to incorporate GN's knowledge of the include path to
- handle other include styles.)
+ - Include paths are assumed to be relative to any of the "include_dirs" for
+ the target (including the implicit current dir).
- GN does not run the preprocessor so will not understand conditional
includes.
@@ -130,9 +128,9 @@
Advice on fixing problems
- If you have a third party project that uses relative includes, it's generally
- best to exclude that target from checking altogether via
- "check_includes = false".
+ If you have a third party project that is difficult to fix or doesn't care
+ about include checks it's generally best to exclude that target from checking
+ altogether via "check_includes = false".
If you have conditional includes, make sure the build conditions and the
preprocessor conditions match, and annotate the line with "nogncheck" (see
diff --git a/tools/gn/header_checker.cc b/tools/gn/header_checker.cc
index 39d53ee..d4fb6f7 100644
--- a/tools/gn/header_checker.cc
+++ b/tools/gn/header_checker.cc
@@ -15,6 +15,7 @@
#include "tools/gn/builder.h"
#include "tools/gn/c_include_iterator.h"
#include "tools/gn/config.h"
+#include "tools/gn/config_values_extractors.h"
#include "tools/gn/err.h"
#include "tools/gn/filesystem_utils.h"
#include "tools/gn/scheduler.h"
@@ -30,19 +31,6 @@
bool is_generated;
};
-// If the given file is in the "gen" folder, trims this so it treats the gen
-// directory as the source root:
-// //out/Debug/gen/foo/bar.h -> //foo/bar.h
-// If the file isn't in the generated root, returns the input unchanged.
-SourceFile RemoveRootGenDirFromFile(const Target* target,
- const SourceFile& file) {
- const SourceDir& gen = target->settings()->toolchain_gen_dir();
- if (!gen.is_null() && base::StartsWith(file.value(), gen.value(),
- base::CompareCase::SENSITIVE))
- return SourceFile("//" + file.value().substr(gen.value().size()));
- return file;
-}
-
// This class makes InputFiles on the stack as it reads files to check. When
// we throw an error, the Err indicates a locatin which has a pointer to
// an InputFile that must persist as long as the Err does.
@@ -201,14 +189,11 @@
std::map<SourceFile, PublicGeneratedPair> files_to_public;
- // First collect the normal files, they get the default visibility. Always
- // trim the root gen dir if it exists. This will only exist on outputs of an
- // action, but those are often then wired into the sources of a compiled
- // target to actually compile generated code. If you depend on the compiled
- // target, it should be enough to be able to include the header.
+ // First collect the normal files, they get the default visibility. If you
+ // depend on the compiled target, it should be enough to be able to include
+ // the header.
for (const auto& source : target->sources()) {
- SourceFile file = RemoveRootGenDirFromFile(target, source);
- files_to_public[file].is_public = default_public;
+ files_to_public[source].is_public = default_public;
}
// Add in the public files, forcing them to public. This may overwrite some
@@ -216,8 +201,7 @@
if (default_public) // List only used when default is not public.
DCHECK(target->public_headers().empty());
for (const auto& source : target->public_headers()) {
- SourceFile file = RemoveRootGenDirFromFile(target, source);
- files_to_public[file].is_public = true;
+ files_to_public[source].is_public = true;
}
// Add in outputs from actions. These are treated as public (since if other
@@ -225,12 +209,7 @@
std::vector<SourceFile> outputs;
target->action_values().GetOutputsAsSourceFiles(target, &outputs);
for (const auto& output : outputs) {
- // For generated files in the "gen" directory, add the filename to the
- // map assuming "gen" is the source root. This means that when files include
- // the generated header relative to there (the recommended practice), we'll
- // find the file.
- SourceFile output_file = RemoveRootGenDirFromFile(target, output);
- PublicGeneratedPair* pair = &files_to_public[output_file];
+ PublicGeneratedPair* pair = &files_to_public[output];
pair->is_public = true;
pair->is_generated = true;
}
@@ -247,17 +226,27 @@
return file.value().compare(0, build_dir.size(), build_dir) == 0;
}
-// This current assumes all include paths are relative to the source root
-// which is generally the case for Chromium.
-//
-// A future enhancement would be to search the include path for the target
-// containing the source file containing this include and find the file to
-// handle the cases where people do weird things with the paths.
SourceFile HeaderChecker::SourceFileForInclude(
- const base::StringPiece& input) const {
- std::string str("//");
- input.AppendToString(&str);
- return SourceFile(str);
+ const base::StringPiece& relative_file_path,
+ const std::vector<SourceDir>& include_dirs,
+ const InputFile& source_file,
+ const LocationRange& range,
+ Err* err) const {
+ using base::FilePath;
+
+ Value relative_file_value(nullptr, relative_file_path.as_string());
+ auto it = std::find_if(
+ include_dirs.begin(), include_dirs.end(),
+ [relative_file_value, err, this](const SourceDir& dir) -> bool {
+ SourceFile include_file =
+ dir.ResolveRelativeFile(relative_file_value, err);
+ return file_map_.find(include_file) != file_map_.end();
+ });
+
+ if (it != include_dirs.end())
+ return it->ResolveRelativeFile(relative_file_value, err);
+
+ return SourceFile();
}
bool HeaderChecker::CheckFile(const Target* from_target,
@@ -285,13 +274,25 @@
InputFile input_file(file);
input_file.SetContents(contents);
+ std::vector<SourceDir> include_dirs;
+ include_dirs.push_back(file.GetDir());
+ for (ConfigValuesIterator iter(from_target); !iter.done(); iter.Next()) {
+ const std::vector<SourceDir>& target_include_dirs =
+ iter.cur().include_dirs();
+ include_dirs.insert(include_dirs.end(), target_include_dirs.begin(),
+ target_include_dirs.end());
+ }
+
CIncludeIterator iter(&input_file);
base::StringPiece current_include;
LocationRange range;
while (iter.GetNextIncludeString(¤t_include, &range)) {
- SourceFile include = SourceFileForInclude(current_include);
- if (!CheckInclude(from_target, input_file, include, range, err))
- return false;
+ SourceFile include = SourceFileForInclude(current_include, include_dirs,
+ input_file, range, err);
+ if (!include.is_null()) {
+ if (!CheckInclude(from_target, input_file, include, range, err))
+ return false;
+ }
}
return true;
diff --git a/tools/gn/header_checker.h b/tools/gn/header_checker.h
index c6411eb..3645d7a 100644
--- a/tools/gn/header_checker.h
+++ b/tools/gn/header_checker.h
@@ -16,6 +16,7 @@
#include "base/synchronization/condition_variable.h"
#include "base/synchronization/lock.h"
#include "tools/gn/err.h"
+#include "tools/gn/source_dir.h"
class BuildSettings;
class InputFile;
@@ -23,6 +24,10 @@
class SourceFile;
class Target;
+namespace base {
+class FilePath;
+}
+
class HeaderChecker : public base::RefCountedThreadSafe<HeaderChecker> {
public:
// Represents a dependency chain.
@@ -63,6 +68,9 @@
FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, CheckInclude);
FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, PublicFirst);
FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, CheckIncludeAllowCircular);
+ FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, SourceFileForInclude);
+ FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest,
+ SourceFileForInclude_FileNotFound);
~HeaderChecker();
struct TargetInfo {
@@ -84,6 +92,8 @@
typedef std::vector<TargetInfo> TargetVector;
typedef std::map<SourceFile, TargetVector> FileMap;
+ typedef base::RepeatingCallback<bool(const base::FilePath& path)>
+ PathExistsCallback;
// Backend for Run() that takes the list of files to check. The errors_ list
// will be populate on failure.
@@ -98,7 +108,11 @@
bool IsFileInOuputDir(const SourceFile& file) const;
// Resolves the contents of an include to a SourceFile.
- SourceFile SourceFileForInclude(const base::StringPiece& input) const;
+ SourceFile SourceFileForInclude(const base::StringPiece& relative_file_path,
+ const std::vector<SourceDir>& include_dirs,
+ const InputFile& source_file,
+ const LocationRange& range,
+ Err* err) const;
// from_target is the target the file was defined from. It will be used in
// error messages.
diff --git a/tools/gn/header_checker_unittest.cc b/tools/gn/header_checker_unittest.cc
index 2aa0860..5bb5c70 100644
--- a/tools/gn/header_checker_unittest.cc
+++ b/tools/gn/header_checker_unittest.cc
@@ -2,8 +2,10 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
+#include <ostream>
#include <vector>
+#include "base/bind.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "tools/gn/config.h"
#include "tools/gn/header_checker.h"
@@ -67,6 +69,10 @@
} // namespace
+void PrintTo(const SourceFile& source_file, ::std::ostream* os) {
+ *os << source_file.value();
+}
+
TEST_F(HeaderCheckerTest, IsDependencyOf) {
scoped_refptr<HeaderChecker> checker(
new HeaderChecker(setup_.build_settings(), targets_));
@@ -288,3 +294,57 @@
EXPECT_TRUE(checker->CheckInclude(&b_, input_file, a_public, range, &err));
EXPECT_FALSE(err.has_error());
}
+
+TEST_F(HeaderCheckerTest, SourceFileForInclude) {
+ using base::FilePath;
+ const std::vector<SourceDir> kIncludeDirs = {
+ SourceDir("/c/custom_include/"), SourceDir("//"), SourceDir("//subdir")};
+ a_.sources().push_back(SourceFile("//lib/header1.h"));
+ b_.sources().push_back(SourceFile("/c/custom_include/header2.h"));
+
+ InputFile dummy_input_file(SourceFile("//some_file.cc"));
+ dummy_input_file.SetContents(std::string());
+ LocationRange dummy_range;
+
+ scoped_refptr<HeaderChecker> checker(
+ new HeaderChecker(setup_.build_settings(), targets_));
+ {
+ Err err;
+ SourceFile source_file = checker->SourceFileForInclude(
+ "lib/header1.h", kIncludeDirs, dummy_input_file, dummy_range, &err);
+ EXPECT_FALSE(err.has_error());
+ EXPECT_EQ(SourceFile("//lib/header1.h"), source_file);
+ }
+
+ {
+ Err err;
+ SourceFile source_file = checker->SourceFileForInclude(
+ "header2.h", kIncludeDirs, dummy_input_file, dummy_range, &err);
+ EXPECT_FALSE(err.has_error());
+ EXPECT_EQ(SourceFile("/c/custom_include/header2.h"), source_file);
+ }
+}
+
+TEST_F(HeaderCheckerTest, SourceFileForInclude_FileNotFound) {
+ using base::FilePath;
+ const char kFileContents[] = "Some dummy contents";
+ const std::vector<SourceDir> kIncludeDirs = {SourceDir("//")};
+ scoped_refptr<HeaderChecker> checker(
+ new HeaderChecker(setup_.build_settings(), targets_));
+
+ Err err;
+ InputFile input_file(SourceFile("//input.cc"));
+ input_file.SetContents(std::string(kFileContents));
+ const int kLineNumber = 10;
+ const int kColumnNumber = 16;
+ const int kLength = 8;
+ const int kByteNumber = 100;
+ LocationRange range(
+ Location(&input_file, kLineNumber, kColumnNumber, kByteNumber),
+ Location(&input_file, kLineNumber, kColumnNumber + kLength, kByteNumber));
+
+ SourceFile source_file = checker->SourceFileForInclude(
+ "header.h", kIncludeDirs, input_file, range, &err);
+ EXPECT_TRUE(source_file.is_null());
+ EXPECT_FALSE(err.has_error());
+}
