Merge topic 'cmake-ninja-workdir'

2725ecff38 Ninja: Handle depfiles with absolute paths to generated files
bc40cd7a4e Tests: Add case covering a unity build with a generated source
ae927f936d cmGlobalNinjaGenerator: Improve allocation pattern in WriteBuild
68e5f92cad cmGlobalNinjaGenerator: Factor out custom command output collection
c5195193d3 cmGlobalNinjaGenerator: Reduce string copies in WriteCustomCommandBuild
8bac527b0c cmGlobalNinjaGenerator: Re-order logic in WriteCustomCommandBuild
ddc030f5ca cmGlobalNinjaGenerator: Record implicit outputs as known too
ceb82752ef cmLocalNinjaGenerator: Use variable for main custom command output path

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

10 files changed, 160 insertions, 59 deletions

diff --git a/Source/cmGlobalNinjaGenerator.cxx b/Source/cmGlobalNinjaGenerator.cxx
index 565b951..6034434 100644
--- a/Source/cmGlobalNinjaGenerator.cxx
+++ b/Source/cmGlobalNinjaGenerator.cxx
@@ -216,22 +216,32 @@ void cmGlobalNinjaGenerator::WriteBuild(std::ostream& os,
   {
     // Write explicit outputs
     for (std::string const& output : build.Outputs) {
-      buildStr += cmStrCat(' ', this->EncodePath(output));
+      buildStr = cmStrCat(buildStr, ' ', this->EncodePath(output));
       if (this->ComputingUnknownDependencies) {
         this->CombinedBuildOutputs.insert(output);
       }
     }
     // Write implicit outputs
-    if (!build.ImplicitOuts.empty()) {
-      buildStr += " |";
+    if (!build.ImplicitOuts.empty() || !build.WorkDirOuts.empty()) {
+      buildStr = cmStrCat(buildStr, " |");
       for (std::string const& implicitOut : build.ImplicitOuts) {
-        buildStr += cmStrCat(' ', this->EncodePath(implicitOut));
+        buildStr = cmStrCat(buildStr, ' ', this->EncodePath(implicitOut));
+        if (this->ComputingUnknownDependencies) {
+          this->CombinedBuildOutputs.insert(implicitOut);
+        }
+      }
+      for (std::string const& workdirOut : build.WorkDirOuts) {
+        // Repeat some outputs, but expressed as absolute paths.
+        // This helps Ninja handle absolute paths found in a depfile.
+        // FIXME: Unfortunately this causes Ninja to stat the file twice.
+        // We could avoid this if Ninja Issue 1251 were fixed.
+        buildStr = cmStrCat(buildStr, " ${cmake_ninja_workdir}",
+                            this->EncodePath(workdirOut));
       }
     }
-    buildStr += ':';
 
     // Write the rule.
-    buildStr += cmStrCat(' ', build.Rule);
+    buildStr = cmStrCat(buildStr, ": ", build.Rule);
   }
 
   std::string arguments;
@@ -307,21 +317,46 @@ void cmGlobalNinjaGenerator::AddCustomCommandRule()
   this->AddRule(rule);
 }
 
+void cmGlobalNinjaGenerator::CCOutputs::Add(
+  std::vector<std::string> const& paths)
+{
+  for (std::string const& path : paths) {
+    std::string out = this->GG->ConvertToNinjaPath(path);
+    if (this->GG->SupportsImplicitOuts() &&
+        !cmSystemTools::FileIsFullPath(out)) {
+      // This output is expressed as a relative path.  Repeat it,
+      // but expressed as an absolute path for Ninja Issue 1251.
+      this->WorkDirOuts.emplace_back(out);
+    }
+    this->GG->SeenCustomCommandOutput(out);
+    this->ExplicitOuts.emplace_back(std::move(out));
+  }
+}
+
 void cmGlobalNinjaGenerator::WriteCustomCommandBuild(
-  const std::string& command, const std::string& description,
-  const std::string& comment, const std::string& depfile,
-  const std::string& job_pool, bool uses_terminal, bool restat,
-  const cmNinjaDeps& outputs, const std::string& config,
-  const cmNinjaDeps& explicitDeps, const cmNinjaDeps& orderOnlyDeps)
+  std::string const& command, std::string const& description,
+  std::string const& comment, std::string const& depfile,
+  std::string const& job_pool, bool uses_terminal, bool restat,
+  std::string const& config, CCOutputs outputs, cmNinjaDeps explicitDeps,
+  cmNinjaDeps orderOnlyDeps)
 {
   this->AddCustomCommandRule();
 
+  if (this->ComputingUnknownDependencies) {
+    // we need to track every dependency that comes in, since we are trying
+    // to find dependencies that are side effects of build commands
+    for (std::string const& dep : explicitDeps) {
+      this->CombinedCustomCommandExplicitDependencies.insert(dep);
+    }
+  }
+
   {
     cmNinjaBuild build("CUSTOM_COMMAND");
     build.Comment = comment;
-    build.Outputs = outputs;
-    build.ExplicitDeps = explicitDeps;
-    build.OrderOnlyDeps = orderOnlyDeps;
+    build.Outputs = std::move(outputs.ExplicitOuts);
+    build.WorkDirOuts = std::move(outputs.WorkDirOuts);
+    build.ExplicitDeps = std::move(explicitDeps);
+    build.OrderOnlyDeps = std::move(orderOnlyDeps);
 
     cmNinjaVars& vars = build.Variables;
     {
@@ -351,14 +386,6 @@ void cmGlobalNinjaGenerator::WriteCustomCommandBuild(
       this->WriteBuild(*this->GetImplFileStream(config), build);
     }
   }
-
-  if (this->ComputingUnknownDependencies) {
-    // we need to track every dependency that comes in, since we are trying
-    // to find dependencies that are side effects of build commands
-    for (std::string const& dep : explicitDeps) {
-      this->CombinedCustomCommandExplicitDependencies.insert(dep);
-    }
-  }
 }
 
 void cmGlobalNinjaGenerator::AddMacOSXContentRule()
@@ -1198,6 +1225,8 @@ void cmGlobalNinjaGenerator::WriteDisclaimer(std::ostream& os) const
 void cmGlobalNinjaGenerator::WriteAssumedSourceDependencies()
 {
   for (auto const& asd : this->AssumedSourceDependencies) {
+    CCOutputs outputs(this);
+    outputs.ExplicitOuts.emplace_back(asd.first);
     cmNinjaDeps orderOnlyDeps;
     std::copy(asd.second.begin(), asd.second.end(),
               std::back_inserter(orderOnlyDeps));
@@ -1206,8 +1235,8 @@ void cmGlobalNinjaGenerator::WriteAssumedSourceDependencies()
       "Assume dependencies for generated source file.",
       /*depfile*/ "", /*job_pool*/ "",
       /*uses_terminal*/ false,
-      /*restat*/ true, cmNinjaDeps(1, asd.first), "", cmNinjaDeps(),
-      orderOnlyDeps);
+      /*restat*/ true, std::string(), outputs, cmNinjaDeps(),
+      std::move(orderOnlyDeps));
   }
 }
 
