Ninja: Fix inter-target order-only dependencies of custom commands

Custom command dependencies are followed for each target's source files
and add their transitive closure to the corresponding target.  This
means that when a custom command in one target has a dependency on a
custom command in another target, both will appear in the dependent
target's sources.  For the Makefile, VS IDE, and Xcode generators this
is not a problem because each target gets its own independent build
system that is evaluated in target dependency order.  By the time the
dependent target is built the custom command that belongs to one of its
dependencies will already have been brought up to date.

For the Ninja generator we need to generate a monolithic build system
covering all targets so we can have only one copy of a custom command.
This means that we need to reconcile the target-level ordering
dependencies from its appearance in multiple targets to include only the
least-dependent common set.  This is done by computing the set
intersection of the dependencies of all the targets containing a custom
command.  However, we previously included only the direct dependencies
so any target-level dependency not directly added to all targets into
which a custom command propagates was discarded.

Fix this by computing the transitive closure of dependencies for each
target and then intersecting those sets.  That will get the common set
of dependencies.  Also add a test to cover a case in which the
incorrectly dropped target ordering dependencies would fail.

Source/cmGlobalNinjaGenerator.cxx

@@ -493,6 +493,8 @@ void cmGlobalNinjaGenerator::Generate()
   this->OpenBuildFileStream();
   this->OpenRulesFileStream();
 
+  this->TargetDependsClosures.clear();
+
   this->InitOutputPathPrefix();
   this->TargetAll = this->NinjaOutputPath("all");
   this->CMakeCacheFile = this->NinjaOutputPath("CMakeCache.txt");
@@ -905,6 +907,42 @@ void cmGlobalNinjaGenerator::AppendTargetDepends(
   }
 }
 
+void cmGlobalNinjaGenerator::AppendTargetDependsClosure(
+  cmGeneratorTarget const* target, cmNinjaDeps& outputs)
+{
+  TargetDependsClosureMap::iterator i =
+    this->TargetDependsClosures.find(target);
+  if (i == this->TargetDependsClosures.end()) {
+    TargetDependsClosureMap::value_type e(
+      target, std::set<cmGeneratorTarget const*>());
+    i = this->TargetDependsClosures.insert(e).first;
+    this->ComputeTargetDependsClosure(target, i->second);
+  }
+  std::set<cmGeneratorTarget const*> const& targets = i->second;
+  cmNinjaDeps outs;
+  for (std::set<cmGeneratorTarget const*>::const_iterator ti = targets.begin();
+       ti != targets.end(); ++ti) {
+    this->AppendTargetOutputs(*ti, outs);
+  }
+  std::sort(outs.begin(), outs.end());
+  outputs.insert(outputs.end(), outs.begin(), outs.end());
+}
+
+void cmGlobalNinjaGenerator::ComputeTargetDependsClosure(
+  cmGeneratorTarget const* target, std::set<cmGeneratorTarget const*>& depends)
+{
+  cmTargetDependSet const& targetDeps = this->GetTargetDirectDepends(target);
+  for (cmTargetDependSet::const_iterator i = targetDeps.begin();
+       i != targetDeps.end(); ++i) {
+    if ((*i)->GetType() == cmState::INTERFACE_LIBRARY) {
+      continue;
+    }
+    if (depends.insert(*i).second) {
+      this->ComputeTargetDependsClosure(*i, depends);
+    }
+  }
+}
+
 void cmGlobalNinjaGenerator::AddTargetAlias(const std::string& alias,
                                             cmGeneratorTarget* target)
 {

Source/cmGlobalNinjaGenerator.h

@@ -301,6 +301,8 @@ public:
                            cmNinjaDeps& outputs);
   void AppendTargetDepends(cmGeneratorTarget const* target,
                            cmNinjaDeps& outputs);
+  void AppendTargetDependsClosure(cmGeneratorTarget const* target,
+                                  cmNinjaDeps& outputs);
   void AddDependencyToAll(cmGeneratorTarget* target);
   void AddDependencyToAll(const std::string& input);
 
@@ -361,6 +363,10 @@ private:
   void WriteTargetClean(std::ostream& os);
   void WriteTargetHelp(std::ostream& os);
 
