Improve run config argument handling, should work with spaces a bit better. (#524)

src/main/java/net/fabricmc/loom/configuration/ide/RunConfig.java

@@ -28,10 +28,11 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 
-import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
 import com.google.gson.JsonElement;
 import com.google.gson.JsonObject;
@@ -44,7 +45,6 @@ import org.w3c.dom.Node;
 
 import net.fabricmc.loom.LoomGradleExtension;
 import net.fabricmc.loom.configuration.InstallerData;
-import net.fabricmc.loom.util.OperatingSystem;
 
 public class RunConfig {
 	public String configName;
@@ -53,8 +53,8 @@ public class RunConfig {
 	public String mainClass;
 	public String runDirIdeaUrl;
 	public String runDir;
-	public String vmArgs;
-	public String programArgs;
+	public List<String> vmArgs = new ArrayList<>();
+	public List<String> programArgs = new ArrayList<>();
 	public SourceSet sourceSet;
 
 	public Element genRuns(Element doc) {
@@ -65,12 +65,12 @@ public class RunConfig {
 		this.addXml(root, "option", ImmutableMap.of("name", "MAIN_CLASS_NAME", "value", mainClass));
 		this.addXml(root, "option", ImmutableMap.of("name", "WORKING_DIRECTORY", "value", runDirIdeaUrl));
 
-		if (!Strings.isNullOrEmpty(vmArgs)) {
-			this.addXml(root, "option", ImmutableMap.of("name", "VM_PARAMETERS", "value", vmArgs));
+		if (!vmArgs.isEmpty()) {
+			this.addXml(root, "option", ImmutableMap.of("name", "VM_PARAMETERS", "value", joinArguments(vmArgs)));
 		}
 
-		if (!Strings.isNullOrEmpty(programArgs)) {
-			this.addXml(root, "option", ImmutableMap.of("name", "PROGRAM_PARAMETERS", "value", programArgs));
+		if (!programArgs.isEmpty()) {
+			this.addXml(root, "option", ImmutableMap.of("name", "PROGRAM_PARAMETERS", "value", joinArguments(programArgs)));
 		}
 
 		return root;
@@ -106,11 +106,10 @@ public class RunConfig {
 	private static void populate(Project project, LoomGradleExtension extension, RunConfig runConfig, String environment) {
 		runConfig.configName += extension.isRootProject() ? "" : " (" + project.getPath() + ")";
 		runConfig.eclipseProjectName = project.getExtensions().getByType(EclipseModel.class).getProject().getName();
-		runConfig.vmArgs = "";
-		runConfig.programArgs = "";
 
 		runConfig.mainClass = "net.fabricmc.devlaunchinjector.Main";
-		runConfig.vmArgs = "-Dfabric.dli.config=" + encodeEscaped(extension.getFiles().getDevLauncherConfig().getAbsolutePath()) + " -Dfabric.dli.env=" + environment.toLowerCase();
+		runConfig.vmArgs.add("-Dfabric.dli.config=" + encodeEscaped(extension.getFiles().getDevLauncherConfig().getAbsolutePath()));
+		runConfig.vmArgs.add("-Dfabric.dli.env=" + environment.toLowerCase());
 	}
 
 	// Turns camelCase/PascalCase into Capital Case
@@ -165,19 +164,9 @@ public class RunConfig {
 		runConfig.sourceSet = sourceSet;
 
 		// Custom parameters
-		for (String progArg : settings.getProgramArgs()) {
-			runConfig.programArgs += " " + progArg;
-		}
-
-		for (String vmArg : settings.getVmArgs()) {
-			runConfig.vmArgs += " " + vmArg;
-		}
-
-		runConfig.vmArgs += " -Dfabric.dli.main=" + getMainClass(environment, extension, defaultMain);
-
-		// Remove unnecessary leading/trailing whitespaces we might have generated
-		runConfig.programArgs = runConfig.programArgs.trim();
-		runConfig.vmArgs = runConfig.vmArgs.trim();
+		runConfig.programArgs.addAll(settings.getProgramArgs());
+		runConfig.vmArgs.addAll(settings.getVmArgs());
+		runConfig.vmArgs.add("-Dfabric.dli.main=" + getMainClass(environment, extension, defaultMain));
 
 		return runConfig;
 	}
@@ -204,18 +193,26 @@ public class RunConfig {
 		dummyConfig = dummyConfig.replace("%ECLIPSE_PROJECT%", eclipseProjectName);
 		dummyConfig = dummyConfig.replace("%IDEA_MODULE%", ideaModuleName);
 		dummyConfig = dummyConfig.replace("%RUN_DIRECTORY%", runDir);
-		dummyConfig = dummyConfig.replace("%PROGRAM_ARGS%", programArgs.replaceAll("\"", "&quot;"));
-		dummyConfig = dummyConfig.replace("%VM_ARGS%", vmArgs.replaceAll("\"", "&quot;"));
+		dummyConfig = dummyConfig.replace("%PROGRAM_ARGS%", joinArguments(programArgs).replaceAll("\"", "&quot;"));
+		dummyConfig = dummyConfig.replace("%VM_ARGS%", joinArguments(vmArgs).replaceAll("\"", "&quot;"));
 
 		return dummyConfig;
 	}
 
-	public static String getOSClientJVMArgs() {
-		if (OperatingSystem.getOS().equalsIgnoreCase("osx")) {
-			return " -XstartOnFirstThread";
+	public static String joinArguments(List<String> args) {
+		final var sb = new StringBuilder();
+		boolean first = true;
+
+		for (String arg : args) {
+			if (!first) {
+				sb.append(" ");
+			}
+
+			first = false;
+			sb.append("\"").append(arg).append("\"");
 		}
 
-		return "";
+		return sb.toString();
 	}
 
 	private static String getMainClass(String side, LoomGradleExtension extension, String defaultMainClass) {

src/main/java/net/fabricmc/loom/task/AbstractRunTask.java

@@ -26,7 +26,6 @@ package net.fabricmc.loom.task;
 
 import java.io.File;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.function.Function;
@@ -46,39 +45,11 @@ public abstract class AbstractRunTask extends JavaExec {
 		this.config = configProvider.apply(getProject());
 
 		setClasspath(config.sourceSet.getRuntimeClasspath());
+		args(config.programArgs);
 	}
 
 	@Override
 	public void exec() {
-		List<String> argsSplit = new ArrayList<>();
-		String[] args = config.programArgs.split(" ");
-		int partPos = -1;
-
-		for (int i = 0; i < args.length; i++) {
-			if (partPos < 0) {
-				if (args[i].startsWith("\"")) {
-					if (args[i].endsWith("\"")) {
-						argsSplit.add(args[i].substring(1, args[i].length() - 1));
-					} else {
-						partPos = i;
-					}
-				} else {
-					argsSplit.add(args[i]);
-				}
-			} else if (args[i].endsWith("\"")) {
-				StringBuilder builder = new StringBuilder(args[partPos].substring(1));
-
-				for (int j = partPos + 1; j < i; j++) {
-					builder.append(" ").append(args[j]);
-				}
-
-				builder.append(" ").append(args[i], 0, args[i].length() - 1);
-				argsSplit.add(builder.toString());
-				partPos = -1;
-			}
-		}
-
-		args(argsSplit);
 		setWorkingDir(new File(getProject().getRootDir(), config.runDir));
 
 		super.exec();
@@ -102,7 +73,7 @@ public abstract class AbstractRunTask extends JavaExec {
 	public List<String> getJvmArgs() {
 		List<String> superArgs = super.getJvmArgs();
 		List<String> args = new ArrayList<>(superArgs != null ? superArgs : Collections.emptyList());
-		args.addAll(Arrays.asList(config.vmArgs.split(" ")));
+		args.addAll(config.vmArgs);
 		return args;
 	}
 }

src/main/java/net/fabricmc/loom/task/GenVsCodeProjectTask.java

@@ -103,8 +103,8 @@ public class GenVsCodeProjectTask extends AbstractLoomTask {
 		VsCodeConfiguration(RunConfig runConfig) {
 			this.name = runConfig.configName;
 			this.mainClass = runConfig.mainClass;
-			this.vmArgs = runConfig.vmArgs;
-			this.args = runConfig.programArgs;
+			this.vmArgs = RunConfig.joinArguments(runConfig.vmArgs);
+			this.args = RunConfig.joinArguments(runConfig.programArgs);
 			this.cwd = "${workspaceFolder}/" + runConfig.runDir;
 
 			if (getProject().getRootProject() != getProject()) {

src/test/groovy/net/fabricmc/loom/test/integration/RunConfigTest.groovy

@@ -42,6 +42,7 @@ class RunConfigTest extends Specification implements GradleProjectTestTrait {
 
 		then:
 			result.task(":${task}").outcome == SUCCESS
+			result.output.contains("This contains a space")
 
 		where:
 			task                | _

src/test/groovy/net/fabricmc/loom/test/unit/RunConfigUnitTest.groovy

@@ -0,0 +1,38 @@
+/*
+ * This file is part of fabric-loom, licensed under the MIT License (MIT).
+ *
+ * Copyright (c) 2021 FabricMC
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in all
+ * copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+package net.fabricmc.loom.test.unit
+
+import net.fabricmc.loom.configuration.ide.RunConfig
+import spock.lang.Specification
+
+class RunConfigUnitTest extends Specification {
+    def "escape arguments"() {
+        when:
+            def args = RunConfig.joinArguments(["-Dfabric.test=123", "-Dfabric.test=abc 123"])
+
+        then:
+            args == '"-Dfabric.test=123" "-Dfabric.test=abc 123"'
+    }
+}

src/test/resources/projects/runconfigs/build.gradle

@@ -21,6 +21,10 @@ loom {
 			vmArg "-Dfabric.autoTest"
 		}
 	}
+
+	runConfigs.configureEach {
+		vmArg "-Dfabric.loom.test.space=This contains a space"
+	}
 }
 
 archivesBaseName = "fabric-example-mod"

src/test/resources/projects/runconfigs/src/main/java/net/fabricmc/example/ExampleMod.java

@@ -7,6 +7,8 @@ public class ExampleMod implements ModInitializer {
 	public void onInitialize() {
 		System.out.println("Hello Fabric world!");
 
+		System.out.println(System.getProperty("fabric.loom.test.space"));
+
 		// Quit now, we dont need to load the whole game to know the run configs are works
 		System.exit(0);
 	}