diff --git a/Source/cmGlobalNinjaGenerator.h b/Source/cmGlobalNinjaGenerator.h
index 9f31708..7a3674e 100644
--- a/Source/cmGlobalNinjaGenerator.h
+++ b/Source/cmGlobalNinjaGenerator.h
@@ -110,13 +110,29 @@ public:
   void WriteBuild(std::ostream& os, cmNinjaBuild const& build,
                   int cmdLineLimit = 0, bool* usedResponseFile = nullptr);
 
-  void WriteCustomCommandBuild(
-    const std::string& command, const std::string& description,
-    const std::string& comment, const std::string& depfile,
-    const std::string& pool, bool uses_terminal, bool restat,
-    const cmNinjaDeps& outputs, const std::string& config,
-    const cmNinjaDeps& explicitDeps = cmNinjaDeps(),
-    const cmNinjaDeps& orderOnlyDeps = cmNinjaDeps());
+  class CCOutputs
+  {
+    cmGlobalNinjaGenerator* GG;
+
+  public:
+    CCOutputs(cmGlobalNinjaGenerator* gg)
+      : GG(gg)
+    {
+    }
+    void Add(std::vector<std::string> const& outputs);
+    cmNinjaDeps ExplicitOuts;
+    cmNinjaDeps WorkDirOuts;
+  };
+
+  void WriteCustomCommandBuild(std::string const& command,
+                               std::string const& description,
+                               std::string const& comment,
+                               std::string const& depfile,
+                               std::string const& pool, bool uses_terminal,
+                               bool restat, std::string const& config,
+                               CCOutputs outputs,
+                               cmNinjaDeps explicitDeps = cmNinjaDeps(),
+                               cmNinjaDeps orderOnlyDeps = cmNinjaDeps());
 
   void WriteMacOSXContentBuild(std::string input, std::string output,
                                const std::string& config);
