Merge topic 'makefile-target-special'

a4173ef165 Tests: Enable coverage of special chars in include dirs for Makefiles
d74e651b78 Makefiles: Re-implement makefile target path escaping and quoting
031bfaa865 Makefiles: Factor out makefile target path escaping and quoting
ca343dad07 Makefiles: Convert paths with '#' on command-lines to short path on Windows
af7de05853 Makefiles: Do not use '\#' escape sequence with Windows-style make tools
1639ee70ef cmDepends: Update types to always use a Makefile generator
413d26030f cmGlobalNinjaGenerator: Remove outdated comment

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

Source/cmDepends.cxx

@@ -9,12 +9,12 @@
 #include "cmFileTime.h"
 #include "cmFileTimeCache.h"
 #include "cmGeneratedFileStream.h"
-#include "cmLocalGenerator.h"
+#include "cmLocalUnixMakefileGenerator3.h"
 #include "cmMakefile.h"
 #include "cmStringAlgorithms.h"
 #include "cmSystemTools.h"
 
-cmDepends::cmDepends(cmLocalGenerator* lg, std::string targetDir)
+cmDepends::cmDepends(cmLocalUnixMakefileGenerator3* lg, std::string targetDir)
   : LocalGenerator(lg)
   , TargetDirectory(std::move(targetDir))
 {

Source/cmDepends.h

@@ -12,7 +12,7 @@
 #include <vector>
 
 class cmFileTimeCache;
-class cmLocalGenerator;
+class cmLocalUnixMakefileGenerator3;
 
 /** \class cmDepends
  * \brief Dependency scanner superclass.
@@ -29,7 +29,8 @@ public:
 public:
   /** Instances need to know the build directory name and the relative
       path from the build directory to the target file.  */
-  cmDepends(cmLocalGenerator* lg = nullptr, std::string targetDir = "");
+  cmDepends(cmLocalUnixMakefileGenerator3* lg = nullptr,
+            std::string targetDir = "");
 
   cmDepends(cmDepends const&) = delete;
   cmDepends& operator=(cmDepends const&) = delete;
@@ -38,7 +39,10 @@ public:
       scanning dependencies.  This is not a full local generator; it
       has been setup to do relative path conversions for the current
       directory.  */
-  void SetLocalGenerator(cmLocalGenerator* lg) { this->LocalGenerator = lg; }
+  void SetLocalGenerator(cmLocalUnixMakefileGenerator3* lg)
+  {
+    this->LocalGenerator = lg;
+  }
 
   /** Set the specific language to be scanned.  */
   void SetLanguage(const std::string& lang) { this->Language = lang; }
@@ -92,7 +96,7 @@ protected:
                         std::ostream& internalDepends);
 
   // The local generator.
-  cmLocalGenerator* LocalGenerator;
+  cmLocalUnixMakefileGenerator3* LocalGenerator;
 
   // Flag for verbose output.
   bool Verbose = false;

Source/cmDependsC.cxx

@@ -7,7 +7,7 @@
 #include "cmsys/FStream.hxx"
 
 #include "cmFileTime.h"
-#include "cmLocalGenerator.h"
+#include "cmLocalUnixMakefileGenerator3.h"
 #include "cmMakefile.h"
 #include "cmStringAlgorithms.h"
 #include "cmSystemTools.h"
@@ -22,8 +22,9 @@
 
 cmDependsC::cmDependsC() = default;
 
