From b03e4c5e4df4728b943945370141c047b9d98039 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 30 Jul 2024 07:36:21 -0700 Subject: [PATCH] `$ bazel test`: always configure `--run_under=//foo` for the exec configuration. 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 https://github.com/bazelbuild/bazel/issues/22624. PiperOrigin-RevId: 657586281 Change-Id: I04e7d712015535caa84824a7cec0b79a663bcfbe --- .../build/lib/analysis/BaseRuleClasses.java | 17 +++++++++++++-- .../RuleConfiguredTarget.java | 12 ----------- .../starlark/StarlarkRuleClassFunctions.java | 5 ++++- .../skyframe/ConfiguredTargetFunction.java | 21 +++++++++++++++++++ 4 files changed, 40 insertions(+), 15 deletions(-) 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 06a9b4b4b4ca2a..88fd2f720d7850 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 @@ -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( diff --git a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/RuleConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/RuleConfiguredTarget.java index 1ad0e808dfaa3b..1e483d34e0ada7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/RuleConfiguredTarget.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/RuleConfiguredTarget.java @@ -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; @@ -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()) { diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java index 3b3f1e98458c5b..be490366a26d22 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java @@ -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) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java index b35213baf522a0..280aae6511e6f7 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java @@ -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; @@ -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 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 toolchainContexts = null; if (prereqs.getUnloadedToolchainContexts() != null) {