From ad754b44aaa7dbb1a062e00ef7d6782e663799bd Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Fri, 5 Apr 2019 15:51:11 +0200 Subject: [PATCH 1/3] 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. Fixes https://github.com/bazelbuild/bazel/issues/7956 Unblocks https://github.com/bazelbuild/bazel/issues/6622 --- src/tools/launcher/util/BUILD | 6 +++++- tools/test/BUILD | 1 + tools/test/windows/tw.cc | 19 ++++++++++--------- 3 files changed, 16 insertions(+), 10 deletions(-) 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..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& args, + const std::vector& args, std::unique_ptr* 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& 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 +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* out_args) { + std::vector* 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& args, + const std::vector& args, const Path& test_outerr, Duration* test_duration) { std::unique_ptr 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 args; + std::vector args; if (!ParseArgs(argc, argv, &argv0, &test_path_arg, &args) || !PrintTestLogStartMarker() || !FindTestBinary(argv0, test_path_arg, &test_path) || From 811b0308348640c5023c9a86449276f6ef319f5d Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Fri, 5 Apr 2019 17:48:37 +0200 Subject: [PATCH 2/3] Use exe file for arg-testing in test_wrapper_test --- src/test/py/bazel/BUILD | 10 +++ src/test/py/bazel/printargs.cc | 7 ++ src/test/py/bazel/test_wrapper_test.py | 102 +++++++++++-------------- 3 files changed, 61 insertions(+), 58 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..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 +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..490070ee440252 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', @@ -559,54 +549,49 @@ def _AssertXmlGeneratedByTestIsRetained(self, flag, split_xml=False): def testTestExecutionWithTestSetupSh(self): self._CreateMockWorkspace() flag = '--noincompatible_windows_native_test_wrapper' - self._AssertPassingTest(flag) - self._AssertFailingTest(flag) - self._AssertPrintingTest(flag) - self._AssertRunfiles(flag) - self._AssertShardedTest(flag) - self._AssertUnexportsEnvvars(flag) +# self._AssertPassingTest(flag) +# self._AssertFailingTest(flag) +# self._AssertPrintingTest(flag) +# self._AssertRunfiles(flag) +# self._AssertShardedTest(flag) +# self._AssertUnexportsEnvvars(flag) 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) - self._AssertXmlGeneration(flag, split_xml=False) - self._AssertXmlGeneration(flag, split_xml=True) - self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=False) - self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=True) +# self._AssertUndeclaredOutputs(flag) +# self._AssertUndeclaredOutputsAnnotations(flag) +# self._AssertXmlGeneration(flag, split_xml=False) +# self._AssertXmlGeneration(flag, split_xml=True) +# self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=False) +# self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=True) def testTestExecutionWithTestWrapperExe(self): self._CreateMockWorkspace() flag = '--incompatible_windows_native_test_wrapper' - self._AssertPassingTest(flag) - self._AssertFailingTest(flag) - self._AssertPrintingTest(flag) - self._AssertRunfiles(flag) - self._AssertShardedTest(flag) - self._AssertUnexportsEnvvars(flag) +# self._AssertPassingTest(flag) +# self._AssertFailingTest(flag) +# self._AssertPrintingTest(flag) +# self._AssertRunfiles(flag) +# self._AssertShardedTest(flag) +# self._AssertUnexportsEnvvars(flag) 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,19 +601,20 @@ 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) - self._AssertXmlGeneration(flag, split_xml=False) - self._AssertXmlGeneration(flag, split_xml=True) - self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=False) - self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=True) +# self._AssertUndeclaredOutputs(flag) +# self._AssertUndeclaredOutputsAnnotations(flag) +# self._AssertXmlGeneration(flag, split_xml=False) +# self._AssertXmlGeneration(flag, split_xml=True) +# self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=False) +# self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=True) if __name__ == '__main__': From 1b5e2a8e088b92edf2599770276dc2bdca0b066f Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Fri, 5 Apr 2019 18:34:19 +0200 Subject: [PATCH 3/3] Uncomment test cases --- src/test/py/bazel/test_wrapper_test.py | 48 +++++++++++++------------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/test/py/bazel/test_wrapper_test.py b/src/test/py/bazel/test_wrapper_test.py index 490070ee440252..b214dac24628f1 100644 --- a/src/test/py/bazel/test_wrapper_test.py +++ b/src/test/py/bazel/test_wrapper_test.py @@ -549,12 +549,12 @@ def _AssertXmlGeneratedByTestIsRetained(self, flag, split_xml=False): def testTestExecutionWithTestSetupSh(self): self._CreateMockWorkspace() flag = '--noincompatible_windows_native_test_wrapper' -# self._AssertPassingTest(flag) -# self._AssertFailingTest(flag) -# self._AssertPrintingTest(flag) -# self._AssertRunfiles(flag) -# self._AssertShardedTest(flag) -# self._AssertUnexportsEnvvars(flag) + self._AssertPassingTest(flag) + self._AssertFailingTest(flag) + self._AssertPrintingTest(flag) + self._AssertRunfiles(flag) + self._AssertShardedTest(flag) + self._AssertUnexportsEnvvars(flag) self._AssertTestArgs( flag, [ @@ -573,22 +573,22 @@ def testTestExecutionWithTestSetupSh(self): # merely to guard against unwanted future change of behavior. '(\\" qux)' ]) -# self._AssertUndeclaredOutputs(flag) -# self._AssertUndeclaredOutputsAnnotations(flag) -# self._AssertXmlGeneration(flag, split_xml=False) -# self._AssertXmlGeneration(flag, split_xml=True) -# self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=False) -# self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=True) + self._AssertUndeclaredOutputs(flag) + self._AssertUndeclaredOutputsAnnotations(flag) + self._AssertXmlGeneration(flag, split_xml=False) + self._AssertXmlGeneration(flag, split_xml=True) + self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=False) + self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=True) def testTestExecutionWithTestWrapperExe(self): self._CreateMockWorkspace() flag = '--incompatible_windows_native_test_wrapper' -# self._AssertPassingTest(flag) -# self._AssertFailingTest(flag) -# self._AssertPrintingTest(flag) -# self._AssertRunfiles(flag) -# self._AssertShardedTest(flag) -# self._AssertUnexportsEnvvars(flag) + self._AssertPassingTest(flag) + self._AssertFailingTest(flag) + self._AssertPrintingTest(flag) + self._AssertRunfiles(flag) + self._AssertShardedTest(flag) + self._AssertUnexportsEnvvars(flag) self._AssertTestArgs( flag, [ @@ -609,12 +609,12 @@ def testTestExecutionWithTestWrapperExe(self): '("")', '(qux)', ]) -# self._AssertUndeclaredOutputs(flag) -# self._AssertUndeclaredOutputsAnnotations(flag) -# self._AssertXmlGeneration(flag, split_xml=False) -# self._AssertXmlGeneration(flag, split_xml=True) -# self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=False) -# self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=True) + self._AssertUndeclaredOutputs(flag) + self._AssertUndeclaredOutputsAnnotations(flag) + self._AssertXmlGeneration(flag, split_xml=False) + self._AssertXmlGeneration(flag, split_xml=True) + self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=False) + self._AssertXmlGeneratedByTestIsRetained(flag, split_xml=True) if __name__ == '__main__':