Repository: bazelbuild/bazel
Issue: incompatible_windows_style_arg_escaping: enables correct subprocess argument escaping on Windows
Labels: area-Windows
incompatible-change
team-OSS

# Description

The option `--incompatible_windows_style_arg_escaping` enables correct subprocess argument escaping on Windows. This flag has NO effect on other platforms.

When enabled, [WindowsSubprocessFactory will use ShellUtils.windowsEscapeArg to escape command line arguments](https://github.com/bazelbuild/bazel/blob/1da48619d94ccc9fda59c114c6cd14904dbc6e34/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java#L77). This is correct, as [verified by tests](https://github.com/bazelbuild/bazel/blob/1da48619d94ccc9fda59c114c6cd14904dbc6e34/src/test/java/com/google/devtools/build/lib/windows/WindowsSubprocessTest.java#L230).

When disabled, [WindowsSubprocessFactory will use ShellUtils.quoteCommandLine](https://github.com/bazelbuild/bazel/blob/1da48619d94ccc9fda59c114c6cd14904dbc6e34/src/main/java/com/google/devtools/build/lib/windows/WindowsSubprocessFactory.java#L90). This is buggy, as shown by https://github.com/bazelbuild/bazel/issues/7122.

# Migration recipe

None, as of 2019-02-18.

We don't expect any breakages when this flag is enabled. However if it breaks your build, please let us know so we can help fixing it and provide a migration recipe.

# Rollout plan

- Bazel 0.23.0 will not support this flag.
- Bazel 0.24.0 is expected to support this flag, with default value being `false`.
- Bazel 0.25.0 is expected to flip this flag to true.

Issue comments:
Fixed by https://github.com/laszlocsomor/bazel/commit/363016efe0973f12f28ac42e43c79c84bb0a05df. Closing issue. 
No, https://github.com/laszlocsomor/bazel/commit/363016efe0973f12f28ac42e43c79c84bb0a05df was not merged! This flag is not a breaking change in 0.25
Sorry about that, thanks for updating the labels
I re-added breaking change 26 because that will put this bug in the queue for the next release manager to check up on it. No pressure if it doesn't get flipped, the protocol is to extend the window in that case. But I think if it doesn't have a breaking change label it will fall through the cracks eventually. 
Good to know. Thank you!
This flag is marked as `migration-0.25`, causing it to be run by the bazelisk incompatible change pipeline. But this flag is not available (at least under linux), causing the build to [fail](https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/104#5b8edad8-db18-440d-a0cb-08614a27c374/1958).
We can remove the migration tag; the flag is now flipped (but not yet removed).

This flag is a startup flag, which Bazelisk cannot handle.
I don't see any failure in the log you linked, can you check the link again?
The link goes to line 1958 of the Subpar build on 14.04 (OpenJDK 8). Copy/pasting the section:

```
Running Bazel with --incompatible_windows_style_arg_escaping | 0s
-- | --
  |  
  | bazel test --flaky_test_attempts=3 --build_tests_only --local_test_jobs=12 --show_progress_rate_limit=5 --curses=yes --color=yes --verbose_failures --keep_going --jobs=32 --announce_rc --experimental_multi_threaded_digest --experimental_repository_cache_hardlinks --disk_cache= --sandbox_tmpfs_path=/tmp --experimental_build_event_json_file_path_conversion=false --build_event_json_file=/tmp/tmp117gcv3x/test_bep.json --remote_timeout=60 --remote_max_connections=200 --remote_http_cache=https://storage.googleapis.com/bazel-untrusted-buildkite-cache/e0979f59aee862bb5636c07921415ff395dc7b7a24c297265429c5fae66e56f6 --google_default_credentials --test_env=HOME --sandbox_writable_path=/home/bazel/.cache/bazelisk ... --incompatible_windows_style_arg_escaping
  | ERROR: Unrecognized option: --incompatible_windows_style_arg_escaping
  | INFO: Invocation ID: 8c007530-88e6-423f-b651-02fe88d5071c
```

Bazelisk being unable to handle startup options is a good reason for this to fail the pipeline. I'll ignore.
Maybe we should label issues for startup flags such that bazelisk can exclude issues with the label.
@laurentlb can you fix the labels here for bazelisk, with 0.25.2 this fails because the flag isn't recognized
removed

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/tools/launcher/util/BUILD b/src/tools/launcher/util/BUILD
index 5077e2300775eb..e3aa79a8fe6ad7 100644
--- a/src/tools/launcher/util/BUILD
+++ b/src/tools/launcher/util/BUILD
@@ -1,6 +1,6 @@
 package(default_visibility = ["//src/tools/launcher:__subpackages__"])
 
-load("//src/tools/launcher:win_rules.bzl", "cc_library", "cc_binary", "cc_test")
+load("//src/tools/launcher:win_rules.bzl", "cc_binary", "cc_library", "cc_test")
 
 filegroup(
     name = "srcs",
@@ -19,6 +19,10 @@ cc_library(
     name = "util",
     srcs = ["launcher_util.cc"],
     hdrs = ["launcher_util.h"],
+    visibility = [
+        "//src/tools/launcher:__subpackages__",
+        "//tools/test:__pkg__",
+    ],
     deps = ["//src/main/cpp/util:filesystem"],
 )
 


Test patch:
diff --git a/src/test/py/bazel/BUILD b/src/test/py/bazel/BUILD
index d5f59d680c4530..ae947e2c9ba5c8 100644
--- a/src/test/py/bazel/BUILD
+++ b/src/test/py/bazel/BUILD
@@ -164,6 +164,16 @@ py_test(
         "//src/conditions:windows": [":test_base"],
         "//conditions:default": [],
     }),
+    data = select({
+        "//src/conditions:windows": [":printargs"],
+        "//conditions:default": [],
+    }),
+)
+
+cc_binary(
+    name = "printargs",
+    srcs = ["printargs.cc"],
+    testonly = 1,
 )
 
 py_test(
diff --git a/src/test/py/bazel/printargs.cc b/src/test/py/bazel/printargs.cc
new file mode 100644
index 00000000000000..745398b824be99
--- /dev/null
+++ b/src/test/py/bazel/printargs.cc
@@ -0,0 +1,7 @@
+#include <stdio.h>
+int main(int argc, char** argv) {
+  for (int i = 1; i < argc; ++i) {
+    printf("arg=(%s)\n", argv[i]);
+  }
+  return 0;
+}
diff --git a/src/test/py/bazel/test_wrapper_test.py b/src/test/py/bazel/test_wrapper_test.py
index 7665f6810d93f3..b214dac24628f1 100644
--- a/src/test/py/bazel/test_wrapper_test.py
+++ b/src/test/py/bazel/test_wrapper_test.py
@@ -81,9 +81,9 @@ def _CreateMockWorkspace(self):
         '    srcs = ["unexported.bat"],',
         ')',
         'sh_test(',
-        '    name = "testargs_test.bat",',
-        '    srcs = ["testargs.bat"],',
-        '    args = ["foo", "a b", "", "bar"],',
+        '    name = "testargs_test.exe",',
+        '    srcs = ["testargs.exe"],',
+        r'    args = ["foo", "a b", "", "\"c d\"", "\"\"", "bar"],',
         ')',
         'py_test(',
         '    name = "undecl_test",',
@@ -134,21 +134,11 @@ def _CreateMockWorkspace(self):
             '@echo BAD=%TEST_UNDECLARED_OUTPUTS_MANIFEST%',
         ],
         executable=True)
-    self.ScratchFile(
-        'foo/testargs.bat',
-        [
-            '@echo arg=(%~nx0)',  # basename of $0
-            '@echo arg=(%1)',
-            '@echo arg=(%2)',
-            '@echo arg=(%3)',
-            '@echo arg=(%4)',
-            '@echo arg=(%5)',
-            '@echo arg=(%6)',
-            '@echo arg=(%7)',
-            '@echo arg=(%8)',
-            '@echo arg=(%9)',
-        ],
-        executable=True)
+
+    self.CopyFile(
+        src_path = self.Rlocation("io_bazel/src/test/py/bazel/printargs.exe"),
+        dst_path = "foo/testargs.exe",
+        executable = True)
 
     # A single white pixel as an ".ico" file. /usr/bin/file should identify this
     # as "image/x-icon".
@@ -385,7 +375,7 @@ def _AssertTestArgs(self, flag, expected):
 
     exit_code, stdout, stderr = self.RunBazel([
         'test',
-        '//foo:testargs_test.bat',
+        '//foo:testargs_test.exe',
         '-t-',
         '--test_output=all',
         '--test_arg=baz',
@@ -568,24 +558,20 @@ def testTestExecutionWithTestSetupSh(self):
     self._AssertTestArgs(
         flag,
         [
-            '(testargs_test.bat)',
             '(foo)',
             '(a)',
             '(b)',
+            '(c d)',
+            '()',
             '(bar)',
-            # Note: debugging shows that test-setup.sh receives more-or-less
-            # good arguments (let's ignore issues #6276 and #6277 for now), but
-            # mangles the last few.
-            # I (laszlocsomor@) don't know the reason (as of 2018-10-01) but
-            # since I'm planning to phase out test-setup.sh on Windows in favor
-            # of the native test wrapper, I don't intend to debug this further.
-            # The test is here merely to guard against unwanted future change of
-            # behavior.
             '(baz)',
-            '("\\"x)',
-            '(y\\"")',
-            '("\\\\\\")',
-            '(qux")'
+            '("x y")',
+            # I (laszlocsomor@) don't know the exact reason (as of 2019-04-05)
+            # why () and (qux) are mangled as they are, but since I'm planning
+            # to phase out test-setup.sh on Windows in favor of the native test
+            # wrapper, I don't intend to debug this further.  The test is here
+            # merely to guard against unwanted future change of behavior.
+            '(\\" qux)'
         ])
     self._AssertUndeclaredOutputs(flag)
     self._AssertUndeclaredOutputsAnnotations(flag)
@@ -606,7 +592,6 @@ def testTestExecutionWithTestWrapperExe(self):
     self._AssertTestArgs(
         flag,
         [
-            '(testargs_test.bat)',
             '(foo)',
             # TODO(laszlocsomor): assert that "a b" is passed as one argument,
             # not two, after https://github.com/bazelbuild/bazel/issues/6277
@@ -616,12 +601,13 @@ def testTestExecutionWithTestWrapperExe(self):
             # TODO(laszlocsomor): assert that the empty string argument is
             # passed, after https://github.com/bazelbuild/bazel/issues/6276
             # is fixed.
+            '(c d)',
+            '()',
             '(bar)',
             '(baz)',
             '("x y")',
             '("")',
             '(qux)',
-            '()'
         ])
     self._AssertUndeclaredOutputs(flag)
     self._AssertUndeclaredOutputsAnnotations(flag)
diff --git a/tools/test/BUILD b/tools/test/BUILD
index 735e2fbb0132e1..6deb7dde275004 100644
--- a/tools/test/BUILD
+++ b/tools/test/BUILD
@@ -80,6 +80,7 @@ cc_library(
             "//src/main/cpp/util:strings",
             "//src/main/native/windows:lib-file",
             "//src/main/native/windows:lib-util",
+            "//src/tools/launcher/util",
             "//third_party/ijar:zip",
             "@bazel_tools//tools/cpp/runfiles",
         ],
diff --git a/tools/test/windows/tw.cc b/tools/test/windows/tw.cc
index aec0853c21eb46..cacbd66477084c 100644
--- a/tools/test/windows/tw.cc
+++ b/tools/test/windows/tw.cc
@@ -43,6 +43,7 @@
 #include "src/main/cpp/util/strings.h"
 #include "src/main/native/windows/file.h"
 #include "src/main/native/windows/util.h"
+#include "src/tools/launcher/util/launcher_util.h"
 #include "third_party/ijar/common.h"
 #include "third_party/ijar/platform_utils.h"
 #include "third_party/ijar/zip.h"
@@ -1118,7 +1119,7 @@ bool AddCommandLineArg(const wchar_t* arg, const size_t arg_size,
 }
 
 bool CreateCommandLine(const Path& path,
-                       const std::vector<const wchar_t*>& args,
+                       const std::vector<std::wstring>& args,
                        std::unique_ptr<WCHAR[]>* result) {
   // kMaxCmdline value: see lpCommandLine parameter of CreateProcessW.
   static constexpr size_t kMaxCmdline = 32767;
@@ -1132,9 +1133,9 @@ bool CreateCommandLine(const Path& path,
     return false;
   }
 
-  for (const auto arg : args) {
-    if (!AddCommandLineArg(arg, wcslen(arg), false, result->get(), kMaxCmdline,
-                           &total_len)) {
+  for (const std::wstring& arg : args) {
+    if (!AddCommandLineArg(arg.c_str(), arg.size(), false, result->get(),
+                           kMaxCmdline, &total_len)) {
       return false;
     }
   }
@@ -1145,7 +1146,7 @@ bool CreateCommandLine(const Path& path,
   return true;
 }
 
-bool StartSubprocess(const Path& path, const std::vector<const wchar_t*>& args,
+bool StartSubprocess(const Path& path, const std::vector<std::wstring>& args,
                      const Path& outerr, std::unique_ptr<Tee>* tee,
                      LARGE_INTEGER* start_time,
                      bazel::windows::AutoHandle* process) {
@@ -1342,7 +1343,7 @@ bool CreateUndeclaredOutputsAnnotations(const Path& undecl_annot_dir,
 
 bool ParseArgs(int argc, wchar_t** argv, Path* out_argv0,
                std::wstring* out_test_path_arg,
-               std::vector<const wchar_t*>* out_args) {
+               std::vector<std::wstring>* out_args) {
   if (!out_argv0->Set(argv[0])) {
     return false;
   }
@@ -1358,7 +1359,7 @@ bool ParseArgs(int argc, wchar_t** argv, Path* out_argv0,
   out_args->clear();
   out_args->reserve(argc - 1);
   for (int i = 1; i < argc; i++) {
-    out_args->push_back(argv[i]);
+    out_args->push_back(bazel::launcher::WindowsEscapeArg2(argv[i]));
   }
   return true;
 }
@@ -1434,7 +1435,7 @@ bool TeeImpl::MainFunc() const {
 }
 
 int RunSubprocess(const Path& test_path,
-                  const std::vector<const wchar_t*>& args,
+                  const std::vector<std::wstring>& args,
                   const Path& test_outerr, Duration* test_duration) {
   std::unique_ptr<Tee> tee;
   bazel::windows::AutoHandle process;
@@ -1871,7 +1872,7 @@ int TestWrapperMain(int argc, wchar_t** argv) {
   std::wstring test_path_arg;
   Path test_path, exec_root, srcdir, tmpdir, test_outerr, xml_log;
   UndeclaredOutputs undecl;
-  std::vector<const wchar_t*> args;
+  std::vector<std::wstring> args;
   if (!ParseArgs(argc, argv, &argv0, &test_path_arg, &args) ||
       !PrintTestLogStartMarker() ||
       !FindTestBinary(argv0, test_path_arg, &test_path) ||