Skip to content

Commit

Permalink
$ bazel test: always configure --run_under=//foo for the exec con…
Browse files Browse the repository at this point in the history
…figuration.

Rationale: test actions run on exec platforms, not the target platform. Configuring a test action for the target platform and running it on the exec platform makes no sense.

This reverts an earlier conversion of `--run_under` from the exec to target platform . The rationale there was:

1. Using the target configuration is consistent with `$ bazel run --run_under`
2. When running tests, bazel uploads both the run_under target and the test itself to the same machine. Therefore they should have the same config.

We should address these points by:

1. Converting `$ bazel run --run_under` similarly(added a TODO)
2. Updating the default test runner similarly

Fixes bazelbuild#22624.

PiperOrigin-RevId: 657586281
Change-Id: I04e7d712015535caa84824a7cec0b79a663bcfbe
  • Loading branch information
gregestren authored and copybara-github committed Jul 30, 2024
1 parent 201da64 commit b03e4c5
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -255,8 +255,21 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env)
.value(
coverageReportGeneratorAttribute(
env.getToolsLabel(DEFAULT_COVERAGE_REPORT_GENERATOR_VALUE))))
// The target itself and run_under both run on the same machine.
.add(attr(":run_under", LABEL).value(RUN_UNDER).skipPrereqValidatorCheck());
// --run_under targets always run on exec machines:
// * For "$ bazel run", they run directly on the machine running bazel.
// * For "$ bazel test", they run on the build machine that executes tests.
//
// This may be different than the target platform. For example, if testing embedded device
// code where the test runner's job is to upload the target binary to the embedded device.
//
// TODO: https://github.com/bazelbuild/bazel/discussions/21805 Setting cfg() here makes
// this work for "$ bazel test" but not "$ bazel run". Make this work for "$ bazel run"
// by updating RunCommand.java to self-transition --run_under to the exec configuration.
.add(
attr(":run_under", LABEL)
.cfg(ExecutionTransitionFactory.createFactory())
.value(RUN_UNDER)
.skipPrereqValidatorCheck());

env.getNetworkAllowlistForTests()
.ifPresent(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import com.google.devtools.build.lib.analysis.TransitiveInfoProviderMapBuilder;
import com.google.devtools.build.lib.analysis.Util;
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.config.RunUnder;
import com.google.devtools.build.lib.analysis.starlark.StarlarkApiProvider;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
Expand Down Expand Up @@ -135,17 +134,6 @@ public RuleConfiguredTarget(
Util.findImplicitDeps(ruleContext),
ruleContext.getRule().getRuleClassObject().getRuleClassId(),
actions);

// If this rule is the run_under target, then check that we have an executable; note that
// run_under is only set in the target configuration, and the target must also be analyzed for
// the target configuration.
RunUnder runUnder = ruleContext.getConfiguration().getRunUnder();
if (runUnder != null && getLabel().equals(runUnder.getLabel())) {
if (getProvider(FilesToRunProvider.class).getExecutable() == null) {
ruleContext.ruleError("run_under target " + runUnder.getLabel() + " is not executable");
}
}

// Make sure that all declared output files are also created as artifacts. The
// CachingAnalysisEnvironment makes sure that they all have generating actions.
if (!ruleContext.hasErrors()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,10 @@ public static RuleClass getTestBaseRule(RuleDefinitionEnvironment env) {
labelCache.get(
toolsRepository
+ BaseRuleClasses.DEFAULT_COVERAGE_REPORT_GENERATOR_VALUE))))
.add(attr(":run_under", LABEL).value(RUN_UNDER))
.add(
attr(":run_under", LABEL)
.cfg(ExecutionTransitionFactory.createFactory())
.value(RUN_UNDER))
.addAttribute(
attr(Rule.IS_EXECUTABLE_ATTRIBUTE_NAME, BOOLEAN)
.value(true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.google.devtools.build.lib.analysis.DependencyResolutionHelpers;
import com.google.devtools.build.lib.analysis.ExecGroupCollection;
import com.google.devtools.build.lib.analysis.ExecGroupCollection.InvalidExecGroupException;
import com.google.devtools.build.lib.analysis.FilesToRunProvider;
import com.google.devtools.build.lib.analysis.ResolvedToolchainContext;
import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
import com.google.devtools.build.lib.analysis.ToolchainCollection;
Expand Down Expand Up @@ -288,6 +289,26 @@ public SkyValue compute(SkyKey key, Environment env)
return incompatibleTarget.get();
}

// IF this build has a --run_under target, check it's an executable. We have to check this at
// the parent: --run_under targets are configured in the exec configuration, but the
// --run_under build option doesn't pass to the exec config.
BuildConfigurationValue config = prereqs.getTargetAndConfiguration().getConfiguration();
if (config != null
&& config.getRunUnder() != null
&& config.getRunUnder().getLabel() != null) {
Optional<ConfiguredTarget> runUnderTarget =
prereqs.getDepValueMap().values().stream()
.map(ConfiguredTargetAndData::getConfiguredTarget)
.filter(d -> d.getLabel().equals(config.getRunUnder().getLabel()))
.findAny();
if (runUnderTarget.isPresent()
&& runUnderTarget.get().getProvider(FilesToRunProvider.class).getExecutable() == null) {
throw new ConfiguredValueCreationException(
prereqs.getTargetAndConfiguration().getTarget(),
"run_under target " + config.getRunUnder().getLabel() + " is not executable");
}
}

// Load the requested toolchains into the ToolchainContext, now that we have dependencies.
ToolchainCollection<ResolvedToolchainContext> toolchainContexts = null;
if (prereqs.getUnloadedToolchainContexts() != null) {
Expand Down

0 comments on commit b03e4c5

Please sign in to comment.