From 5379fdf9bb6e9bc06031849d3a1e1f735110597e Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Mon, 8 Apr 2019 05:34:29 -0700 Subject: [PATCH] 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 https://github.com/bazelbuild/bazel/issues/7956 Unblocks https://github.com/bazelbuild/bazel/issues/6622 Closes #7957. PiperOrigin-RevId: 242446422 --- src/test/py/bazel/BUILD | 10 +++++ src/test/py/bazel/printargs.cc | 23 ++++++++++++ src/test/py/bazel/test_wrapper_test.py | 52 ++++++++++---------------- src/tools/launcher/util/BUILD | 6 ++- tools/test/BUILD | 1 + tools/test/windows/tw.cc | 21 +++++------ 6 files changed, 68 insertions(+), 45 deletions(-) create mode 100644 src/test/py/bazel/printargs.cc diff --git a/src/test/py/bazel/BUILD b/src/test/py/bazel/BUILD index d5f59d680c4530..31432e159eff86 100644 --- a/src/test/py/bazel/BUILD +++ b/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"], diff --git a/src/test/py/bazel/printargs.cc b/src/test/py/bazel/printargs.cc new file mode 100644 index 00000000000000..315ac48bec160c --- /dev/null +++ b/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=(). +// This program aids testing the flags Bazel passes on the command line. +#include +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..49e88e30e16043 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,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) 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"], ) 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..d577a0284061bd 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" @@ -1117,8 +1118,7 @@ bool AddCommandLineArg(const wchar_t* arg, const size_t arg_size, } } -bool CreateCommandLine(const Path& path, - const std::vector& args, +bool CreateCommandLine(const Path& path, const std::vector& args, std::unique_ptr* 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& args, +bool StartSubprocess(const Path& path, const std::vector& args, const Path& outerr, std::unique_ptr* 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* out_args) { + std::vector* 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& args, +int RunSubprocess(const Path& test_path, const std::vector& args, const Path& test_outerr, Duration* test_duration) { std::unique_ptr 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 args; + std::vector args; if (!ParseArgs(argc, argv, &argv0, &test_path_arg, &args) || !PrintTestLogStartMarker() || !FindTestBinary(argv0, test_path_arg, &test_path) ||