From 037c9dcf873e8037f7e6a03d4e4c6230dd5a1b27 Mon Sep 17 00:00:00 2001 From: lberki Date: Fri, 9 Feb 2018 06:06:49 -0800 Subject: Remove hard-wired basenames for the test setup and the coverage collection script. Accomplished by creating an explicit attribute for both of them on test actions. RELNOTES: None. PiperOrigin-RevId: 185132460 --- .../build/lib/analysis/BaseRuleClasses.java | 8 ++++++++ .../skylark/SkylarkRuleClassFunctions.java | 8 ++++++++ .../build/lib/analysis/test/TestActionBuilder.java | 9 +++++++- .../build/lib/analysis/test/TestRunnerAction.java | 24 +++++++++++----------- .../build/lib/exec/StandaloneTestStrategy.java | 7 ++----- .../devtools/build/lib/exec/TestStrategy.java | 9 +++----- .../build/lib/runtime/commands/RunCommand.java | 2 +- .../build/lib/worker/WorkerTestStrategy.java | 2 +- 8 files changed, 43 insertions(+), 26 deletions(-) (limited to 'src/main/java/com/google/devtools') diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java index 2292c9d857..5ea8584bbe 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/BaseRuleClasses.java @@ -138,6 +138,14 @@ public class BaseRuleClasses { attr("$test_runtime", LABEL_LIST) .cfg(HostTransition.INSTANCE) .value(ImmutableList.of(env.getToolsLabel("//tools/test:runtime")))) + .add(attr("$test_setup_script", LABEL) + .cfg(HostTransition.INSTANCE) + .singleArtifact() + .value(env.getToolsLabel("//tools/test:test_setup"))) + .add(attr("$collect_coverage_script", LABEL) + .cfg(HostTransition.INSTANCE) + .singleArtifact() + .value(env.getToolsLabel("//tools/test:collect_coverage"))) // Input files for test actions collecting code coverage .add( attr("$coverage_support", LABEL) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java index 7a83ac553b..6f7a1bfd62 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java @@ -190,6 +190,14 @@ public class SkylarkRuleClassFunctions { .value( ImmutableList.of( labelCache.getUnchecked(toolsRepository + "//tools/test:runtime")))) + .add(attr("$test_setup_script", LABEL) + .cfg(HostTransition.INSTANCE) + .singleArtifact() + .value(labelCache.getUnchecked(toolsRepository + "//tools/test:test_setup"))) + .add(attr("$collect_coverage_script", LABEL) + .cfg(HostTransition.INSTANCE) + .singleArtifact() + .value(labelCache.getUnchecked(toolsRepository + "//tools/test:collect_coverage"))) // Input files for test actions collecting code coverage .add( attr("$coverage_support", LABEL) 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 37cfbbe39f..bbce349638 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 @@ -202,10 +202,16 @@ public final class TestActionBuilder { final boolean collectCodeCoverage = config.isCodeCoverageEnabled() && instrumentedFiles != null; + Artifact testSetupScript = ruleContext.getHostPrerequisiteArtifact("$test_setup_script"); + inputsBuilder.add(testSetupScript); + + Artifact collectCoverageScript = null; TreeMap extraTestEnv = new TreeMap<>(); TestTargetExecutionSettings executionSettings; if (collectCodeCoverage) { + collectCoverageScript = ruleContext.getHostPrerequisiteArtifact("$collect_coverage_script"); + inputsBuilder.add(collectCoverageScript); inputsBuilder.addTransitive(instrumentedFiles.getCoverageSupportFiles()); // Add instrumented file manifest artifact to the list of inputs. This file will contain // exec paths of all source files that should be included into the code coverage output. @@ -301,7 +307,8 @@ public final class TestActionBuilder { } env.registerAction(new TestRunnerAction( - ruleContext.getActionOwner(), inputs, testRuntime, + ruleContext.getActionOwner(), inputs, + testSetupScript, collectCoverageScript, testLog, cacheStatus, coverageArtifact, testProperties, extraTestEnv, executionSettings, 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 8a7661f443..0166545c99 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 @@ -32,13 +32,11 @@ import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.CommandLineExpansionException; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit; -import com.google.devtools.build.lib.actions.UserExecException; import com.google.devtools.build.lib.analysis.RunfilesSupplierImpl; import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.analysis.config.RunUnder; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.ImmutableIterable; -import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.util.Fingerprint; import com.google.devtools.build.lib.util.LoggingUtil; import com.google.devtools.build.lib.vfs.FileSystem; @@ -71,7 +69,8 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa private static final String GUID = "cc41f9d0-47a6-11e7-8726-eb6ce83a8cc8"; - private final NestedSet runtime; + private final Artifact testSetupScript; + private final Artifact collectCoverageScript; private final BuildConfiguration configuration; private final TestConfiguration testConfiguration; private final Artifact testLog; @@ -133,7 +132,8 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa TestRunnerAction( ActionOwner owner, Iterable inputs, - NestedSet runtime, // Must be a subset of inputs + Artifact testSetupScript, // Must be in inputs + @Nullable Artifact collectCoverageScript, // Must be in inputs, if not null Artifact testLog, Artifact cacheStatus, Artifact coverageArtifact, @@ -151,7 +151,9 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa // Note that this action only cares about the runfiles, not the mapping. new RunfilesSupplierImpl(PathFragment.create("runfiles"), executionSettings.getRunfiles()), list(testLog, cacheStatus, coverageArtifact)); - this.runtime = runtime; + Preconditions.checkState((collectCoverageScript == null) == (coverageArtifact == null)); + this.testSetupScript = testSetupScript; + this.collectCoverageScript = collectCoverageScript; this.configuration = Preconditions.checkNotNull(configuration); this.testConfiguration = Preconditions.checkNotNull(configuration.getFragment(TestConfiguration.class)); @@ -693,14 +695,12 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa return getOutputs(); } - public Artifact getRuntimeArtifact(String basename) throws ExecException { - for (Artifact runtimeArtifact : runtime) { - if (runtimeArtifact.getExecPath().getBaseName().equals(basename)) { - return runtimeArtifact; - } - } + public Artifact getTestSetupScript() { + return testSetupScript; + } - throw new UserExecException("'" + basename + "' not found in test runtime"); + @Nullable public Artifact getCollectCoverageScript() { + return collectCoverageScript; } public PathFragment getShExecutable() { 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 a6fbd41fd8..b5748ee847 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 @@ -53,15 +53,12 @@ import java.util.List; import java.util.Map; /** Runs TestRunnerAction actions. */ +// TODO(bazel-team): add tests for this strategy. @ExecutionStrategy( contextType = TestActionContext.class, name = {"standalone"} ) public class StandaloneTestStrategy extends TestStrategy { - // TODO(bazel-team) - add tests for this strategy. - public static final String COLLECT_COVERAGE = - "external/bazel_tools/tools/test/collect_coverage.sh"; - private static final ImmutableMap ENV_VARS = ImmutableMap.builder() .put("TZ", "UTC") @@ -121,7 +118,7 @@ public class StandaloneTestStrategy extends TestStrategy { Spawn spawn = new SimpleSpawn( action, - getArgs(COLLECT_COVERAGE, action), + getArgs(action), ImmutableMap.copyOf(env), executionInfo.build(), new RunfilesSupplierImpl( 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 3b3e6914cb..0e0dab07ed 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 @@ -156,14 +156,11 @@ public abstract class TestStrategy implements TestActionContext { * Generates a command line to run for the test action, taking into account coverage and {@code * --run_under} settings. * - * @param coverageScript a script interjected between setup script and rest of command line to - * 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 */ - public static ImmutableList getArgs(String coverageScript, TestRunnerAction testAction) - throws ExecException { + public static ImmutableList getArgs(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: @@ -174,11 +171,11 @@ public abstract class TestStrategy implements TestActionContext { args.add("$0 $*"); } - Artifact testSetup = testAction.getRuntimeArtifact(TEST_SETUP_BASENAME); + Artifact testSetup = testAction.getTestSetupScript(); args.add(testSetup.getExecPath().getCallablePathString()); if (testAction.isCoverageMode()) { - args.add(coverageScript); + args.add(testAction.getCollectCoverageScript().getExecPathString()); } TestTargetExecutionSettings execSettings = testAction.getExecutionSettings(); 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 af7a541fb3..5c50705231 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 @@ -461,7 +461,7 @@ public class RunCommand implements BlazeCommand { // It's unfortunate that this method requires the path to the coverage collection script. // Fortunately, this is "blaze run" and not "blaze coverage", so it's okay unless someone // calls "blaze run --collect_code_coverage". - cmdLine = TestStrategy.getArgs("dummy-coverage-script", testAction); + cmdLine = TestStrategy.getArgs(testAction); } catch (ExecException e) { env.getReporter().handle(Event.error(e.getMessage())); return BlazeCommandResult.exitCode(ExitCode.LOCAL_ENVIRONMENTAL_ERROR); 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 274c953484..0fb173cf75 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 @@ -242,7 +242,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(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