diff options
author | Ulf Adams <ulfjack@google.com> | 2017-02-17 09:11:52 +0000 |
---|---|---|
committer | Dmitry Lomov <dslomov@google.com> | 2017-02-17 14:55:21 +0000 |
commit | 1cb5059396e46d9ff2c317b8a0a046af32b3a1d8 (patch) | |
tree | 33bf2c21000aa3216eca23d2c34de6ce20fc7ede /src/main/java/com/google/devtools/build/lib/exec | |
parent | a61e421ea0d619433f1a537978a59129b5cbb572 (diff) |
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
Diffstat (limited to 'src/main/java/com/google/devtools/build/lib/exec')
-rw-r--r-- | src/main/java/com/google/devtools/build/lib/exec/TestStrategy.java | 66 |
1 files changed, 36 insertions, 30 deletions
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<String> getArgs(String coverageScript, TestRunnerAction testAction) throws ExecException { List<String> 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<String> 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=<command-prefix>" 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=<command-prefix>" 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<String> 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. * |