From 1cb5059396e46d9ff2c317b8a0a046af32b3a1d8 Mon Sep 17 00:00:00 2001 From: Ulf Adams Date: Fri, 17 Feb 2017 09:11:52 +0000 Subject: 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 coverage is enabled at the same time. Inside Google, it's very rare for --run_under to be used in combination with coverage, and it seems likely to be rare externally as well, so I don't think it warrants covering it in the release notes. -- PiperOrigin-RevId: 147815017 MOS_MIGRATED_REVID=147815017 --- .../devtools/build/lib/exec/TestStrategy.java | 66 ++++++++++++---------- 1 file changed, 36 insertions(+), 30 deletions(-) (limited to 'src/main/java/com/google/devtools/build/lib/exec') 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 4c94254613..d7f94ec1d2 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,7 +16,6 @@ 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; @@ -33,7 +32,6 @@ 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; @@ -49,7 +47,6 @@ 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; @@ -159,10 +156,14 @@ 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"); @@ -170,41 +171,46 @@ public abstract class TestStrategy implements TestActionContext { } Artifact testSetup = testAction.getRuntimeArtifact(TEST_SETUP_BASENAME); - args.add(testSetup.getExecPathString()); - TestTargetExecutionSettings execSettings = testAction.getExecutionSettings(); + args.add(testSetup.getExecPath().getCallablePathString()); - List execArgs = new ArrayList<>(); - if (!coverageScript.isEmpty() && testAction.isCoverageMode()) { - execArgs.add(coverageScript); + if (testAction.isCoverageMode()) { + args.add(coverageScript); } - // 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 { - args.add(testAction.getConfiguration().getShellExecutable().getPathString()); - args.add("-c"); + TestTargetExecutionSettings execSettings = testAction.getExecutionSettings(); - String runUnderCommand = ShellEscaper.escapeString(execSettings.getRunUnder().getCommand()); - args.add( - runUnderCommand - + ' ' - + ShellEscaper.escapeJoinAll( - Iterables.concat(execSettings.getRunUnder().getOptions(), execArgs))); + // Insert the command prefix specified by the "--run_under=" option, if any. + if (execSettings.getRunUnder() != null) { + addRunUnderArgs(testAction, args); } + + // 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()); + } 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.addAll(testAction.getExecutionSettings().getRunUnder().getOptions()); + } + /** * Returns the number of attempts specific test action can be retried. * -- cgit v1.2.3