Merge topic 'ninja-absolute-paths'

c564a3e3ff Ninja: Always compile sources using absolute paths
eb98d45111 Ninja: Handle depfiles with absolute paths to generated files in Ninja < 1.7
48471cfd18 cmNinjaNormalTargetGenerator: Factor out build event byproduct collection
18408c0b88 cmGlobalNinjaGenerator: Add helper to compute absolute paths for build.ninja
efb8d7b4a1 cmNinjaTargetGenerator: Reduce string copies in ConvertToNinjaPath wrapper
fb3a57575a cmNinjaTargetGenerator: Rename source file path lookup method for clarity
0f2b1c9d1b cmNinjaTargetGenerator: Remove GetSourceFilePath call with different semantics
dfc98774a2 cmNinjaTargetGenerator: Rename local variable for clarity
...

Acked-by: Kitware Robot <kwrobot@kitware.com>
Acked-by: Ben Boeckel <ben.boeckel@kitware.com>
Merge-request: !6148

8 files changed, 67 insertions, 41 deletions

diff --git a/Help/release/dev/ninja-absolute-paths.rst b/Help/release/dev/ninja-absolute-paths.rst
new file mode 100644
index 0000000..2dfc1b7
--- /dev/null
+++ b/Help/release/dev/ninja-absolute-paths.rst
@@ -0,0 +1,6 @@
+ninja-absolute-paths
+--------------------
+
+* The :ref:`Ninja Generators` now pass source files and include directories
+  to the compiler using absolute paths.  This makes diagnostic messages and
+  debug symbols more consistent, and matches the :ref:`Makefile Generators`.
diff --git a/Source/cmGlobalNinjaGenerator.cxx b/Source/cmGlobalNinjaGenerator.cxx
index 6034434..cbe1bc8 100644
--- a/Source/cmGlobalNinjaGenerator.cxx
+++ b/Source/cmGlobalNinjaGenerator.cxx
@@ -222,7 +222,9 @@ void cmGlobalNinjaGenerator::WriteBuild(std::ostream& os,
       }
     }
     // Write implicit outputs
-    if (!build.ImplicitOuts.empty() || !build.WorkDirOuts.empty()) {
+    if (!build.ImplicitOuts.empty()) {
+      // Assume Ninja is new enough to support implicit outputs.
+      // Callers should not populate this field otherwise.
       buildStr = cmStrCat(buildStr, " |");
       for (std::string const& implicitOut : build.ImplicitOuts) {
         buildStr = cmStrCat(buildStr, ' ', this->EncodePath(implicitOut));
@@ -230,11 +232,18 @@ void cmGlobalNinjaGenerator::WriteBuild(std::ostream& os,
           this->CombinedBuildOutputs.insert(implicitOut);
         }
       }
+    }
+
+    // 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.
+    if (!build.WorkDirOuts.empty()) {
+      if (this->SupportsImplicitOuts() && build.ImplicitOuts.empty()) {
+        // Make them implicit outputs if supported by this version of Ninja.
+        buildStr = cmStrCat(buildStr, " |");
+      }
       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));
       }
