From 7b09310da6939773c3092c4315a8aa8b4526cadc Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Fri, 10 May 2019 11:33:27 +0200 Subject: [PATCH] Windows, bazel run: fix a bug in PR #8241 To "bazel run" a binary, the RunCommand creates an ExecRequest and sends it to the Bazel client, which then fulfills the request. In PR #8241, RunCommand merged all command line arguments and added that to the request as a single argv entry. The client uses the first argv element as the executable, but if all arguments are joined into one string then argv0 contains the whole argument vector as a single string. Unfortunately the test didn't catch this because it didn't attempt passing any arguments. Fixes https://github.com/bazelbuild/bazel/issues/8229 See https://github.com/bazelbuild/bazel/issues/8240 --- .../lib/runtime/commands/RunCommand.java | 7 +-- src/test/py/bazel/first_time_use_test.py | 43 ++++++++++++++----- 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java index 6fc93372aa1e65..9542fe38d87778 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java +++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java @@ -504,9 +504,10 @@ public BlazeCommandResult exec(CommandEnvironment env, OptionsParsingResult opti ByteString.copyFrom(workingDir.getPathString(), StandardCharsets.ISO_8859_1)); if (OS.getCurrent() == OS.WINDOWS && runOptions.bashlessRun) { - String joinedCommands = - Joiner.on(" ").join(Iterables.transform(cmdLine, x -> ShellUtils.windowsEscapeArg(x))); - execDescription.addArgv(ByteString.copyFrom(joinedCommands, StandardCharsets.ISO_8859_1)); + for (String arg : cmdLine) { + execDescription.addArgv( + ByteString.copyFrom(ShellUtils.windowsEscapeArg(arg), StandardCharsets.ISO_8859_1)); + } } else { PathFragment shExecutable = ShToolchain.getPath(configuration); if (shExecutable.isEmpty()) { diff --git a/src/test/py/bazel/first_time_use_test.py b/src/test/py/bazel/first_time_use_test.py index 7d662f89f94273..5710744ae50174 100644 --- a/src/test/py/bazel/first_time_use_test.py +++ b/src/test/py/bazel/first_time_use_test.py @@ -31,6 +31,22 @@ def testNoPythonRequirement(self): if 'python' in line and 'not found on PATH' in line: self._FailWithOutput(stdout + stderr) + def _AssertBazelRunBinaryOutput(self, exit_code, stdout, stderr, flag): + self.AssertExitCode(exit_code, 0, stderr) + found_hello = found_arg_a = found_arg_bc = False + for line in stdout + stderr: + if 'ERROR' in line and 'needs a shell' in line: + self._FailWithOutput(['flag=' + flag] + stdout + stderr) + if not found_hello and 'hello python' in line: + found_hello = True + elif not found_arg_a and 'arg[1]=(a)': + found_arg_a = True + elif not found_arg_bc and 'arg[2]=(b c)': + found_arg_bc = True + break + if not found_hello or not found_arg_a or not found_arg_bc: + self._FailWithOutput(['flag=' + flag] + stdout + stderr) + def testNoBashRequiredForSimpleBazelRun(self): """Regression test for https://github.com/bazelbuild/bazel/issues/8229.""" self.ScratchFile('WORKSPACE') @@ -38,16 +54,21 @@ def testNoBashRequiredForSimpleBazelRun(self): 'py_binary(', ' name = "x",' ' srcs = ["x.py"],', + ' args = ["a", "\'b c\'"],', ')', ]) self.ScratchFile('foo/x.py', [ 'from __future__ import print_function', + 'import sys', 'print("hello python")', + 'for i in range(len(sys.argv)):', + ' print("arg%d=(%s)" % (i, sys.argv[i]))', ]) if test_base.TestBase.IsWindows(): exit_code, stdout, stderr = self.RunBazel([ 'run', + # Run without shell but with shell-based "bazel run". Should fail. '--shell_executable=', '--noincompatible_windows_bashless_run_command', '//foo:x', @@ -61,22 +82,16 @@ def testNoBashRequiredForSimpleBazelRun(self): if not found_error: self._FailWithOutput(stdout + stderr) + flag = '--incompatible_windows_bashless_run_command' exit_code, stdout, stderr = self.RunBazel([ 'run', + # Run without shell and with bashless "bazel run". Should succeed. '--shell_executable=', - '--incompatible_windows_bashless_run_command', + flag, '//foo:x', ]) - self.AssertExitCode(exit_code, 0, stderr) - found_output = False - for line in stdout + stderr: - if 'ERROR' in line and 'needs a shell' in line: - self._FailWithOutput(stdout + stderr) - if 'hello python' in line: - found_output = True - break - if not found_output: - self._FailWithOutput(stdout + stderr) + + self._AssertBazelRunBinaryOutput(exit_code, stdout, stderr, flag) else: # The --incompatible_windows_bashless_run_command should be a no-op on # platforms other than Windows. @@ -86,6 +101,8 @@ def testNoBashRequiredForSimpleBazelRun(self): ]: exit_code, stdout, stderr = self.RunBazel([ 'run', + # Run fails because we provide no shell. Platforms other than + # Windows always use Bash for "bazel run". '--shell_executable=', flag, '//foo:x', @@ -99,6 +116,10 @@ def testNoBashRequiredForSimpleBazelRun(self): if not found_error: self._FailWithOutput(['flag=' + flag] + stdout + stderr) + # Run succeeds because there is a shell. + exit_code, stdout, stderr = self.RunBazel(['run', flag, '//foo:x']) + self._AssertBazelRunBinaryOutput(exit_code, stdout, stderr, flag) + if __name__ == '__main__': unittest.main()