Don't add implicit deps to copy commands in GN.

As explained in the long comment in ninja_copy_target_writer.cc, writing implicit deps for the copy command is both unnecessary and causes problems given the way Chrome specifies the command.

This also updates the expectations for the binary output given the new variables added in a previous patch (this test isn't being run on the bot yet).

R=scottmg@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@285052 0039d316-1c4b-4281-b951-d872f2087c98

tools/gn/function_toolchain.cc

@@ -159,7 +159,7 @@ const char kTool_Help[] =
     "  Used inside a toolchain definition to define a command to run for a\n"
     "  given file type. See also \"gn help toolchain\".\n"
     "\n"
-    "Command types:\n"
+    "Command types\n"
     "\n"
     "  The following values may be passed to the tool() function for the type\n"
     "  of the command:\n"
@@ -167,6 +167,18 @@ const char kTool_Help[] =
     "  \"cc\", \"cxx\", \"objc\", \"objcxx\", \"asm\", \"alink\", \"solink\",\n"
     "  \"link\", \"stamp\", \"copy\"\n"
     "\n"
+    "Tool-specific notes\n"
+    "\n"
+    "  copy\n"
+    "    The copy command should be a native OS command since it does not\n"
+    "    implement toolchain dependencies (which would enable a copy tool to\n"
+    "    be compiled by a previous step).\n"
+    "\n"
+    "    It is legal for the copy to not update the timestamp of the output\n"
+    "    file (as long as it's greater than or equal to the input file). This\n"
+    "    allows the copy command to be implemented as a hard link which can\n"
+    "    be more efficient.\n"
+    "\n"
     "Command flags\n"
     "\n"
     "  These variables may be specified in the { } block after the tool call.\n"

tools/gn/ninja_binary_target_writer_unittest.cc

@@ -39,6 +39,9 @@ TEST(NinjaBinaryTargetWriter, SourceSet) {
         "cflags_cc =\n"
         "cflags_objc =\n"
         "cflags_objcc =\n"
+        "target_name = bar\n"
+        "target_out_dir = obj/foo\n"
+        "root_out_dir = \n"
         "\n"
         "build obj/foo/bar.input1.obj: cxx ../../foo/input1.cc\n"
         "build obj/foo/bar.input2.obj: cxx ../../foo/input2.cc\n"
@@ -73,6 +76,9 @@ TEST(NinjaBinaryTargetWriter, SourceSet) {
         "cflags_cc =\n"
         "cflags_objc =\n"
         "cflags_objcc =\n"
+        "target_name = shlib\n"
+        "target_out_dir = obj/foo\n"
+        "root_out_dir = \n"
         "\n"
         "\n"
         "manifests = obj/foo/shlib.intermediate.manifest\n"
@@ -116,6 +122,9 @@ TEST(NinjaBinaryTargetWriter, SourceSet) {
         "cflags_cc =\n"
         "cflags_objc =\n"
         "cflags_objcc =\n"
+        "target_name = stlib\n"
+        "target_out_dir = obj/foo\n"
+        "root_out_dir = \n"
         "\n"
         "\n"
         "manifests = obj/foo/stlib.intermediate.manifest\n"
@@ -159,6 +168,9 @@ TEST(NinjaBinaryTargetWriter, ProductExtension) {
       "cflags_cc =\n"
       "cflags_objc =\n"
       "cflags_objcc =\n"
+      "target_name = shlib\n"
+      "target_out_dir = obj/foo\n"
+      "root_out_dir = \n"
       "\n"
       "build obj/foo/shlib.input1.o: cxx ../../foo/input1.cc\n"
       "build obj/foo/shlib.input2.o: cxx ../../foo/input2.cc\n"
@@ -205,6 +217,9 @@ TEST(NinjaBinaryTargetWriter, EmptyProductExtension) {
       "cflags_cc =\n"
       "cflags_objc =\n"
       "cflags_objcc =\n"
+      "target_name = shlib\n"
+      "target_out_dir = obj/foo\n"
+      "root_out_dir = \n"
       "\n"
       "build obj/foo/shlib.input1.o: cxx ../../foo/input1.cc\n"
       "build obj/foo/shlib.input2.o: cxx ../../foo/input2.cc\n"

tools/gn/ninja_copy_target_writer.cc

@@ -25,9 +25,27 @@ void NinjaCopyTargetWriter::Run() {
 
   std::string rule_prefix = helper_.GetRulePrefix(target_->settings());
 
-  std::string implicit_deps =
-      WriteInputDepsStampAndGetDep(std::vector<const Target*>());
-
+  // Note that we don't write implicit deps for copy steps. "copy" only
+  // depends on the output files themselves, rather than having includes
+  // (the possibility of generated #includes is the main reason for implicit
+  // dependencies).
+  //
+  // It would seem that specifying implicit dependencies on the deps of the
+  // copy command would still be harmeless. But Chrome implements copy tools
+  // as hard links (much faster) which don't change the timestamp. If the
+  // ninja rule looks like this:
+  //   output: copy input | foo.stamp
+  // The copy will not make a new timestamp on the output file, but the
+  // foo.stamp file generated from a previous step will have a new timestamp.
+  // The copy rule will therefore look out-of-date to Ninja and the rule will
+  // get rebuilt.
+  //
+  // If this copy is copying a generated file, not listing the implicit
+  // dependency will be fine as long as the input to the copy is properly
+  // listed as the output from the step that generated it.
+  //
+  // Moreover, doing this assumes that the copy step is always a simple
+  // locally run command, so there is no need for a toolchain dependency.
   for (size_t i = 0; i < target_->sources().size(); i++) {
     const SourceFile& input_file = target_->sources()[i];
 
@@ -47,7 +65,7 @@ void NinjaCopyTargetWriter::Run() {
     path_output_.WriteFile(out_, output_file);
     out_ << ": " << rule_prefix << "copy ";
     path_output_.WriteFile(out_, input_file);
-    out_ << implicit_deps << std::endl;
+    out_ << std::endl;
   }
 
   // Write out the rule for the target to copy all of them.

tools/gn/ninja_copy_target_writer_unittest.cc

@@ -37,7 +37,7 @@ TEST(NinjaCopyTargetWriter, Run) {
   EXPECT_EQ(expected_linux, out_str);
 }
 
-// Tests a single file with no output pattern and a toolchain dependency.
+// Tests a single file with no output pattern.
 TEST(NinjaCopyTargetWriter, ToolchainDeps) {
   TestWithScope setup;
   setup.settings()->set_target_os(Settings::LINUX);
@@ -45,15 +45,6 @@ TEST(NinjaCopyTargetWriter, ToolchainDeps) {
   Target target(setup.settings(), Label(SourceDir("//foo/"), "bar"));
   target.set_output_type(Target::COPY_FILES);
 
-  // Toolchain dependency. Here we make a target in the same toolchain for
-  // simplicity, but in real life (using the Builder) this would be rejected
-  // because it would be a circular dependency (the target depends on its
-  // toolchain, and the toolchain depends on this target).
-  Target toolchain_dep_target(setup.settings(),
-                              Label(SourceDir("//foo/"), "setup"));
-  toolchain_dep_target.set_output_type(Target::ACTION);
-  setup.toolchain()->deps().push_back(LabelTargetPair(&toolchain_dep_target));
-
   target.sources().push_back(SourceFile("//foo/input1.txt"));
 
   target.action_values().outputs().push_back("//out/Debug/output.out");
@@ -63,9 +54,7 @@ TEST(NinjaCopyTargetWriter, ToolchainDeps) {
   writer.Run();
 
   const char expected_linux[] =
-      "build obj/foo/bar.inputdeps.stamp: stamp obj/foo/setup.stamp\n"
-      "build output.out: copy ../../foo/input1.txt | "
-          "obj/foo/bar.inputdeps.stamp\n"
+      "build output.out: copy ../../foo/input1.txt\n"
       "\n"
       "build obj/foo/bar.stamp: stamp output.out\n";
   std::string out_str = out.str();