Reduce input dependencies in GN.

Converts input dependencies for binary targets to be order-only, rather than implicit. See added comments in the patch for reasoning behind this.

Long ago this was order-only, which is not correct for actions (which might directly depend on the output of previous actions). So I changed them to be implicit, which then caused unnecessary rebuilds. This change makes actions and binary targets behave differently. Actions remain implicit, binary targets are order-only.

Add unit test for input deps on binary targets.

R=scottmg@chromium.org

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

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

tools/gn/ninja_action_target_writer.cc

@@ -40,7 +40,7 @@ void NinjaActionTargetWriter::Run() {
   // build rule. This should probably be handled by WriteInputDepsStampAndGetDep
   // automatically if we supply a count of sources (so it can optimize based on
   // how many times things would be duplicated).
-  std::string implicit_deps = WriteInputDepsStampAndGetDep(extra_hard_deps);
+  OutputFile input_dep = WriteInputDepsStampAndGetDep(extra_hard_deps);
   out_ << std::endl;
 
   // Collects all output files for writing below.
@@ -48,7 +48,7 @@ void NinjaActionTargetWriter::Run() {
 
   if (target_->output_type() == Target::ACTION_FOREACH) {
     // Write separate build lines for each input source file.
-    WriteSourceRules(custom_rule_name, implicit_deps, &output_files);
+    WriteSourceRules(custom_rule_name, input_dep, &output_files);
   } else {
     DCHECK(target_->output_type() == Target::ACTION);
 
@@ -59,7 +59,14 @@ void NinjaActionTargetWriter::Run() {
         settings_, target_->action_values().outputs(), &output_files);
     path_output_.WriteFiles(out_, output_files);
 
-    out_ << ": " << custom_rule_name << implicit_deps << std::endl;
+    out_ << ": " << custom_rule_name;
+    if (!input_dep.value().empty()) {
+      // As in WriteSourceRules, we want to force this target to rebuild any
+      // time any of its dependencies change.
+      out_ << " | ";
+      path_output_.WriteFile(out_, input_dep);
+    }
+    out_ << std::endl;
     if (target_->action_values().has_depfile()) {
       out_ << "  depfile = ";
       WriteDepfile(SourceFile());
@@ -144,7 +151,7 @@ std::string NinjaActionTargetWriter::WriteRuleDefinition() {
 
 void NinjaActionTargetWriter::WriteSourceRules(
     const std::string& custom_rule_name,
-    const std::string& implicit_deps,
+    const OutputFile& input_dep,
     std::vector<OutputFile>* output_files) {
   EscapeOptions args_escape_options;
   args_escape_options.mode = ESCAPE_NINJA_COMMAND;
@@ -162,7 +169,15 @@ void NinjaActionTargetWriter::WriteSourceRules(
 
     out_ << ": " << custom_rule_name << " ";
     path_output_.WriteFile(out_, sources[i]);
-    out_ << implicit_deps << std::endl;
+    if (!input_dep.value().empty()) {
+      // Using "|" for the dependencies forces all implicit dependencies to be
+      // fully up-to-date before running the action, and will re-run this
+      // action if any input dependencies change. This is important because
+      // this action may consume the outputs of previous steps.
+      out_ << " | ";
+      path_output_.WriteFile(out_, input_dep);
+    }
+    out_ << std::endl;
 
     // Windows needs a unique ID for the response file.
     if (target_->settings()->IsWin())

tools/gn/ninja_action_target_writer.h

@@ -39,10 +39,10 @@ class NinjaActionTargetWriter : public NinjaTargetWriter {
   // Writes the rules for compiling each source, writing all output files
   // to the given vector.
   //
-  // implicit_deps is a precomputed string of all ninja files that are common
-  // to each build step, it starts with a "|" if it's nonempty.
+  // input_dep is a file expressing the depencies common to all build steps.
+  // It will be a stamp file if there is more than one.
   void WriteSourceRules(const std::string& custom_rule_name,
-                        const std::string& implicit_deps,
+                        const OutputFile& input_dep,
                         std::vector<OutputFile>* output_files);
 
   // Writes the output files generated by the output template for the given

tools/gn/ninja_binary_target_writer.cc

@@ -127,7 +127,7 @@ void NinjaBinaryTargetWriter::WriteSources(
   const Target::FileList& sources = target_->sources();
   object_files->reserve(sources.size());
 
-  std::string implicit_deps =
+  OutputFile input_dep =
       WriteInputDepsStampAndGetDep(std::vector<const Target*>());
 
   std::string rule_prefix = GetNinjaRulePrefixForToolchain(settings_);
@@ -145,7 +145,35 @@ void NinjaBinaryTargetWriter::WriteSources(
       out_ << ": " << rule_prefix << Toolchain::ToolTypeToName(tool_type);
       out_ << " ";
       path_output_.WriteFile(out_, sources[i]);
-      out_ << implicit_deps << std::endl;
+      if (!input_dep.value().empty()) {
+        // Write out the input dependencies as an order-only dependency. This
+        // will cause Ninja to make sure the inputs are up-to-date before
+        // compiling this source, but changes in the inputs deps won't cause
+        // the file to be recompiled.
+        //
+        // This is important to prevent changes in unrelated actions that
+        // are upstream of this target from causing everything to be recompiled.
+        //
+        // Why can we get away with this rather than using implicit deps ("|",
+        // which will force rebuilds when the inputs change)?  For source code,
+        // the computed dependencies of all headers will be computed by the
+        // compiler, which will cause source rebuilds if any "real" upstream
+        // dependencies change.
+        //
+        // If a .cc file is generated by an input dependency, Ninja will see
+        // the input to the build rule doesn't exist, and that it is an output
+        // from a previous step, and build the previous step first.  This is a
+        // "real" dependency and doesn't need | or || to express.
+        //
+        // The only case where this rule matters is for the first build where
+        // no .d files exist, and Ninja doesn't know what that source file
+        // depends on. In this case it's sufficient to ensure that the upstream
+        // dependencies are built first. This is exactly what Ninja's order-
+        // only dependencies expresses.
+        out_ << " || ";
+        path_output_.WriteFile(out_, input_dep);
+      }
+      out_ << std::endl;
     }
 
     // It's theoretically possible for a compiler to produce more than one
@@ -215,6 +243,18 @@ void NinjaBinaryTargetWriter::WriteLinkerStuff(
   }
 
   // Append data dependencies as order-only dependencies.
+  //
+  // This will include data dependencies and input dependencies (like when
+  // this target depends on an action). Having the data dependencies in this
+  // list ensures that the data is available at runtime when the user builds
+  // this target.
+  //
+  // The action dependencies are not strictly necessary in this case. They
+  // should also have been collected via the input deps stamp that each source
+  // file has for an order-only dependency, and since this target depends on
+  // the sources, there is already an implicit order-only dependency. However,
+  // it's extra work to separate these out and there's no disadvantage to
+  // listing them again.
   WriteOrderOnlyDependencies(non_linkable_deps);
 
   // End of the link "build" line.

tools/gn/ninja_binary_target_writer_unittest.cc

@@ -125,17 +125,26 @@ TEST(NinjaBinaryTargetWriter, SourceSet) {
   }
 }
 
-TEST(NinjaBinaryTargetWriter, ProductExtension) {
+// This tests that output extension overrides apply, and input dependencies
+// are applied.
+TEST(NinjaBinaryTargetWriter, ProductExtensionAndInputDeps) {
   TestWithScope setup;
   setup.build_settings()->SetBuildDir(SourceDir("//out/Debug/"));
   setup.settings()->set_target_os(Settings::LINUX);
 
+  // An action for our library to depend on.
+  Target action(setup.settings(), Label(SourceDir("//foo/"), "action"));
+  action.set_output_type(Target::ACTION_FOREACH);
+  action.SetToolchain(setup.toolchain());
+  action.OnResolved();
+
   // A shared library w/ the product_extension set to a custom value.
   Target target(setup.settings(), Label(SourceDir("//foo/"), "shlib"));
   target.set_output_type(Target::SHARED_LIBRARY);
   target.set_output_extension(std::string("so.6"));
   target.sources().push_back(SourceFile("//foo/input1.cc"));
   target.sources().push_back(SourceFile("//foo/input2.cc"));
+  target.deps().push_back(LabelTargetPair(&action));
   target.SetToolchain(setup.toolchain());
   target.OnResolved();
 
@@ -155,11 +164,17 @@ TEST(NinjaBinaryTargetWriter, ProductExtension) {
       "target_out_dir = obj/foo\n"
       "target_output_name = libshlib\n"
       "\n"
-      "build obj/foo/libshlib.input1.o: cxx ../../foo/input1.cc\n"
-      "build obj/foo/libshlib.input2.o: cxx ../../foo/input2.cc\n"
+      "build obj/foo/shlib.inputdeps.stamp: stamp obj/foo/action.stamp\n"
+      "build obj/foo/libshlib.input1.o: cxx ../../foo/input1.cc"
+        " || obj/foo/shlib.inputdeps.stamp\n"
+      "build obj/foo/libshlib.input2.o: cxx ../../foo/input2.cc"
+        " || obj/foo/shlib.inputdeps.stamp\n"
       "\n"
       "build ./libshlib.so.6: solink obj/foo/libshlib.input1.o "
-          "obj/foo/libshlib.input2.o\n"
+      // The order-only dependency here is stricly unnecessary since the
+      // sources list this as an order-only dep. See discussion in the code
+      // that writes this.
+          "obj/foo/libshlib.input2.o || obj/foo/action.stamp\n"
       "  ldflags =\n"
       "  libs =\n"
       "  output_extension = .so.6\n";

tools/gn/ninja_target_writer.cc

@@ -16,6 +16,7 @@
 #include "tools/gn/ninja_copy_target_writer.h"
 #include "tools/gn/ninja_group_target_writer.h"
 #include "tools/gn/ninja_utils.h"
+#include "tools/gn/output_file.h"
 #include "tools/gn/scheduler.h"
 #include "tools/gn/string_utils.h"
 #include "tools/gn/substitution_writer.h"
@@ -142,7 +143,7 @@ void NinjaTargetWriter::WriteSharedVars(const SubstitutionBits& bits) {
     out_ << std::endl;
 }
 
-std::string NinjaTargetWriter::WriteInputDepsStampAndGetDep(
+OutputFile NinjaTargetWriter::WriteInputDepsStampAndGetDep(
     const std::vector<const Target*>& extra_hard_deps) const {
   CHECK(target_->toolchain())
       << "Toolchain not set on target "
@@ -163,7 +164,7 @@ std::string NinjaTargetWriter::WriteInputDepsStampAndGetDep(
       target_->recursive_hard_deps().empty() &&
       (!list_sources_as_input_deps || target_->sources().empty()) &&
       target_->toolchain()->deps().empty())
-    return std::string();  // No input/hard deps.
+    return OutputFile();  // No input/hard deps.
 
   // One potential optimization is if there are few input dependencies (or
   // potentially few sources that depend on these) it's better to just write
@@ -178,11 +179,9 @@ std::string NinjaTargetWriter::WriteInputDepsStampAndGetDep(
   input_stamp_file.value().append(target_->label().name());
   input_stamp_file.value().append(".inputdeps.stamp");
 
-  std::ostringstream stamp_file_stream;
-  path_output_.WriteFile(stamp_file_stream, input_stamp_file);
-  std::string stamp_file_string = stamp_file_stream.str();
-
-  out_ << "build " << stamp_file_string << ": "
+  out_ << "build ";
+  path_output_.WriteFile(out_, input_stamp_file);
+  out_ << ": "
        << GetNinjaRulePrefixForToolchain(settings_)
        << Toolchain::ToolTypeToName(Toolchain::TYPE_STAMP);
 
@@ -236,7 +235,7 @@ std::string NinjaTargetWriter::WriteInputDepsStampAndGetDep(
   }
 
   out_ << "\n";
-  return " | " + stamp_file_string;
+  return input_stamp_file;
 }
 
 void NinjaTargetWriter::WriteStampForTarget(

tools/gn/ninja_target_writer.h

@@ -12,6 +12,7 @@
 #include "tools/gn/substitution_type.h"
 
 class FileTemplate;
+class OutputFile;
 class Settings;
 class Target;
 
@@ -33,12 +34,11 @@ class NinjaTargetWriter {
   void WriteSharedVars(const SubstitutionBits& bits);
 
   // Writes to the output stream a stamp rule for input dependencies, and
-  // returns the string to be appended to source rules that encodes the
-  // order-only dependencies for the current target. This will include the "|"
-  // character so can just be appended to the source rules. If there are no
-  // implicit dependencies and no extra target dependencies passed in, returns
-  // the empty string.
-  std::string WriteInputDepsStampAndGetDep(
+  // returns the file to be appended to source rules that encodes the
+  // order-only dependencies for the current target. The returned OutputFile
+  // will be empty if there are no implicit dependencies and no extra target
+  // dependencies passed in.
+  OutputFile WriteInputDepsStampAndGetDep(
       const std::vector<const Target*>& extra_hard_deps) const;
 
   // Writes to the output file a final stamp rule for the target that stamps

tools/gn/ninja_target_writer_unittest.cc

@@ -22,7 +22,7 @@ class TestingNinjaTargetWriter : public NinjaTargetWriter {
   virtual void Run() OVERRIDE {}
 
   // Make this public so the test can call it.
-  std::string WriteInputDepsStampAndGetDep(
+  OutputFile WriteInputDepsStampAndGetDep(
       const std::vector<const Target*>& extra_hard_deps) {
     return NinjaTargetWriter::WriteInputDepsStampAndGetDep(extra_hard_deps);
   }
@@ -65,10 +65,10 @@ TEST(NinjaTargetWriter, WriteInputDepsStampAndGetDep) {
   {
     std::ostringstream stream;
     TestingNinjaTargetWriter writer(&base_target, setup.toolchain(), stream);
-    std::string dep =
+    OutputFile dep =
         writer.WriteInputDepsStampAndGetDep(std::vector<const Target*>());
 
-    EXPECT_EQ(" | obj/foo/base.inputdeps.stamp", dep);
+    EXPECT_EQ("obj/foo/base.inputdeps.stamp", dep.value());
     EXPECT_EQ("build obj/foo/base.inputdeps.stamp: stamp "
                   "../../foo/script.py\n",
               stream.str());
@@ -78,10 +78,10 @@ TEST(NinjaTargetWriter, WriteInputDepsStampAndGetDep) {
   {
     std::ostringstream stream;
     TestingNinjaTargetWriter writer(&target, setup.toolchain(), stream);
-    std::string dep =
+    OutputFile dep =
         writer.WriteInputDepsStampAndGetDep(std::vector<const Target*>());
 
-    EXPECT_EQ(" | obj/foo/target.inputdeps.stamp", dep);
+    EXPECT_EQ("obj/foo/target.inputdeps.stamp", dep.value());
     EXPECT_EQ("build obj/foo/target.inputdeps.stamp: stamp "
                   "../../foo/input.txt obj/foo/base.stamp\n",
               stream.str());
@@ -92,10 +92,10 @@ TEST(NinjaTargetWriter, WriteInputDepsStampAndGetDep) {
   {
     std::ostringstream stream;
     TestingNinjaTargetWriter writer(&action, setup.toolchain(), stream);
-    std::string dep =
+    OutputFile dep =
         writer.WriteInputDepsStampAndGetDep(std::vector<const Target*>());
 
-    EXPECT_EQ(" | obj/foo/action.inputdeps.stamp", dep);
+    EXPECT_EQ("obj/foo/action.inputdeps.stamp", dep.value());
     EXPECT_EQ("build obj/foo/action.inputdeps.stamp: stamp ../../foo/script.py "
                   "../../foo/action_source.txt obj/foo/base.stamp\n",
               stream.str());
@@ -125,10 +125,10 @@ TEST(NinjaTargetWriter, WriteInputDepsStampAndGetDepWithToolchainDeps) {
 
   std::ostringstream stream;
   TestingNinjaTargetWriter writer(&target, setup.toolchain(), stream);
-  std::string dep =
+  OutputFile dep =
       writer.WriteInputDepsStampAndGetDep(std::vector<const Target*>());
 
-  EXPECT_EQ(" | obj/foo/target.inputdeps.stamp", dep);
+  EXPECT_EQ("obj/foo/target.inputdeps.stamp", dep.value());
   EXPECT_EQ("build obj/foo/target.inputdeps.stamp: stamp "
                 "obj/foo/setup.stamp\n",
             stream.str());