From 774ed5f7438228b3ed5c380f9ed3cb9136110b78 Mon Sep 17 00:00:00 2001 From: Fredrik Medley Date: Sat, 8 Aug 2026 16:01:06 +0200 Subject: [PATCH] Let use_target_platform_constraints copy unmatched constraints Instead of requiring empty target_compatible_with and exec_compatible_with, use the following backwards compatible logic: exec_constraints = toolchain.exec_compatible_with if toolchain.use_target_platform_constraints: unmatched_constraints = target_platform.constraints - toolchain.target_compatible_with exec_constraints += unmatched_constraints This is useful for the following test toolchain, where building a test for a target platform with wasm+gpu constraints should not execute the test on a platform without gpu. In this example, the execution requirements will be linux+x86_64+gpu. toolchain( name = "wasm_on_linux", exec_compatible_with = ["@platforms//os:linux", "@platforms//cpu:x86_64"], target_compatible_with = ["@platforms//cpu:wasm64"], use_target_platform_constraints = True, toolchain = "@bazel_tools//tools/test:empty_toolchain", toolchain_type = "@bazel_tools//tools/test:default_test_toolchain_type", ) Fixes #30560 --- .../platform/ConstraintCollection.java | 9 ++ .../platform/DeclaredToolchainInfo.java | 35 ++--- .../build/lib/rules/platform/Toolchain.java | 24 +-- .../lib/rules/platform/ToolchainRule.java | 6 +- .../SingleToolchainResolutionFunction.java | 33 ++++- .../analysis/test/TestActionBuilderTest.java | 138 ++++++++++++++++++ 6 files changed, 191 insertions(+), 54 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/ConstraintCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/platform/ConstraintCollection.java index 985bfb04e1ac0d..85cd9a07ab115f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/platform/ConstraintCollection.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/platform/ConstraintCollection.java @@ -129,6 +129,15 @@ public boolean containsAll(Iterable expected) { return findMissing(expected).isEmpty(); } + /** + * Returns the set of {@link ConstraintValueInfo constraints} from {@code expected} that are not + * present in this {@link ConstraintCollection}, either directly, or by being the default for + * their {@link ConstraintSettingInfo}. + */ + public ImmutableList findMissing(ConstraintCollection expected) { + return findMissing(expected.constraints().values()); + } + /** * Returns the set of {@link ConstraintValueInfo constraints} from {@code expected} that are not * present in this {@link ConstraintCollection}, either directly, or by being the default for diff --git a/src/main/java/com/google/devtools/build/lib/analysis/platform/DeclaredToolchainInfo.java b/src/main/java/com/google/devtools/build/lib/analysis/platform/DeclaredToolchainInfo.java index f450c5c0eaf466..3862ee582c016c 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/platform/DeclaredToolchainInfo.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/platform/DeclaredToolchainInfo.java @@ -44,6 +44,7 @@ public record DeclaredToolchainInfo( ToolchainTypeInfo toolchainType, ConstraintCollection execConstraints, ConstraintCollection targetConstraints, + boolean useTargetPlatformConstraints, ImmutableList targetSettings, Label targetLabel, Label resolvedToolchainLabel) @@ -58,20 +59,7 @@ public record DeclaredToolchainInfo( } public boolean hasTargetToExecConstraints() { - // This needs to check identity as the special ConstraintCollection is otherwise equal to the - // empty one. This avoids adding a new field or making ConstraintCollection more complex. - return execConstraints == USE_TARGET_PLATFORM_CONSTRAINTS - && targetConstraints == USE_TARGET_PLATFORM_CONSTRAINTS; - } - - private static final ConstraintCollection USE_TARGET_PLATFORM_CONSTRAINTS; - - static { - try { - USE_TARGET_PLATFORM_CONSTRAINTS = ConstraintCollection.builder().build(); - } catch (ConstraintCollection.DuplicateConstraintException e) { - throw new IllegalStateException(e); - } + return useTargetPlatformConstraints; } /** Builder class to assist in creating {@link DeclaredToolchainInfo} instances. */ @@ -79,6 +67,7 @@ public static class Builder { private ToolchainTypeInfo toolchainType; private final ConstraintCollection.Builder execConstraints = ConstraintCollection.builder(); private final ConstraintCollection.Builder targetConstraints = ConstraintCollection.builder(); + private boolean useTargetPlatformConstraints = false; private final ImmutableList.Builder targetSettings = new ImmutableList.Builder<>(); private Label targetLabel; @@ -117,6 +106,13 @@ public Builder addTargetConstraints(ConstraintValueInfo... constraints) { return addTargetConstraints(ImmutableList.copyOf(constraints)); } + /** Sets the flag to add unmatched target platform constraints as exec constraints. */ + @CanIgnoreReturnValue + public Builder useTargetPlatformConstraints(boolean enabled) { + this.useTargetPlatformConstraints = enabled; + return this; + } + @CanIgnoreReturnValue public Builder addTargetSettings(Iterable targetSettings) { this.targetSettings.addAll(targetSettings); @@ -164,16 +160,7 @@ public DeclaredToolchainInfo build() throws DuplicateConstraintException { toolchainType, execConstraints, targetConstraints, - targetSettings.build(), - targetLabel, - resolvedToolchainLabel); - } - - public DeclaredToolchainInfo buildWithTargetToExecConstraints() { - return new DeclaredToolchainInfo( - toolchainType, - USE_TARGET_PLATFORM_CONSTRAINTS, - USE_TARGET_PLATFORM_CONSTRAINTS, + useTargetPlatformConstraints, targetSettings.build(), targetLabel, resolvedToolchainLabel); diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/Toolchain.java b/src/main/java/com/google/devtools/build/lib/rules/platform/Toolchain.java index 7b6bcee2d1073a..a265b87157a422 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/platform/Toolchain.java +++ b/src/main/java/com/google/devtools/build/lib/rules/platform/Toolchain.java @@ -62,31 +62,19 @@ public ConfiguredTarget create(RuleContext ruleContext) ruleContext .attributes() .get(ToolchainRule.USE_TARGET_PLATFORM_CONSTRAINTS_ATTR, Type.BOOLEAN); - if (targetToExecConstraints && !(execConstraints.isEmpty() && targetConstraints.isEmpty())) { - ruleContext.attributeError( - ToolchainRule.USE_TARGET_PLATFORM_CONSTRAINTS_ATTR, - "Cannot set use_target_platform_constraints to True and also set exec_compatible_with or " - + "target_compatible_with"); - return null; - } DeclaredToolchainInfo registeredToolchain; try { - var registeredToolchainBuilder = + registeredToolchain = DeclaredToolchainInfo.builder() .toolchainType(toolchainType) .addTargetSettings(targetSettings) .resolvedToolchainLabel(resolvedToolchainLabel) - .targetLabel(ruleContext.getLabel()); - if (targetToExecConstraints) { - registeredToolchain = registeredToolchainBuilder.buildWithTargetToExecConstraints(); - } else { - registeredToolchain = - registeredToolchainBuilder - .addExecConstraints(execConstraints) - .addTargetConstraints(targetConstraints) - .build(); - } + .targetLabel(ruleContext.getLabel()) + .addExecConstraints(execConstraints) + .addTargetConstraints(targetConstraints) + .useTargetPlatformConstraints(targetToExecConstraints) + .build(); } catch (DeclaredToolchainInfo.DuplicateConstraintException e) { if (e.execConstraintsException() != null) { ruleContext.attributeError( diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/ToolchainRule.java b/src/main/java/com/google/devtools/build/lib/rules/platform/ToolchainRule.java index 7829b351eb6205..aad04f981ddad8 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/platform/ToolchainRule.java +++ b/src/main/java/com/google/devtools/build/lib/rules/platform/ToolchainRule.java @@ -84,10 +84,8 @@ public RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment env) .allowedFileTypes(FileTypeSet.NO_FILE) .nonconfigurable("part of toolchain configuration")) /* - If True, this toolchain behaves as if its exec_compatible_with and - target_compatible_with constraints are set to those of the current target - platform. exec_compatible_with and target_compatible_with must not - be set in that case. + If True, the constraints in the current target platform that are not part of + target_compatible_with will be added to exec_compatible_with. */ .add( attr(USE_TARGET_PLATFORM_CONSTRAINTS_ATTR, Type.BOOLEAN) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/SingleToolchainResolutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/SingleToolchainResolutionFunction.java index fa41eb0b73999c..e24eba91eee648 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/SingleToolchainResolutionFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/toolchains/SingleToolchainResolutionFunction.java @@ -170,11 +170,8 @@ private static SingleToolchainResolutionValue resolveConstraints( .collect(toImmutableList()); for (DeclaredToolchainInfo toolchain : filteredToolchains) { - // Make sure the target platform matches. A toolchain with use_target_platform_constraints - // matches - // any target platform. - if (!toolchain.hasTargetToExecConstraints() - && !checkConstraints( + // Make sure the target platform matches. + if (!checkConstraints( debugPrinter, toolchain.targetConstraints(), /* isTargetPlatform= */ true, @@ -183,6 +180,17 @@ private static SingleToolchainResolutionValue resolveConstraints( toolchain.resolvedToolchainLabel())) { continue; } + ConstraintCollection unmatchedTargetConstraints = null; + if (toolchain.useTargetPlatformConstraints()) { + try { + unmatchedTargetConstraints = ConstraintCollection.builder().addConstraints( + toolchain.targetConstraints().findMissing(targetPlatform.constraints()) + ).build(); + } catch (ConstraintCollection.DuplicateConstraintException e) { + // This should never happen because this is a subset of targetPlatform.constraints(). + throw new IllegalStateException("unreachable: " + e); + } + } debugPrinter.reportCompatibleTargetPlatform( toolchain.targetLabel(), toolchain.resolvedToolchainLabel()); @@ -213,9 +221,18 @@ private static SingleToolchainResolutionValue resolveConstraints( // Check if the execution constraints match. if (!checkConstraints( debugPrinter, - toolchain.hasTargetToExecConstraints() - ? targetPlatform.constraints() - : toolchain.execConstraints(), + toolchain.execConstraints(), + /* isTargetPlatform= */ false, + executionPlatform, + toolchain.targetLabel(), + toolchain.resolvedToolchainLabel())) { + // Keep looking for a valid toolchain for this exec platform + done = false; + continue; + } + if (toolchain.useTargetPlatformConstraints() && !checkConstraints( + debugPrinter, + unmatchedTargetConstraints, /* isTargetPlatform= */ false, executionPlatform, toolchain.targetLabel(), diff --git a/src/test/java/com/google/devtools/build/lib/analysis/test/TestActionBuilderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/test/TestActionBuilderTest.java index 0a852de8220d01..2e9e40779e00b9 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/test/TestActionBuilderTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/test/TestActionBuilderTest.java @@ -757,6 +757,144 @@ def _some_test_impl(ctx): assertThat(testAction.getExecProperties()).containsExactly("os", targetOs, "cpu", targetCpu); } + /** + * With the default test toolchain, a test action should run on a platform that matches all + * constraints of the target platform. + */ + @Test + public void testExecPlatformMatchesTargetConstraintsWithDefaultTestToolchainFRME() throws Exception { + scratch.file( + "some_test.bzl", + """ + def _some_test_impl(ctx): + script = ctx.actions.declare_file(ctx.attr.name + ".sh") + ctx.actions.write(script, "shell script goes here", is_executable = True) + return [ + DefaultInfo(executable = script), + ] + + some_test = rule( + implementation = _some_test_impl, + test = True, + exec_groups = { + "test": exec_group( + toolchains = [Label(":my_test_toolchain_type")], + ), + }, + ) + """); + scratch.file( + "BUILD", + """ + load(":some_test.bzl", "some_test") + + toolchain_type(name = "my_test_toolchain_type") + + constraint_setting(name = "exec") + constraint_value( + name = "is_exec", + constraint_setting = ":exec", + ) + + constraint_setting(name = "target") + constraint_value( + name = "is_device", + constraint_setting = ":target", + ) + + platform( + name = "target_on_device", + constraint_values = [ + "%1$sos:linux", + "%1$scpu:aarch64", + ":is_device", + ], + ) + + toolchain( + name = "aarch64_test_toolchain", + target_compatible_with = [ + "%1$scpu:aarch64", + "%1$sos:linux", + ], + exec_compatible_with = [ + "%1$scpu:aarch64", + "//:is_exec", + ], + toolchain_type = ":my_test_toolchain_type", + use_target_platform_constraints = True, + toolchain = "%2$s//tools/test:empty_toolchain", + ) + + # Default test toolchain will require exec platform with: + # :linux + # :aarch64 + # :is_device + # linux_gpu_test_toolchain will require exec platform with: + # :aarch64 - Toolchain exec_compatible_with (and target_compatible with). + # :is_device - Requirement from target platform (use_target_platform_constraints). + # :is_exec - Toolchain exec_compatible_with. + + platform( + name = "exec_missing_aarch64", + constraint_values = [ + "%1$sos:linux", + ":is_exec", + ":is_device", + ], + exec_properties = {"key": "bad"}, + ) + + platform( + name = "exec_missing_is_device", + constraint_values = [ + "%1$sos:linux", + "%1$scpu:aarch64", + ":is_exec", + ], + exec_properties = {"key": "bad"}, + ) + + platform( + name = "exec_missing_is_exec", + constraint_values = [ + "%1$sos:linux", + "%1$scpu:aarch64", + ":is_device", + ], + exec_properties = {"key": "bad"}, + ) + + platform( + name = "exec_expected", + constraint_values = [ + "%1$scpu:aarch64", + ":is_exec", + ":is_device", + # macos is not needed: Toolchain target_compatible_with Linux, not exec_compatible_with any os. + "%1$sos:macos", + ], + exec_properties = {"key": "good"}, + ) + + some_test(name = "some_test") + """ + .formatted(TestConstants.CONSTRAINTS_PACKAGE_ROOT, TestConstants.TOOLS_REPOSITORY.getCanonicalForm())); + useConfiguration( + String.format( + "--%s//tools/test:incompatible_use_default_test_toolchain", + TestConstants.TOOLS_REPOSITORY.getCanonicalForm()), + "--platforms=//:target_on_device", + "--extra_toolchains=//:aarch64_test_toolchain", + "--extra_execution_platforms=//:exec_missing_aarch64,//:exec_missing_is_device,//:exec_missing_is_exec,//:exec_expected"); + ImmutableList testStatusList = getTestStatusArtifacts("//:some_test"); + assertThat(testStatusList).hasSize(1); + TestRunnerAction testAction = (TestRunnerAction) getGeneratingAction(testStatusList.get(0)); + assertThat(testAction.getExecutionPlatform().label()) + .isEqualTo(Label.parseCanonicalUnchecked("//:exec_expected")); + assertThat(testAction.getExecProperties()).containsExactly("key", "good"); + } + /** * With the default test toolchain, a failure to find a suitable execution platform will result in * a toolchain resolution error.