+  void ComputeTargetDependsClosure(
+    cmGeneratorTarget const* target,
+    std::set<cmGeneratorTarget const*>& depends);
+
   std::string ninjaCmd() const;
 
   /// The file containing the build statement. (the relationship of the
@@ -410,6 +416,11 @@ private:
   typedef std::map<std::string, cmGeneratorTarget*> TargetAliasMap;
   TargetAliasMap TargetAliases;
 
+  typedef std::map<cmGeneratorTarget const*,
+                   std::set<cmGeneratorTarget const*> >
+    TargetDependsClosureMap;
+  TargetDependsClosureMap TargetDependsClosures;
+
   std::string NinjaCommand;
   std::string NinjaVersion;
 

Source/cmLocalNinjaGenerator.cxx

@@ -450,13 +450,14 @@ void cmLocalNinjaGenerator::WriteCustomCommandBuildStatements()
     std::set<cmGeneratorTarget*>::iterator j = i->second.begin();
     assert(j != i->second.end());
     std::vector<std::string> ccTargetDeps;
-    this->AppendTargetDepends(*j, ccTargetDeps);
+    this->GetGlobalNinjaGenerator()->AppendTargetDependsClosure(*j,
+                                                                ccTargetDeps);
     std::sort(ccTargetDeps.begin(), ccTargetDeps.end());
     ++j;
 
     for (; j != i->second.end(); ++j) {
       std::vector<std::string> jDeps, depsIntersection;
-      this->AppendTargetDepends(*j, jDeps);
+      this->GetGlobalNinjaGenerator()->AppendTargetDependsClosure(*j, jDeps);
       std::sort(jDeps.begin(), jDeps.end());
       std::set_intersection(ccTargetDeps.begin(), ccTargetDeps.end(),
                             jDeps.begin(), jDeps.end(),

Tests/RunCMake/add_custom_target/RunCMakeTest.cmake

@@ -4,3 +4,17 @@ run_cmake(NoArguments)
 run_cmake(BadTargetName)
 run_cmake(ByproductsNoCommand)
 run_cmake(UsesTerminalNoCommand)
+
+function(run_TargetOrder)
+  # Use a single build tree for a few tests without cleaning.
+  set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/TargetOrder-build)
+  set(RunCMake_TEST_NO_CLEAN 1)
+  file(REMOVE_RECURSE "${RunCMake_TEST_BINARY_DIR}")
+  file(MAKE_DIRECTORY "${RunCMake_TEST_BINARY_DIR}")
+  run_cmake(TargetOrder)
+  if(RunCMake_GENERATOR STREQUAL "Ninja")
+    set(build_flags -j 1 -v)
+  endif()
+  run_cmake_command(TargetOrder-build ${CMAKE_COMMAND} --build . -- ${build_flags})
+endfunction()
+run_TargetOrder()

Tests/RunCMake/add_custom_target/TargetOrder.cmake

@@ -0,0 +1,31 @@
+# Add a target that requires step1 to run first but enforces
+# it only by target-level ordering dependency.
+add_custom_command(
+  OUTPUT step2.txt
+  COMMAND ${CMAKE_COMMAND} -E copy step1.txt step2.txt
+  )
+add_custom_target(step2 DEPENDS step2.txt)
+add_dependencies(step2 step1)
+
+# Add a target that requires step1 and step2 to work,
+# only depends on step1 transitively through step2, but
+# also gets a copy of step2's custom command.
+# The Ninja generator in particular must be careful with
+# this case because it needs to compute the proper set of
+# target ordering dependencies for the step2 custom command
+# even though it appears in both the step2 and step3
+# targets due to dependency propagation.
+add_custom_command(
+  OUTPUT step3.txt
+  COMMAND ${CMAKE_COMMAND} -E copy step1.txt step3-1.txt
+  COMMAND ${CMAKE_COMMAND} -E copy step2.txt step3.txt
+  DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/step2.txt
+  )
+add_custom_target(step3 ALL DEPENDS step3.txt)
+add_dependencies(step3 step2)
+
+# We want this target to always run first.  Add it last so
+# that serial builds require dependencies to order it first.
+add_custom_target(step1
+  COMMAND ${CMAKE_COMMAND} -E touch step1.txt
+  )