Add filters for "gn check"

GN's check command will now read a list of filters from the .gn file. This
allows us to specify a subset of the targets to check so we can incrementally
fix issues while maintaining proper dependencies on good areas.

Adds checking for unused variables in the .gn file to catch misspellings.

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

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

.gn

@@ -1,5 +1,6 @@
-# This file is used by the experimental meta-buildsystem in src/tools/gn to
-# find the root of the source tree and to set startup options.
+# This file is used by the GN meta build system to find the root of the source
+# tree and to set startup options. For documentation on the values set in this
+# file, run "gn help dotfile" at the command line.
 
 # The location of the build configuration file.
 buildconfig = "//build/config/BUILDCONFIG.gn"
@@ -8,3 +9,11 @@ buildconfig = "//build/config/BUILDCONFIG.gn"
 # GN build files are placed when they can not be placed directly
 # in the source tree, e.g. for third party source trees.
 secondary_source = "//build/secondary/"
+
+# These are the targets to check headers for by default. The files in targets
+# matching these patterns (see "gn help label_pattern" for format) will have
+# their includes checked for proper dependencies when you run either
+# "gn check" or "gn gen --check".
+check_targets = [
+  "//base"
+]

tools/gn/command_check.cc

@@ -2,10 +2,14 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/command_line.h"
+#include "base/strings/stringprintf.h"
 #include "tools/gn/commands.h"
 #include "tools/gn/header_checker.h"
 #include "tools/gn/setup.h"
 #include "tools/gn/standard_out.h"
+#include "tools/gn/switches.h"
+#include "tools/gn/target.h"
 #include "tools/gn/trace.h"
 
 namespace commands {
@@ -14,23 +18,28 @@ const char kCheck[] = "check";
 const char kCheck_HelpShort[] =
     "check: Check header dependencies.";
 const char kCheck_Help[] =
-    "gn check <out_dir> [<target label>] [--force]\n"
+    "gn check <out_dir> [<label_pattern>] [--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"
     "  intended to be an easy way to manually trigger include file checking.\n"
     "\n"
     "  The <label_pattern> can take exact labels or patterns that match more\n"
     "  than one (although not general regular expressions). If specified,\n"
-    "  only those matching targets will be checked.\n"
-    "  See \"gn help label_pattern\" for details.\n"
+    "  only those matching targets will be checked. See\n"
+    "  \"gn help label_pattern\" for details.\n"
+    "\n"
+    "  The .gn file may specify a list of targets to be checked. Only these\n"
+    "  targets will be checked if no label_pattern is specified on the\n"
+    "  command line. Otherwise, the command-line list is used instead. See\n"
+    "  \"gn help dotfile\".\n"
+    "\n"
+    "Command-specific switches\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"
     "\n"
     "  gn check out/Debug\n"
@@ -56,28 +65,51 @@ int RunCheck(const std::vector<std::string>& args) {
   if (!setup->Run())
     return 1;
 
+  std::vector<const Target*> all_targets =
+      setup->builder()->GetAllResolvedTargets();
+
+  bool filtered_by_build_config = false;
   std::vector<const Target*> targets_to_check;
   if (args.size() == 2) {
-    // Compute the target to check (empty means everything).
+    // Compute the target to check.
     if (!ResolveTargetsFromCommandLinePattern(setup, args[1], false,
                                               &targets_to_check))
       return 1;
     if (targets_to_check.size() == 0) {
       OutputString("No matching targets.\n");
       return 1;
     }
+  } else {
+    // No argument means to check everything allowed by the filter in
+    // the build config file.
+    if (setup->check_patterns()) {
+      FilterTargetsByPatterns(all_targets, *setup->check_patterns(),
+                              &targets_to_check);
+      filtered_by_build_config = targets_to_check.size() != all_targets.size();
+    } else {
+      // No global filter, check everything.
+      targets_to_check = all_targets;
+    }
   }
 
   const CommandLine* cmdline = CommandLine::ForCurrentProcess();
   bool force = cmdline->HasSwitch("force");
 
-  if (!CheckPublicHeaders(&setup->build_settings(),
-                          setup->builder()->GetAllResolvedTargets(),
-                          targets_to_check,
-                          force))
+  if (!CheckPublicHeaders(&setup->build_settings(), all_targets,
+                          targets_to_check, force))
     return 1;
 
-  OutputString("Header dependency check OK\n", DECORATION_GREEN);
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kQuiet)) {
+    if (filtered_by_build_config) {
+      // Tell the user about the implicit filtering since this is obscure.
+      OutputString(base::StringPrintf(
+          "%d targets out of %d checked based on the check_targets defined in"
+          " \".gn\".\n",
+          static_cast<int>(targets_to_check.size()),
+          static_cast<int>(all_targets.size())));
+    }
+    OutputString("Header dependency check OK\n", DECORATION_GREEN);
+  }
   return 0;
 }
 