diff --git a/Source/cmLocalNinjaGenerator.cxx b/Source/cmLocalNinjaGenerator.cxx
index a8570e2..fb6c730 100644
--- a/Source/cmLocalNinjaGenerator.cxx
+++ b/Source/cmLocalNinjaGenerator.cxx
@@ -263,6 +263,7 @@ void cmLocalNinjaGenerator::WriteBuildFileTop()
                                           this->GetConfigNames().front());
   }
   this->WriteNinjaFilesInclusionCommon(this->GetCommonFileStream());
+  this->WriteNinjaWorkDir(this->GetCommonFileStream());
 
   // For the rule file.
   this->WriteProjectHeader(this->GetRulesFileStream());
@@ -364,6 +365,17 @@ void cmLocalNinjaGenerator::WriteNinjaFilesInclusionCommon(std::ostream& os)
   os << "\n";
 }
 
+void cmLocalNinjaGenerator::WriteNinjaWorkDir(std::ostream& os)
+{
+  cmGlobalNinjaGenerator::WriteDivider(os);
+  cmGlobalNinjaGenerator::WriteComment(
+    os, "Logical path to working directory; prefix for absolute paths.");
+  cmGlobalNinjaGenerator* ng = this->GetGlobalNinjaGenerator();
+  std::string ninja_workdir = this->GetBinaryDirectory();
+  ng->StripNinjaOutputPathPrefixAsSuffix(ninja_workdir); // Also appends '/'.
+  os << "cmake_ninja_workdir = " << ng->EncodePath(ninja_workdir) << "\n";
+}
+
 void cmLocalNinjaGenerator::WriteProcessedMakefile(std::ostream& os)
 {
   cmGlobalNinjaGenerator::WriteDivider(os);
@@ -638,16 +650,11 @@ void cmLocalNinjaGenerator::WriteCustomCommandBuildStatement(
       }
     }
 
-    cmNinjaDeps ninjaOutputs(outputs.size() + byproducts.size());
-    std::transform(outputs.begin(), outputs.end(), ninjaOutputs.begin(),
-                   gg->MapToNinjaPath());
-    std::transform(byproducts.begin(), byproducts.end(),
-                   ninjaOutputs.begin() + outputs.size(),
-                   gg->MapToNinjaPath());
+    cmGlobalNinjaGenerator::CCOutputs ccOutputs(gg);
+    ccOutputs.Add(outputs);
+    ccOutputs.Add(byproducts);
 
-    for (std::string const& ninjaOutput : ninjaOutputs) {
-      gg->SeenCustomCommandOutput(ninjaOutput);
-    }
+    std::string mainOutput = ccOutputs.ExplicitOuts[0];
 
     cmNinjaDeps ninjaDeps;
     this->AppendCustomCommandDeps(ccg, ninjaDeps, fileConfig);
