Fix running executables with spaces in their paths in Windows (#777)

Author: asafgabai

build-info-extractor-go/src/main/java/org/jfrog/build/extractor/go/GoDriver.java

@@ -1,8 +1,6 @@
 package org.jfrog.build.extractor.go;
 
-import com.google.common.collect.Maps;
 import org.apache.commons.lang3.StringUtils;
-import org.apache.commons.lang3.SystemUtils;
 import org.jfrog.build.api.util.Log;
 import org.jfrog.build.extractor.executor.CommandExecutor;
 import org.jfrog.build.extractor.executor.CommandResults;
@@ -26,60 +24,18 @@ public class GoDriver implements Serializable {
     private static final String GO_GET_CMD = "get";
     private static final String GO_LIST_MODULE_CMD = "list -m";
     private static final String GO_VERSION_CMD = "version";
-    private static final String DEFAULT_EXECUTABLE_NAME = "go";
 
     private static final long serialVersionUID = 1L;
     private final CommandExecutor commandExecutor;
     private final File workingDirectory;
     private final Log logger;
 
     public GoDriver(String executablePath, Map<String, String> env, File workingDirectory, Log logger) {
-        this.commandExecutor = generateCommandExecutor(executablePath, env);
+        this.commandExecutor = new CommandExecutor(StringUtils.defaultIfEmpty(executablePath, "go"), env);
         this.workingDirectory = workingDirectory;
         this.logger = logger;
     }
 
-    /**
-     * Generate a new mutable copy of environment variables map with the Go executable directory path inserted to the beginning of the Path.
-     *
-     * @param executablePath Go executable path
-     * @param env            Environment variables map
-     * @return a new Environment variables map
-     */
-    static Map<String, String> generateWindowsEnv(String executablePath, Map<String, String> env) {
-        // If executablePath ends with "go" or "go.exe" - remove it from the directory path
-        executablePath = StringUtils.removeEnd(executablePath, ".exe");
-        executablePath = StringUtils.removeEnd(executablePath, DEFAULT_EXECUTABLE_NAME);
-
-        // Insert the Go executable directory path to the beginning of the Path environment variable
-        // Make sure to copy the environment variables map to avoid changing the original map or in case it is immutable
-        Map<String, String> newEnv = Maps.newHashMap(env);
-        String windowsPathEnvKey = "Path";
-        if (newEnv.containsKey(windowsPathEnvKey)) {
-            newEnv.put(windowsPathEnvKey, executablePath + File.pathSeparator + newEnv.get(windowsPathEnvKey));
-        } else {
-            newEnv.put(windowsPathEnvKey, executablePath);
-        }
-        return newEnv;
-    }
-
-    /**
-     * Create a CommandExecutor with the given executable path and environment variables.
-     *
-     * @param executablePath Go executable path
-     * @param env            Environment variables map
-     * @return CommandExecutor
-     */
-    private static CommandExecutor generateCommandExecutor(String executablePath, Map<String, String> env) {
-        if (!SystemUtils.IS_OS_WINDOWS || StringUtils.isBlank(executablePath) || StringUtils.equals(executablePath, DEFAULT_EXECUTABLE_NAME) || env == null) {
-            return new CommandExecutor(StringUtils.defaultIfEmpty(executablePath, DEFAULT_EXECUTABLE_NAME), env);
-        }
-        // Handling Windows case with a given executable path:
-        // A bug was identified for the Go executable in Windows where the executable path may be incorrectly parsed
-        // as two command arguments when the path contains spaces (e.g., "C:\Program Files\Go\bin\go.exe").
-        return new CommandExecutor(DEFAULT_EXECUTABLE_NAME, generateWindowsEnv(executablePath, env));
-    }
-
     public CommandResults runCmd(String args, boolean verbose) throws IOException {
         List<String> argsList = new ArrayList<>(Arrays.asList(args.split(" ")));
         return runCmd(argsList, verbose);

build-info-extractor-go/src/test/java/org/jfrog/build/extractor/go/GoDriverTest.java

@@ -2,11 +2,9 @@
 
 import com.google.common.collect.Sets;
 import org.apache.commons.io.FileUtils;
-import org.apache.commons.lang3.SystemUtils;
 import org.jfrog.build.api.util.NullLog;
 import org.jfrog.build.extractor.executor.CommandResults;
 import org.testng.Assert;
-import org.testng.SkipException;
 import org.testng.annotations.Test;
 
 import java.io.File;
@@ -15,7 +13,6 @@
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.Arrays;
-import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
 
@@ -117,20 +114,4 @@ public void testGoGet() throws IOException {
             FileUtils.deleteDirectory(projectDir);
         }
     }
-
-    @Test
-    public void testGoDriverWindowsInit() throws IOException {
-        if (!SystemUtils.IS_OS_WINDOWS) {
-            throw new SkipException("Skipping non-windows test");
-        }
-        File projectDir = Files.createTempDirectory("").toFile();
-        try {
-            Map<String, String> systemEnv = System.getenv();
-            String executablePath = "C:\\Program Files\\Go\\bin\\go";
-            Map<String, String> executorEnv = GoDriver.generateWindowsEnv(executablePath, systemEnv);
-            assertTrue(executorEnv.get("Path").startsWith("C:\\Program Files\\Go\\bin"));
-        } finally {
-            FileUtils.deleteDirectory(projectDir);
-        }
-    }
 }

build-info-extractor/src/main/java/org/jfrog/build/extractor/executor/CommandExecutor.java

@@ -1,5 +1,6 @@
 package org.jfrog.build.extractor.executor;
 
+import com.google.common.collect.Maps;
 import org.apache.commons.lang3.SystemUtils;
 import org.jfrog.build.api.util.Log;
 import org.jfrog.build.extractor.UrlUtils;
@@ -8,11 +9,12 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.Serializable;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.*;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
-import java.util.stream.Collectors;
 
 import static java.lang.String.format;
 import static java.lang.String.join;
@@ -26,15 +28,15 @@ public class CommandExecutor implements Serializable {
     private static final int READER_SHUTDOWN_TIMEOUT_SECONDS = 30;
     private static final int PROCESS_TERMINATION_TIMEOUT_SECONDS = 30;
     private static final int EXECUTION_TIMEOUT_MINUTES = 120;
-    private Map<String, String> env;
+    private final Map<String, String> env;
     private final String executablePath;
 
     /**
      * @param executablePath - Executable path.
      * @param env            - Environment variables to use during execution.
      */
     public CommandExecutor(String executablePath, Map<String, String> env) {
-        this.executablePath = escapeSpacesInPath(executablePath);
+        this.executablePath = executablePath.trim();
         Map<String, String> finalEnvMap = new HashMap<>(System.getenv());
         if (env != null) {
             Map<String, String> fixedEnvMap = new HashMap<>(env);
@@ -133,10 +135,9 @@ public CommandResults exeCommand(File execDir, List<String> args, List<String> c
      */
     public CommandResults exeCommand(File execDir, List<String> args, List<String> credentials, Log logger, long timeout, TimeUnit unit) throws InterruptedException, IOException {
         List<String> command = new ArrayList<>(args);
-        command.add(0, executablePath);
         ExecutorService service = Executors.newFixedThreadPool(2);
         try {
-            Process process = runProcess(execDir, command, credentials, env, logger);
+            Process process = runProcess(execDir, executablePath, command, credentials, env, logger);
             // The output stream is not necessary in non-interactive scenarios, therefore we can close it now.
             process.getOutputStream().close();
             try (InputStream inputStream = process.getInputStream(); InputStream errorStream = process.getErrorStream()) {
@@ -179,38 +180,86 @@ private CommandResults getCommandResults(boolean terminatedProperly, List<String
         return commandRes;
     }
 
-    private static Process runProcess(File execDir, List<String> args, List<String> credentials, Map<String, String> env, Log logger) throws IOException {
+    private static Process runProcess(File execDir, String executablePath, List<String> args, List<String> credentials, Map<String, String> env, Log logger) throws IOException {
+        // Make sure to copy the environment variables map to avoid changing the original map or in case it is immutable.
+        Map<String, String> newEnv = Maps.newHashMap(env);
+
+        args = formatCommand(args, credentials, executablePath, newEnv);
+        logCommand(logger, args, credentials);
+        ProcessBuilder processBuilder = new ProcessBuilder(args)
+                .directory(execDir);
+        processBuilder.environment().putAll(newEnv);
+        return processBuilder.start();
+    }
+
+    /**
+     * Formats a command for execution, incorporating credentials and environment variables.
+     *
+     * @param args           the list of arguments to be included in the command
+     * @param credentials    if specified, the credentials will be concatenated to the command
+     * @param executablePath the path to the executable to be executed
+     * @param env            environment variables map. It might be modified as part of the formatting process
+     * @return the formatted command as a list of strings, ready for execution
+     */
+    private static List<String> formatCommand(List<String> args, List<String> credentials, String executablePath, Map<String, String> env) {
         if (credentials != null) {
             args.addAll(credentials);
         }
+
         if (SystemUtils.IS_OS_WINDOWS) {
-            args.addAll(0, Arrays.asList("cmd", "/c"));
+            formatWindowsCommand(args, executablePath, env);
+            return args;
+        }
+        return formatUnixCommand(args, executablePath);
+    }
+
+    /**
+     * Formats a Windows command for execution.
+     *
+     * @param args           the list of arguments to be included in the command
+     * @param executablePath the path to the executable to be executed
+     * @param env            environment variables map. It might be modified as part of the formatting process
+     */
+    private static void formatWindowsCommand(List<String> args, String executablePath, Map<String, String> env) {
+        Path execPath = Paths.get(executablePath);
+        if (execPath.isAbsolute()) {
+            addToWindowsPath(env, execPath);
+            args.add(0, execPath.getFileName().toString());
         } else {
-            String strArgs = join(" ", args);
-            args = new ArrayList<String>() {{
-                add("/bin/sh");
-                add("-c");
-                add(strArgs);
-            }};
+            args.add(0, executablePath.replaceAll(" ", "^ "));
         }
-        logCommand(logger, args, credentials);
-        ProcessBuilder processBuilder = new ProcessBuilder(args)
-                .directory(execDir);
-        processBuilder.environment().putAll(env);
-        return processBuilder.start();
+        args.addAll(0, Arrays.asList("cmd", "/c"));
+    }
+
+    private static List<String> formatUnixCommand(List<String> args, String executablePath) {
+        args.add(0, executablePath.replaceAll(" ", "\\\\ "));
+        String strArgs = join(" ", args);
+        return new ArrayList<String>() {{
+            add("/bin/sh");
+            add("-c");
+            add(strArgs);
+        }};
     }
 
     /**
-     * Escape spaces in the input executable path and trim leading and trailing whitespaces.
+     * Inserts the executable directory path at the beginning of the Path environment variable.
+     * This is done to handle cases where the executable path contains spaces. In such scenarios, the "cmd" command used
+     * to execute this command in Windows may incorrectly parse the path, treating the section after the space as an
+     * argument for the command.
      *
-     * @param executablePath - the executable path to process
-     * @return escaped and trimmed executable path.
+     * @param env      environment variables map
+     * @param execPath the executable path
      */
-    private static String escapeSpacesInPath(String executablePath) {
-        if (executablePath == null) {
-            return null;
+    static void addToWindowsPath(Map<String, String> env, Path execPath) {
+        String execDirPath = execPath.getParent().toString();
+
+        // Insert the executable directory path to the beginning of the Path environment variable.
+        String windowsPathEnvKey = "Path";
+        if (env.containsKey(windowsPathEnvKey)) {
+            env.put(windowsPathEnvKey, execDirPath + File.pathSeparator + env.get(windowsPathEnvKey));
+        } else {
+            env.put(windowsPathEnvKey, execDirPath);
         }
-        return executablePath.trim().replaceAll(" ", SystemUtils.IS_OS_WINDOWS ? "^ " : "\\\\ ");
     }
 
     private static void logCommand(Log logger, List<String> args, List<String> credentials) {

build-info-extractor/src/test/java/org/jfrog/build/extractor/executor/CommandExecutorTest.java

@@ -1,8 +1,11 @@
 package org.jfrog.build.extractor.executor;
 
+import com.google.common.collect.Maps;
 import org.apache.commons.io.FileUtils;
+import org.apache.commons.lang3.SystemUtils;
 import org.apache.commons.lang3.exception.ExceptionUtils;
 import org.jfrog.build.api.util.NullLog;
+import org.testng.SkipException;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 import org.testng.collections.Lists;
@@ -12,8 +15,10 @@
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Map;
 
 import static org.testng.Assert.*;
 
@@ -85,4 +90,20 @@ public void testExeCommandWithSpaces() throws IOException, InterruptedException
             FileUtils.forceDelete(tmpDir.toFile());
         }
     }
+
+    @Test
+    public void testGenerateWindowsEnv() throws IOException {
+        if (!SystemUtils.IS_OS_WINDOWS) {
+            throw new SkipException("Skipping test on non-Windows OS");
+        }
+        File projectDir = Files.createTempDirectory("").toFile();
+        try {
+            Map<String, String> env = Maps.newHashMap(System.getenv());
+            Path execPath = Paths.get("C:\\Program Files\\Go\\bin\\go");
+            CommandExecutor.addToWindowsPath(env, execPath);
+            assertTrue(env.get("Path").startsWith("C:\\Program Files\\Go\\bin"));
+        } finally {
+            FileUtils.deleteDirectory(projectDir);
+        }
+    }
 }