@@ -100,4 +132,17 @@ bool CheckPublicHeaders(const BuildSettings* build_settings,
   return header_errors.empty();
 }
 
+void FilterTargetsByPatterns(const std::vector<const Target*>& input,
+                             const std::vector<LabelPattern>& filter,
+                             std::vector<const Target*>* output) {
+  for (const auto& target : input) {
+    for (const auto& pattern : filter) {
+      if (pattern.Matches(target->label())) {
+        output->push_back(target);
+        break;
+      }
+    }
+  }
+}
+
 }  // namespace commands

tools/gn/commands.cc

@@ -110,13 +110,10 @@ bool ResolveTargetsFromCommandLinePattern(
     }
   }
 
-  std::vector<const Target*> all_targets =
-      setup->builder()->GetAllResolvedTargets();
-
-  for (const auto& target : all_targets) {
-    if (pattern.Matches(target->label()))
-      matches->push_back(target);
-  }
+  std::vector<LabelPattern> pattern_vector;
+  pattern_vector.push_back(pattern);
+  FilterTargetsByPatterns(setup->builder()->GetAllResolvedTargets(),
+                          pattern_vector, matches);
   return true;
 }
 

tools/gn/commands.h

@@ -12,6 +12,7 @@
 #include "base/strings/string_piece.h"
 
 class BuildSettings;
+class LabelPattern;
 class Setup;
 class Target;
 
@@ -101,8 +102,7 @@ bool ResolveTargetsFromCommandLinePattern(
     std::vector<const Target*>* matches);
 
 // Runs the header checker. All targets in the build should be given in
-// all_targets, and the specific targets to check should be in to_check. If
-// to_check is empty, all targets will be checked.
+// all_targets, and the specific targets to check should be in to_check.
 //
 // force_check, if true, will override targets opting out of header checking
 // with "check_includes = false" and will check them anyway.
@@ -114,6 +114,13 @@ bool CheckPublicHeaders(const BuildSettings* build_settings,
                         const std::vector<const Target*>& to_check,
                         bool force_check);
 
+// Filters the given list of targets by the given pattern list. This is a
+// helper function for setting up a call to CheckPublicHeaders based on a check
+// filter.
+void FilterTargetsByPatterns(const std::vector<const Target*>& input,
+                             const std::vector<LabelPattern>& filter,
+                             std::vector<const Target*>* output);
+
 }  // namespace commands
 
 #endif  // TOOLS_GN_COMMANDS_H_

tools/gn/header_checker.cc

@@ -131,16 +131,10 @@ 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_, force_check);
-  } else {
-    // Run only over the files in the given targets.
-    FileMap files_to_check;
-    for (const auto& check : to_check)
-      AddTargetToFileMap(check, &files_to_check);
-    RunCheckOverFiles(files_to_check, force_check);
-  }
+  FileMap files_to_check;
+  for (const auto& check : to_check)
+    AddTargetToFileMap(check, &files_to_check);
+  RunCheckOverFiles(files_to_check, force_check);
 
   if (errors_.empty())
     return true;

tools/gn/header_checker.h