@@ -322,11 +331,11 @@ void cmGlobalNinjaGenerator::CCOutputs::Add(
 {
   for (std::string const& path : paths) {
     std::string out = this->GG->ConvertToNinjaPath(path);
-    if (this->GG->SupportsImplicitOuts() &&
-        !cmSystemTools::FileIsFullPath(out)) {
+    if (!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(this->GG->ConvertToNinjaAbsPath(path));
     }
     this->GG->SeenCustomCommandOutput(out);
     this->ExplicitOuts.emplace_back(std::move(out));
@@ -1160,6 +1169,15 @@ std::string const& cmGlobalNinjaGenerator::ConvertToNinjaPath(
     .first->second;
 }
 
+std::string cmGlobalNinjaGenerator::ConvertToNinjaAbsPath(
+  std::string path) const
+{
+#ifdef _WIN32
+  std::replace(path.begin(), path.end(), '/', '\\');
+#endif
+  return path;
+}
+
 void cmGlobalNinjaGenerator::AddAdditionalCleanFile(std::string fileName,
                                                     const std::string& config)
 {
diff --git a/Source/cmGlobalNinjaGenerator.h b/Source/cmGlobalNinjaGenerator.h
index 7a3674e..bb4ce2b 100644
--- a/Source/cmGlobalNinjaGenerator.h
+++ b/Source/cmGlobalNinjaGenerator.h
@@ -261,6 +261,7 @@ public:
   }
 
   std::string const& ConvertToNinjaPath(const std::string& path) const;
+  std::string ConvertToNinjaAbsPath(std::string path) const;
 
   struct MapToNinjaPathImpl
   {
diff --git a/Source/cmLocalNinjaGenerator.cxx b/Source/cmLocalNinjaGenerator.cxx
index fb6c730..8142599 100644
--- a/Source/cmLocalNinjaGenerator.cxx
+++ b/Source/cmLocalNinjaGenerator.cxx
@@ -208,13 +208,9 @@ std::string cmLocalNinjaGenerator::ConvertToIncludeReference(
   std::string const& path, IncludePathStyle pathStyle,
   cmOutputConverter::OutputFormat format)
 {
-  if (pathStyle == IncludePathStyle::Absolute) {
-    return this->ConvertToOutputFormat(
-      cmSystemTools::CollapseFullPath(path, this->GetCurrentBinaryDirectory()),
-      format);
-  }
-  return this->ConvertToOutputFormat(this->MaybeRelativeToTopBinDir(path),
-                                     format);
+  // FIXME: Remove IncludePathStyle infrastructure.  It is no longer used.
+  static_cast<void>(pathStyle);
+  return this->ConvertToOutputFormat(path, format);
 }
 
 // Private methods.
diff --git a/Source/cmNinjaNormalTargetGenerator.cxx b/Source/cmNinjaNormalTargetGenerator.cxx
index 1b514b8..4c33334 100644
--- a/Source/cmNinjaNormalTargetGenerator.cxx
+++ b/Source/cmNinjaNormalTargetGenerator.cxx
@@ -1073,7 +1073,7 @@ void cmNinjaNormalTargetGenerator::WriteLinkStatement(
       for (const auto& source : sources) {
         oss << " "
             << LocalGen->ConvertToOutputFormat(
-                 this->ConvertToNinjaPath(this->GetSourceFilePath(source)),
+                 this->GetCompiledSourceNinjaPath(source),
                  cmOutputConverter::SHELL);
       }
       return oss.str();
@@ -1094,8 +1094,8 @@ void cmNinjaNormalTargetGenerator::WriteLinkStatement(
     for (const auto& source : sources) {
       linkBuild.Outputs.push_back(
         this->ConvertToNinjaPath(this->GetObjectFilePath(source, config)));
-      linkBuild.ExplicitDeps.push_back(
-        this->ConvertToNinjaPath(this->GetSourceFilePath(source)));
+      linkBuild.ExplicitDeps.emplace_back(
+        this->GetCompiledSourceNinjaPath(source));
     }
     linkBuild.Outputs.push_back(vars["SWIFT_MODULE"]);
   } else {
@@ -1195,7 +1195,7 @@ void cmNinjaNormalTargetGenerator::WriteLinkStatement(
     }
   }
 
-  cmNinjaDeps byproducts;
+  cmGlobalNinjaGenerator::CCOutputs byproducts(this->GetGlobalGenerator());
 
   if (!tgtNames.ImportLibrary.empty()) {
     const std::string impLibPath = localGen.ConvertToOutputFormat(
@@ -1203,7 +1203,8 @@ void cmNinjaNormalTargetGenerator::WriteLinkStatement(
     vars["TARGET_IMPLIB"] = impLibPath;
     this->EnsureParentDirectoryExists(impLibPath);
     if (gt->HasImportLibrary(config)) {
-      byproducts.push_back(targetOutputImplib);
+      // Some linkers may update a binary without touching its import lib.
+      byproducts.ExplicitOuts.emplace_back(targetOutputImplib);
       if (firstForConfig) {
         globalGen->GetByproductsForCleanTarget(config).push_back(
           targetOutputImplib);
@@ -1261,8 +1262,7 @@ void cmNinjaNormalTargetGenerator::WriteLinkStatement(
                                      true, config);
         localGen.AppendCustomCommandLines(ccg, *cmdLineLists[i]);
         std::vector<std::string> const& ccByproducts = ccg.GetByproducts();
-        std::transform(ccByproducts.begin(), ccByproducts.end(),
-                       std::back_inserter(byproducts), this->MapToNinjaPath());
+        byproducts.Add(ccByproducts);
         std::transform(
           ccByproducts.begin(), ccByproducts.end(),
           std::back_inserter(globalGen->GetByproductsForCleanTarget()),
@@ -1382,12 +1382,13 @@ void cmNinjaNormalTargetGenerator::WriteLinkStatement(
   }
 
   // Ninja should restat after linking if and only if there are byproducts.
-  vars["RESTAT"] = byproducts.empty() ? "" : "1";
+  vars["RESTAT"] = byproducts.ExplicitOuts.empty() ? "" : "1";
 
-  for (std::string const& o : byproducts) {
-    globalGen->SeenCustomCommandOutput(o);
-    linkBuild.Outputs.push_back(o);
-  }
+  linkBuild.Outputs.reserve(linkBuild.Outputs.size() +
+                            byproducts.ExplicitOuts.size());
+  std::move(byproducts.ExplicitOuts.begin(), byproducts.ExplicitOuts.end(),
+            std::back_inserter(linkBuild.Outputs));
+  linkBuild.WorkDirOuts = std::move(byproducts.WorkDirOuts);
 
   // Write the build statement for this target.
   bool usedResponseFile = false;
diff --git a/Source/cmNinjaTargetGenerator.cxx b/Source/cmNinjaTargetGenerator.cxx
index 3ebf364..4feb0bb 100644
--- a/Source/cmNinjaTargetGenerator.cxx
+++ b/Source/cmNinjaTargetGenerator.cxx
@@ -379,10 +379,11 @@ cmNinjaDeps cmNinjaTargetGenerator::ComputeLinkDeps(
   return result;
 }
 
-std::string cmNinjaTargetGenerator::GetSourceFilePath(
+std::string cmNinjaTargetGenerator::GetCompiledSourceNinjaPath(
   cmSourceFile const* source) const
 {
-  return this->ConvertToNinjaPath(source->GetFullPath());
+  // Pass source files to the compiler by absolute path.
+  return this->ConvertToNinjaAbsPath(source->GetFullPath());
 }
 
 std::string cmNinjaTargetGenerator::GetObjectFilePath(
@@ -987,7 +988,7 @@ void cmNinjaTargetGenerator::WriteObjectBuildStatements(
     this->GeneratorTarget->GetExternalObjects(externalObjects, config);
     for (cmSourceFile const* sf : externalObjects) {
       auto objectFileName = this->GetGlobalGenerator()->ExpandCFGIntDir(
-        this->GetSourceFilePath(sf), config);
+        this->ConvertToNinjaPath(sf->GetFullPath()), config);
       if (!cmSystemTools::StringEndsWith(objectFileName,
                                          cmToCStr(pchExtension))) {
         this->Configs[config].Objects.push_back(objectFileName);
@@ -1199,8 +1200,7 @@ void cmNinjaTargetGenerator::WriteObjectBuildStatement(
   const std::string& fileConfig, bool firstForConfig)
 {
   std::string const language = source->GetLanguage();
-  std::string const sourceFileName =
-    language == "RC" ? source->GetFullPath() : this->GetSourceFilePath(source);
+  std::string const sourceFilePath = this->GetCompiledSourceNinjaPath(source);
   std::string const objectDir = this->ConvertToNinjaPath(
     cmStrCat(this->GeneratorTarget->GetSupportDirectory(),
              this->GetGlobalGenerator()->ConfigDirectory(config)));
@@ -1251,7 +1251,7 @@ void cmNinjaTargetGenerator::WriteObjectBuildStatement(
   }
 
   this->ExportObjectCompileCommand(
-    language, sourceFileName, objectDir, objectFileName, objectFileDir,
+    language, sourceFilePath, objectDir, objectFileName, objectFileDir,
     vars["FLAGS"], vars["DEFINES"], vars["INCLUDES"], config);
 
   objBuild.Outputs.push_back(objectFileName);
@@ -1265,7 +1265,7 @@ void cmNinjaTargetGenerator::WriteObjectBuildStatement(
     }
   }
 
-  objBuild.ExplicitDeps.push_back(sourceFileName);
+  objBuild.ExplicitDeps.push_back(sourceFilePath);
 
   // Add precompile headers dependencies
   std::vector<std::string> depList;
@@ -1323,9 +1323,9 @@ void cmNinjaTargetGenerator::WriteObjectBuildStatement(
   if (source->GetIsGenerated() &&
       !source->GetPropertyAsBool("__CMAKE_GENERATED_BY_CMAKE") &&
       !source->GetCustomCommand() &&
-      !this->GetGlobalGenerator()->HasCustomCommandOutput(sourceFileName)) {
+      !this->GetGlobalGenerator()->HasCustomCommandOutput(sourceFilePath)) {
     this->GetGlobalGenerator()->AddAssumedSourceDependencies(
-      sourceFileName, objBuild.OrderOnlyDeps);
+      sourceFilePath, objBuild.OrderOnlyDeps);
   }
 
   // For some cases we scan to dynamically discover dependencies.
@@ -1575,8 +1575,7 @@ void cmNinjaTargetGenerator::WriteTargetDependInfo(std::string const& lang,
 void cmNinjaTargetGenerator::EmitSwiftDependencyInfo(
   cmSourceFile const* source, const std::string& config)
 {
-  std::string const sourceFilePath =
-    this->ConvertToNinjaPath(this->GetSourceFilePath(source));
+  std::string const sourceFilePath = this->GetCompiledSourceNinjaPath(source);
   std::string const objectFilePath =
     this->ConvertToNinjaPath(this->GetObjectFilePath(source, config));
   std::string const swiftDepsPath = [source, objectFilePath]() -> std::string {
diff --git a/Source/cmNinjaTargetGenerator.h b/Source/cmNinjaTargetGenerator.h
index 3a28cef..4b4cf8d 100644
--- a/Source/cmNinjaTargetGenerator.h
+++ b/Source/cmNinjaTargetGenerator.h
@@ -101,7 +101,7 @@ protected:
                               const std::string& language,
                               const std::string& config);
 
-  std::string ConvertToNinjaPath(const std::string& path) const
+  std::string const& ConvertToNinjaPath(const std::string& path) const
   {
     return this->GetGlobalGenerator()->ConvertToNinjaPath(path);
   }
@@ -110,13 +110,18 @@ protected:
     return this->GetGlobalGenerator()->MapToNinjaPath();
   }
 
+  std::string ConvertToNinjaAbsPath(std::string path) const
+  {
+    return this->GetGlobalGenerator()->ConvertToNinjaAbsPath(std::move(path));
+  }
+
   /// @return the list of link dependency for the given target @a target.
   cmNinjaDeps ComputeLinkDeps(const std::string& linkLanguage,
                               const std::string& config,
                               bool ignoreType = false) const;
 
   /// @return the source file path for the given @a source.
-  std::string GetSourceFilePath(cmSourceFile const* source) const;
+  std::string GetCompiledSourceNinjaPath(cmSourceFile const* source) const;
 
   /// @return the object file path for the given @a source.
   std::string GetObjectFilePath(cmSourceFile const* source,
diff --git a/Tests/RunCMake/Ninja/RunCMakeTest.cmake b/Tests/RunCMake/Ninja/RunCMakeTest.cmake
index 1350326..8c91b34 100644
--- a/Tests/RunCMake/Ninja/RunCMakeTest.cmake
+++ b/Tests/RunCMake/Ninja/RunCMakeTest.cmake
@@ -163,12 +163,12 @@ 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")
+  run_ninja("${RunCMake_TEST_BINARY_DIR}" "${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")
+  run_ninja("${RunCMake_TEST_BINARY_DIR}" "${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'.")