Add GN variables for controlling header checking.

BUG=
R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/chromium/src/+/2f985cb

Committed: https://chromium.googlesource.com/chromium/src/+/26bbbb9

Review URL: https://codereview.chromium.org/516683002

Cr-Commit-Position: refs/heads/master@{#292703}

tools/gn/binary_target_generator.cc

@@ -6,7 +6,9 @@
 
 #include "tools/gn/config_values_generator.h"
 #include "tools/gn/err.h"
+#include "tools/gn/functions.h"
 #include "tools/gn/scope.h"
+#include "tools/gn/value_extractors.h"
 #include "tools/gn/variables.h"
 
 BinaryTargetGenerator::BinaryTargetGenerator(
@@ -41,6 +43,10 @@ void BinaryTargetGenerator::DoRun() {
   if (err_->has_error())
     return;
 
+  FillCheckIncludes();
+  if (err_->has_error())
+    return;
+
   FillInputs();
   if (err_->has_error())
     return;
@@ -49,6 +55,10 @@ void BinaryTargetGenerator::DoRun() {
   if (err_->has_error())
     return;
 
+  FillAllowCircularIncludesFrom();
+  if (err_->has_error())
+    return;
+
   // Config values (compiler flags, etc.) set directly on this target.
   ConfigValuesGenerator gen(&target_->config_values(), scope_,
                             scope_->GetSourceDir(), err_);
@@ -57,6 +67,15 @@ void BinaryTargetGenerator::DoRun() {
     return;
 }
 
+void BinaryTargetGenerator::FillCheckIncludes() {
+  const Value* value = scope_->GetValue(variables::kCheckIncludes, true);
+  if (!value)
+    return;
+  if (!value->VerifyTypeIs(Value::BOOLEAN, err_))
+    return;
+  target_->set_check_includes(value->boolean_value());
+}
+
 void BinaryTargetGenerator::FillOutputName() {
   const Value* value = scope_->GetValue(variables::kOutputName, true);
   if (!value)
@@ -74,3 +93,40 @@ void BinaryTargetGenerator::FillOutputExtension() {
     return;
   target_->set_output_extension(value->string_value());
 }
+
+void BinaryTargetGenerator::FillAllowCircularIncludesFrom() {
+  const Value* value = scope_->GetValue(
+      variables::kAllowCircularIncludesFrom, true);
+  if (!value)
+    return;
+
+  UniqueVector<Label> circular;
+  ExtractListOfUniqueLabels(*value, scope_->GetSourceDir(),
+                            ToolchainLabelForScope(scope_), &circular, err_);
+  if (err_->has_error())
+    return;
+
+  // Validate that all circular includes entries are in the deps.
+  const LabelTargetVector& deps = target_->deps();
+  for (size_t circular_i = 0; circular_i < circular.size(); circular_i++) {
+    bool found_dep = false;
+    for (size_t dep_i = 0; dep_i < deps.size(); dep_i++) {
+      if (deps[dep_i].label == circular[circular_i]) {
+        found_dep = true;
+        break;
+      }
+    }
+    if (!found_dep) {
+      *err_ = Err(*value, "Label not in deps.",
+          "The label \"" + circular[circular_i].GetUserVisibleName(false) +
+          "\"\nwas not in the deps of this target. "
+          "allow_circular_includes_from only allows\ntargets present in the "
+          "deps.");
+      return;
+    }
+  }
+
+  // Add to the set.
+  for (size_t i = 0; i < circular.size(); i++)
+    target_->allow_circular_includes_from().insert(circular[i]);
+}

tools/gn/binary_target_generator.h

@@ -23,8 +23,10 @@ class BinaryTargetGenerator : public TargetGenerator {
   virtual void DoRun() OVERRIDE;
 
  private:
+  void FillCheckIncludes();
   void FillOutputName();
   void FillOutputExtension();
+  void FillAllowCircularIncludesFrom();
 
   Target::OutputType output_type_;
 

tools/gn/command_check.cc

@@ -14,7 +14,7 @@ const char kCheck[] = "check";
 const char kCheck_HelpShort[] =
     "check: Check header dependencies.";
 const char kCheck_Help[] =
-    "gn check <out_dir> [<target label>]\n"
+    "gn check <out_dir> [<target label>] [--force]\n"
     "\n"
     "  \"gn check\" is the same thing as \"gn gen\" with the \"--check\" flag\n"
     "  except that this command does not write out any build files. It's\n"
@@ -25,6 +25,10 @@ const char kCheck_Help[] =
     "  only those matching targets will be checked.\n"
     "  See \"gn help label_pattern\" for details.\n"
     "\n"
+    "  --force\n"
+    "      Ignores specifications of \"check_includes = false\" and checks\n"
+    "      all target's files that match the target label.\n"
+    "\n"
     "  See \"gn help\" for the common command-line switches.\n"
     "\n"
     "Examples\n"
@@ -64,9 +68,13 @@ int RunCheck(const std::vector<std::string>& args) {
     }
   }
 
+  const CommandLine* cmdline = CommandLine::ForCurrentProcess();
+  bool force = cmdline->HasSwitch("force");
+
   if (!CheckPublicHeaders(&setup->build_settings(),
                           setup->builder()->GetAllResolvedTargets(),
-                          targets_to_check))
+                          targets_to_check,
+                          force))
     return 1;
 
   OutputString("Header dependency check OK\n", DECORATION_GREEN);
@@ -75,14 +83,15 @@ int RunCheck(const std::vector<std::string>& args) {
 
 bool CheckPublicHeaders(const BuildSettings* build_settings,
                         const std::vector<const Target*>& all_targets,
-                        const std::vector<const Target*>& to_check) {
+                        const std::vector<const Target*>& to_check,
+                        bool force_check) {
   ScopedTrace trace(TraceItem::TRACE_CHECK_HEADERS, "Check headers");
 
   scoped_refptr<HeaderChecker> header_checker(
       new HeaderChecker(build_settings, all_targets));
 
   std::vector<Err> header_errors;
-  header_checker->Run(to_check, &header_errors);
+  header_checker->Run(to_check, force_check, &header_errors);
   for (size_t i = 0; i < header_errors.size(); i++) {
     if (i > 0)
       OutputString("___________________\n", DECORATION_YELLOW);

tools/gn/commands.h

@@ -99,11 +99,15 @@ bool ResolveTargetsFromCommandLinePattern(
 // all_targets, and the specific targets to check should be in to_check. If
 // to_check is empty, all targets will be checked.
 //
+// force_check, if true, will override targets opting out of header checking
+// with "check_includes = false" and will check them anyway.
+//
 // 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);
+                        const std::vector<const Target*>& to_check,
+                        bool force_check);
 
 }  // namespace commands
 

tools/gn/header_checker.cc

@@ -130,16 +130,17 @@ HeaderChecker::~HeaderChecker() {
 }
 
 bool HeaderChecker::Run(const std::vector<const Target*>& to_check,
+                        bool force_check,
                         std::vector<Err>* errors) {
   if (to_check.empty()) {
     // Check all files.
-    RunCheckOverFiles(file_map_);
+    RunCheckOverFiles(file_map_, force_check);
   } else {
     // Run only over the files in the given targets.
     FileMap files_to_check;
     for (size_t i = 0; i < to_check.size(); i++)
       AddTargetToFileMap(to_check[i], &files_to_check);
-    RunCheckOverFiles(files_to_check);
+    RunCheckOverFiles(files_to_check, force_check);
   }
 
   if (errors_.empty())
@@ -148,7 +149,7 @@ bool HeaderChecker::Run(const std::vector<const Target*>& to_check,
   return false;
 }
 
-void HeaderChecker::RunCheckOverFiles(const FileMap& files) {
+void HeaderChecker::RunCheckOverFiles(const FileMap& files, bool force_check) {
   if (files.empty())
     return;
 
@@ -164,6 +165,16 @@ void HeaderChecker::RunCheckOverFiles(const FileMap& files) {
         type != SOURCE_M && type != SOURCE_MM && type != SOURCE_RC)
       continue;
 
+    // Do a first pass to find if this should be skipped. All targets including
+    // this source file must exclude it from checking.
+    if (!force_check) {
+      bool check_includes = false;
+      for (size_t vect_i = 0; vect_i < vect.size(); ++vect_i)
+        check_includes |= vect[vect_i].target->check_includes();
+      if (!check_includes)
+        continue;
+    }
+
     for (size_t vect_i = 0; vect_i < vect.size(); ++vect_i) {
       pool->PostWorkerTaskWithShutdownBehavior(
           FROM_HERE,
@@ -349,6 +360,11 @@ bool HeaderChecker::CheckInclude(const Target* from_target,
         return false;
       }
 
+      found_dependency = true;
+    } else if (
+        to_target->allow_circular_includes_from().find(from_target->label()) !=
+        to_target->allow_circular_includes_from().end()) {
+      // Not a dependency, but this include is whitelisted from the destination.
       found_dependency = true;
     }
   }

tools/gn/header_checker.h

@@ -39,7 +39,11 @@ class HeaderChecker : public base::RefCountedThreadSafe<HeaderChecker> {
   // This assumes that the current thread already has a message loop. On
   // error, fills the given vector with the errors and returns false. Returns
   // true on success.
+  //
+  // force_check, if true, will override targets opting out of header checking
+  // with "check_includes = false" and will check them anyway.
   bool Run(const std::vector<const Target*>& to_check,
+           bool force_check,
            std::vector<Err>* errors);
 
  private:
@@ -48,6 +52,7 @@ class HeaderChecker : public base::RefCountedThreadSafe<HeaderChecker> {
   FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest,
                            IsDependencyOf_ForwardsDirectDependentConfigs);
   FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, CheckInclude);
+  FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest, CheckIncludeAllowCircular);
   FRIEND_TEST_ALL_PREFIXES(HeaderCheckerTest,
                            GetDependentConfigChainProblemIndex);
   ~HeaderChecker();
@@ -65,7 +70,7 @@ class HeaderChecker : public base::RefCountedThreadSafe<HeaderChecker> {
 
   // Backend for Run() that takes the list of files to check. The errors_ list
   // will be populate on failure.
-  void RunCheckOverFiles(const FileMap& flies);
+  void RunCheckOverFiles(const FileMap& flies, bool force_check);
 
   void DoWork(const Target* target, const SourceFile& file);
 

tools/gn/header_checker_unittest.cc

@@ -226,6 +226,33 @@ TEST_F(HeaderCheckerTest, CheckInclude) {
   }
 }
 
+// Checks that the allow_circular_includes_from list works.
+TEST_F(HeaderCheckerTest, CheckIncludeAllowCircular) {
+  InputFile input_file(SourceFile("//some_file.cc"));
+  input_file.SetContents(std::string());
+  LocationRange range;  // Dummy value.
+
+  // Add an include file to A.
+  SourceFile a_public("//a_public.h");
+  a_.sources().push_back(a_public);
+
+  scoped_refptr<HeaderChecker> checker(
+      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());
+
+  // 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());
+}
+
 TEST_F(HeaderCheckerTest, GetDependentConfigChainProblemIndex) {
   // Assume we have a chain A -> B -> C -> D.
   Target target_a(setup_.settings(), Label(SourceDir("//a/"), "a"));

tools/gn/setup.cc

@@ -190,7 +190,8 @@ bool CommonSetup::RunPostMessageLoop() {
   if (check_public_headers_) {
     if (!commands::CheckPublicHeaders(&build_settings_,
                                       builder_->GetAllResolvedTargets(),
-                                      std::vector<const Target*>())) {
+                                      std::vector<const Target*>(),
+                                      false)) {
       return false;
     }
   }

tools/gn/target.cc

@@ -45,6 +45,7 @@ Target::Target(const Settings* settings, const Label& label)
     : Item(settings, label),
       output_type_(UNKNOWN),
       all_headers_public_(true),
+      check_includes_(true),
       hard_dep_(false),
       toolchain_(NULL) {
 }

tools/gn/target.h

@@ -91,6 +91,10 @@ class Target : public Item {
   const FileList& public_headers() const { return public_headers_; }
   FileList& public_headers() { return public_headers_; }
 
+  // Whether this target's includes should be checked by "gn check".
+  bool check_includes() const { return check_includes_; }
+  void set_check_includes(bool ci) { check_includes_ = ci; }
+
   // Compile-time extra dependencies.
   const FileList& inputs() const { return inputs_; }
   FileList& inputs() { return inputs_; }
@@ -148,6 +152,14 @@ class Target : public Item {
     return forward_dependent_configs_;
   }
 
+  // Dependencies that can include files from this target.
+  const std::set<Label>& allow_circular_includes_from() const {
+    return allow_circular_includes_from_;
+  }
+  std::set<Label>& allow_circular_includes_from() {
+    return allow_circular_includes_from_;
+  }
+
   const UniqueVector<const Target*>& inherited_libraries() const {
     return inherited_libraries_;
   }
@@ -217,6 +229,7 @@ class Target : public Item {
   FileList sources_;
   bool all_headers_public_;
   FileList public_headers_;
+  bool check_includes_;
   FileList inputs_;
   FileList data_;
 
@@ -241,6 +254,8 @@ class Target : public Item {
   UniqueVector<LabelConfigPair> direct_dependent_configs_;
   UniqueVector<LabelTargetPair> forward_dependent_configs_;
 
+  std::set<Label> allow_circular_includes_from_;
+
   bool external_;
 
   // Static libraries and source sets from transitive deps. These things need

tools/gn/value_extractors.cc

@@ -101,12 +101,28 @@ struct RelativeDirConverter {
   const SourceDir& current_dir;
 };
 
-// Fills the label part of a LabelPtrPair, leaving the pointer null.
+// Fills in a label.
 template<typename T> struct LabelResolver {
   LabelResolver(const SourceDir& current_dir_in,
                 const Label& current_toolchain_in)
       : current_dir(current_dir_in),
         current_toolchain(current_toolchain_in) {}
+  bool operator()(const Value& v, Label* out, Err* err) const {
+    if (!v.VerifyTypeIs(Value::STRING, err))
+      return false;
+    *out = Label::Resolve(current_dir, current_toolchain, v, err);
+    return !err->has_error();
+  }
+  const SourceDir& current_dir;
+  const Label& current_toolchain;
+};
+
+// Fills the label part of a LabelPtrPair, leaving the pointer null.
+template<typename T> struct LabelPtrResolver {
+  LabelPtrResolver(const SourceDir& current_dir_in,
+                   const Label& current_toolchain_in)
+      : current_dir(current_dir_in),
+        current_toolchain(current_toolchain_in) {}
   bool operator()(const Value& v, LabelPtrPair<T>* out, Err* err) const {
     if (!v.VerifyTypeIs(Value::STRING, err))
       return false;
@@ -159,28 +175,38 @@ bool ExtractListOfLabels(const Value& value,
                          LabelTargetVector* dest,
                          Err* err) {
   return ListValueExtractor(value, dest, err,
-                            LabelResolver<Target>(current_dir,
-                                                  current_toolchain));
+                            LabelPtrResolver<Target>(current_dir,
+                                                     current_toolchain));
 }
 
 bool ExtractListOfUniqueLabels(const Value& value,
                                const SourceDir& current_dir,
                                const Label& current_toolchain,
-                               UniqueVector<LabelConfigPair>* dest,
+                               UniqueVector<Label>* dest,
                                Err* err) {
   return ListValueUniqueExtractor(value, dest, err,
                                   LabelResolver<Config>(current_dir,
                                                         current_toolchain));
 }
 
+bool ExtractListOfUniqueLabels(const Value& value,
+                               const SourceDir& current_dir,
+                               const Label& current_toolchain,
+                               UniqueVector<LabelConfigPair>* dest,
+                               Err* err) {
+  return ListValueUniqueExtractor(value, dest, err,
+                                  LabelPtrResolver<Config>(current_dir,
+                                                           current_toolchain));
+}
+
 bool ExtractListOfUniqueLabels(const Value& value,
                                const SourceDir& current_dir,
                                const Label& current_toolchain,
                                UniqueVector<LabelTargetPair>* dest,
                                Err* err) {
   return ListValueUniqueExtractor(value, dest, err,
-                                  LabelResolver<Target>(current_dir,
-                                                        current_toolchain));
+                                  LabelPtrResolver<Target>(current_dir,
+                                                           current_toolchain));
 }
 
 bool ExtractRelativeFile(const BuildSettings* build_settings,

tools/gn/value_extractors.h

@@ -45,9 +45,15 @@ bool ExtractListOfLabels(const Value& value,
                          LabelTargetVector* dest,
                          Err* err);
 
-// Extracts the list of labels and their origins to the given vector. Only the
-// labels are filled in, the ptr for each pair in the vector will be null. Sets
-// an error and returns false if a label is maformed or there are duplicates.
+// Extracts the list of labels and their origins to the given vector. For the
+// version taking Label*Pair, only the labels are filled in, the ptr for each
+// pair in the vector will be null. Sets an error and returns false if a label
+// is maformed or there are duplicates.
+bool ExtractListOfUniqueLabels(const Value& value,
+                               const SourceDir& current_dir,
+                               const Label& current_toolchain,
+                               UniqueVector<Label>* dest,
+                               Err* err);
 bool ExtractListOfUniqueLabels(const Value& value,
                                const SourceDir& current_dir,
                                const Label& current_toolchain,

tools/gn/variables.cc

@@ -262,6 +262,41 @@ const char kAllDependentConfigs_Help[] =
     "  See also \"direct_dependent_configs\".\n"
     COMMON_ORDERING_HELP;
 
+const char kAllowCircularIncludesFrom[] = "allow_circular_includes_from";
+const char kAllowCircularIncludesFrom_HelpShort[] =
+    "allow_circular_includes_from: [label list] Permit includes from deps.";
+const char kAllowCircularIncludesFrom_Help[] =
+    "allow_circular_includes_from: Permit includes from deps.\n"
+    "\n"
+    "  A list of target labels. Must be a subset of the target's \"deps\".\n"
+    "  These targets will be permitted to include headers from the current\n"
+    "  target despite the dependency going in the opposite direction.\n"
+    "\n"
+    "Tedious exposition\n"
+    "\n"
+    "  Normally, for a file in target A to include a file from target B,\n"
+    "  A must list B as a dependency. This invariant is enforced by the\n"
+    "  \"gn check\" command (and the --check flag to \"gn gen\").\n"
+    "\n"
+    "  Sometimes, two targets might be the same unit for linking purposes\n"
+    "  (two source sets or static libraries that would always be linked\n"
+    "  together in a final executable or shared library). In this case,\n"
+    "  you want A to be able to include B's headers, and B to include A's\n"
+    "  headers.\n"
+    "\n"
+    "  This list, if specified, lists which of the dependencies of the\n"
+    "  current target can include header files from the current target.\n"
+    "  That is, if A depends on B, B can only include headers from A if it is\n"
+    "  in A's allow_circular_includes_from list.\n"
+    "\n"
+    "Example\n"
+    "\n"
+    "  source_set(\"a\") {\n"
+    "    deps = [ \":b\", \":c\" ]\n"
+    "    allow_circular_includes_from = [ \":b\" ]\n"
+    "    ...\n"
+    "  }\n";
+
 const char kArgs[] = "args";
 const char kArgs_HelpShort[] =
     "args: [string list] Arguments passed to an action.";
@@ -311,6 +346,28 @@ const char kCflagsObjCC_HelpShort[] =
     "cflags_objcc: [string list] Flags passed to the Objective C++ compiler.";
 const char* kCflagsObjCC_Help = kCommonCflagsHelp;
 
+const char kCheckIncludes[] = "check_includes";
+const char kCheckIncludes_HelpShort[] =
+    "check_includes: [boolean] Controls whether a target's files are checked.";
+const char kCheckIncludes_Help[] =
+    "check_includes: [boolean] Controls whether a target's files are checked.\n"
+    "\n"
+    "  When true (the default), the \"gn check\" command (as well as\n"
+    "  \"gn gen\" with the --check flag) will check this target's sources\n"
+    "  and headers for proper dependencies.\n"
+    "\n"
+    "  When false, the files in this target will be skipped by default.\n"
+    "  This does not affect other targets that depend on the current target,\n"
+    "  it just skips checking the includes of the current target's files.\n"
+    "\n"
+    "Example\n"
+    "\n"
+    "  source_set(\"busted_includes\") {\n"
+    "    # This target's includes are messed up, exclude it from checking.\n"
+    "    check_includes = false\n"
+    "    ...\n"
+    "  }\n";
+
 const char kConfigs[] = "configs";
 const char kConfigs_HelpShort[] =
     "configs: [label list] Configs applying to this target.";
@@ -831,12 +888,14 @@ const VariableInfoMap& GetTargetVariables() {
   static VariableInfoMap info_map;
   if (info_map.empty()) {
     INSERT_VARIABLE(AllDependentConfigs)
+    INSERT_VARIABLE(AllowCircularIncludesFrom)
     INSERT_VARIABLE(Args)
     INSERT_VARIABLE(Cflags)
     INSERT_VARIABLE(CflagsC)
     INSERT_VARIABLE(CflagsCC)
     INSERT_VARIABLE(CflagsObjC)
     INSERT_VARIABLE(CflagsObjCC)
+    INSERT_VARIABLE(CheckIncludes)
     INSERT_VARIABLE(Configs)
     INSERT_VARIABLE(Data)
     INSERT_VARIABLE(Datadeps)

tools/gn/variables.h

@@ -67,6 +67,10 @@ extern const char kAllDependentConfigs[];
 extern const char kAllDependentConfigs_HelpShort[];
 extern const char kAllDependentConfigs_Help[];
 
+extern const char kAllowCircularIncludesFrom[];
+extern const char kAllowCircularIncludesFrom_HelpShort[];
+extern const char kAllowCircularIncludesFrom_Help[];
+
 extern const char kArgs[];
 extern const char kArgs_HelpShort[];
 extern const char kArgs_Help[];
@@ -91,6 +95,10 @@ extern const char kCflagsObjCC[];
 extern const char kCflagsObjCC_HelpShort[];
 extern const char* kCflagsObjCC_Help;
 
+extern const char kCheckIncludes[];
+extern const char kCheckIncludes_HelpShort[];
+extern const char kCheckIncludes_Help[];
+
 extern const char kConfigs[];
 extern const char kConfigs_HelpShort[];
 extern const char kConfigs_Help[];