Windows: remove incompatible_windows_style_arg_escaping
In this PR:
- Removed the
`--incompatible_windows_style_arg_escaping` flag
and all code for its "false" case. The flag is
flipped to true.
- In WindowsProcessesTest, the
testDoesNotQuoteArgWithDoubleQuote method is
replaced by testQuotesArgWithDoubleQuote, i.e.
the test logic is reversed. The new test shows
the correct behavior, the old test was wrong.
See https://github.com/bazelbuild/bazel/issues/7122
See https://github.com/bazelbuild/bazel/issues/7454
RELNOTES[INC]: The --incompatible_windows_style_arg_escaping flag is flipped to "true", and the "false" case unsupported. Bazel no longer accepts this flag.
Closes #8003.
PiperOrigin-RevId: 255929367
diff --git a/src/test/cpp/bazel_startup_options_test.cc b/src/test/cpp/bazel_startup_options_test.cc
index 5f7cf4d..0326441 100644
--- a/src/test/cpp/bazel_startup_options_test.cc
+++ b/src/test/cpp/bazel_startup_options_test.cc
@@ -87,7 +87,6 @@
ExpectIsNullaryOption(options, "fatal_event_bus_exceptions");
ExpectIsNullaryOption(options, "home_rc");
ExpectIsNullaryOption(options, "host_jvm_debug");
- ExpectIsNullaryOption(options, "incompatible_windows_style_arg_escaping");
ExpectIsNullaryOption(options, "shutdown_on_low_sys_mem");
ExpectIsNullaryOption(options, "ignore_all_rc_files");
ExpectIsNullaryOption(options, "master_bazelrc");
diff --git a/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java b/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java
index 91e4fe4..e1b4ec7 100644
--- a/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/windows/WindowsProcessesTest.java
@@ -18,7 +18,8 @@
import static java.nio.charset.StandardCharsets.UTF_16LE;
import static java.nio.charset.StandardCharsets.UTF_8;
-import com.google.common.collect.ImmutableList;
+import com.google.common.base.Joiner;
+import com.google.devtools.build.lib.shell.ShellUtils;
import com.google.devtools.build.lib.testutil.TestSpec;
import com.google.devtools.build.lib.util.OS;
import com.google.devtools.build.lib.windows.jni.WindowsProcesses;
@@ -65,16 +66,26 @@
}
}
+ private static List<String> quoteArgs(List<String> argv, String... args) {
+ for (String arg : args) {
+ argv.add(ShellUtils.windowsEscapeArg(arg));
+ }
+ return argv;
+ }
+
+ private static List<String> quoteArgs(String... args) {
+ List<String> argv = new ArrayList<>();
+ return quoteArgs(argv, args);
+ }
+
private String mockArgs(String... args) {
List<String> argv = new ArrayList<>();
argv.add("-jar");
argv.add(mockSubprocess);
- for (String arg : args) {
- argv.add(arg);
- }
+ quoteArgs(argv, args);
- return WindowsProcesses.quoteCommandLine(argv);
+ return Joiner.on(" ").join(argv);
}
private void assertNoProcessError() throws Exception {
@@ -87,35 +98,32 @@
@Test
public void testDoesNotQuoteSimpleArg() throws Exception {
- assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a"))).isEqualTo("x a");
+ assertThat(quoteArgs("x", "a")).containsExactly("x", "a").inOrder();
}
@Test
public void testQuotesEmptyArg() throws Exception {
- assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", ""))).isEqualTo("x \"\"");
+ assertThat(quoteArgs("x", "")).containsExactly("x", "\"\"").inOrder();
}
@Test
public void testQuotesArgWithSpace() throws Exception {
- assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a b")))
- .isEqualTo("x \"a b\"");
+ assertThat(quoteArgs("x", "a b")).containsExactly("x", "\"a b\"").inOrder();
}
@Test
public void testDoesNotQuoteArgWithBackslash() throws Exception {
- assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a\\b")))
- .isEqualTo("x a\\b");
+ assertThat(quoteArgs("x", "a\\b")).containsExactly("x", "a\\b").inOrder();
}
@Test
public void testDoesNotQuoteArgWithSingleQuote() throws Exception {
- assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a'b"))).isEqualTo("x a'b");
+ assertThat(quoteArgs("x", "a'b")).containsExactly("x", "a'b").inOrder();
}
@Test
- public void testDoesNotQuoteArgWithDoubleQuote() throws Exception {
- assertThat(WindowsProcesses.quoteCommandLine(ImmutableList.of("x", "a\"b")))
- .isEqualTo("x a\\\"b");
+ public void testQuotesArgWithDoubleQuote() throws Exception {
+ assertThat(quoteArgs("x", "a\"b", "y")).containsExactly("x", "\"a\\\"b\"", "y").inOrder();
}
@Test
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 d392230..77ccd85 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
@@ -17,17 +17,14 @@
import static com.google.common.truth.Truth.assertThat;
import static java.nio.charset.StandardCharsets.UTF_8;
-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;
import com.google.devtools.build.lib.util.OS;
-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;
@@ -65,9 +62,9 @@
}
}
- private void assertSystemRootIsSetByDefault(boolean windowsStyleArgEscaping) throws Exception {
- SubprocessBuilder subprocessBuilder =
- new SubprocessBuilder(new WindowsSubprocessFactory(windowsStyleArgEscaping));
+ @Test
+ public void testSystemRootIsSetByDefault() throws Exception {
+ SubprocessBuilder subprocessBuilder = new SubprocessBuilder(WindowsSubprocessFactory.INSTANCE);
subprocessBuilder.setWorkingDirectory(new File("."));
subprocessBuilder.setArgv(mockBinary, "-jar", mockSubprocess, "O$SYSTEMROOT");
process = subprocessBuilder.start();
@@ -80,18 +77,8 @@
}
@Test
- 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));
+ public void testSystemDriveIsSetByDefault() throws Exception {
+ SubprocessBuilder subprocessBuilder = new SubprocessBuilder(WindowsSubprocessFactory.INSTANCE);
subprocessBuilder.setWorkingDirectory(new File("."));
subprocessBuilder.setArgv(mockBinary, "-jar", mockSubprocess, "O$SYSTEMDRIVE");
process = subprocessBuilder.start();
@@ -104,18 +91,8 @@
}
@Test
- 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));
+ public void testSystemRootIsSet() throws Exception {
+ SubprocessBuilder subprocessBuilder = new SubprocessBuilder(WindowsSubprocessFactory.INSTANCE);
subprocessBuilder.setWorkingDirectory(new File("."));
subprocessBuilder.setArgv(mockBinary, "-jar", mockSubprocess, "O$SYSTEMROOT");
// Case shouldn't matter on Windows
@@ -130,18 +107,8 @@
}
@Test
- 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));
+ public void testSystemDriveIsSet() throws Exception {
+ SubprocessBuilder subprocessBuilder = new SubprocessBuilder(WindowsSubprocessFactory.INSTANCE);
subprocessBuilder.setWorkingDirectory(new File("."));
subprocessBuilder.setArgv(mockBinary, "-jar", mockSubprocess, "O$SYSTEMDRIVE");
// Case shouldn't matter on Windows
@@ -155,16 +122,6 @@
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.
*
@@ -181,9 +138,7 @@
};
/** Asserts that a subprocess correctly receives command line arguments. */
- private void assertSubprocessReceivesArgsAsIntended(
- boolean windowsStyleArgEscaping, Function<String, String> escaper, ArgPair... args)
- throws Exception {
+ private void assertSubprocessReceivesArgsAsIntended(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");
@@ -191,11 +146,11 @@
for (ArgPair arg : args) {
// Assert that the command-line encoding logic works as intended.
- assertThat(escaper.apply(arg.original)).isEqualTo(arg.escaped);
+ assertThat(ShellUtils.windowsEscapeArg(arg.original)).isEqualTo(arg.escaped);
// Create a separate subprocess just for this argument.
SubprocessBuilder subprocessBuilder =
- new SubprocessBuilder(new WindowsSubprocessFactory(windowsStyleArgEscaping));
+ new SubprocessBuilder(WindowsSubprocessFactory.INSTANCE);
subprocessBuilder.setWorkingDirectory(new File("."));
subprocessBuilder.setArgv(printArgExe, arg.original);
process = subprocessBuilder.start();
@@ -212,25 +167,8 @@
}
@Test
- public void testSubprocessReceivesArgsAsIntendedNoWindowsStyleArgEscaping() throws Exception {
+ public void testSubprocessReceivesArgsAsIntended() throws Exception {
assertSubprocessReceivesArgsAsIntended(
- false,
- x -> WindowsProcesses.quoteCommandLine(ImmutableList.of(x)),
- new ArgPair("", "\"\""),
- new ArgPair(" ", "\" \""),
- new ArgPair("foo", "foo"),
- new ArgPair("foo\\bar", "foo\\bar"),
- new ArgPair("foo bar", "\"foo bar\""));
- // TODO(laszlocsomor): the escaping logic in WindowsProcesses.quoteCommandLine is wrong, because
- // 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("\"", "\"\\\"\""),
diff --git a/src/test/py/bazel/test_wrapper_test.py b/src/test/py/bazel/test_wrapper_test.py
index 77fdb01..9a756d4 100644
--- a/src/test/py/bazel/test_wrapper_test.py
+++ b/src/test/py/bazel/test_wrapper_test.py
@@ -346,10 +346,6 @@
bazel_bin = bazel_bin[0]
exit_code, stdout, stderr = self.RunBazel([
- # --[no]incompatible_windows_style_arg_escaping affects what arguments
- # the test receives. Run with --incompatible_windows_style_arg_escaping
- # to test for future (as of 2019-04-05) behavior.
- '--incompatible_windows_style_arg_escaping',
'test',
'//foo:testargs_test',
'-t-',
diff --git a/src/test/shell/bazel/windows_arg_esc_test.sh b/src/test/shell/bazel/windows_arg_esc_test.sh
index 2667a8d..dd5be79 100755
--- a/src/test/shell/bazel/windows_arg_esc_test.sh
+++ b/src/test/shell/bazel/windows_arg_esc_test.sh
@@ -87,9 +87,11 @@
return [DefaultInfo(executable = output)]
def _impl1(ctx):
+ # See https://github.com/bazelbuild/bazel/issues/7122
return _impl(ctx, '${1+"$@"}', "foo1")
def _impl2(ctx):
+ # See https://github.com/bazelbuild/bazel/issues/7122
return _impl(ctx, '${1+"$@"} ', "foo2")
args1_test = rule(implementation = _impl1, test = True)
@@ -115,58 +117,16 @@
expect_log "not all outputs were created"
}
-function assert_build() {
- local -r enable_windows_style_arg_escaping=$1
-
- if $enable_windows_style_arg_escaping; then
- local -r flag="--incompatible_windows_style_arg_escaping"
- local -r expect_foo1_fails="false"
- else
- local -r flag="--noincompatible_windows_style_arg_escaping"
- local -r expect_foo1_fails="$is_windows"
- fi
-
+function test_windows_style_arg_escaping() {
create_pkg
- bazel $flag build foo:x --verbose_failures 2>$TEST_log \
+ bazel build foo:x --verbose_failures 2>$TEST_log \
&& fail "expected failure" || true
- if $expect_foo1_fails; then
- # foo1's command does not work on Windows without the Windows-style argument
- # escaping, see https://github.com/bazelbuild/bazel/issues/7122
- assert_foo1_failed
- else
- assert_command_succeeded foo1
- fi
+ assert_command_succeeded foo1
- # foo2's command works on Windows even with the incorrect escaping semantics,
- # see https://github.com/bazelbuild/bazel/issues/7122
- bazel $flag build foo:y --verbose_failures 2>$TEST_log \
+ bazel build foo:y --verbose_failures 2>$TEST_log \
&& fail "expected failure" || true
assert_command_succeeded foo2
}
-function test_nowindows_style_arg_escaping() {
- # On Windows: assert that with --noincompatible_windows_style_arg_escaping,
- # Bazel incorrectly escapes the shell command arguments of "//foo:x" and
- # reports a bogus error ("/usr/bin/bash: $: command not found"), but it
- # correctly invokes the command for "//foo:y".
- #
- # On other platforms: assert that Bazel correctly invokes the commands for
- # both "//foo:x" and "//foo:y". This is not affected by
- # --noincompatible_windows_style_arg_escaping.
- # The test should behave the same as test_windows_style_arg_escaping.
- assert_build false
-}
-
-function test_windows_style_arg_escaping() {
- # On Windows: assert that with --incompatible_windows_style_arg_escaping,
- # Bazel correctly invokes the commands for both "//foo:x" and "//foo:y".
- #
- # On other platforms: assert that Bazel correctly invokes the commands for
- # both "//foo:x" and "//foo:y". This is not affected by
- # --incompatible_windows_style_arg_escaping.
- # The test should behave the same as test_nowindows_style_arg_escaping.
- assert_build true
-}
-
run_suite "Windows argument escaping test"
