Avoid breaking toolchain resolution if platform has defaulted constraint.
### Summary
See https://github.com/bazelbuild/bazel/issues/8778 and https://github.com/bazelbuild/bazel/issues/8274 for context / motivation. In summary, the presence of a `constraint_setting` with `default_constraint_value` in a Platform's constraints will cause toolchain resolution to break for all toolchains that do not explicitly set that constraint value. This makes `default_constraint_value` dangerous because setting it can break other rulesets composed into the same top-level workspace.
cc @jayconrod @m3rcuriel @katre
### Test Plan
I updated the unit test `SingleToolchainResolutionFunction`, verified it failed, then altered the implementation to pass. I also built `//src:bazel` and ran it with a test workspace to verify the end result worked as expected.
Test workspace:
```
$ cat WORKSPACE
register_toolchains("//:my_toolchain")
$ cat BUILD
load(":rules.bzl", "my_toolchain_info", "my_rule")
my_rule(name = "hello")
toolchain_type(name = "my_toolchain_type")
my_toolchain_info(name = "my_toolchain_info")
toolchain(
name = "my_toolchain",
toolchain = "//:my_toolchain_info",
toolchain_type = "//:my_toolchain_type",
)
constraint_setting(
name = "setting",
default_constraint_value = ":value_a",
)
constraint_value(
name = "value_a",
constraint_setting = ":setting",
)
constraint_value(
name = "value_b",
constraint_setting = ":setting",
)
platform(
name = "platform_a",
constraint_values = ["//:value_a"],
)
platform(
name = "platform_b",
constraint_values = ["//:value_b"],
)
$ cat rules.bzl
def _my_toolchain_info(ctx):
return platform_common.ToolchainInfo()
my_toolchain_info = rule(_my_toolchain_info)
def _my_rule(ctx):
pass
my_rule = rule(_my_rule, toolchains = ["//:my_toolchain_type"])
```
Before:
```
$ bazel build --toolchain_resolution_debug --host_platform=//:platform_b //:hello
[...]
INFO: ToolchainResolution: Looking for toolchain of type //:my_toolchain_type...
INFO: ToolchainResolution: Considering toolchain //:my_toolchain_info...
INFO: ToolchainResolution: Toolchain constraint //:setting has value //:value_a, which does not match value //:value_b from the target platform //:platform_b
INFO: ToolchainResolution: Rejected toolchain //:my_toolchain_info, because of target platform mismatch
INFO: ToolchainResolution: No toolchains found
ERROR: While resolving toolchains for target //:hello: no matching toolchains found for types //:my_toolchain_type
```
After:
```
$ bazel build --toolchain_resolution_debug --host_platform=//:platform_b //:hello
[...]
INFO: ToolchainResolution: Looking for toolchain of type //:my_toolchain_type...
INFO: ToolchainResolution: Considering toolchain //:my_toolchain_info...
INFO: ToolchainResolution: For toolchain type //:my_toolchain_type, possible execution platforms and toolchains: {//:platform_b -> //:my_toolchain_info}
INFO: ToolchainResolution: Selected execution platform //:platform_b, type //:my_toolchain_type -> toolchain //:my_toolchain_info
INFO: ToolchainResolution: Selected execution platform //:platform_b,
INFO: Analyzed target //:hello (9 packages loaded, 27 targets configured).
[...]
```
Closes #10102.
PiperOrigin-RevId: 278645171
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 ab791a4..f4dfdb2 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
@@ -185,7 +185,7 @@
return true;
}
- // Then, check the parent, directly to ignore defaults.
+ // Then, check the parent.
if (parent() != null) {
return parent().has(constraint);
}
@@ -193,6 +193,20 @@
return constraint.hasDefaultConstraintValue();
}
+ public boolean hasWithoutDefault(ConstraintSettingInfo constraint) {
+ // First, check locally.
+ if (constraints().containsKey(constraint)) {
+ return true;
+ }
+
+ // Then, check the parent, directly to ignore defaults.
+ if (parent() != null) {
+ return parent().hasWithoutDefault(constraint);
+ }
+
+ return false;
+ }
+
/**
* Returns the {@link ConstraintValueInfo} for the given {@link ConstraintSettingInfo}, or {@code
* null} if none exists.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java
index 1e37fe4..42b4fc9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SingleToolchainResolutionFunction.java
@@ -214,7 +214,21 @@
// Check every constraint_setting in either the toolchain or the platform.
ImmutableSet<ConstraintSettingInfo> mismatchSettings =
toolchainConstraints.diff(platform.constraints());
+ boolean matches = true;
for (ConstraintSettingInfo mismatchSetting : mismatchSettings) {
+ // If a constraint_setting has a default_constraint_value, and the platform
+ // sets a non-default constraint value for the same constraint_setting, then
+ // even toolchains with no reference to that constraint_setting will detect
+ // a mismatch here. This manifests as a toolchain resolution failure (#8778).
+ //
+ // To allow combining rulesets with their own toolchains in a single top-level
+ // workspace, toolchains that do not reference a constraint_setting should not
+ // be forced to match with it.
+ if (!toolchainConstraints.hasWithoutDefault(mismatchSetting)) {
+ continue;
+ }
+ matches = false;
+
debugMessage(
eventHandler,
" Toolchain constraint %s has value %s, "
@@ -229,7 +243,7 @@
platformType,
platform.label());
}
- return mismatchSettings.isEmpty();
+ return matches;
}
@Nullable
diff --git a/src/test/java/com/google/devtools/build/lib/rules/platform/ToolchainTestCase.java b/src/test/java/com/google/devtools/build/lib/rules/platform/ToolchainTestCase.java
index 1fb12ce..dc12122 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/platform/ToolchainTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/platform/ToolchainTestCase.java
@@ -44,8 +44,10 @@
public PlatformInfo macPlatform;
public ConstraintSettingInfo setting;
+ public ConstraintSettingInfo defaultedSetting;
public ConstraintValueInfo linuxConstraint;
public ConstraintValueInfo macConstraint;
+ public ConstraintValueInfo defaultedConstraint;
public Label testToolchainTypeLabel;
public ToolchainTypeInfo testToolchainType;
@@ -101,28 +103,40 @@
"constraint_value(name = 'linux',",
" constraint_setting = ':os')",
"constraint_value(name = 'mac',",
- " constraint_setting = ':os')");
+ " constraint_setting = ':os')",
+ "constraint_setting(name = 'setting_with_default',",
+ " default_constraint_value = ':default_value')",
+ "constraint_value(name = 'default_value',",
+ " constraint_setting = ':setting_with_default')",
+ "constraint_value(name = 'non_default_value',",
+ " constraint_setting = ':setting_with_default')");
scratch.file(
"platforms/BUILD",
"platform(name = 'linux',",
- " constraint_values = ['//constraints:linux'])",
+ " constraint_values = ['//constraints:linux', '//constraints:non_default_value'])",
"platform(name = 'mac',",
- " constraint_values = ['//constraints:mac'])");
+ " constraint_values = ['//constraints:mac', '//constraints:non_default_value'])");
setting = ConstraintSettingInfo.create(makeLabel("//constraints:os"));
linuxConstraint = ConstraintValueInfo.create(setting, makeLabel("//constraints:linux"));
macConstraint = ConstraintValueInfo.create(setting, makeLabel("//constraints:mac"));
+ defaultedSetting =
+ ConstraintSettingInfo.create(makeLabel("//constraints:setting_with_default"));
+ defaultedConstraint =
+ ConstraintValueInfo.create(defaultedSetting, makeLabel("//constraints:non_default_value"));
linuxPlatform =
PlatformInfo.builder()
.setLabel(makeLabel("//platforms:linux"))
.addConstraint(linuxConstraint)
+ .addConstraint(defaultedConstraint)
.build();
macPlatform =
PlatformInfo.builder()
.setLabel(makeLabel("//platforms:mac"))
.addConstraint(macConstraint)
+ .addConstraint(defaultedConstraint)
.build();
}
diff --git a/src/test/shell/bazel/toolchain_test.sh b/src/test/shell/bazel/toolchain_test.sh
index 51c0996..e29a454 100755
--- a/src/test/shell/bazel/toolchain_test.sh
+++ b/src/test/shell/bazel/toolchain_test.sh
@@ -1105,12 +1105,21 @@
constraint_value(name = 'value_foo', constraint_setting = ':setting1')
constraint_value(name = 'value_bar', constraint_setting = ':setting1')
+# Default constraint values don't block toolchain resolution.
+constraint_setting(name = 'setting2', default_constraint_value = ':value_unused')
+constraint_value(name = 'value_unused', constraint_setting = ':setting2')
+
platform(
name = 'platform_default',
- constraint_values = [])
+ constraint_values = [
+ ':value_unused',
+ ])
platform(
name = 'platform_no_default',
- constraint_values = [':value_bar'])
+ constraint_values = [
+ ':value_bar',
+ ':value_unused',
+ ])
EOF
# Add test toolchains using the constraints.
@@ -1135,7 +1144,7 @@
toolchain_type = '//toolchain:test_toolchain',
exec_compatible_with = [],
target_compatible_with = [
- # No constraint set, takes the default.
+ '//platforms:value_foo',
],
toolchain = ':test_toolchain_impl_foo',
visibility = ['//visibility:public'])
@@ -1144,7 +1153,6 @@
toolchain_type = '//toolchain:test_toolchain',
exec_compatible_with = [],
target_compatible_with = [
- # Explicitly sets a non-default value.
'//platforms:value_bar',
],
toolchain = ':test_toolchain_impl_bar',