@@ -50,8 +50,7 @@ class HeaderChecker : public base::RefCountedThreadSafe<HeaderChecker> {
   HeaderChecker(const BuildSettings* build_settings,
                 const std::vector<const Target*>& targets);
 
-  // Runs the check. The targets in to_check will be checked. If this list is
-  // empty, all targets will be checked.
+  // Runs the check. The targets in to_check will be checked.
   //
   // This assumes that the current thread already has a message loop. On
   // error, fills the given vector with the errors and returns false. Returns

tools/gn/setup.cc

@@ -59,6 +59,15 @@ extern const char kDotfile_Help[] =
     "      Label of the build config file. This file will be used to set up\n"
     "      the build file execution environment for each toolchain.\n"
     "\n"
+    "  check_targets [optional]\n"
+    "      A list of labels and label patterns that should be checked when\n"
+    "      running \"gn check\" or \"gn gen --check\". If unspecified, all\n"
+    "      targets will be checked. If it is the empty list, no targets will\n"
+    "      be checked.\n"
+    "\n"
+    "      The format of this list is identical to that of \"visibility\"\n"
+    "      so see \"gn help visibility\" for examples.\n"
+    "\n"
     "  root [optional]\n"
     "      Label of the root build target. The GN build will start by loading\n"
     "      the build file containing this target name. This defaults to\n"
@@ -80,6 +89,11 @@ extern const char kDotfile_Help[] =
     "\n"
     "  buildconfig = \"//build/config/BUILDCONFIG.gn\"\n"
     "\n"
+    "  check_targets = [\n"
+    "    \"//doom_melon/*\",  # Check everything in this subtree.\n"
+    "    \"//tools:mind_controlling_ant\",  # Check this specific target.\n"
+    "  ]\n"
+    "\n"
     "  root = \"//:root\"\n"
     "\n"
     "  secondary_source = \"//build/config/temporary_buildfiles/\"\n";
@@ -172,10 +186,17 @@ bool CommonSetup::RunPostMessageLoop() {
   }
 
   if (check_public_headers_) {
-    if (!commands::CheckPublicHeaders(&build_settings_,
-                                      builder_->GetAllResolvedTargets(),
-                                      std::vector<const Target*>(),
-                                      false)) {
+    std::vector<const Target*> all_targets = builder_->GetAllResolvedTargets();
+    std::vector<const Target*> to_check;
+    if (check_patterns()) {
+      commands::FilterTargetsByPatterns(all_targets, *check_patterns(),
+                                        &to_check);
+    } else {
+      to_check = all_targets;
+    }
+
+    if (!commands::CheckPublicHeaders(&build_settings_, all_targets,
+                                      to_check, false)) {
       return false;
     }
   }
@@ -230,6 +251,13 @@ bool Setup::DoSetup(const std::string& build_dir, bool force_create) {
   if (!FillBuildDir(build_dir, !force_create))
     return false;
 
+  // Check for unused variables in the .gn file.
+  Err err;
+  if (!dotfile_scope_.CheckForUnusedVars(&err)) {
+    err.PrintToStdout();
+    return false;
+  }
+
   if (fill_arguments_) {
     if (!FillArguments(*cmdline))
       return false;
@@ -563,6 +591,28 @@ bool Setup::FillOtherConfig(const CommandLine& cmdline) {
   build_settings_.set_build_config_file(
       SourceFile(build_config_value->string_value()));
 
+  // Targets to check.
+  const Value* check_targets_value =
+      dotfile_scope_.GetValue("check_targets", true);
+  if (check_targets_value) {
+    check_patterns_.reset(new std::vector<LabelPattern>);
+
+    // Fill the list of targets to check.
+    if (!check_targets_value->VerifyTypeIs(Value::LIST, &err)) {
+      err.PrintToStdout();
+      return false;
+    }
+    SourceDir current_dir("//");
+    for (const auto& item : check_targets_value->list_value()) {
+      check_patterns_->push_back(
+          LabelPattern::GetPattern(current_dir, item, &err));
+      if (err.has_error()) {
+        err.PrintToStdout();
+        return false;
+      }
+    }
+  }
+
   return true;
 }
 

tools/gn/setup.h

@@ -12,6 +12,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "tools/gn/build_settings.h"
 #include "tools/gn/builder.h"
+#include "tools/gn/label_pattern.h"
 #include "tools/gn/loader.h"
 #include "tools/gn/scheduler.h"
 #include "tools/gn/scope.h"
@@ -53,6 +54,13 @@ class CommonSetup {
     check_public_headers_ = s;
   }
 
+  // Read from the .gn file, these are the targets to check. If the .gn file
+  // does not specify anything, this will be null. If the .gn file specifies
+  // the empty list, this will be non-null but empty.
+  const std::vector<LabelPattern>* check_patterns() const {
+    return check_patterns_.get();
+  }
+
   BuildSettings& build_settings() { return build_settings_; }
   Builder* builder() { return builder_.get(); }
   LoaderImpl* loader() { return loader_.get(); }
@@ -80,6 +88,9 @@ class CommonSetup {
   bool check_for_unused_overrides_;
   bool check_public_headers_;
 
+  // See getter for info.
+  scoped_ptr<std::vector<LabelPattern> > check_patterns_;
+
  private:
   CommonSetup& operator=(const CommonSetup& other);  // Disallow.
 };

tools/gn/variables.cc

@@ -948,7 +948,7 @@ const char kVisibility_Help[] =
     "    visibility = [ \"//bar:*\" ]\n"
     "\n"
     "  Any target in \"//bar/\" or any subdirectory thereof:\n"
-    "    visibility = [ \"//bar/*\"\n ]"
+    "    visibility = [ \"//bar/*\" ]\n"
     "\n"
     "  Just these specific targets:\n"
     "    visibility = [ \":mything\", \"//foo:something_else\" ]\n"