Browse Source

execute_process: Improve invocation of .cmd/.bat with spaces

Extend the fix from commit 74c9d40876 (execute_process: Fix invocation
of .cmd/.bat with spaces, 2025-01-31) to work without relying on
conversion to a "short path", which may not exist.  Instead, extending
the `cmd /c` wrapper to `cmd /c call` seems to support spaces directly.

Suggested-by: Alexandru Croitor <[email protected]>
Fixes: #26655
Brad King 9 months ago
parent
commit
e388ed687a

+ 2 - 2
Help/command/execute_process.rst

@@ -54,8 +54,8 @@ Options:
 
  * .. versionchanged:: 4.0
      On Windows platforms, if the command runs a ``.bat`` or ``.cmd`` script,
-     it is automatically executed through the command interpreter, ``cmd /c``.
-     However, paths with spaces may fail if a "short path" is not available.
+     it is automatically executed through the command interpreter with
+     ``cmd /c call ...``.
 
  No intermediate shell is used, so shell operators such as ``>``
  are treated as normal arguments.

+ 1 - 10
Source/cmExecuteProcessCommand.cxx

@@ -166,16 +166,7 @@ bool cmExecuteProcessCommand(std::vector<std::string> const& args,
       status.SetError(" given COMMAND argument with no value.");
       return false;
     }
-#ifdef _WIN32
-    cmsys::Status shortPathRes = cmSystemTools::MaybePrependCmdExe(cmd);
-    if (!shortPathRes) {
-      status.GetMakefile().IssueMessage(
-        MessageType::WARNING,
-        cmStrCat("Conversion of COMMAND:\n  ", cmd[2], '\n',
-                 "to a short path without spaces failed:\n  ",
-                 shortPathRes.GetString()));
-    }
-#endif
+    cmSystemTools::MaybePrependCmdExe(cmd);
   }
 
   // Parse the timeout string.

+ 5 - 12
Source/cmSystemTools.cxx

@@ -795,38 +795,31 @@ std::size_t cmSystemTools::CalculateCommandLineLengthLimit()
   return sz;
 }
 
-cmsys::Status cmSystemTools::MaybePrependCmdExe(
-  std::vector<std::string>& cmdLine)
+void cmSystemTools::MaybePrependCmdExe(std::vector<std::string>& cmdLine)
 {
 #if defined(_WIN32) && !defined(__CYGWIN__)
-  cmsys::Status status;
   if (!cmdLine.empty()) {
     std::string& applicationName = cmdLine.at(0);
     static cmsys::RegularExpression const winCmdRegex(
       "\\.([Bb][Aa][Tt]|[Cc][Mm][Dd])$");
     cmsys::RegularExpressionMatch winCmdMatch;
     if (winCmdRegex.find(applicationName.c_str(), winCmdMatch)) {
+      // Wrap `.bat` and `.cmd` commands with `cmd /c call`.
       std::vector<std::string> output;
-      output.reserve(cmdLine.size() + 2);
+      output.reserve(cmdLine.size() + 3);
       output.emplace_back(cmSystemTools::GetComspec());
       output.emplace_back("/c");
+      output.emplace_back("call");
       // Convert the batch file path to use backslashes for cmd.exe to parse.
       std::replace(applicationName.begin(), applicationName.end(), '/', '\\');
-      if (applicationName.find(' ') != std::string::npos) {
-        // Convert the batch file path to a short path to avoid spaces.
-        // Otherwise, cmd.exe may not handle arguments with spaces.
-        status = cmSystemTools::GetShortPath(applicationName, applicationName);
-      }
-      output.push_back(applicationName);
+      output.emplace_back(applicationName);
       std::move(cmdLine.begin() + 1, cmdLine.end(),
                 std::back_inserter(output));
       cmdLine = std::move(output);
     }
   }
-  return status;
 #else
   static_cast<void>(cmdLine);
-  return cmsys::Status::Success();
 #endif
 }
 

+ 7 - 12
Source/cmSystemTools.h

@@ -235,23 +235,18 @@ public:
                                            std::string const& destination);
 
   /**
-   *  According to the CreateProcessW documentation which is the underlying
-   *  function for all RunProcess calls:
+   * According to the CreateProcessW documentation:
    *
-   *  "To run a batch file, you must start the command interpreter; set"
-   *  "lpApplicationName to cmd.exe and set lpCommandLine to the following"
-   *  "arguments: /c plus the name of the batch file."
+   *   To run a batch file, you must start the command interpreter; set
+   *   lpApplicationName to cmd.exe and set lpCommandLine to the following
+   *   arguments: /c plus the name of the batch file.
    *
-   *  we should to take care of the correctness of the command line when
-   *  attempting to execute the batch files.
-   *
-   *  Also cmd.exe is unable to parse batch file names correctly if they
-   *  contain spaces. This function uses cmSystemTools::GetShortPath
-   *  conversion to suppress this behavior, and returns its status.
+   * Additionally, "cmd /c" does not always parse batch file names correctly
+   * if they contain spaces, but using "cmd /c call" seems to work.
    *
    *  The function is noop on platforms different from the pure WIN32 one.
    */
-  static cmsys::Status MaybePrependCmdExe(std::vector<std::string>& cmdLine);
+  static void MaybePrependCmdExe(std::vector<std::string>& cmdLine);
 
   /**
    * Run a single executable command

+ 1 - 5
Tests/RunCMake/execute_process/RunCMakeTest.cmake

@@ -63,11 +63,7 @@ if(WIN32 OR CYGWIN)
   run_cmake_command(WindowsNoExtension-build ${CMAKE_COMMAND} --build . --config Debug --target RunScript)
 endif()
 
-if(CMAKE_HOST_WIN32
-    # By default, only C: has short paths enabled.
-    # Since querying with `fsutil 8dot3name query C:`
-    # requires admin, just test the drive letter.
-    AND RunCMake_SOURCE_DIR MATCHES "^[Cc]:")
+if(CMAKE_HOST_WIN32)
   run_cmake_command(WindowsBatch ${CMAKE_COMMAND} -P ${RunCMake_SOURCE_DIR}/WindowsBatch.cmake)
 endif()