From 36c3a0e5765fddfcf5b3707d1c52794caae9a8a7 Mon Sep 17 00:00:00 2001 From: Laszlo Csomor Date: Fri, 26 Apr 2019 14:34:47 +0200 Subject: [PATCH] Windows, testing: only look up shell if needed TestActionBuilder now only looks up the shell (via ShToolchain.getPathOrError) when the shell is required. Now, when using the Windows-native test wrapper with --shell_toolchain="" (and without a shell-command-looking --run_under argument) the TestActionBuilder won't depend on Bash. Related: https://github.com/bazelbuild/bazel/issues/4319 --- .../lib/analysis/test/TestActionBuilder.java | 6 +- .../lib/analysis/test/TestRunnerAction.java | 7 +- .../test/TestTargetExecutionSettings.java | 15 +++ .../lib/exec/StandaloneTestStrategy.java | 6 +- .../devtools/build/lib/exec/TestStrategy.java | 21 ++-- src/test/py/bazel/test_wrapper_test.py | 113 ++++++++---------- 6 files changed, 91 insertions(+), 77 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java index bf172c6c52c964..92ef1639b45074 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java @@ -350,7 +350,6 @@ private TestParams createTestAction(int shards) { coverageArtifacts.add(coverageArtifact); } - PathFragment shExecutable = ShToolchain.getPathOrError(ruleContext); env.registerAction( new TestRunnerAction( ruleContext.getActionOwner(), @@ -369,7 +368,10 @@ private TestParams createTestAction(int shards) { run, config, ruleContext.getWorkspaceName(), - shExecutable)); + (!isUsingTestWrapperInsteadOfTestSetupScript || + executionSettings.needsShell(isExecutedOnWindows)) + ? ShToolchain.getPathOrError(ruleContext) + : null)); results.add(cacheStatus); } } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java index ead826f758260b..2a04dd453873a7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java @@ -94,7 +94,7 @@ public class TestRunnerAction extends AbstractAction private final Artifact cacheStatus; private final PathFragment testWarningsPath; private final PathFragment unusedRunfilesLogPath; - private final PathFragment shExecutable; + @Nullable private final PathFragment shExecutable; private final PathFragment splitLogsPath; private final PathFragment splitLogsDir; private final PathFragment undeclaredOutputsDir; @@ -165,7 +165,7 @@ private static ImmutableList list(Artifact... artifacts) { int runNumber, BuildConfiguration configuration, String workspaceName, - PathFragment shExecutable) { + @Nullable PathFragment shExecutable) { super( owner, /*tools=*/ ImmutableList.of(), @@ -827,7 +827,8 @@ public Artifact getCollectCoverageScript() { return collectCoverageScript; } - public PathFragment getShExecutable() { + @Nullable + public PathFragment getShExecutableMaybe() { return shExecutable; } diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java index 29c35c08d38127..3277d11cb93c2c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestTargetExecutionSettings.java @@ -138,4 +138,19 @@ public Artifact getInputManifest() { public Artifact getInstrumentedFileManifest() { return instrumentedFileManifest; } + + public boolean needsShell(boolean executedOnWindows) { + RunUnder r = getRunUnder(); + if (r == null) { + return false; + } + String command = r.getCommand(); + if (command == null) { + return false; + } + // --run_under commands that do not contain '/' are either shell built-ins or need to be + // located on the PATH env, so we wrap them in a shell invocation. Note that we shell-tokenize + // the --run_under parameter and getCommand only returns the first such token. + return !command.contains("/") && (!executedOnWindows || !command.contains("\\")); + } } diff --git a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java index 2eb8c081ebd60a..f96f517537aa00 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/StandaloneTestStrategy.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.exec; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Lists; @@ -313,7 +314,10 @@ private Spawn createXmlGeneratingSpawn(TestRunnerAction action, SpawnResult resu // configuration, not the machine Bazel happens to run on. Change this to something like: // testAction.getConfiguration().getExecOS() == OS.WINDOWS if (OS.getCurrent() == OS.WINDOWS && !action.isUsingTestWrapperInsteadOfTestSetupScript()) { - args.add(action.getShExecutable().getPathString()); + // TestActionBuilder constructs TestRunnerAction with a 'null' shell path only when we use the + // native test wrapper. Something clearly went wrong. + Preconditions.checkNotNull(action.getShExecutableMaybe(), "%s", action); + args.add(action.getShExecutableMaybe().getPathString()); args.add("-c"); args.add("$0 $*"); } diff --git a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java index c8462544a6bbfc..637be71375577a 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.exec; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; @@ -165,7 +166,10 @@ public static ImmutableList getArgs(TestRunnerAction testAction) throws final boolean useTestWrapper = testAction.isUsingTestWrapperInsteadOfTestSetupScript(); if (executedOnWindows && !useTestWrapper) { - args.add(testAction.getShExecutable().getPathString()); + // TestActionBuilder constructs TestRunnerAction with a 'null' shell path only when we use the + // native test wrapper. Something clearly went wrong. + Preconditions.checkNotNull(testAction.getShExecutableMaybe(), "%s", testAction); + args.add(testAction.getShExecutableMaybe().getPathString()); args.add("-c"); args.add("$0 \"$@\""); } @@ -200,20 +204,17 @@ private static void addRunUnderArgs( if (execSettings.getRunUnderExecutable() != null) { args.add(execSettings.getRunUnderExecutable().getRootRelativePath().getCallablePathString()); } else { - String command = execSettings.getRunUnder().getCommand(); - // --run_under commands that do not contain '/' are either shell built-ins or need to be - // located on the PATH env, so we wrap them in a shell invocation. Note that we shell tokenize - // the --run_under parameter and getCommand only returns the first such token. - boolean needsShell = - !command.contains("/") && (!executedOnWindows || !command.contains("\\")); - if (needsShell) { - String shellExecutable = testAction.getShExecutable().getPathString(); + if (execSettings.needsShell(executedOnWindows)) { + // TestActionBuilder constructs TestRunnerAction with a 'null' shell only when none is + // required. Something clearly went wrong. + Preconditions.checkNotNull(testAction.getShExecutableMaybe(), "%s", testAction); + String shellExecutable = testAction.getShExecutableMaybe().getPathString(); args.add(shellExecutable); args.add("-c"); args.add("\"$@\""); args.add(shellExecutable); // Sets $0. } - args.add(command); + args.add(execSettings.getRunUnder().getCommand()); } args.addAll(testAction.getExecutionSettings().getRunUnder().getOptions()); } diff --git a/src/test/py/bazel/test_wrapper_test.py b/src/test/py/bazel/test_wrapper_test.py index 340996ef56f237..2569468caff161 100644 --- a/src/test/py/bazel/test_wrapper_test.py +++ b/src/test/py/bazel/test_wrapper_test.py @@ -232,32 +232,29 @@ def _CreateMockWorkspace(self): ], executable=True) - def _AssertPassingTest(self, flag): + def _AssertPassingTest(self, flags): exit_code, _, stderr = self.RunBazel([ 'test', '//foo:passing_test.bat', '-t-', - flag, - ]) + ] + flags) self.AssertExitCode(exit_code, 0, stderr) - def _AssertFailingTest(self, flag): + def _AssertFailingTest(self, flags): exit_code, _, stderr = self.RunBazel([ 'test', '//foo:failing_test.bat', '-t-', - flag, - ]) + ] + flags) self.AssertExitCode(exit_code, 3, stderr) - def _AssertPrintingTest(self, flag): + def _AssertPrintingTest(self, flags): exit_code, stdout, stderr = self.RunBazel([ 'test', '//foo:printing_test.bat', '-t-', '--test_output=all', - flag, - ]) + ] + flags) self.AssertExitCode(exit_code, 0, stderr) lorem = False for line in stderr + stdout: @@ -290,7 +287,7 @@ def _AssertPrintingTest(self, flag): if not user: self._FailWithOutput(stderr + stdout) - def _AssertRunfiles(self, flag): + def _AssertRunfiles(self, flags): exit_code, stdout, stderr = self.RunBazel([ 'test', '//foo:runfiles_test.bat', @@ -298,8 +295,7 @@ def _AssertRunfiles(self, flag): '--test_output=all', # Ensure Bazel does not create a runfiles tree. '--enable_runfiles=no', - flag, - ]) + ] + flags) self.AssertExitCode(exit_code, 0, stderr) mf = mf_only = rf_dir = None for line in stderr + stdout: @@ -325,14 +321,13 @@ def _AssertRunfiles(self, flag): if not os.path.isdir(rf_dir): self._FailWithOutput(stderr + stdout) - def _AssertShardedTest(self, flag): + def _AssertShardedTest(self, flags): exit_code, stdout, stderr = self.RunBazel([ 'test', '//foo:sharded_test.bat', '-t-', '--test_output=all', - flag, - ]) + ] + flags) self.AssertExitCode(exit_code, 0, stderr) status = None index_lines = [] @@ -350,14 +345,13 @@ def _AssertShardedTest(self, flag): if sorted(index_lines) != ['INDEX=0 TOTAL=2', 'INDEX=1 TOTAL=2']: self._FailWithOutput(stderr + stdout) - def _AssertUnexportsEnvvars(self, flag): + def _AssertUnexportsEnvvars(self, flags): exit_code, stdout, stderr = self.RunBazel([ 'test', '//foo:unexported_test.bat', '-t-', '--test_output=all', - flag, - ]) + ] + flags) self.AssertExitCode(exit_code, 0, stderr) good = bad = None for line in stderr + stdout: @@ -368,7 +362,7 @@ def _AssertUnexportsEnvvars(self, flag): if not good or bad: self._FailWithOutput(stderr + stdout) - def _AssertTestArgs(self, flag): + def _AssertTestArgs(self, flags): exit_code, bazel_bin, stderr = self.RunBazel(['info', 'bazel-bin']) self.AssertExitCode(exit_code, 0, stderr) bazel_bin = bazel_bin[0] @@ -386,8 +380,7 @@ def _AssertTestArgs(self, flag): '--test_arg="x y"', '--test_arg=""', '--test_arg=qux', - flag, - ]) + ] + flags) self.AssertExitCode(exit_code, 0, stderr) actual = [] @@ -415,7 +408,7 @@ def _AssertTestArgs(self, flag): ], actual) - def _AssertUndeclaredOutputs(self, flag): + def _AssertUndeclaredOutputs(self, flags): exit_code, bazel_testlogs, stderr = self.RunBazel( ['info', 'bazel-testlogs']) self.AssertExitCode(exit_code, 0, stderr) @@ -426,8 +419,7 @@ def _AssertUndeclaredOutputs(self, flag): '//foo:undecl_test', '-t-', '--test_output=errors', - flag, - ]) + ] + flags) self.AssertExitCode(exit_code, 0, stderr) undecl_zip = os.path.join(bazel_testlogs, 'foo', 'undecl_test', @@ -472,7 +464,7 @@ def _AssertUndeclaredOutputs(self, flag): if mf_content[1] != 'out2/data2.dat\t16\tapplication/octet-stream': self._FailWithOutput(mf_content) - def _AssertUndeclaredOutputsAnnotations(self, flag): + def _AssertUndeclaredOutputsAnnotations(self, flags): exit_code, bazel_testlogs, stderr = self.RunBazel( ['info', 'bazel-testlogs']) self.AssertExitCode(exit_code, 0, stderr) @@ -483,8 +475,7 @@ def _AssertUndeclaredOutputsAnnotations(self, flag): '//foo:annot_test', '-t-', '--test_output=errors', - flag, - ]) + ] + flags) self.AssertExitCode(exit_code, 0, stderr) undecl_annot = os.path.join(bazel_testlogs, 'foo', 'annot_test', @@ -496,7 +487,7 @@ def _AssertUndeclaredOutputsAnnotations(self, flag): self.assertListEqual(annot_content, ['Hello aHello c']) - def _AssertXmlGeneration(self, flag, split_xml=False): + def _AssertXmlGeneration(self, flags, split_xml=False): exit_code, bazel_testlogs, stderr = self.RunBazel( ['info', 'bazel-testlogs']) self.AssertExitCode(exit_code, 0, stderr) @@ -508,8 +499,7 @@ def _AssertXmlGeneration(self, flag, split_xml=False): '-t-', '--test_output=errors', '--%sexperimental_split_xml_generation' % ('' if split_xml else 'no'), - flag, - ]) + ] + flags) self.AssertExitCode(exit_code, 0, stderr) test_xml = os.path.join(bazel_testlogs, 'foo', 'xml_test', 'test.xml') @@ -546,7 +536,7 @@ def _AssertXmlGeneration(self, flag, split_xml=False): 'stderr_line_2' not in stderr_lines[1]): self._FailWithOutput(xml_contents) - def _AssertXmlGeneratedByTestIsRetained(self, flag, split_xml=False): + def _AssertXmlGeneratedByTestIsRetained(self, flags, split_xml=False): exit_code, bazel_testlogs, stderr = self.RunBazel( ['info', 'bazel-testlogs']) self.AssertExitCode(exit_code, 0, stderr) @@ -558,8 +548,7 @@ def _AssertXmlGeneratedByTestIsRetained(self, flag, split_xml=False): '-t-', '--test_output=errors', '--%sexperimental_split_xml_generation' % ('' if split_xml else 'no'), - flag, - ]) + ] + flags) self.AssertExitCode(exit_code, 0, stderr) test_xml = os.path.join(bazel_testlogs, 'foo', 'xml2_test', 'test.xml') @@ -588,6 +577,7 @@ def testRunningTestFromExternalRepo(self): 'test', '-t-', '--incompatible_windows_native_test_wrapper', + '--shell_executable=', '--test_output=errors', '--verbose_failures', flag, @@ -599,37 +589,38 @@ def testRunningTestFromExternalRepo(self): 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._AssertTestArgs(flag) - 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) + flags = '--noincompatible_windows_native_test_wrapper' + self._AssertPassingTest(flags) + self._AssertFailingTest(flags) + self._AssertPrintingTest(flags) + self._AssertRunfiles(flags) + self._AssertShardedTest(flags) + self._AssertUnexportsEnvvars(flags) + self._AssertTestArgs(flags) + self._AssertUndeclaredOutputs(flags) + self._AssertUndeclaredOutputsAnnotations(flags) + self._AssertXmlGeneration(flags, split_xml=False) + self._AssertXmlGeneration(flags, split_xml=True) + self._AssertXmlGeneratedByTestIsRetained(flags, split_xml=False) + self._AssertXmlGeneratedByTestIsRetained(flags, 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._AssertTestArgs(flag) - 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) + flags = ['--incompatible_windows_native_test_wrapper', + '--shell_executable='] + self._AssertPassingTest(flags) + self._AssertFailingTest(flags) + self._AssertPrintingTest(flags) + self._AssertRunfiles(flags) + self._AssertShardedTest(flags) + self._AssertUnexportsEnvvars(flags) + self._AssertTestArgs(flags) + self._AssertUndeclaredOutputs(flags) + self._AssertUndeclaredOutputsAnnotations(flags) + self._AssertXmlGeneration(flags, split_xml=False) + self._AssertXmlGeneration(flags, split_xml=True) + self._AssertXmlGeneratedByTestIsRetained(flags, split_xml=False) + self._AssertXmlGeneratedByTestIsRetained(flags, split_xml=True) if __name__ == '__main__':