subprocesses: customizable SubprocessFactory
Individual SubprocessBuilder instances can now use
a SubprocessFactory object other than the static
SubprocessBuilder.factory.
Also, WindowsSubprocessFactory is no longer a
singleton because it now stores state: whether to
use windows-style argument escaping or to use the
(broken) Bash-style escaping (causing https://github.com/bazelbuild/bazel/issues/7122).
These two changes allow:
- a safer way to mock out SubprocessFactory in
tests, because it's no longer necessary to
change global state (i.e. the static
SubprocessBuilder.factory member)
- testing old and new argument escaping semantics
in WindowsSubprocessTest
- adding a flag that switches between old and new
semantics in the SubprocessFactory, and thus
allows rolling out the bugfix with just a flag
flip
See https://github.com/bazelbuild/bazel/issues/7122
PiperOrigin-RevId: 234105692
diff --git a/src/test/java/com/google/devtools/build/lib/windows/WindowsSubprocessTest.java b/src/test/java/com/google/devtools/build/lib/windows/WindowsSubprocessTest.java
index 4fb0968..d392230 100644
--- a/src/test/java/com/google/devtools/build/lib/windows/WindowsSubprocessTest.java
+++ b/src/test/java/com/google/devtools/build/lib/windows/WindowsSubprocessTest.java
@@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.shell.ShellUtils;
import com.google.devtools.build.lib.shell.Subprocess;
import com.google.devtools.build.lib.shell.SubprocessBuilder;
import com.google.devtools.build.lib.testutil.TestSpec;
@@ -26,6 +27,7 @@
import com.google.devtools.build.lib.windows.jni.WindowsProcesses;
import com.google.devtools.build.runfiles.Runfiles;
import java.io.File;
+import java.util.function.Function;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
@@ -63,11 +65,10 @@
}
}
- @Test
- public void testSystemRootIsSetByDefault() throws Exception {
- SubprocessBuilder subprocessBuilder = new SubprocessBuilder();
+ private void assertSystemRootIsSetByDefault(boolean windowsStyleArgEscaping) throws Exception {
+ SubprocessBuilder subprocessBuilder =
+ new SubprocessBuilder(new WindowsSubprocessFactory(windowsStyleArgEscaping));
subprocessBuilder.setWorkingDirectory(new File("."));
- subprocessBuilder.setSubprocessFactory(WindowsSubprocessFactory.INSTANCE);
subprocessBuilder.setArgv(mockBinary, "-jar", mockSubprocess, "O$SYSTEMROOT");
process = subprocessBuilder.start();
process.waitFor();
@@ -79,10 +80,19 @@
}
@Test
- public void testSystemDriveIsSetByDefault() throws Exception {
- SubprocessBuilder subprocessBuilder = new SubprocessBuilder();
+ public void testSystemRootIsSetByDefaultNoWindowsStyleArgEscaping() throws Exception {
+ assertSystemRootIsSetByDefault(false);
+ }
+
+ @Test
+ public void testSystemRootIsSetByDefaultWithWindowsStyleArgEscaping() throws Exception {
+ assertSystemRootIsSetByDefault(true);
+ }
+
+ private void assertSystemDriveIsSetByDefault(boolean windowsStyleArgEscaping) throws Exception {
+ SubprocessBuilder subprocessBuilder =
+ new SubprocessBuilder(new WindowsSubprocessFactory(windowsStyleArgEscaping));
subprocessBuilder.setWorkingDirectory(new File("."));
- subprocessBuilder.setSubprocessFactory(WindowsSubprocessFactory.INSTANCE);
subprocessBuilder.setArgv(mockBinary, "-jar", mockSubprocess, "O$SYSTEMDRIVE");
process = subprocessBuilder.start();
process.waitFor();
@@ -94,10 +104,19 @@
}
@Test
- public void testSystemRootIsSet() throws Exception {
- SubprocessBuilder subprocessBuilder = new SubprocessBuilder();
+ public void testSystemDriveIsSetByDefaultNoWindowsStyleArgEscaping() throws Exception {
+ assertSystemDriveIsSetByDefault(false);
+ }
+
+ @Test
+ public void testSystemDriveIsSetByDefaultWithWindowsStyleArgEscaping() throws Exception {
+ assertSystemDriveIsSetByDefault(true);
+ }
+
+ private void assertSystemRootIsSet(boolean windowsStyleArgEscaping) throws Exception {
+ SubprocessBuilder subprocessBuilder =
+ new SubprocessBuilder(new WindowsSubprocessFactory(windowsStyleArgEscaping));
subprocessBuilder.setWorkingDirectory(new File("."));
- subprocessBuilder.setSubprocessFactory(WindowsSubprocessFactory.INSTANCE);
subprocessBuilder.setArgv(mockBinary, "-jar", mockSubprocess, "O$SYSTEMROOT");
// Case shouldn't matter on Windows
subprocessBuilder.setEnv(ImmutableMap.of("SystemRoot", "C:\\MySystemRoot"));
@@ -111,10 +130,19 @@
}
@Test
- public void testSystemDriveIsSet() throws Exception {
- SubprocessBuilder subprocessBuilder = new SubprocessBuilder();
+ public void testSystemRootIsSetNoWindowsStyleArgEscaping() throws Exception {
+ assertSystemRootIsSet(false);
+ }
+
+ @Test
+ public void testSystemRootIsSetWithWindowsStyleArgEscaping() throws Exception {
+ assertSystemRootIsSet(true);
+ }
+
+ private void assertSystemDriveIsSet(boolean windowsStyleArgEscaping) throws Exception {
+ SubprocessBuilder subprocessBuilder =
+ new SubprocessBuilder(new WindowsSubprocessFactory(windowsStyleArgEscaping));
subprocessBuilder.setWorkingDirectory(new File("."));
- subprocessBuilder.setSubprocessFactory(WindowsSubprocessFactory.INSTANCE);
subprocessBuilder.setArgv(mockBinary, "-jar", mockSubprocess, "O$SYSTEMDRIVE");
// Case shouldn't matter on Windows
subprocessBuilder.setEnv(ImmutableMap.of("SystemDrive", "X:"));
@@ -127,6 +155,16 @@
assertThat(new String(buf, UTF_8).trim()).isEqualTo("X:");
}
+ @Test
+ public void testSystemDriveIsSetNoWindowsStyleArgEscaping() throws Exception {
+ assertSystemDriveIsSet(false);
+ }
+
+ @Test
+ public void testSystemDriveIsSetWithWindowsStyleArgEscaping() throws Exception {
+ assertSystemDriveIsSet(true);
+ }
+
/**
* An argument and its command-line-escaped counterpart.
*
@@ -143,7 +181,9 @@
};
/** Asserts that a subprocess correctly receives command line arguments. */
- private void assertSubprocessReceivesArgsAsIntended(ArgPair... args) throws Exception {
+ private void assertSubprocessReceivesArgsAsIntended(
+ boolean windowsStyleArgEscaping, Function<String, String> escaper, ArgPair... args)
+ throws Exception {
// Look up the path of the printarg.exe utility.
String printArgExe =
runfiles.rlocation("io_bazel/src/test/java/com/google/devtools/build/lib/printarg.exe");
@@ -151,13 +191,12 @@
for (ArgPair arg : args) {
// Assert that the command-line encoding logic works as intended.
- assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of(arg.original)))
- .isEqualTo(arg.escaped);
+ assertThat(escaper.apply(arg.original)).isEqualTo(arg.escaped);
// Create a separate subprocess just for this argument.
- SubprocessBuilder subprocessBuilder = new SubprocessBuilder();
+ SubprocessBuilder subprocessBuilder =
+ new SubprocessBuilder(new WindowsSubprocessFactory(windowsStyleArgEscaping));
subprocessBuilder.setWorkingDirectory(new File("."));
- subprocessBuilder.setSubprocessFactory(WindowsSubprocessFactory.INSTANCE);
subprocessBuilder.setArgv(printArgExe, arg.original);
process = subprocessBuilder.start();
process.waitFor();
@@ -173,8 +212,10 @@
}
@Test
- public void testSubprocessReceivesArgsAsIntended() throws Exception {
+ public void testSubprocessReceivesArgsAsIntendedNoWindowsStyleArgEscaping() throws Exception {
assertSubprocessReceivesArgsAsIntended(
+ false,
+ x -> WindowsProcesses.quoteCommandLine(ImmutableList.of(x)),
new ArgPair("", "\"\""),
new ArgPair(" ", "\" \""),
new ArgPair("foo", "foo"),
@@ -184,4 +225,54 @@
// it fails to properly escape things like a single backslash followed by a quote, e.g. a\"b
// Fix the escaping logic and add more test here.
}
+
+ @Test
+ public void testSubprocessReceivesArgsAsIntendedWithWindowsStyleArgEscaping() throws Exception {
+ assertSubprocessReceivesArgsAsIntended(
+ true,
+ x -> ShellUtils.windowsEscapeArg(x),
+ new ArgPair("", "\"\""),
+ new ArgPair(" ", "\" \""),
+ new ArgPair("\"", "\"\\\"\""),
+ new ArgPair("\"\\", "\"\\\"\\\\\""),
+ new ArgPair("\\", "\\"),
+ new ArgPair("\\\"", "\"\\\\\\\"\""),
+ new ArgPair("with space", "\"with space\""),
+ new ArgPair("with^caret", "with^caret"),
+ new ArgPair("space ^caret", "\"space ^caret\""),
+ new ArgPair("caret^ space", "\"caret^ space\""),
+ new ArgPair("with\"quote", "\"with\\\"quote\""),
+ new ArgPair("with\\backslash", "with\\backslash"),
+ new ArgPair("one\\ backslash and \\space", "\"one\\ backslash and \\space\""),
+ new ArgPair("two\\\\backslashes", "two\\\\backslashes"),
+ new ArgPair("two\\\\ backslashes \\\\and space", "\"two\\\\ backslashes \\\\and space\""),
+ new ArgPair("one\\\"x", "\"one\\\\\\\"x\""),
+ new ArgPair("two\\\\\"x", "\"two\\\\\\\\\\\"x\""),
+ new ArgPair("a \\ b", "\"a \\ b\""),
+ new ArgPair("a \\\" b", "\"a \\\\\\\" b\""),
+ new ArgPair("A", "A"),
+ new ArgPair("\"a\"", "\"\\\"a\\\"\""),
+ new ArgPair("B C", "\"B C\""),
+ new ArgPair("\"b c\"", "\"\\\"b c\\\"\""),
+ new ArgPair("D\"E", "\"D\\\"E\""),
+ new ArgPair("\"d\"e\"", "\"\\\"d\\\"e\\\"\""),
+ new ArgPair("C:\\F G", "\"C:\\F G\""),
+ new ArgPair("\"C:\\f g\"", "\"\\\"C:\\f g\\\"\""),
+ new ArgPair("C:\\H\"I", "\"C:\\H\\\"I\""),
+ new ArgPair("\"C:\\h\"i\"", "\"\\\"C:\\h\\\"i\\\"\""),
+ new ArgPair("C:\\J\\\"K", "\"C:\\J\\\\\\\"K\""),
+ new ArgPair("\"C:\\j\\\"k\"", "\"\\\"C:\\j\\\\\\\"k\\\"\""),
+ new ArgPair("C:\\L M ", "\"C:\\L M \""),
+ new ArgPair("\"C:\\l m \"", "\"\\\"C:\\l m \\\"\""),
+ new ArgPair("C:\\N O\\", "\"C:\\N O\\\\\""),
+ new ArgPair("\"C:\\n o\\\"", "\"\\\"C:\\n o\\\\\\\"\""),
+ new ArgPair("C:\\P Q\\ ", "\"C:\\P Q\\ \""),
+ new ArgPair("\"C:\\p q\\ \"", "\"\\\"C:\\p q\\ \\\"\""),
+ new ArgPair("C:\\R\\S\\", "C:\\R\\S\\"),
+ new ArgPair("C:\\R x\\S\\", "\"C:\\R x\\S\\\\\""),
+ new ArgPair("\"C:\\r\\s\\\"", "\"\\\"C:\\r\\s\\\\\\\"\""),
+ new ArgPair("\"C:\\r x\\s\\\"", "\"\\\"C:\\r x\\s\\\\\\\"\""),
+ new ArgPair("C:\\T U\\W\\", "\"C:\\T U\\W\\\\\""),
+ new ArgPair("\"C:\\t u\\w\\\"", "\"\\\"C:\\t u\\w\\\\\\\"\""));
+ }
}