-cmDependsC::cmDependsC(cmLocalGenerator* lg, const std::string& targetDir,
-                       const std::string& lang, const DependencyMap* validDeps)
+cmDependsC::cmDependsC(cmLocalUnixMakefileGenerator3* lg,
+                       const std::string& targetDir, const std::string& lang,
+                       const DependencyMap* validDeps)
   : cmDepends(lg, targetDir)
   , ValidDeps(validDeps)
 {
@@ -211,12 +212,12 @@ bool cmDependsC::WriteDependencies(const std::set<std::string>& sources,
   // written by the original local generator for this directory
   // convert the dependencies to paths relative to the home output
   // directory.  We must do the same here.
-  std::string obj_m = cmSystemTools::ConvertToOutputPath(obj_i);
+  std::string obj_m = this->LocalGenerator->ConvertToMakefilePath(obj_i);
   internalDepends << obj_i << '\n';
 
   for (std::string const& dep : dependencies) {
     makeDepends << obj_m << ": "
-                << cmSystemTools::ConvertToOutputPath(
+                << this->LocalGenerator->ConvertToMakefilePath(
                      this->LocalGenerator->MaybeConvertToRelativePath(binDir,
                                                                       dep))
                 << '\n';

Source/cmDependsC.h

@@ -16,7 +16,7 @@
 
 #include "cmDepends.h"
 
-class cmLocalGenerator;
+class cmLocalUnixMakefileGenerator3;
 
 /** \class cmDependsC
  * \brief Dependency scanner for C and C++ object files.
@@ -27,7 +27,7 @@ public:
   /** Checking instances need to know the build directory name and the
       relative path from the build directory to the target file.  */
   cmDependsC();
-  cmDependsC(cmLocalGenerator* lg, const std::string& targetDir,
+  cmDependsC(cmLocalUnixMakefileGenerator3* lg, const std::string& targetDir,
              const std::string& lang, const DependencyMap* validDeps);
 
   /** Virtual destructor to cleanup subclasses properly.  */

Source/cmDependsFortran.cxx

@@ -12,7 +12,7 @@
 
 #include "cmFortranParser.h" /* Interface to parser object.  */
 #include "cmGeneratedFileStream.h"
-#include "cmLocalGenerator.h"
+#include "cmLocalUnixMakefileGenerator3.h"
 #include "cmMakefile.h"
 #include "cmOutputConverter.h"
 #include "cmStateDirectory.h"
@@ -70,7 +70,7 @@ public:
 
 cmDependsFortran::cmDependsFortran() = default;
 
-cmDependsFortran::cmDependsFortran(cmLocalGenerator* lg)
+cmDependsFortran::cmDependsFortran(cmLocalUnixMakefileGenerator3* lg)
   : cmDepends(lg)
   , Internal(new cmDependsFortranInternals)
 {

Source/cmDependsFortran.h

@@ -15,7 +15,7 @@
 
 class cmDependsFortranInternals;
 class cmFortranSourceInfo;
-class cmLocalGenerator;
+class cmLocalUnixMakefileGenerator3;
 
 /** \class cmDependsFortran
  * \brief Dependency scanner for Fortran object files.
@@ -31,7 +31,7 @@ public:
       path from the build directory to the target file, the source
       file from which to start scanning, the include file search
       path, and the target directory.  */
-  cmDependsFortran(cmLocalGenerator* lg);
+  cmDependsFortran(cmLocalUnixMakefileGenerator3* lg);
 
   /** Virtual destructor to cleanup subclasses properly.  */
   ~cmDependsFortran() override;

Source/cmGlobalBorlandMakefileGenerator.h

@@ -46,6 +46,7 @@ public:
 
   bool AllowNotParallel() const override { return false; }
   bool AllowDeleteOnError() const override { return false; }
+  bool CanEscapeOctothorpe() const override { return true; }
 
 protected:
   std::vector<GeneratedMakeCommand> GenerateBuildCommand(

Source/cmGlobalNinjaGenerator.cxx

@@ -436,8 +436,6 @@ cmGlobalNinjaGenerator::cmGlobalNinjaGenerator(cmake* cm)
 #ifdef _WIN32
   cm->GetState()->SetWindowsShell(true);
 #endif
-  // // Ninja is not ported to non-Unix OS yet.
-  // this->ForceUnixPaths = true;
   this->FindMakeProgramFile = "CMakeNinjaFindMake.cmake";
 }
 

Source/cmGlobalUnixMakefileGenerator3.cxx

@@ -117,6 +117,12 @@ void cmGlobalUnixMakefileGenerator3::ComputeTargetObjectDirectory(
   gt->ObjectDirectory = dir;
 }
 
+bool cmGlobalUnixMakefileGenerator3::CanEscapeOctothorpe() const
+{
+  // Make tools that use UNIX-style '/' paths also support '\' escaping.
+  return this->ForceUnixPaths;
+}
+
 void cmGlobalUnixMakefileGenerator3::Configure()
 {
   // Initialize CMAKE_EDIT_COMMAND cache entry.
@@ -480,6 +486,78 @@ void cmGlobalUnixMakefileGenerator3::WriteDirectoryRules2(
   }
 }
 
+namespace {
+std::string ConvertToMakefilePathForUnix(std::string const& path)
+{
+  std::string result;
+  result.reserve(path.size());
+  for (char c : path) {
+    switch (c) {
+      case '=':
+        // We provide 'EQUALS = =' to encode '=' in a non-assignment case.
+        result.append("$(EQUALS)");
+        break;
+      case '$':
+        result.append("$$");
+        break;
+      case '\\':
+      case ' ':
+      case '#':
+        result.push_back('\\');
+        CM_FALLTHROUGH;
+      default:
+        result.push_back(c);
+        break;
+    }
+  }
+  return result;
+}
+
+#if defined(_WIN32) && !defined(__CYGWIN__)
+std::string ConvertToMakefilePathForWindows(std::string const& path)
+{
+  bool const quote = path.find_first_of(" #") != std::string::npos;
+  std::string result;
+  result.reserve(path.size() + (quote ? 2 : 0));
+  if (quote) {
+    result.push_back('"');
+  }
+  for (char c : path) {
+    switch (c) {
+      case '=':
+        // We provide 'EQUALS = =' to encode '=' in a non-assignment case.
+        result.append("$(EQUALS)");
+        break;
+      case '$':
+        result.append("$$");
+        break;
+      case '/':
+        result.push_back('\\');
+        break;
+      default:
+        result.push_back(c);
+        break;
+    }
+  }
+  if (quote) {
+    result.push_back('"');
+  }
+  return result;
+}
+#endif
+}
+
+std::string cmGlobalUnixMakefileGenerator3::ConvertToMakefilePath(
+  std::string const& path) const
+{
+#if defined(_WIN32) && !defined(__CYGWIN__)
+  if (!this->ForceUnixPaths) {
+    return ConvertToMakefilePathForWindows(path);
+  }
+#endif
+  return ConvertToMakefilePathForUnix(path);
+}
+
 std::vector<cmGlobalGenerator::GeneratedMakeCommand>
 cmGlobalUnixMakefileGenerator3::GenerateBuildCommand(
   const std::string& makeProgram, const std::string& /*projectName*/,

Source/cmGlobalUnixMakefileGenerator3.h

@@ -136,6 +136,12 @@ public:
       or dependencies.  */
   std::string GetEmptyRuleHackDepends() { return this->EmptyRuleHackDepends; }
 
+  /**
+   * Convert a file path to a Makefile target or dependency with
+   * escaping and quoting suitable for the generator's make tool.
+   */
+  std::string ConvertToMakefilePath(std::string const& path) const;
+
   // change the build command for speed
   std::vector<GeneratedMakeCommand> GenerateBuildCommand(
     const std::string& makeProgram, const std::string& projectName,
@@ -157,6 +163,9 @@ public:
   /** Does the make tool tolerate .DELETE_ON_ERROR? */
   virtual bool AllowDeleteOnError() const { return true; }
 
+  /** Does the make tool interpret '\#' as '#'?  */
+  virtual bool CanEscapeOctothorpe() const;
+
   bool IsIPOSupported() const override { return true; }
 
   void ComputeTargetObjectDirectory(cmGeneratorTarget* gt) const override;

Source/cmLocalUnixMakefileGenerator3.cxx

@@ -48,37 +48,6 @@
 #  include "cmDependsJava.h"
 #endif
 
-// Escape special characters in Makefile dependency lines
-class cmMakeSafe
-{
-public:
-  cmMakeSafe(const char* s)
-    : Data(s)
-  {
-  }
-  cmMakeSafe(std::string const& s)
-    : Data(s.c_str())
-  {
-  }
-
-private:
-  const char* Data;
-  friend std::ostream& operator<<(std::ostream& os, cmMakeSafe const& self)
-  {
-    for (const char* c = self.Data; *c; ++c) {
-      switch (*c) {
-        case '=':
-          os << "$(EQUALS)";
-          break;
-        default:
-          os << *c;
-          break;
-      }
-    }
-    return os;
-  }
-};
-
 // Helper function used below.
 static std::string cmSplitExtension(std::string const& in, std::string& base)
 {
@@ -498,6 +467,14 @@ const std::string& cmLocalUnixMakefileGenerator3::GetHomeRelativeOutputPath()
   return this->HomeRelativeOutputPath;
 }
 
+std::string cmLocalUnixMakefileGenerator3::ConvertToMakefilePath(
+  std::string const& path) const
+{
+  cmGlobalUnixMakefileGenerator3* gg =
+    static_cast<cmGlobalUnixMakefileGenerator3*>(this->GlobalGenerator);
+  return gg->ConvertToMakefilePath(path);
+}
+
 void cmLocalUnixMakefileGenerator3::WriteMakeRule(
   std::ostream& os, const char* comment, const std::string& target,
   const std::vector<std::string>& depends,
@@ -528,7 +505,7 @@ void cmLocalUnixMakefileGenerator3::WriteMakeRule(
   }
 
   // Construct the left hand side of the rule.
-  std::string tgt = cmSystemTools::ConvertToOutputPath(
+  std::string tgt = this->ConvertToMakefilePath(
     this->MaybeConvertToRelativePath(this->GetBinaryDirectory(), target));
 
   const char* space = "";
@@ -542,30 +519,30 @@ void cmLocalUnixMakefileGenerator3::WriteMakeRule(
   if (symbolic) {
     if (const char* sym =
           this->Makefile->GetDefinition("CMAKE_MAKE_SYMBOLIC_RULE")) {
-      os << cmMakeSafe(tgt) << space << ": " << sym << "\n";
+      os << tgt << space << ": " << sym << "\n";
     }
   }
 
   // Write the rule.
   if (depends.empty()) {
     // No dependencies.  The commands will always run.
-    os << cmMakeSafe(tgt) << space << ":\n";
+    os << tgt << space << ":\n";
   } else {
     // Split dependencies into multiple rule lines.  This allows for
     // very long dependency lists even on older make implementations.
     std::string binDir = this->GetBinaryDirectory();
     for (std::string const& depend : depends) {
-      replace = depend;
-      replace = cmSystemTools::ConvertToOutputPath(
-        this->MaybeConvertToRelativePath(binDir, replace));
-      os << cmMakeSafe(tgt) << space << ": " << cmMakeSafe(replace) << "\n";
+      os << tgt << space << ": "
+         << this->ConvertToMakefilePath(
+              this->MaybeConvertToRelativePath(binDir, depend))
+         << '\n';
     }
   }
 
   // Write the list of commands.
   os << cmWrap("\t", commands, "", "\n") << "\n";
   if (symbolic && !this->IsWatcomWMake()) {
-    os << ".PHONY : " << cmMakeSafe(tgt) << "\n";
+    os << ".PHONY : " << tgt << "\n";
   }
   os << "\n";
   // Add the output to the local help if requested.

Source/cmLocalUnixMakefileGenerator3.h

@@ -46,6 +46,12 @@ public:
   // local generators StartOutputDirectory
   const std::string& GetHomeRelativeOutputPath();
 
+  /**
+   * Convert a file path to a Makefile target or dependency with
+   * escaping and quoting suitable for the generator's make tool.
+   */
+  std::string ConvertToMakefilePath(std::string const& path) const;
+
   // Write out a make rule
   void WriteMakeRule(std::ostream& os, const char* comment,
                      const std::string& target,

Source/cmMakefileTargetGenerator.cxx

@@ -341,12 +341,16 @@ void cmMakefileTargetGenerator::WriteTargetLanguageFlags()
                           << "\n";
   }
 
+  bool const escapeOctothorpe = this->GlobalGenerator->CanEscapeOctothorpe();
+
   for (std::string const& language : languages) {
     std::string defines = this->GetDefines(language, this->GetConfigName());
     std::string includes = this->GetIncludes(language, this->GetConfigName());
-    // Escape comment characters so they do not terminate assignment.
-    cmSystemTools::ReplaceString(defines, "#", "\\#");
-    cmSystemTools::ReplaceString(includes, "#", "\\#");
+    if (escapeOctothorpe) {
+      // Escape comment characters so they do not terminate assignment.
+      cmSystemTools::ReplaceString(defines, "#", "\\#");
+      cmSystemTools::ReplaceString(includes, "#", "\\#");
+    }
     *this->FlagFileStream << language << "_DEFINES = " << defines << "\n\n";
     *this->FlagFileStream << language << "_INCLUDES = " << includes << "\n\n";
 
@@ -357,7 +361,9 @@ void cmMakefileTargetGenerator::WriteTargetLanguageFlags()
     for (const std::string& arch : architectures) {
       std::string flags =
         this->GetFlags(language, this->GetConfigName(), arch);
-      cmSystemTools::ReplaceString(flags, "#", "\\#");
+      if (escapeOctothorpe) {
+        cmSystemTools::ReplaceString(flags, "#", "\\#");
+      }
       *this->FlagFileStream << language << "_FLAGS" << arch << " = " << flags
                             << "\n\n";
     }

Source/cmOutputConverter.cxx

@@ -26,7 +26,7 @@ std::string cmOutputConverter::ConvertToOutputForExisting(
   // already exists, we can use a short-path to reference it without a
   // space.
   if (this->GetState()->UseWindowsShell() &&
-      remote.find(' ') != std::string::npos &&
+      remote.find_first_of(" #") != std::string::npos &&
       cmSystemTools::FileExists(remote)) {
     std::string tmp;
     if (cmSystemTools::GetShortPath(remote, tmp)) {

Tests/CMakeLists.txt

@@ -3477,6 +3477,8 @@ ${CMake_SOURCE_DIR}/Utilities/Release/push.bash --dir dev -- '${CMake_BUILD_NIGH
     --build-two-config
     ${build_generator_args}
     --build-project IncludeDirectories
+    --build-options
+      -DMAKE_SUPPORTS_SPACES=${MAKE_SUPPORTS_SPACES}
     --test-command IncludeDirectories)
   list(APPEND TEST_BUILD_DIRS "${CMake_BINARY_DIR}/Tests/IncludeDirectories")
 

Tests/IncludeDirectories/CMakeLists.txt

@@ -66,21 +66,36 @@ else()
 endif()
 
 # Test escaping of special characters in include directory paths.
-# FIXME: Implement full support in Makefile generators
-if(NOT CMAKE_GENERATOR MATCHES "Make")
-  set(special_chars "~@#$%^&=[]{}()!'")
-  if(NOT CMAKE_GENERATOR STREQUAL "Visual Studio 9 2008")
-    string(APPEND special_chars ",")
-  endif()
-  if(NOT WIN32 AND NOT CYGWIN)
-    string(APPEND special_chars "*?<>")
-  endif()
-  set(special_dir "${CMAKE_CURRENT_BINARY_DIR}/special-${special_chars}-include")
-  file(WRITE "${special_dir}/SpecialDir.h" "#define SPECIAL_DIR_H\n")
+set(special_chars "~@%&{}()!'")
+if(NOT CMAKE_GENERATOR STREQUAL "Watcom WMake")
+  # Watcom seems to have no way to encode these characters.
+  string(APPEND special_chars "#=[]")
+endif()
+if(NOT (MINGW AND CMAKE_GENERATOR MATCHES "(Unix|MSYS) Makefiles"))
+  # FIXME: Dependencies work but command-line generation does not handle '$'.
+  string(APPEND special_chars "$")
+endif()
+if(NOT CMAKE_GENERATOR MATCHES "(Borland|NMake) Makefiles")
+  # NMake and Borland seem to have no way to encode a single '^'.
+  string(APPEND special_chars "^")
+endif()
+if(NOT CMAKE_GENERATOR MATCHES "Visual Studio 9 2008|Watcom WMake")
+  # The vcproj format separates values with ','.
+  string(APPEND special_chars ",")
+endif()
+if(NOT WIN32 AND NOT CYGWIN)
+  string(APPEND special_chars "*?<>")
+endif()
+set(special_dir "${CMAKE_CURRENT_BINARY_DIR}/special-${special_chars}-include")
+file(WRITE "${special_dir}/SpecialDir.h" "#define SPECIAL_DIR_H\n")
+target_include_directories(IncludeDirectories PRIVATE "${special_dir}")
+target_compile_definitions(IncludeDirectories PRIVATE INCLUDE_SPECIAL_DIR)
+
+if(MAKE_SUPPORTS_SPACES)
   set(special_space_dir "${CMAKE_CURRENT_BINARY_DIR}/special-space ${special_chars}-include")
   file(WRITE "${special_space_dir}/SpecialSpaceDir.h" "#define SPECIAL_SPACE_DIR_H\n")
-  target_include_directories(IncludeDirectories PRIVATE "${special_dir}" "${special_space_dir}")
-  target_compile_definitions(IncludeDirectories PRIVATE INCLUDE_SPECIAL_DIR)
+  target_include_directories(IncludeDirectories PRIVATE "${special_space_dir}")
+  target_compile_definitions(IncludeDirectories PRIVATE INCLUDE_SPECIAL_SPACE_DIR)
 endif()
 
 add_library(ordertest ordertest.cpp)

Tests/IncludeDirectories/main.cpp

@@ -8,6 +8,9 @@
 #  ifndef SPECIAL_DIR_H
 #    error "SPECIAL_DIR_H not defined"
 #  endif
+#endif
+
+#ifdef INCLUDE_SPECIAL_SPACE_DIR
 #  include "SpecialSpaceDir.h"
 #  ifndef SPECIAL_SPACE_DIR_H
 #    error "SPECIAL_SPACE_DIR_H not defined"

Tests/TryCompile/CMakeLists.txt

@@ -187,7 +187,7 @@ try_compile(SHOULD_FAIL_DUE_TO_BAD_SOURCE
 if(SHOULD_FAIL_DUE_TO_BAD_SOURCE AND NOT CMAKE_GENERATOR MATCHES "Watcom WMake|NMake Makefiles")
   string(REPLACE "\n" "\n  " output "  ${output}")
   message(SEND_ERROR "try_compile with bad#source.c did not fail:\n${output}")
-elseif(NOT output MATCHES [[(bad#source\.c|bad\\)]])
+elseif(NOT output MATCHES [[(bad#source\.c|bad\.c|bad')]])
   string(REPLACE "\n" "\n  " output "  ${output}")
   message(SEND_ERROR "try_compile with bad#source.c failed without mentioning bad source:\n${output}")
 else()