From 0145a3b81bb7077ac4a2daffedf4bf9d115df189 Mon Sep 17 00:00:00 2001 From: Ulf Adams Date: Mon, 20 Feb 2017 10:16:41 +0000 Subject: Rollback of commit 1cb5059396e46d9ff2c317b8a0a046af32b3a1d8. *** Reason for rollback *** Breaks TEST_BINARY env setting if --run_under is used. *** Original change description *** Rewrite TestStrategy.getArgs This changes command-line computation for tests: - run the coverage collector before the run_under command - no shell escaping: if all tools just call "$@", then this should work Note that we still wrap the command in a sub-shell to support shell built-ins and PATH lookup if the command does not contain a slash character '/'. A side effect of this change is that the --run_under command now executes in the test's runfiles directory, rather than in the exec root, if... *** -- PiperOrigin-RevId: 148003525 MOS_MIGRATED_REVID=148003525 --- .../devtools/build/lib/exec/TestStrategy.java | 66 ++++++++++------------ .../build/lib/worker/WorkerTestStrategy.java | 6 +- 2 files changed, 31 insertions(+), 41 deletions(-) (limited to 'src/main/java/com/google/devtools') 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 d7f94ec1d2..4c94254613 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 @@ -16,6 +16,7 @@ package com.google.devtools.build.lib.exec; import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.io.ByteStreams; import com.google.common.io.Closeables; @@ -32,6 +33,7 @@ import com.google.devtools.build.lib.rules.test.TestResult; import com.google.devtools.build.lib.rules.test.TestRunnerAction; import com.google.devtools.build.lib.rules.test.TestTargetExecutionSettings; import com.google.devtools.build.lib.util.OS; +import com.google.devtools.build.lib.util.ShellEscaper; import com.google.devtools.build.lib.util.io.FileWatcher; import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.vfs.FileStatus; @@ -47,6 +49,7 @@ import java.io.Closeable; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; import java.util.List; @@ -156,14 +159,10 @@ public abstract class TestStrategy implements TestActionContext { * collect coverage data. If this is an empty string, it is ignored. * @param testAction The test action. * @return the command line as string list. - * @throws ExecException */ protected List getArgs(String coverageScript, TestRunnerAction testAction) throws ExecException { List args = Lists.newArrayList(); - // TODO(ulfjack): This is incorrect for remote execution, where we need to consider the target - // configuration, not the machine Bazel happens to run on. Change this to something like: - // testAction.getConfiguration().getTargetOS() == OS.WINDOWS if (OS.getCurrent() == OS.WINDOWS) { args.add(testAction.getShExecutable().getPathString()); args.add("-c"); @@ -171,44 +170,39 @@ public abstract class TestStrategy implements TestActionContext { } Artifact testSetup = testAction.getRuntimeArtifact(TEST_SETUP_BASENAME); - args.add(testSetup.getExecPath().getCallablePathString()); - - if (testAction.isCoverageMode()) { - args.add(coverageScript); - } - + args.add(testSetup.getExecPathString()); TestTargetExecutionSettings execSettings = testAction.getExecutionSettings(); - // Insert the command prefix specified by the "--run_under=" option, if any. - if (execSettings.getRunUnder() != null) { - addRunUnderArgs(testAction, args); + List execArgs = new ArrayList<>(); + if (!coverageScript.isEmpty() && testAction.isCoverageMode()) { + execArgs.add(coverageScript); } - // Execute the test using the alias in the runfiles tree, as mandated by the Test Encyclopedia. - args.add(execSettings.getExecutable().getRootRelativePath().getCallablePathString()); - args.addAll(execSettings.getArgs()); - return args; - } - - private static void addRunUnderArgs(TestRunnerAction testAction, List args) { - TestTargetExecutionSettings execSettings = testAction.getExecutionSettings(); - if (execSettings.getRunUnderExecutable() != null) { - args.add(execSettings.getRunUnderExecutable().getRootRelativePath().getCallablePathString()); + // Execute the test using the alias in the runfiles tree, as mandated by + // the Test Encyclopedia. + execArgs.add(execSettings.getExecutable().getRootRelativePath().getPathString()); + execArgs.addAll(execSettings.getArgs()); + + // Insert the command prefix specified by the "--run_under=" option, + // if any. + if (execSettings.getRunUnder() == null) { + args.addAll(execArgs); + } else if (execSettings.getRunUnderExecutable() != null) { + args.add(execSettings.getRunUnderExecutable().getRootRelativePath().getPathString()); + args.addAll(execSettings.getRunUnder().getOptions()); + args.addAll(execArgs); } 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("/"); - if (needsShell) { - args.add(testAction.getConfiguration().getShellExecutable().getPathString()); - args.add("-c"); - args.add("\"$@\""); - args.add("/bin/sh"); // Sets $0. - } - args.add(command); + args.add(testAction.getConfiguration().getShellExecutable().getPathString()); + args.add("-c"); + + String runUnderCommand = ShellEscaper.escapeString(execSettings.getRunUnder().getCommand()); + args.add( + runUnderCommand + + ' ' + + ShellEscaper.escapeJoinAll( + Iterables.concat(execSettings.getRunUnder().getOptions(), execArgs))); } - args.addAll(testAction.getExecutionSettings().getRunUnder().getOptions()); + return args; } /** diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java index 36ab7bf8e5..d4a0fa8140 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerTestStrategy.java @@ -87,10 +87,6 @@ public class WorkerTestStrategy extends StandaloneTestStrategy { throw new UserExecException("Tests that do not use the default test runner are incompatible" + " with the persistent worker test strategy. Please use another test strategy"); } - if (!action.isCoverageMode()) { - throw new UserExecException("Coverage is currently incompatible" - + " with the persistent worker test strategy. Please use another test strategy"); - } List startupArgs = getStartUpArgs(action); return execInWorker( @@ -196,7 +192,7 @@ public class WorkerTestStrategy extends StandaloneTestStrategy { } private List getStartUpArgs(TestRunnerAction action) throws ExecException { - List args = getArgs(/*coverageScript=*/ "coverage-is-not-supported", action); + List args = getArgs(/*coverageScript=*/ "", action); ImmutableList.Builder startupArgs = ImmutableList.builder(); // Add test setup with no echo to prevent stdout corruption. startupArgs.add(args.get(0)).add("--no_echo"); -- cgit v1.2.3