Windows: fix native test wrapper's arg. escaping

The native test wrapper now correctly escapes the
arguments for the subprocess, using
bazel::launcher::WindowsEscapeArg2 from the native
launcher.

The test_wrapper_test now uses a mock C++ binary
instead of a .bat file to verify the test
arguments. Doing so trades the weird command line
flag parsing logic of cmd.exe (inherent in using a
.bat file) for the saner C++ flag parsing one.

Fixes bazelbuild#7956
Unblocks bazelbuild#6622

Closes bazelbuild#7957.

PiperOrigin-RevId: 242446422

src/test/py/bazel/BUILD

@@ -156,6 +156,10 @@ py_test(
         "//src/conditions:windows": ["test_wrapper_test.py"],
         "//conditions:default": ["empty_test.py"],
     }),
+    data = select({
+        "//src/conditions:windows": [":printargs"],
+        "//conditions:default": [],
+    }),
     main = select({
         "//src/conditions:windows": "test_wrapper_test.py",
         "//conditions:default": "empty_test.py",
@@ -166,6 +170,12 @@ py_test(
     }),
 )
 
+cc_binary(
+    name = "printargs",
+    testonly = 1,
+    srcs = ["printargs.cc"],
+)
+
 py_test(
     name = "first_time_use_test",
     srcs = ["first_time_use_test.py"],

src/test/py/bazel/printargs.cc

@@ -0,0 +1,23 @@
+// Copyright 2019 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Prints the command line arguments, one on each line, as arg=(<ARG>).
+// This program aids testing the flags Bazel passes on the command line.
+#include <stdio.h>
+int main(int argc, char** argv) {
+  for (int i = 1; i < argc; ++i) {
+    printf("arg=(%s)\n", argv[i]);
+  }
+  return 0;
+}

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,20 +134,10 @@ 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)',
-        ],
+
+    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
@@ -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)

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"],
 )
 

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",
         ],

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"
@@ -1117,8 +1118,7 @@ bool AddCommandLineArg(const wchar_t* arg, const size_t arg_size,
   }
 }
 
-bool CreateCommandLine(const Path& path,
-                       const std::vector<const wchar_t*>& args,
+bool CreateCommandLine(const Path& path, 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 +1132,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 +1145,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 +1342,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 +1358,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;
 }
@@ -1433,8 +1433,7 @@ bool TeeImpl::MainFunc() const {
   return true;
 }
 
-int RunSubprocess(const Path& test_path,
-                  const std::vector<const wchar_t*>& args,
+int RunSubprocess(const Path& test_path, const std::vector<std::wstring>& args,
                   const Path& test_outerr, Duration* test_duration) {
   std::unique_ptr<Tee> tee;
   bazel::windows::AutoHandle process;
@@ -1871,7 +1870,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) ||