@@ -657,13 +664,14 @@ void cmLocalNinjaGenerator::WriteCustomCommandBuildStatement(
 
     if (cmdLines.empty()) {
       cmNinjaBuild build("phony");
-      build.Comment = "Phony custom command for " + ninjaOutputs[0];
-      build.Outputs = std::move(ninjaOutputs);
+      build.Comment = cmStrCat("Phony custom command for ", mainOutput);
+      build.Outputs = std::move(ccOutputs.ExplicitOuts);
+      build.WorkDirOuts = std::move(ccOutputs.WorkDirOuts);
       build.ExplicitDeps = std::move(ninjaDeps);
       build.OrderOnlyDeps = orderOnlyDeps;
       gg->WriteBuild(this->GetImplFileStream(fileConfig), build);
     } else {
-      std::string customStep = cmSystemTools::GetFilenameName(ninjaOutputs[0]);
+      std::string customStep = cmSystemTools::GetFilenameName(mainOutput);
       if (this->GlobalGenerator->IsMultiConfig()) {
         customStep += '-';
         customStep += fileConfig;
@@ -673,7 +681,7 @@ void cmLocalNinjaGenerator::WriteCustomCommandBuildStatement(
       // Hash full path to make unique.
       customStep += '-';
       cmCryptoHash hash(cmCryptoHash::AlgoSHA256);
-      customStep += hash.HashString(ninjaOutputs[0]).substr(0, 7);
+      customStep += hash.HashString(mainOutput).substr(0, 7);
 
       std::string depfile = ccg.GetDepfile();
       if (!depfile.empty()) {
@@ -701,13 +709,14 @@ void cmLocalNinjaGenerator::WriteCustomCommandBuildStatement(
         }
       }
 
+      std::string comment = cmStrCat("Custom command for ", mainOutput);
       gg->WriteCustomCommandBuild(
         this->BuildCommandLine(cmdLines, ccg.GetOutputConfig(), fileConfig,
                                customStep),
-        this->ConstructComment(ccg), "Custom command for " + ninjaOutputs[0],
-        depfile, cc->GetJobPool(), cc->GetUsesTerminal(),
-        /*restat*/ !symbolic || !byproducts.empty(), ninjaOutputs, fileConfig,
-        ninjaDeps, orderOnlyDeps);
+        this->ConstructComment(ccg), comment, depfile, cc->GetJobPool(),
+        cc->GetUsesTerminal(),
+        /*restat*/ !symbolic || !byproducts.empty(), fileConfig,
+        std::move(ccOutputs), std::move(ninjaDeps), std::move(orderOnlyDeps));
     }
   }
 }
diff --git a/Source/cmLocalNinjaGenerator.h b/Source/cmLocalNinjaGenerator.h
index a73fa27..6404037 100644
--- a/Source/cmLocalNinjaGenerator.h
+++ b/Source/cmLocalNinjaGenerator.h
@@ -108,6 +108,7 @@ private:
                                        const std::string& config);
   void WriteNinjaFilesInclusionConfig(std::ostream& os);
   void WriteNinjaFilesInclusionCommon(std::ostream& os);
+  void WriteNinjaWorkDir(std::ostream& os);
   void WriteProcessedMakefile(std::ostream& os);
   void WritePools(std::ostream& os);
 
diff --git a/Source/cmNinjaTypes.h b/Source/cmNinjaTypes.h
index 320f41b..c8a411e 100644
--- a/Source/cmNinjaTypes.h
+++ b/Source/cmNinjaTypes.h
@@ -53,6 +53,7 @@ public:
   std::string Rule;
   cmNinjaDeps Outputs;
   cmNinjaDeps ImplicitOuts;
