Merge topic 'loosen-object-deps'

d96e5d9a Tests: use BYPRODUCTS in the CustomCommandWorkingDirectory test
664591ce RunCMake.Ninja: add a test for assumed sources
adf60b28 ninja: break unnecessary target dependencies
01c5bb95 RunCMake.Ninja: support passing arguments when running ninja
7f947b60 ninja: remove duplicate order-only dependencies
e9827eba ninja: describe the intermediate order depends target better
b57b7d8e Ninja: Order Fortran dyndep file generation explicitly

Acked-by: Kitware Robot <kwrobot@kitware.com>
Merge-request: !430

Help/release/dev/ninja-loosen-object-deps.rst

@@ -0,0 +1,8 @@
+ninja-loosen-object-deps
+------------------------
+
+* The :generator:`Ninja` generator has loosened dependencies on object
+  compilation to depend on the custom targets and commands of dependent
+  libraries instead of the libraries themselves. This helps projects with deep
+  dependency graphs to be blocked only on their link steps at the deeper
+  levels rather than also blocking object compilation on dependent link steps.

Source/cmGlobalNinjaGenerator.cxx

@@ -966,8 +966,14 @@ void cmGlobalNinjaGenerator::WriteAssumedSourceDependencies()
   }
 }
 
+std::string OrderDependsTargetForTarget(cmGeneratorTarget const* target)
+{
+  return "cmake_object_order_depends_target_" + target->GetName();
+}
+
 void cmGlobalNinjaGenerator::AppendTargetOutputs(
-  cmGeneratorTarget const* target, cmNinjaDeps& outputs)
+  cmGeneratorTarget const* target, cmNinjaDeps& outputs,
+  cmNinjaTargetDepends depends)
 {
   std::string configName =
     target->Target->GetMakefile()->GetSafeDefinition("CMAKE_BUILD_TYPE");
@@ -979,15 +985,27 @@ void cmGlobalNinjaGenerator::AppendTargetOutputs(
   bool realname = target->IsFrameworkOnApple();
 
   switch (target->GetType()) {
-    case cmStateEnums::EXECUTABLE:
     case cmStateEnums::SHARED_LIBRARY:
     case cmStateEnums::STATIC_LIBRARY:
     case cmStateEnums::MODULE_LIBRARY: {
+      if (depends == DependOnTargetOrdering) {
+        outputs.push_back(OrderDependsTargetForTarget(target));
+        break;
+      }
+    }
+    // FALLTHROUGH
+    case cmStateEnums::EXECUTABLE: {
       outputs.push_back(this->ConvertToNinjaPath(target->GetFullPath(
         configName, cmStateEnums::RuntimeBinaryArtifact, realname)));
       break;
     }
-    case cmStateEnums::OBJECT_LIBRARY:
+    case cmStateEnums::OBJECT_LIBRARY: {
+      if (depends == DependOnTargetOrdering) {
+        outputs.push_back(OrderDependsTargetForTarget(target));
+        break;
+      }
+    }
+    // FALLTHROUGH
     case cmStateEnums::GLOBAL_TARGET:
     case cmStateEnums::UTILITY: {
       std::string path =
@@ -1003,7 +1021,8 @@ void cmGlobalNinjaGenerator::AppendTargetOutputs(
 }
 
 void cmGlobalNinjaGenerator::AppendTargetDepends(
-  cmGeneratorTarget const* target, cmNinjaDeps& outputs)
+  cmGeneratorTarget const* target, cmNinjaDeps& outputs,
+  cmNinjaTargetDepends depends)
 {
   if (target->GetType() == cmStateEnums::GLOBAL_TARGET) {
     // These depend only on other CMake-provided targets, e.g. "all".
@@ -1023,7 +1042,7 @@ void cmGlobalNinjaGenerator::AppendTargetDepends(
       if ((*i)->GetType() == cmStateEnums::INTERFACE_LIBRARY) {
         continue;
       }
-      this->AppendTargetOutputs(*i, outs);
+      this->AppendTargetOutputs(*i, outs, depends);
     }
     std::sort(outs.begin(), outs.end());
     outputs.insert(outputs.end(), outs.begin(), outs.end());

Source/cmGlobalNinjaGenerator.h

@@ -316,10 +316,12 @@ public:
     ASD.insert(deps.begin(), deps.end());
   }
 
-  void AppendTargetOutputs(cmGeneratorTarget const* target,
-                           cmNinjaDeps& outputs);
-  void AppendTargetDepends(cmGeneratorTarget const* target,
-                           cmNinjaDeps& outputs);
+  void AppendTargetOutputs(
+    cmGeneratorTarget const* target, cmNinjaDeps& outputs,
+    cmNinjaTargetDepends depends = DependOnTargetArtifact);
+  void AppendTargetDepends(
+    cmGeneratorTarget const* target, cmNinjaDeps& outputs,
+    cmNinjaTargetDepends depends = DependOnTargetArtifact);
   void AppendTargetDependsClosure(cmGeneratorTarget const* target,
                                   cmNinjaDeps& outputs);
   void AddDependencyToAll(cmGeneratorTarget* target);

Source/cmLocalNinjaGenerator.cxx

@@ -281,9 +281,11 @@ void cmLocalNinjaGenerator::AppendTargetOutputs(cmGeneratorTarget* target,
 }
 
 void cmLocalNinjaGenerator::AppendTargetDepends(cmGeneratorTarget* target,
-                                                cmNinjaDeps& outputs)
+                                                cmNinjaDeps& outputs,
+                                                cmNinjaTargetDepends depends)
 {
-  this->GetGlobalNinjaGenerator()->AppendTargetDepends(target, outputs);
+  this->GetGlobalNinjaGenerator()->AppendTargetDepends(target, outputs,
+                                                       depends);
 }
 
 void cmLocalNinjaGenerator::AppendCustomCommandDeps(

Source/cmLocalNinjaGenerator.h

@@ -63,7 +63,9 @@ public:
   std::string BuildCommandLine(const std::vector<std::string>& cmdLines);
 
   void AppendTargetOutputs(cmGeneratorTarget* target, cmNinjaDeps& outputs);
-  void AppendTargetDepends(cmGeneratorTarget* target, cmNinjaDeps& outputs);
+  void AppendTargetDepends(
+    cmGeneratorTarget* target, cmNinjaDeps& outputs,
+    cmNinjaTargetDepends depends = DependOnTargetArtifact);
 
   void AddCustomCommandTarget(cmCustomCommand const* cc,
                               cmGeneratorTarget* target);

Source/cmNinjaTargetGenerator.cxx

@@ -117,7 +117,7 @@ bool cmNinjaTargetGenerator::NeedDyndep(std::string const& lang) const
 
 std::string cmNinjaTargetGenerator::OrderDependsTargetForTarget()
 {
-  return "cmake_order_depends_target_" + this->GetTargetName();
+  return "cmake_object_order_depends_target_" + this->GetTargetName();
 }
 
 // TODO: Most of the code is picked up from
@@ -718,8 +718,8 @@ void cmNinjaTargetGenerator::WriteObjectBuildStatements()
   }
 
   cmNinjaDeps orderOnlyDeps;
-  this->GetLocalGenerator()->AppendTargetDepends(this->GeneratorTarget,
-                                                 orderOnlyDeps);
+  this->GetLocalGenerator()->AppendTargetDepends(
+    this->GeneratorTarget, orderOnlyDeps, DependOnTargetOrdering);
 
   // Add order-only dependencies on other files associated with the target.
   orderOnlyDeps.insert(orderOnlyDeps.end(), this->ExtraFiles.begin(),
@@ -740,7 +740,11 @@ void cmNinjaTargetGenerator::WriteObjectBuildStatements()
                    std::back_inserter(orderOnlyDeps), MapToNinjaPath());
   }
 
-  if (!orderOnlyDeps.empty()) {
+  std::sort(orderOnlyDeps.begin(), orderOnlyDeps.end());
+  orderOnlyDeps.erase(std::unique(orderOnlyDeps.begin(), orderOnlyDeps.end()),
+                      orderOnlyDeps.end());
+
+  {
     cmNinjaDeps orderOnlyTarget;
     orderOnlyTarget.push_back(this->OrderDependsTargetForTarget());
     this->GetGlobalGenerator()->WritePhonyBuild(
@@ -753,7 +757,7 @@ void cmNinjaTargetGenerator::WriteObjectBuildStatements()
   for (std::vector<cmSourceFile const*>::const_iterator si =
          objectSources.begin();
        si != objectSources.end(); ++si) {
-    this->WriteObjectBuildStatement(*si, !orderOnlyDeps.empty());
+    this->WriteObjectBuildStatement(*si);
   }
 
   if (!this->DDIFiles.empty()) {
@@ -770,6 +774,17 @@ void cmNinjaTargetGenerator::WriteObjectBuildStatements()
 
     ddOutputs.push_back(this->GetDyndepFilePath("Fortran"));
 
+    // Make sure dyndep files for all our dependencies have already
+    // been generated so that the 'FortranModules.json' files they
+    // produced as side-effects are available for us to read.
+    // Ideally we should depend on the 'FortranModules.json' files
+    // from our dependencies directly, but we don't know which of
+    // our dependencies produces them.  Fixing this will require
+    // refactoring the Ninja generator to generate targets in
+    // dependency order so that we can collect the needed information.
+    this->GetLocalGenerator()->AppendTargetDepends(
+      this->GeneratorTarget, ddOrderOnlyDeps, DependOnTargetArtifact);
+
     this->GetGlobalGenerator()->WriteBuild(
       this->GetBuildFileStream(), ddComment, ddRule, ddOutputs, ddImplicitOuts,
       ddExplicitDeps, ddImplicitDeps, ddOrderOnlyDeps, ddVars);
@@ -779,7 +794,7 @@ void cmNinjaTargetGenerator::WriteObjectBuildStatements()
 }
 
 void cmNinjaTargetGenerator::WriteObjectBuildStatement(
-  cmSourceFile const* source, bool writeOrderDependsTargetForTarget)
+  cmSourceFile const* source)
 {
   std::string const language = source->GetLanguage();
   std::string const sourceFileName =
@@ -830,9 +845,7 @@ void cmNinjaTargetGenerator::WriteObjectBuildStatement(
   }
 
   cmNinjaDeps orderOnlyDeps;
-  if (writeOrderDependsTargetForTarget) {
-    orderOnlyDeps.push_back(this->OrderDependsTargetForTarget());
-  }
+  orderOnlyDeps.push_back(this->OrderDependsTargetForTarget());
 
   // If the source file is GENERATED and does not have a custom command
   // (either attached to this source file or another one), assume that one of

Source/cmNinjaTargetGenerator.h

@@ -119,8 +119,7 @@ protected:
   void WriteLanguageRules(const std::string& language);
   void WriteCompileRule(const std::string& language);
   void WriteObjectBuildStatements();
-  void WriteObjectBuildStatement(cmSourceFile const* source,
-                                 bool writeOrderDependsTargetForTarget);
+  void WriteObjectBuildStatement(cmSourceFile const* source);
   void WriteTargetDependInfo(std::string const& lang);
 
   void ExportObjectCompileCommand(

Source/cmNinjaTypes.h

@@ -9,6 +9,12 @@
 #include <string>
 #include <vector>
 
+enum cmNinjaTargetDepends
+{
+  DependOnTargetArtifact,
+  DependOnTargetOrdering
+};
+
 typedef std::vector<std::string> cmNinjaDeps;
 typedef std::map<std::string, std::string> cmNinjaVars;
 

Tests/CustomCommandWorkingDirectory/CMakeLists.txt

@@ -19,6 +19,7 @@ add_executable(working "${TestWorkingDir_BINARY_DIR}/working.c"
 add_custom_target(
   Custom ALL
   COMMAND "${CMAKE_COMMAND}" -E copy_if_different ./customTarget.c "${TestWorkingDir_BINARY_DIR}/customTarget.c"
+  BYPRODUCTS "${TestWorkingDir_BINARY_DIR}/customTarget.c"
   WORKING_DIRECTORY  "${TestWorkingDir_SOURCE_DIR}"
 )
 
@@ -36,6 +37,7 @@ add_executable(working2 working2.c ${TestWorkingDir_BINARY_DIR}/customTarget2.c)
 add_custom_target(
   Custom2 ALL
   COMMAND "${CMAKE_COMMAND}" -E copy_if_different ${TestWorkingDir_SOURCE_DIR}/customTarget.c ../customTarget2.c
+  BYPRODUCTS "${TestWorkingDir_BINARY_DIR}/customTarget2.c"
   WORKING_DIRECTORY work/ # Relative to build tree, trailing slash
 )
 

Tests/RunCMake/CMakeLists.txt

@@ -119,6 +119,10 @@ if(CMAKE_GENERATOR MATCHES "Make")
   add_RunCMake_test(Make)
 endif()
 if(CMAKE_GENERATOR STREQUAL "Ninja")
+  set(Ninja_ARGS
+    -DCMAKE_C_OUTPUT_EXTENSION=${CMAKE_C_OUTPUT_EXTENSION}
+    -DCMAKE_SHARED_LIBRARY_PREFIX=${CMAKE_SHARED_LIBRARY_PREFIX}
+    -DCMAKE_SHARED_LIBRARY_SUFFIX=${CMAKE_SHARED_LIBRARY_SUFFIX})
   add_RunCMake_test(Ninja)
 endif()
 add_RunCMake_test(CTest)

Tests/RunCMake/Ninja/AssumedSources.cmake

@@ -0,0 +1,20 @@
+cmake_minimum_required(VERSION 3.8)
+project(AssumedSources)
+
+set_source_files_properties(
+  "${CMAKE_CURRENT_BINARY_DIR}/target.c"
+  "${CMAKE_CURRENT_BINARY_DIR}/target-no-depends.c"
+  PROPERTIES GENERATED 1)
+
+add_executable(working
+  "${CMAKE_CURRENT_BINARY_DIR}/target.c"
+  "${CMAKE_CURRENT_BINARY_DIR}/target-no-depends.c")
+
+add_custom_target(
+  gen-target.c ALL
+  COMMAND "${CMAKE_COMMAND}" -E copy_if_different "${CMAKE_CURRENT_SOURCE_DIR}/dep.c" "${CMAKE_CURRENT_BINARY_DIR}/target.c")
+add_custom_target(
+  gen-target-no-depends.c ALL
+  COMMAND "${CMAKE_COMMAND}" -E copy_if_different "${CMAKE_CURRENT_SOURCE_DIR}/dep.c" "${CMAKE_CURRENT_BINARY_DIR}/target-no-depends.c")
+
+add_dependencies(working gen-target.c)

Tests/RunCMake/Ninja/LooseObjectDepends.cmake

@@ -0,0 +1,26 @@
+cmake_minimum_required(VERSION 3.8)
+project(LooseObjectDepends C)
+
+add_custom_command(
+  OUTPUT  "${CMAKE_CURRENT_BINARY_DIR}/command.h"
+  COMMAND "${CMAKE_COMMAND}" -E touch
+          "${CMAKE_CURRENT_BINARY_DIR}/command.h"
+          COMMENT "Creating command.h")
+add_custom_target(create-command.h
+  DEPENDS
+    "${CMAKE_CURRENT_BINARY_DIR}/command.h")
+
+add_custom_target(create-target.h
+  BYPRODUCTS  "${CMAKE_CURRENT_BINARY_DIR}/target.h"
+  COMMAND "${CMAKE_COMMAND}" -E touch
+          "${CMAKE_CURRENT_BINARY_DIR}/target.h"
+  COMMENT "Creating target.h")
+
+add_library(dep SHARED dep.c)
+add_dependencies(dep create-command.h create-target.h)
+target_include_directories(dep
+  PUBLIC
+    "${CMAKE_CURRENT_BINARY_DIR}")
+
+add_library(top top.c)
+target_link_libraries(top PRIVATE dep)

Tests/RunCMake/Ninja/RunCMakeTest.cmake

@@ -73,7 +73,7 @@ run_SubDir()
 
 function(run_ninja dir)
   execute_process(
-    COMMAND "${RunCMake_MAKE_PROGRAM}"
+    COMMAND "${RunCMake_MAKE_PROGRAM}" ${ARGN}
     WORKING_DIRECTORY "${dir}"
     OUTPUT_VARIABLE ninja_stdout
     ERROR_VARIABLE ninja_stderr
@@ -95,6 +95,39 @@ ${ninja_stderr}
   endif()
 endfunction(run_ninja)
 
+function (run_LooseObjectDepends)
+  set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/LooseObjectDepends-build)
+  run_cmake(LooseObjectDepends)
+  run_ninja("${RunCMake_TEST_BINARY_DIR}" "CMakeFiles/top.dir/top.c${CMAKE_C_OUTPUT_EXTENSION}")
+  if (EXISTS "${RunCMake_TEST_BINARY_DIR}/${CMAKE_SHARED_LIBRARY_PREFIX}dep${CMAKE_SHARED_LIBRARY_SUFFIX}")
+    message(FATAL_ERROR
+      "The `dep` library was created when requesting an object file to be "
+      "built; this should no longer be necessary.")
+  endif ()
+  if (EXISTS "${RunCMake_TEST_BINARY_DIR}/CMakeFiles/dep.dir/dep.c${CMAKE_C_OUTPUT_EXTENSION}")
+    message(FATAL_ERROR
+      "The `dep.c` object file was created when requesting an object file to "
+      "be built; this should no longer be necessary.")
+  endif ()
+endfunction ()
+run_LooseObjectDepends()
+
+function (run_AssumedSources)
+  set(RunCMake_TEST_BINARY_DIR ${RunCMake_BINARY_DIR}/AssumedSources-build)
+  run_cmake(AssumedSources)
+  run_ninja("${RunCMake_TEST_BINARY_DIR}" "target.c")
+  if (NOT EXISTS "${RunCMake_TEST_BINARY_DIR}/target.c")
+    message(FATAL_ERROR
+      "Dependencies for an assumed source did not hook up properly for 'target.c'.")
+  endif ()
+  run_ninja("${RunCMake_TEST_BINARY_DIR}" "target-no-depends.c")
+  if (EXISTS "${RunCMake_TEST_BINARY_DIR}/target-no-depends.c")
+    message(FATAL_ERROR
+      "Dependencies for an assumed source were magically hooked up for 'target-no-depends.c'.")
+  endif ()
+endfunction ()
+run_AssumedSources()
+
 function(sleep delay)
   execute_process(
     COMMAND ${CMAKE_COMMAND} -E sleep ${delay}

Tests/RunCMake/Ninja/dep.c

@@ -0,0 +1,4 @@
+int dep()
+{
+  return 0;
+}

Tests/RunCMake/Ninja/top.c

@@ -0,0 +1,7 @@
+#include "command.h"
+#include "target.h"
+
+int top()
+{
+  return 0;
+}