Add testonly flag to GN

This also reworks visibility so both it and the testonly flag are checked when
a target is marked resolved, rather than when it is written out. The previous
code would not check visibility when doing non-"gen" commands like "check",
which is counterintuitive.

This change required OnResolved to be able to report a failure, which required
updating many callers. But it makes visibility mush easier to test, so I
added some additional visibility tests.

BUG=357779
R=hclam@chromium.org

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

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

tools/gn/builder.cc

@@ -418,7 +418,9 @@ bool Builder::ResolveItem(BuilderRecord* record, Err* err) {
   }
 
   record->set_resolved(true);
-  record->item()->OnResolved();
+
+  if (!record->item()->OnResolved(err))
+    return false;
   if (!resolved_callback_.is_null())
     resolved_callback_.Run(record);
 

tools/gn/command_gen.cc

@@ -25,19 +25,8 @@ const char kSwitchQuiet[] = "q";
 
 const char kSwitchCheck[] = "check";
 
-void BackgroundDoWrite(const Target* target,
-                       const std::vector<const Item*>& deps_for_visibility) {
-  // Validate visibility.
-  Err err;
-  for (size_t i = 0; i < deps_for_visibility.size(); i++) {
-    if (!Visibility::CheckItemVisibility(target, deps_for_visibility[i],
-                                         &err)) {
-      g_scheduler->FailWithError(err);
-      break;  // Don't return early since we need DecrementWorkCount below.
-    }
-  }
-
-  if (!err.has_error())
+// Called on worker thread to write the ninja file.
+void BackgroundDoWrite(const Target* target) {
   NinjaTargetWriter::RunAndWriteFile(target);
   g_scheduler->DecrementWorkCount();
 }
@@ -51,16 +40,8 @@ void ItemResolvedCallback(base::subtle::Atomic32* write_counter,
   const Item* item = record->item();
   const Target* target = item->AsTarget();
   if (target) {
-    // Collect all dependencies.
-    std::vector<const Item*> deps;
-    for (BuilderRecord::BuilderRecordSet::const_iterator iter =
-             record->all_deps().begin();
-         iter != record->all_deps().end();
-         ++iter)
-      deps.push_back((*iter)->item());
-
     g_scheduler->IncrementWorkCount();
-    g_scheduler->ScheduleWork(base::Bind(&BackgroundDoWrite, target, deps));
+    g_scheduler->ScheduleWork(base::Bind(&BackgroundDoWrite, target));
   }
 }
 

tools/gn/config_values_extractors_unittest.cc

@@ -28,6 +28,7 @@ struct IncludeWriter {
 
 TEST(ConfigValuesExtractors, IncludeOrdering) {
   TestWithScope setup;
+  Err err;
 
   // Construct a chain of dependencies: target -> dep1 -> dep2
   // Add representative values: cflags (opaque, always copied) and include_dirs
@@ -100,9 +101,9 @@ TEST(ConfigValuesExtractors, IncludeOrdering) {
       SourceDir("//target/"));
 
   // Mark targets resolved. This should push dependent configs.
-  dep2.OnResolved();
-  dep1.OnResolved();
-  target.OnResolved();
+  ASSERT_TRUE(dep2.OnResolved(&err));
+  ASSERT_TRUE(dep1.OnResolved(&err));
+  ASSERT_TRUE(target.OnResolved(&err));
 
   // Verify cflags by serializing.
   std::ostringstream flag_out;

tools/gn/item.cc

@@ -32,3 +32,7 @@ std::string Item::GetItemTypeName() const {
   NOTREACHED();
   return "this thing that I have no idea what it is";
 }
+
+bool Item::OnResolved(Err* err) {
+  return true;
+}

tools/gn/item.h

@@ -48,8 +48,9 @@ class Item {
   std::string GetItemTypeName() const;
 
   // Called when this item is resolved, meaning it and all of its dependents
-  // have no unresolved deps.
-  virtual void OnResolved() {}
+  // have no unresolved deps. Returns true on success. Sets the error and
+  // returns false on failure.
+  virtual bool OnResolved(Err* err);
 
  private:
   const Settings* settings_;

tools/gn/label_ptr.h

@@ -36,7 +36,11 @@ struct LabelPtrPair {
 
   Label label;
   const T* ptr;  // May be NULL.
-  const ParseNode* origin;  // May be NULL.
+
+  // The origin of this dependency. This will be null for internally generated
+  // dependencies. This happens when a group is automatically expanded and that
+  // group's members are added to the target that depends on that group.
+  const ParseNode* origin;
 };
 
 typedef LabelPtrPair<Config> LabelConfigPair;

tools/gn/ninja_action_target_writer_unittest.cc

@@ -13,6 +13,8 @@
 
 TEST(NinjaActionTargetWriter, WriteOutputFilesForBuildLine) {
   TestWithScope setup;
+  Err err;
+
   setup.build_settings()->SetBuildDir(SourceDir("//out/Debug/"));
 
   Target target(setup.settings(), Label(SourceDir("//foo/"), "bar"));
@@ -22,7 +24,7 @@ TEST(NinjaActionTargetWriter, WriteOutputFilesForBuildLine) {
       "//out/Debug/gen/{{source_name_part}}.cc");
 
   target.SetToolchain(setup.toolchain());
-  target.OnResolved();
+  ASSERT_TRUE(target.OnResolved(&err));
 
   std::ostringstream out;
   NinjaActionTargetWriter writer(&target, out);
@@ -37,6 +39,8 @@ TEST(NinjaActionTargetWriter, WriteOutputFilesForBuildLine) {
 // Tests an action with no sources.
 TEST(NinjaActionTargetWriter, ActionNoSources) {
   TestWithScope setup;
+  Err err;
+
   setup.build_settings()->SetBuildDir(SourceDir("//out/Debug/"));
   Target target(setup.settings(), Label(SourceDir("//foo/"), "bar"));
   target.set_output_type(Target::ACTION);
@@ -48,7 +52,7 @@ TEST(NinjaActionTargetWriter, ActionNoSources) {
       SubstitutionList::MakeForTest("//out/Debug/foo.out");
 
   target.SetToolchain(setup.toolchain());
-  target.OnResolved();
+  ASSERT_TRUE(target.OnResolved(&err));
 
   setup.settings()->set_target_os(Settings::LINUX);
   setup.build_settings()->set_python_path(base::FilePath(FILE_PATH_LITERAL(
@@ -76,6 +80,8 @@ TEST(NinjaActionTargetWriter, ActionNoSources) {
 // both sources and inputs (ACTION_FOREACH treats the sources differently).
 TEST(NinjaActionTargetWriter, ActionWithSources) {
   TestWithScope setup;
+  Err err;
+
   setup.build_settings()->SetBuildDir(SourceDir("//out/Debug/"));
   Target target(setup.settings(), Label(SourceDir("//foo/"), "bar"));
   target.set_output_type(Target::ACTION);
@@ -89,7 +95,7 @@ TEST(NinjaActionTargetWriter, ActionWithSources) {
       SubstitutionList::MakeForTest("//out/Debug/foo.out");
 
   target.SetToolchain(setup.toolchain());
-  target.OnResolved();
+  ASSERT_TRUE(target.OnResolved(&err));
 
   // Posix.
   {
@@ -146,6 +152,8 @@ TEST(NinjaActionTargetWriter, ActionWithSources) {
 
 TEST(NinjaActionTargetWriter, ForEach) {
   TestWithScope setup;
+  Err err;
+
   setup.build_settings()->SetBuildDir(SourceDir("//out/Debug/"));
 
   // Some dependencies that the action can depend on. Use actions for these
@@ -155,12 +163,12 @@ TEST(NinjaActionTargetWriter, ForEach) {
   Target dep(setup.settings(), Label(SourceDir("//foo/"), "dep"));
   dep.set_output_type(Target::ACTION);
   dep.SetToolchain(setup.toolchain());
-  dep.OnResolved();
+  ASSERT_TRUE(dep.OnResolved(&err));
 
   Target datadep(setup.settings(), Label(SourceDir("//foo/"), "datadep"));
   datadep.set_output_type(Target::ACTION);
   datadep.SetToolchain(setup.toolchain());
-  datadep.OnResolved();
+  ASSERT_TRUE(datadep.OnResolved(&err));
 
   Target target(setup.settings(), Label(SourceDir("//foo/"), "bar"));
   target.set_output_type(Target::ACTION_FOREACH);
@@ -182,7 +190,7 @@ TEST(NinjaActionTargetWriter, ForEach) {
   target.inputs().push_back(SourceFile("//foo/included.txt"));
 
   target.SetToolchain(setup.toolchain());
-  target.OnResolved();
+  ASSERT_TRUE(target.OnResolved(&err));
 
   // Posix.
   {
@@ -268,6 +276,8 @@ TEST(NinjaActionTargetWriter, ForEach) {
 
 TEST(NinjaActionTargetWriter, ForEachWithDepfile) {
   TestWithScope setup;
+  Err err;
+
   setup.build_settings()->SetBuildDir(SourceDir("//out/Debug/"));
   Target target(setup.settings(), Label(SourceDir("//foo/"), "bar"));
   target.set_output_type(Target::ACTION_FOREACH);
@@ -278,10 +288,9 @@ TEST(NinjaActionTargetWriter, ForEachWithDepfile) {
   target.action_values().set_script(SourceFile("//foo/script.py"));
 
   target.SetToolchain(setup.toolchain());
-  target.OnResolved();
+  ASSERT_TRUE(target.OnResolved(&err));
 
   SubstitutionPattern depfile;
-  Err err;
   ASSERT_TRUE(
       depfile.Parse("//out/Debug/gen/{{source_name_part}}.d", NULL, &err));
   target.action_values().set_depfile(depfile);

tools/gn/ninja_binary_target_writer_unittest.cc

@@ -11,6 +11,8 @@
 
 TEST(NinjaBinaryTargetWriter, SourceSet) {
   TestWithScope setup;
+  Err err;
+
   setup.build_settings()->SetBuildDir(SourceDir("//out/Debug/"));
   setup.settings()->set_target_os(Settings::WIN);
 
@@ -23,7 +25,7 @@ TEST(NinjaBinaryTargetWriter, SourceSet) {
   target.sources().push_back(SourceFile("//foo/input3.o"));
   target.sources().push_back(SourceFile("//foo/input4.obj"));
   target.SetToolchain(setup.toolchain());
-  target.OnResolved();
+  ASSERT_TRUE(target.OnResolved(&err));
 
   // Source set itself.
   {
@@ -57,7 +59,7 @@ TEST(NinjaBinaryTargetWriter, SourceSet) {
   shlib_target.set_output_type(Target::SHARED_LIBRARY);
   shlib_target.deps().push_back(LabelTargetPair(&target));
   shlib_target.SetToolchain(setup.toolchain());
-  shlib_target.OnResolved();
+  ASSERT_TRUE(shlib_target.OnResolved(&err));
 
   {
     std::ostringstream out;
@@ -94,7 +96,7 @@ TEST(NinjaBinaryTargetWriter, SourceSet) {
   stlib_target.set_output_type(Target::STATIC_LIBRARY);
   stlib_target.deps().push_back(LabelTargetPair(&target));
   stlib_target.SetToolchain(setup.toolchain());
-  stlib_target.OnResolved();
+  ASSERT_TRUE(stlib_target.OnResolved(&err));
 
   {
     std::ostringstream out;
@@ -129,6 +131,8 @@ TEST(NinjaBinaryTargetWriter, SourceSet) {
 // are applied.
 TEST(NinjaBinaryTargetWriter, ProductExtensionAndInputDeps) {
   TestWithScope setup;
+  Err err;
+
   setup.build_settings()->SetBuildDir(SourceDir("//out/Debug/"));
   setup.settings()->set_target_os(Settings::LINUX);
 
@@ -136,7 +140,7 @@ TEST(NinjaBinaryTargetWriter, ProductExtensionAndInputDeps) {
   Target action(setup.settings(), Label(SourceDir("//foo/"), "action"));
   action.set_output_type(Target::ACTION_FOREACH);
   action.SetToolchain(setup.toolchain());
-  action.OnResolved();
+  ASSERT_TRUE(action.OnResolved(&err));
 
   // A shared library w/ the product_extension set to a custom value.
   Target target(setup.settings(), Label(SourceDir("//foo/"), "shlib"));
@@ -146,7 +150,7 @@ TEST(NinjaBinaryTargetWriter, ProductExtensionAndInputDeps) {
   target.sources().push_back(SourceFile("//foo/input2.cc"));
   target.deps().push_back(LabelTargetPair(&action));
   target.SetToolchain(setup.toolchain());
-  target.OnResolved();
+  ASSERT_TRUE(target.OnResolved(&err));
 
   std::ostringstream out;
   NinjaBinaryTargetWriter writer(&target, out);
@@ -185,6 +189,8 @@ TEST(NinjaBinaryTargetWriter, ProductExtensionAndInputDeps) {
 
 TEST(NinjaBinaryTargetWriter, EmptyProductExtension) {
   TestWithScope setup;
+  Err err;
+
   setup.build_settings()->SetBuildDir(SourceDir("//out/Debug/"));
   setup.settings()->set_target_os(Settings::LINUX);
 
@@ -197,7 +203,7 @@ TEST(NinjaBinaryTargetWriter, EmptyProductExtension) {
   target.sources().push_back(SourceFile("//foo/input2.cc"));
 
   target.SetToolchain(setup.toolchain());
-  target.OnResolved();
+  ASSERT_TRUE(target.OnResolved(&err));
 
   std::ostringstream out;
   NinjaBinaryTargetWriter writer(&target, out);

tools/gn/ninja_copy_target_writer_unittest.cc

@@ -13,6 +13,7 @@
 // Tests mutliple files with an output pattern and no toolchain dependency.
 TEST(NinjaCopyTargetWriter, Run) {
   TestWithScope setup;
+  Err err;
 
   setup.settings()->set_target_os(Settings::LINUX);
   setup.build_settings()->SetBuildDir(SourceDir("//out/Debug/"));
@@ -26,7 +27,7 @@ TEST(NinjaCopyTargetWriter, Run) {
       SubstitutionList::MakeForTest("//out/Debug/{{source_name_part}}.out");
 
   target.SetToolchain(setup.toolchain());
-  target.OnResolved();
+  ASSERT_TRUE(target.OnResolved(&err));
 
   std::ostringstream out;
   NinjaCopyTargetWriter writer(&target, out);
@@ -44,6 +45,7 @@ TEST(NinjaCopyTargetWriter, Run) {
 // Tests a single file with no output pattern.
 TEST(NinjaCopyTargetWriter, ToolchainDeps) {
   TestWithScope setup;
+  Err err;
 
   setup.settings()->set_target_os(Settings::LINUX);
   setup.build_settings()->SetBuildDir(SourceDir("//out/Debug/"));
@@ -56,7 +58,7 @@ TEST(NinjaCopyTargetWriter, ToolchainDeps) {
       SubstitutionList::MakeForTest("//out/Debug/output.out");
 
   target.SetToolchain(setup.toolchain());
-  target.OnResolved();
+  ASSERT_TRUE(target.OnResolved(&err));
 
   std::ostringstream out;
   NinjaCopyTargetWriter writer(&target, out);

tools/gn/ninja_target_writer_unittest.cc

@@ -32,6 +32,7 @@ class TestingNinjaTargetWriter : public NinjaTargetWriter {
 
 TEST(NinjaTargetWriter, WriteInputDepsStampAndGetDep) {
   TestWithScope setup;
+  Err err;
 
   // Make a base target that's a hard dep (action).
   Target base_target(setup.settings(), Label(SourceDir("//foo/"), "base"));
@@ -57,9 +58,9 @@ TEST(NinjaTargetWriter, WriteInputDepsStampAndGetDep) {
   action.sources().push_back(SourceFile("//foo/action_source.txt"));
   action.deps().push_back(LabelTargetPair(&target));
 
-  base_target.OnResolved();
-  target.OnResolved();
-  action.OnResolved();
+  ASSERT_TRUE(base_target.OnResolved(&err));
+  ASSERT_TRUE(target.OnResolved(&err));
+  ASSERT_TRUE(action.OnResolved(&err));
 
   // Input deps for the base (should be only the script itself).
   {
@@ -105,6 +106,7 @@ TEST(NinjaTargetWriter, WriteInputDepsStampAndGetDep) {
 // Tests WriteInputDepsStampAndGetDep when toolchain deps are present.
 TEST(NinjaTargetWriter, WriteInputDepsStampAndGetDepWithToolchainDeps) {
   TestWithScope setup;
+  Err err;
 
   // Toolchain dependency. Here we make a target in the same toolchain for
   // simplicity, but in real life (using the Builder) this would be rejected
@@ -114,14 +116,14 @@ TEST(NinjaTargetWriter, WriteInputDepsStampAndGetDepWithToolchainDeps) {
                               Label(SourceDir("//foo/"), "setup"));
   toolchain_dep_target.set_output_type(Target::ACTION);
   toolchain_dep_target.SetToolchain(setup.toolchain());
-  toolchain_dep_target.OnResolved();
+  ASSERT_TRUE(toolchain_dep_target.OnResolved(&err));
   setup.toolchain()->deps().push_back(LabelTargetPair(&toolchain_dep_target));
 
   // Make a binary target
   Target target(setup.settings(), Label(SourceDir("//foo/"), "target"));
   target.set_output_type(Target::EXECUTABLE);
   target.SetToolchain(setup.toolchain());
-  target.OnResolved();
+  ASSERT_TRUE(target.OnResolved(&err));
 
   std::ostringstream stream;
   TestingNinjaTargetWriter writer(&target, setup.toolchain(), stream);

tools/gn/substitution_writer_unittest.cc

@@ -173,11 +173,12 @@ TEST(SubstutitionWriter, SourceSubstitutions) {
 
 TEST(SubstitutionWriter, TargetSubstitutions) {
   TestWithScope setup;
+  Err err;
 
   Target target(setup.settings(), Label(SourceDir("//foo/bar/"), "baz"));
   target.set_output_type(Target::STATIC_LIBRARY);
   target.SetToolchain(setup.toolchain());
-  target.OnResolved();
+  ASSERT_TRUE(target.OnResolved(&err));
 
   std::string result;
   EXPECT_TRUE(SubstitutionWriter::GetTargetSubstitution(
@@ -207,11 +208,12 @@ TEST(SubstitutionWriter, TargetSubstitutions) {
 
 TEST(SubstitutionWriter, CompilerSubstitutions) {
   TestWithScope setup;
+  Err err;
 
   Target target(setup.settings(), Label(SourceDir("//foo/bar/"), "baz"));
   target.set_output_type(Target::STATIC_LIBRARY);
   target.SetToolchain(setup.toolchain());
-  target.OnResolved();
+  ASSERT_TRUE(target.OnResolved(&err));
 
   // The compiler substitution is just source + target combined. So test one
   // of each of those classes of things to make sure this is hooked up.
@@ -227,11 +229,12 @@ TEST(SubstitutionWriter, CompilerSubstitutions) {
 
 TEST(SubstitutionWriter, LinkerSubstitutions) {
   TestWithScope setup;
+  Err err;
 
   Target target(setup.settings(), Label(SourceDir("//foo/bar/"), "baz"));
   target.set_output_type(Target::SHARED_LIBRARY);
   target.SetToolchain(setup.toolchain());
-  target.OnResolved();
+  ASSERT_TRUE(target.OnResolved(&err));
 
   const Tool* tool = setup.toolchain()->GetToolForTargetFinalOutput(&target);
 
@@ -246,7 +249,6 @@ TEST(SubstitutionWriter, LinkerSubstitutions) {
 
   // Test that we handle paths that end up in the root build dir properly
   // (no leading "./" or "/").
-  Err err;
   SubstitutionPattern pattern;
   ASSERT_TRUE(
       pattern.Parse("{{root_out_dir}}/{{target_output_name}}.so", NULL, &err));

tools/gn/target.cc

@@ -39,6 +39,33 @@ void MergeAllDependentConfigsFrom(const Target* from_target,
   }
 }
 
+Err MakeTestOnlyError(const Target* from, const Target* to) {
+  return Err(from->defined_from(), "Test-only dependency not allowed.",
+      from->label().GetUserVisibleName(false) + "\n"
+      "which is NOT marked testonly can't depend on\n" +
+      to->label().GetUserVisibleName(false) + "\n"
+      "which is marked testonly. Only targets with \"testonly = true\"\n"
+      "can depend on other test-only targets.\n"
+      "\n"
+      "Either mark it test-only or don't do this dependency.");
+}
+
+// Inserts the given groups dependencies, starting at the given index of the
+// given vector. Returns the number of items inserted.
+size_t InsertGroupDeps(LabelTargetVector* vector,
+                       size_t insert_at,
+                       const Target* group) {
+  const LabelTargetVector& deps = group->deps();
+  vector->insert(vector->begin() + insert_at, deps.begin(), deps.end());
+
+  // Clear the origin of each of the insertions. This marks these dependencies
+  // as internally generated.
+  for (size_t i = insert_at; i < deps.size() + insert_at; i++)
+    (*vector)[i].origin = NULL;
+
+  return deps.size();
+}
+
 }  // namespace
 
 Target::Target(const Settings* settings, const Label& label)
@@ -46,6 +73,7 @@ Target::Target(const Settings* settings, const Label& label)
       output_type_(UNKNOWN),
       all_headers_public_(true),
       check_includes_(true),
+      testonly_(false),
       hard_dep_(false),
       toolchain_(NULL) {
 }
@@ -87,22 +115,11 @@ const Target* Target::AsTarget() const {
   return this;
 }
 
-void Target::OnResolved() {
+bool Target::OnResolved(Err* err) {
   DCHECK(output_type_ != UNKNOWN);
   DCHECK(toolchain_) << "Toolchain should have been set before resolving.";
 
-  // Convert any groups we depend on to just direct dependencies on that
-  // group's deps. We insert the new deps immediately after the group so that
-  // the ordering is preserved. We need to keep the original group so that any
-  // flags, etc. that it specifies itself are applied to us.
-  for (size_t i = 0; i < deps_.size(); i++) {
-    const Target* dep = deps_[i].ptr;
-    if (dep->output_type_ == GROUP) {
-      // TODO(brettw) bug 403488 this should also handle datadeps.
-      deps_.insert(deps_.begin() + i + 1, dep->deps_.begin(), dep->deps_.end());
-      i += dep->deps_.size();
-    }
-  }
+  ExpandGroups();
 
   // Copy our own dependent configs to the list of configs applying to us.
   configs_.Append(all_dependent_configs_.begin(), all_dependent_configs_.end());
@@ -130,6 +147,13 @@ void Target::OnResolved() {
   PullRecursiveHardDeps();
 
   FillOutputFiles();
+
+  if (!CheckVisibility(err))
+    return false;
+  if (!CheckTestonly(err))
+    return false;
+
+  return true;
 }
 
 bool Target::IsLinkable() const {
@@ -183,6 +207,19 @@ bool Target::SetToolchain(const Toolchain* toolchain, Err* err) {
   return false;
 }
 
+void Target::ExpandGroups() {
+  // Convert any groups we depend on to just direct dependencies on that
+  // group's deps. We insert the new deps immediately after the group so that
+  // the ordering is preserved. We need to keep the original group so that any
+  // flags, etc. that it specifies itself are applied to us.
+  // TODO(brettw) bug 403488 this should also handle datadeps.
+  for (size_t i = 0; i < deps_.size(); i++) {
+    const Target* dep = deps_[i].ptr;
+    if (dep->output_type_ == GROUP)
+      i += InsertGroupDeps(&deps_, i + 1, dep);
+  }
+}
+
 void Target::PullDependentTargetInfo() {
   // Gather info from our dependents we need.
   for (size_t dep_i = 0; dep_i < deps_.size(); dep_i++) {
@@ -306,3 +343,48 @@ void Target::FillOutputFiles() {
       NOTREACHED();
   }
 }
+
+bool Target::CheckVisibility(Err* err) const {
+  // Only check visibility when the origin of the dependency is non-null. These
+  // are dependencies added by the GN files. Internally added dependencies
+  // (expanded groups) will have a null origin. We don't want to check
+  // visibility for these, since the point of a group would often be to
+  // forward visibility.
+  for (size_t i = 0; i < deps_.size(); i++) {
+    if (deps_[i].origin &&
+        !Visibility::CheckItemVisibility(this, deps_[i].ptr, err))
+      return false;
+  }
+
+  for (size_t i = 0; i < datadeps_.size(); i++) {
+    if (deps_[i].origin &&
+        !Visibility::CheckItemVisibility(this, datadeps_[i].ptr, err))
+      return false;
+  }
+
+  return true;
+}
+
+bool Target::CheckTestonly(Err* err) const {
+  // If there current target is marked testonly, it can include both testonly
+  // and non-testonly targets, so there's nothing to check.
+  if (testonly())
+    return true;
+
+  // Verify no deps have "testonly" set.
+  for (size_t i = 0; i < deps_.size(); i++) {
+    if (deps_[i].ptr->testonly()) {
+      *err = MakeTestOnlyError(this, deps_[i].ptr);
+      return false;
+    }
+  }
+
+  for (size_t i = 0; i < datadeps_.size(); i++) {
+    if (datadeps_[i].ptr->testonly()) {
+      *err = MakeTestOnlyError(this, datadeps_[i].ptr);
+      return false;
+    }
+  }
+
+  return true;
+}

tools/gn/target.h

@@ -53,7 +53,7 @@ class Target : public Item {
   // Item overrides.
   virtual Target* AsTarget() OVERRIDE;
   virtual const Target* AsTarget() const OVERRIDE;
-  virtual void OnResolved() OVERRIDE;
+  virtual bool OnResolved(Err* err) OVERRIDE;
 
   OutputType output_type() const { return output_type_; }
   void set_output_type(OutputType t) { output_type_ = t; }
@@ -95,6 +95,9 @@ class Target : public Item {
   bool check_includes() const { return check_includes_; }
   void set_check_includes(bool ci) { check_includes_ = ci; }
 
+  bool testonly() const { return testonly_; }
+  void set_testonly(bool value) { testonly_ = value; }
+
   // Compile-time extra dependencies.
   const FileList& inputs() const { return inputs_; }
   FileList& inputs() { return inputs_; }
@@ -210,6 +213,8 @@ class Target : public Item {
   }
 
  private:
+  void ExpandGroups();
+
   // Pulls necessary information from dependencies to this one when all
   // dependencies have been resolved.
   void PullDependentTargetInfo();
@@ -222,6 +227,10 @@ class Target : public Item {
   // Fills the link and dependency output files when a target is resolved.
   void FillOutputFiles();
 
+  // Validates the given thing when a target is resolved.
+  bool CheckVisibility(Err* err) const;
+  bool CheckTestonly(Err* err) const;
+
   OutputType output_type_;
   std::string output_name_;
   std::string output_extension_;
@@ -230,6 +239,7 @@ class Target : public Item {
   bool all_headers_public_;
   FileList public_headers_;
   bool check_includes_;
+  bool testonly_;
   FileList inputs_;
   FileList data_;
 

tools/gn/target_generator.cc

@@ -48,6 +48,10 @@ void TargetGenerator::Run() {
   if (err_->has_error())
     return;
 
+  FillTestonly();
+  if (err_->has_error())
+    return;
+
   if (!Visibility::FillItemVisibility(target_, scope_, err_))
     return;
 
@@ -217,6 +221,15 @@ void TargetGenerator::FillDependencies() {
     return;
 }
 
+void TargetGenerator::FillTestonly() {
+  const Value* value = scope_->GetValue(variables::kTestonly, true);
+  if (value) {
+    if (!value->VerifyTypeIs(Value::BOOLEAN, err_))
+      return;
+    target_->set_testonly(value->boolean_value());
+  }
+}
+
 void TargetGenerator::FillOutputs(bool allow_substitutions) {
   const Value* value = scope_->GetValue(variables::kOutputs, true);
   if (!value)

tools/gn/target_generator.h

@@ -70,6 +70,7 @@ class TargetGenerator {
   void FillDependentConfigs();  // Includes all types of dependent configs.
   void FillData();
   void FillDependencies();  // Includes data dependencies.
+  void FillTestonly();
 
   // Reads configs/deps from the given var name, and uses the given setting on
   // the target to save them.

tools/gn/target_unittest.cc

@@ -14,37 +14,47 @@
 // deps.
 TEST(Target, GroupDeps) {
   TestWithScope setup;
+  Err err;
+
+  // Used as the origin for the not-automatically-set deps.
+  IdentifierNode origin;
 
   // Two low-level targets.
   Target x(setup.settings(), Label(SourceDir("//component/"), "x"));
   x.set_output_type(Target::STATIC_LIBRARY);
   x.SetToolchain(setup.toolchain());
-  x.OnResolved();
+  ASSERT_TRUE(x.OnResolved(&err));
   Target y(setup.settings(), Label(SourceDir("//component/"), "y"));
   y.set_output_type(Target::STATIC_LIBRARY);
   y.SetToolchain(setup.toolchain());
-  y.OnResolved();
+  ASSERT_TRUE(y.OnResolved(&err));
 
   // Make a group for both x and y.
   Target g(setup.settings(), Label(SourceDir("//group/"), "g"));
   g.set_output_type(Target::GROUP);
+  g.visibility().SetPublic();
   g.deps().push_back(LabelTargetPair(&x));
+  g.deps()[0].origin = &origin;
   g.deps().push_back(LabelTargetPair(&y));
+  g.deps()[1].origin = &origin;
 
   // Random placeholder target so we can see the group's deps get inserted at
   // the right place.
   Target b(setup.settings(), Label(SourceDir("//app/"), "b"));
   b.set_output_type(Target::STATIC_LIBRARY);
   b.SetToolchain(setup.toolchain());
-  b.OnResolved();
+  b.visibility().SetPublic();
+  ASSERT_TRUE(b.OnResolved(&err));
 
   // Make a target depending on the group and "b". OnResolved will expand.
   Target a(setup.settings(), Label(SourceDir("//app/"), "a"));
   a.set_output_type(Target::EXECUTABLE);
   a.deps().push_back(LabelTargetPair(&g));
+  a.deps()[0].origin = &origin;
   a.deps().push_back(LabelTargetPair(&b));
+  a.deps()[1].origin = &origin;
   a.SetToolchain(setup.toolchain());
-  a.OnResolved();
+  ASSERT_TRUE(a.OnResolved(&err));
 
   // The group's deps should be inserted after the group itself in the deps
   // list, so we should get "g, x, y, b"
@@ -53,12 +63,20 @@ TEST(Target, GroupDeps) {
   EXPECT_EQ(&x, a.deps()[1].ptr);
   EXPECT_EQ(&y, a.deps()[2].ptr);
   EXPECT_EQ(&b, a.deps()[3].ptr);
+
+  // The "regular" deps on a should have the origin set. The automatically
+  // expanded ones will have a null origin so we know they are generated.
+  EXPECT_EQ(&origin, a.deps()[0].origin);
+  EXPECT_EQ(NULL, a.deps()[1].origin);
+  EXPECT_EQ(NULL, a.deps()[2].origin);
+  EXPECT_EQ(&origin, a.deps()[3].origin);
 }
 
 // Tests that lib[_dir]s are inherited across deps boundaries for static
 // libraries but not executables.
 TEST(Target, LibInheritance) {
   TestWithScope setup;
+  Err err;
 
   const std::string lib("foo");
   const SourceDir libdir("/foo_dir/");
@@ -69,7 +87,7 @@ TEST(Target, LibInheritance) {
   z.config_values().libs().push_back(lib);
   z.config_values().lib_dirs().push_back(libdir);
   z.SetToolchain(setup.toolchain());
-  z.OnResolved();
+  ASSERT_TRUE(z.OnResolved(&err));
 
   // All lib[_dir]s should be set when target is resolved.
   ASSERT_EQ(1u, z.all_libs().size());
@@ -87,7 +105,7 @@ TEST(Target, LibInheritance) {
   shared.config_values().lib_dirs().push_back(second_libdir);
   shared.deps().push_back(LabelTargetPair(&z));
   shared.SetToolchain(setup.toolchain());
-  shared.OnResolved();
+  ASSERT_TRUE(shared.OnResolved(&err));
 
   ASSERT_EQ(2u, shared.all_libs().size());
   EXPECT_EQ(second_lib, shared.all_libs()[0]);
@@ -101,7 +119,7 @@ TEST(Target, LibInheritance) {
   exec.set_output_type(Target::EXECUTABLE);
   exec.deps().push_back(LabelTargetPair(&shared));
   exec.SetToolchain(setup.toolchain());
-  exec.OnResolved();
+  ASSERT_TRUE(exec.OnResolved(&err));
   EXPECT_EQ(0u, exec.all_libs().size());
   EXPECT_EQ(0u, exec.all_lib_dirs().size());
 }
@@ -110,6 +128,7 @@ TEST(Target, LibInheritance) {
 // forward_dependent_configs_from
 TEST(Target, DependentConfigs) {
   TestWithScope setup;
+  Err err;
 
   // Set up a dependency chain of a -> b -> c
   Target a(setup.settings(), Label(SourceDir("//foo/"), "a"));
@@ -136,9 +155,9 @@ TEST(Target, DependentConfigs) {
   Config direct(setup.settings(), Label(SourceDir("//foo/"), "direct"));
   c.direct_dependent_configs().push_back(LabelConfigPair(&direct));
 
-  c.OnResolved();
-  b.OnResolved();
-  a.OnResolved();
+  ASSERT_TRUE(c.OnResolved(&err));
+  ASSERT_TRUE(b.OnResolved(&err));
+  ASSERT_TRUE(a.OnResolved(&err));
 
   // B should have gotten both dependent configs from C.
   ASSERT_EQ(2u, b.configs().size());
@@ -163,8 +182,8 @@ TEST(Target, DependentConfigs) {
   b_fwd.deps().push_back(LabelTargetPair(&c));
   b_fwd.forward_dependent_configs().push_back(LabelTargetPair(&c));
 
-  b_fwd.OnResolved();
-  a_fwd.OnResolved();
+  ASSERT_TRUE(b_fwd.OnResolved(&err));
+  ASSERT_TRUE(a_fwd.OnResolved(&err));
 
   // A_fwd should now have both configs.
   ASSERT_EQ(2u, a_fwd.configs().size());
@@ -178,6 +197,7 @@ TEST(Target, DependentConfigs) {
 // group's deps' dependent configs.
 TEST(Target, ForwardDependentConfigsFromGroups) {
   TestWithScope setup;
+  Err err;
 
   Target a(setup.settings(), Label(SourceDir("//foo/"), "a"));
   a.set_output_type(Target::EXECUTABLE);
@@ -198,9 +218,9 @@ TEST(Target, ForwardDependentConfigsFromGroups) {
   // A forwards the dependent configs from B.
   a.forward_dependent_configs().push_back(LabelTargetPair(&b));
 
-  c.OnResolved();
-  b.OnResolved();
-  a.OnResolved();
+  ASSERT_TRUE(c.OnResolved(&err));
+  ASSERT_TRUE(b.OnResolved(&err));
+  ASSERT_TRUE(a.OnResolved(&err));
 
   // The config should now be on A, and in A's direct dependent configs.
   ASSERT_EQ(1u, a.configs().size());
@@ -211,6 +231,7 @@ TEST(Target, ForwardDependentConfigsFromGroups) {
 
 TEST(Target, InheritLibs) {
   TestWithScope setup;
+  Err err;
 
   // Create a dependency chain:
   //   A (executable) -> B (shared lib) -> C (static lib) -> D (source set)
@@ -230,10 +251,10 @@ TEST(Target, InheritLibs) {
   b.deps().push_back(LabelTargetPair(&c));
   c.deps().push_back(LabelTargetPair(&d));
 
-  d.OnResolved();
-  c.OnResolved();
-  b.OnResolved();
-  a.OnResolved();
+  ASSERT_TRUE(d.OnResolved(&err));
+  ASSERT_TRUE(c.OnResolved(&err));
+  ASSERT_TRUE(b.OnResolved(&err));
+  ASSERT_TRUE(a.OnResolved(&err));
 
   // C should have D in its inherited libs.
   const UniqueVector<const Target*>& c_inherited = c.inherited_libraries();
@@ -255,13 +276,14 @@ TEST(Target, InheritLibs) {
 
 TEST(Target, GetComputedOutputName) {
   TestWithScope setup;
+  Err err;
 
   // Basic target with no prefix (executable type tool in the TestWithScope has
   // no prefix) or output name.
   Target basic(setup.settings(), Label(SourceDir("//foo/"), "bar"));
   basic.set_output_type(Target::EXECUTABLE);
   basic.SetToolchain(setup.toolchain());
-  basic.OnResolved();
+  ASSERT_TRUE(basic.OnResolved(&err));
   EXPECT_EQ("bar", basic.GetComputedOutputName(false));
   EXPECT_EQ("bar", basic.GetComputedOutputName(true));
 
@@ -270,7 +292,7 @@ TEST(Target, GetComputedOutputName) {
   with_name.set_output_type(Target::EXECUTABLE);
   with_name.set_output_name("myoutput");
   with_name.SetToolchain(setup.toolchain());
-  with_name.OnResolved();
+  ASSERT_TRUE(with_name.OnResolved(&err));
   EXPECT_EQ("myoutput", with_name.GetComputedOutputName(false));
   EXPECT_EQ("myoutput", with_name.GetComputedOutputName(true));
 
@@ -279,7 +301,7 @@ TEST(Target, GetComputedOutputName) {
   Target with_prefix(setup.settings(), Label(SourceDir("//foo/"), "bar"));
   with_prefix.set_output_type(Target::STATIC_LIBRARY);
   with_prefix.SetToolchain(setup.toolchain());
-  with_prefix.OnResolved();
+  ASSERT_TRUE(with_prefix.OnResolved(&err));
   EXPECT_EQ("bar", with_prefix.GetComputedOutputName(false));
   EXPECT_EQ("libbar", with_prefix.GetComputedOutputName(true));
 
@@ -289,7 +311,94 @@ TEST(Target, GetComputedOutputName) {
   dup_prefix.set_output_type(Target::STATIC_LIBRARY);
   dup_prefix.set_output_name("libbar");
   dup_prefix.SetToolchain(setup.toolchain());
-  dup_prefix.OnResolved();
+  ASSERT_TRUE(dup_prefix.OnResolved(&err));
   EXPECT_EQ("libbar", dup_prefix.GetComputedOutputName(false));
   EXPECT_EQ("libbar", dup_prefix.GetComputedOutputName(true));
 }
+
+// Test visibility failure case.
+TEST(Target, VisibilityFails) {
+  TestWithScope setup;
+  Err err;
+
+  Target b(setup.settings(), Label(SourceDir("//private/"), "b"));
+  b.set_output_type(Target::STATIC_LIBRARY);
+  b.SetToolchain(setup.toolchain());
+  b.visibility().SetPrivate(b.label().dir());
+  ASSERT_TRUE(b.OnResolved(&err));
+
+  // Make a target depending on "b". The dependency must have an origin to mark
+  // it as user-set so we check visibility. This check should fail.
+  Target a(setup.settings(), Label(SourceDir("//app/"), "a"));
+  a.set_output_type(Target::EXECUTABLE);
+  a.deps().push_back(LabelTargetPair(&b));
+  IdentifierNode origin;  // Dummy origin.
+  a.deps()[0].origin = &origin;
+  a.SetToolchain(setup.toolchain());
+  ASSERT_FALSE(a.OnResolved(&err));
+}
+
+// Tests that A -> Group -> B where the group is visible from A but B isn't,
+// passes visibility even though the group's deps get expanded into A.
+TEST(Target, VisibilityGroup) {
+  TestWithScope setup;
+  Err err;
+
+  IdentifierNode origin;  // Dummy origin.
+
+  // B has private visibility. This lets the group see it since the group is in
+  // the same directory.
+  Target b(setup.settings(), Label(SourceDir("//private/"), "b"));
+  b.set_output_type(Target::STATIC_LIBRARY);
+  b.SetToolchain(setup.toolchain());
+  b.visibility().SetPrivate(b.label().dir());
+  ASSERT_TRUE(b.OnResolved(&err));
+
+  // The group has public visibility and depends on b.
+  Target g(setup.settings(), Label(SourceDir("//private/"), "g"));
+  g.set_output_type(Target::GROUP);
+  g.SetToolchain(setup.toolchain());
+  g.deps().push_back(LabelTargetPair(&b));
+  g.deps()[0].origin = &origin;
+  g.visibility().SetPublic();
+  ASSERT_TRUE(b.OnResolved(&err));
+
+  // Make a target depending on "g". This should succeed.
+  Target a(setup.settings(), Label(SourceDir("//app/"), "a"));
+  a.set_output_type(Target::EXECUTABLE);
+  a.deps().push_back(LabelTargetPair(&g));
+  a.deps()[0].origin = &origin;
+  a.SetToolchain(setup.toolchain());
+  ASSERT_TRUE(a.OnResolved(&err));
+}
+
+// Verifies that only testonly targets can depend on other testonly targets.
+// Many of the above dependency checking cases covered the non-testonly
+// case.
+TEST(Target, Testonly) {
+  TestWithScope setup;
+  Err err;
+
+  // "testlib" is a test-only library.
+  Target testlib(setup.settings(), Label(SourceDir("//test/"), "testlib"));
+  testlib.set_testonly(true);
+  testlib.set_output_type(Target::STATIC_LIBRARY);
+  testlib.SetToolchain(setup.toolchain());
+  ASSERT_TRUE(testlib.OnResolved(&err));
+
+  // "test" is a test-only executable depending on testlib, this is OK.
+  Target test(setup.settings(), Label(SourceDir("//test/"), "test"));
+  test.set_testonly(true);
+  test.set_output_type(Target::EXECUTABLE);
+  test.deps().push_back(LabelTargetPair(&testlib));
+  test.SetToolchain(setup.toolchain());
+  ASSERT_TRUE(test.OnResolved(&err));
+
+  // "product" is a non-test depending on testlib. This should fail.
+  Target product(setup.settings(), Label(SourceDir("//app/"), "product"));
+  product.set_testonly(false);
+  product.set_output_type(Target::EXECUTABLE);
+  product.deps().push_back(LabelTargetPair(&testlib));
+  product.SetToolchain(setup.toolchain());
+  ASSERT_FALSE(product.OnResolved(&err));
+}

tools/gn/variables.cc

@@ -795,6 +795,28 @@ const char kSources_Help[] =
     "\n"
     "  A list of files relative to the current buildfile.\n";
 
+const char kTestonly[] = "testonly";
+const char kTestonly_HelpShort[] =
+    "testonly: [boolean] Declares a target must only be used for testing.";
+const char kTestonly_Help[] =
+    "testonly: Declares a target must only be used for testing.\n"
+    "\n"
+    "  Boolean. Defaults to false.\n"
+    "\n"
+    "  When a target is marked \"testonly = true\", it must only be depended\n"
+    "  on by other test-only targets. Otherwise, GN will issue an error\n"
+    "  that the depenedency is not allowed.\n"
+    "\n"
+    "  This feature is intended to prevent accidentally shipping test code\n"
+    "  in a final product.\n"
+    "\n"
+    "Example\n"
+    "\n"
+    "  source_set(\"test_support\") {\n"
+    "    testonly = true\n"
+    "    ...\n"
+    "  }\n";
+
 const char kVisibility[] = "visibility";
 const char kVisibility_HelpShort[] =
     "visibility: [label list] A list of labels that can depend on a target.";
@@ -915,6 +937,7 @@ const VariableInfoMap& GetTargetVariables() {
     INSERT_VARIABLE(Public)
     INSERT_VARIABLE(Script)
     INSERT_VARIABLE(Sources)
+    INSERT_VARIABLE(Testonly)
     INSERT_VARIABLE(Visibility)
   }
   return info_map;

tools/gn/variables.h

@@ -175,6 +175,10 @@ extern const char kSources[];
 extern const char kSources_HelpShort[];
 extern const char kSources_Help[];
 
+extern const char kTestonly[];
+extern const char kTestonly_HelpShort[];
+extern const char kTestonly_Help[];
+
 extern const char kVisibility[];
 extern const char kVisibility_HelpShort[];
 extern const char kVisibility_Help[];