+  cmNinjaDeps WorkDirOuts; // For cmake_ninja_workdir.
   cmNinjaDeps ExplicitDeps;
   cmNinjaDeps ImplicitDeps;
   cmNinjaDeps OrderOnlyDeps;
diff --git a/Source/cmNinjaUtilityTargetGenerator.cxx b/Source/cmNinjaUtilityTargetGenerator.cxx
index 7a04c47..1f5a7ff 100644
--- a/Source/cmNinjaUtilityTargetGenerator.cxx
+++ b/Source/cmNinjaUtilityTargetGenerator.cxx
@@ -73,7 +73,8 @@ void cmNinjaUtilityTargetGenerator::WriteUtilBuildStatements(
   cmNinjaBuild phonyBuild("phony");
   std::vector<std::string> commands;
   cmNinjaDeps deps;
-  cmNinjaDeps util_outputs(1, utilCommandName);
+  cmGlobalNinjaGenerator::CCOutputs util_outputs(gg);
+  util_outputs.ExplicitOuts.emplace_back(utilCommandName);
 
   bool uses_terminal = false;
   {
@@ -86,10 +87,7 @@ void cmNinjaUtilityTargetGenerator::WriteUtilBuildStatements(
         cmCustomCommandGenerator ccg(ci, fileConfig, lg);
         lg->AppendCustomCommandDeps(ccg, deps, fileConfig);
         lg->AppendCustomCommandLines(ccg, commands);
-        std::vector<std::string> const& ccByproducts = ccg.GetByproducts();
-        std::transform(ccByproducts.begin(), ccByproducts.end(),
-                       std::back_inserter(util_outputs),
-                       this->MapToNinjaPath());
+        util_outputs.Add(ccg.GetByproducts());
         if (ci.GetUsesTerminal()) {
           uses_terminal = true;
         }
@@ -124,7 +122,8 @@ void cmNinjaUtilityTargetGenerator::WriteUtilBuildStatements(
   if (genTarget->Target->GetType() != cmStateEnums::GLOBAL_TARGET) {
     lg->AppendTargetOutputs(genTarget, gg->GetByproductsForCleanTarget(),
                             config);
-    std::copy(util_outputs.begin(), util_outputs.end(),
+    std::copy(util_outputs.ExplicitOuts.begin(),
+              util_outputs.ExplicitOuts.end(),
               std::back_inserter(gg->GetByproductsForCleanTarget()));
   }
   lg->AppendTargetDepends(genTarget, deps, config, fileConfig,
@@ -166,10 +165,6 @@ void cmNinjaUtilityTargetGenerator::WriteUtilBuildStatements(
       return;
     }
 
-    for (std::string const& util_output : util_outputs) {
-      gg->SeenCustomCommandOutput(util_output);
-    }
-
     std::string ccConfig;
     if (genTarget->Target->IsPerConfig() &&
         genTarget->GetType() != cmStateEnums::GLOBAL_TARGET) {
@@ -180,7 +175,7 @@ void cmNinjaUtilityTargetGenerator::WriteUtilBuildStatements(
       gg->WriteCustomCommandBuild(
         command, desc, "Utility command for " + this->GetTargetName(),
         /*depfile*/ "", /*job_pool*/ "", uses_terminal,
-        /*restat*/ true, util_outputs, ccConfig, deps);
+        /*restat*/ true, ccConfig, std::move(util_outputs), std::move(deps));
     }
 
     phonyBuild.ExplicitDeps.push_back(utilCommandName);
diff --git a/Tests/RunCMake/BuildDepends/CustomCommandUnityBuild.cmake b/Tests/RunCMake/BuildDepends/CustomCommandUnityBuild.cmake
new file mode 100644
index 0000000..22e7f27
--- /dev/null
+++ b/Tests/RunCMake/BuildDepends/CustomCommandUnityBuild.cmake
@@ -0,0 +1,19 @@
+enable_language(C)
+
+add_custom_command(
+  OUTPUT main.c
+  COMMAND ${CMAKE_COMMAND} -E copy main.c.in main.c
+  DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/main.c.in
+  )
+add_executable(main main.c)
+set_property(TARGET main PROPERTY UNITY_BUILD ON)
+
+file(GENERATE OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/check-$<LOWER_CASE:$<CONFIG>>.cmake CONTENT "
+set(check_pairs
+  \"$<TARGET_FILE:main>|${CMAKE_CURRENT_BINARY_DIR}/main.c.in\"
+  \"$<TARGET_FILE:main>|${CMAKE_CURRENT_BINARY_DIR}/main.c\"
+  )
+set(check_exes
+  \"$<TARGET_FILE:main>\"
+  )
+")
diff --git a/Tests/RunCMake/BuildDepends/CustomCommandUnityBuild.step1.cmake b/Tests/RunCMake/BuildDepends/CustomCommandUnityBuild.step1.cmake
new file mode 100644
index 0000000..87576eb
--- /dev/null
+++ b/Tests/RunCMake/BuildDepends/CustomCommandUnityBuild.step1.cmake
@@ -0,0 +1,3 @@
+file(WRITE "${RunCMake_TEST_BINARY_DIR}/main.c.in" [[
+int main(void) { return 1; }
+]])
diff --git a/Tests/RunCMake/BuildDepends/CustomCommandUnityBuild.step2.cmake b/Tests/RunCMake/BuildDepends/CustomCommandUnityBuild.step2.cmake
new file mode 100644
index 0000000..69b21b8
--- /dev/null
+++ b/Tests/RunCMake/BuildDepends/CustomCommandUnityBuild.step2.cmake
@@ -0,0 +1,3 @@
+file(WRITE "${RunCMake_TEST_BINARY_DIR}/main.c.in" [[
+int main(void) { return 2; }
+]])
diff --git a/Tests/RunCMake/BuildDepends/RunCMakeTest.cmake b/Tests/RunCMake/BuildDepends/RunCMakeTest.cmake
index 72faddb..0a80580 100644
--- a/Tests/RunCMake/BuildDepends/RunCMakeTest.cmake
+++ b/Tests/RunCMake/BuildDepends/RunCMakeTest.cmake
@@ -1,5 +1,22 @@
 include(RunCMake)
 
+if(RunCMake_GENERATOR MATCHES "Ninja")
+  # Detect ninja version so we know what tests can be supported.
+  execute_process(
+    COMMAND "${RunCMake_MAKE_PROGRAM}" --version
+    OUTPUT_VARIABLE ninja_out
+    ERROR_VARIABLE ninja_out
+    RESULT_VARIABLE ninja_res
+    OUTPUT_STRIP_TRAILING_WHITESPACE
+    )
+  if(ninja_res EQUAL 0 AND "x${ninja_out}" MATCHES "^x[0-9]+\\.[0-9]+")
+    set(ninja_version "${ninja_out}")
+    message(STATUS "ninja version: ${ninja_version}")
+  else()
+    message(FATAL_ERROR "'ninja --version' reported:\n${ninja_out}")
+  endif()
+endif()
+
 if(RunCMake_GENERATOR STREQUAL "Borland Makefiles" OR
    RunCMake_GENERATOR STREQUAL "Watcom WMake")
   set(fs_delay 3)
@@ -164,3 +181,11 @@ endif()
 if(RunCMake_GENERATOR MATCHES "Make")
   run_BuildDepends(MakeDependencies)
 endif()
+
+if(RunCMake_GENERATOR MATCHES "^Visual Studio 9 " OR
+   (RunCMake_GENERATOR MATCHES "Ninja" AND ninja_version VERSION_LESS 1.7))
+  # This build tool misses the dependency.
+  set(run_BuildDepends_skip_step_2 1)
+endif()
+run_BuildDepends(CustomCommandUnityBuild)
+unset(run_BuildDepends_skip_step_2)