Merge topic 'ninja-target-deps'

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

5 files changed, 97 insertions, 2 deletions

diff --git a/Source/cmGlobalNinjaGenerator.cxx b/Source/cmGlobalNinjaGenerator.cxx
index 91f08e6..51175c7 100644
--- a/Source/cmGlobalNinjaGenerator.cxx
+++ b/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)
 {
diff --git a/Source/cmGlobalNinjaGenerator.h b/Source/cmGlobalNinjaGenerator.h
index c74973e..52fa5c9 100644
--- a/Source/cmGlobalNinjaGenerator.h
+++ b/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;
 
diff --git a/Source/cmLocalNinjaGenerator.cxx b/Source/cmLocalNinjaGenerator.cxx
index 0f488a6..46d7e18 100644
--- a/Source/cmLocalNinjaGenerator.cxx
+++ b/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(),
diff --git a/Tests/RunCMake/add_custom_target/RunCMakeTest.cmake b/Tests/RunCMake/add_custom_target/RunCMakeTest.cmake
index 92c4a38..6c4e91b 100644
--- a/Tests/RunCMake/add_custom_target/RunCMakeTest.cmake
+++ b/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()
diff --git a/Tests/RunCMake/add_custom_target/TargetOrder.cmake b/Tests/RunCMake/add_custom_target/TargetOrder.cmake
new file mode 100644
index 0000000..21669c0
--- /dev/null
+++ b/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
+  )