Repository: bazelbuild/bazel
Issue: Constraints should have defaults
Labels: type: feature request
P2

constraint_setting() and constraint_value() should be allowed to select a default value to be used if none is specified in the platform.

Issue comments:
I remember @ianthehat asked for this.

Pull request review comments:
What's the use case for defaults? Are they required for a constraint_setting or are they optional?

For example, if have `constraint_setting(name = "macos_version", default_constraint_value = ":osx10_11_6")` and we build for platform `"osx10_11_6"`, does that risk pulling in toolchains that don't even support OSX because they automatically take the default?

More generally, is there risk in a toolchain "indirectly" advertising support for a constraint the toolchain doesn't even know exists?
Defaults are optional.

During toolchain resolution, only the constraint settings that are present in either the platform or the toolchain are considered. Therefore, if the platform does not declare a constraint value for "macos_version", and the toolchain doesn't declare a constraint value for "macos_version", the setting isn't considered. If either of them does use that setting, the other would be considered to have the default value, which the constraint author should have chosen as a safe value.

This feature is intended less for thing like "macos_version", and more for tunable constraints such as "compilation_mode" or "enable_race_detection", to be used as toolchain modes during resolution.
Does "enable_race_detection" really need a default, or can its absence in a toolchain definition alone imply lack of support? Are there any precise motivating examples?

Sorry, I'm not trying to be difficult about this. There's a very real argument for defaults under circumstances where the alternative is having to, say, add a new mode that just one toolchain variant uses, then being forced to explicitly tag all other variants with the "old" mode setting. That can get unwieldy.

But I've also learned to be more cautious when using defaults. They provide behavior at a distance that can be hard to isolate and in a worst case toolchains, etc. could get selected on properties the toolchain designers didn't even consider.

We've seen this tug and pull with existing compatible_with / restricted_to deployments.
In addition from the uses rules_go wants, there are some uses that RBE wants as well. In both cases, the initial advice given was "use the absence of the value as the default", and that works okay, but in both cases users end up tagging several platform and toolchain definitions with comments like "constraint race_detection disabled by default", where it would be easier to just add the constraint value.

Also, having an explicit constraint value for the default makes it easier to switch the default later.

Patch that resolved the issue:
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 af9ac5701fef34..8f8629a1663a04 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
@@ -73,7 +73,12 @@ private ConstraintSettingInfo convertKey(Object key, Location loc) throws EvalEx
   @Nullable
   @Override
   public ConstraintValueInfo get(ConstraintSettingInfo constraint) {
-    return constraints.get(constraint);
+    if (constraints.containsKey(constraint)) {
+      return constraints.get(constraint);
+    }
+
+    // Since this constraint isn't set, fall back to the default.
+    return constraint.defaultConstraintValue();
   }
 
   @Override


Test patch:
diff --git a/src/test/java/com/google/devtools/build/lib/rules/platform/ConstraintCollectionApiTest.java b/src/test/java/com/google/devtools/build/lib/rules/platform/ConstraintCollectionApiTest.java
index 5cb360d0fef9da..7a18c28dc99062 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/platform/ConstraintCollectionApiTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/platform/ConstraintCollectionApiTest.java
@@ -40,9 +40,11 @@ public class ConstraintCollectionApiTest extends PlatformInfoApiTest {
 
   @Test
   public void testConstraintSettings() throws Exception {
-    platformBuilder().addConstraint("s1", "value1").addConstraint("s2", "value2").build();
+    constraintBuilder("//foo:s1").addConstraintValue("value1").write();
+    constraintBuilder("//foo:s2").addConstraintValue("value2").write();
+    platformBuilder("//foo:my_platform").addConstraint("value1").addConstraint("value2").write();
 
-    ConstraintCollection constraintCollection = fetchConstraintCollection();
+    ConstraintCollection constraintCollection = fetchConstraintCollection("//foo:my_platform");
     assertThat(constraintCollection).isNotNull();
 
     assertThat(collectLabels(constraintCollection.constraintSettings()))
@@ -52,9 +54,11 @@ public void testConstraintSettings() throws Exception {
 
   @Test
   public void testGet() throws Exception {
-    platformBuilder().addConstraint("s1", "value1").addConstraint("s2", "value2").build();
+    constraintBuilder("//foo:s1").addConstraintValue("value1").write();
+    constraintBuilder("//foo:s2").addConstraintValue("value2").write();
+    platformBuilder("//foo:my_platform").addConstraint("value1").addConstraint("value2").write();
 
-    ConstraintCollection constraintCollection = fetchConstraintCollection();
+    ConstraintCollection constraintCollection = fetchConstraintCollection("//foo:my_platform");
     assertThat(constraintCollection).isNotNull();
 
     ConstraintSettingInfo setting =
@@ -66,7 +70,9 @@ public void testGet() throws Exception {
 
   @Test
   public void testGet_starlark() throws Exception {
-    platformBuilder().addConstraint("s1", "value1").addConstraint("s2", "value2").build();
+    constraintBuilder("//foo:s1").addConstraintValue("value1").write();
+    constraintBuilder("//foo:s2").addConstraintValue("value2").write();
+    platformBuilder("//foo:my_platform").addConstraint("value1").addConstraint("value2").write();
 
     scratch.file(
         "verify/verify.bzl",
@@ -130,13 +136,43 @@ public void testGet_starlark() throws Exception {
             ConstraintSettingInfo.create(Label.parseAbsoluteUnchecked("//foo:s2")));
   }
 
+  @Test
+  public void testGet_defaultConstraintValues() throws Exception {
+    constraintBuilder("//constraint/default:basic")
+        .defaultConstraintValue("foo")
+        .addConstraintValue("bar")
+        .write();
+    constraintBuilder("//constraint/default:other").write();
+
+    platformBuilder("//constraint/default:plat_with_default").write();
+    platformBuilder("//constraint/default:plat_without_default").addConstraint("bar").write();
+
+    ConstraintSettingInfo basicConstraintSetting =
+        fetchConstraintSettingInfo("//constraint/default:basic");
+    ConstraintSettingInfo otherConstraintSetting =
+        fetchConstraintSettingInfo("//constraint/default:other");
+
+    ConstraintCollection constraintCollectionWithDefault = fetchConstraintCollection("//constraint/default:plat_with_default");
+    assertThat(constraintCollectionWithDefault).isNotNull();
+    assertThat(constraintCollectionWithDefault.get(basicConstraintSetting)).isNotNull();
+    assertThat(constraintCollectionWithDefault.get(basicConstraintSetting).label())
+        .isEqualTo(makeLabel("//constraint/default:foo"));
+    assertThat(constraintCollectionWithDefault.get(otherConstraintSetting)).isNull();
+
+    ConstraintCollection constraintCollectionWithoutDefault = fetchConstraintCollection("//constraint/default:plat_without_default");
+    assertThat(constraintCollectionWithoutDefault).isNotNull();
+    assertThat(constraintCollectionWithoutDefault.get(basicConstraintSetting)).isNotNull();
+    assertThat(constraintCollectionWithoutDefault.get(basicConstraintSetting).label())
+        .isEqualTo(makeLabel("//constraint/default:bar"));
+  }
+
   private Set<Label> collectLabels(Collection<? extends ConstraintSettingInfo> settings) {
     return settings.stream().map(ConstraintSettingInfo::label).collect(Collectors.toSet());
   }
 
   @Nullable
-  private ConstraintCollection fetchConstraintCollection() throws Exception {
-    PlatformInfo platformInfo = fetchPlatformInfo();
+  private ConstraintCollection fetchConstraintCollection(String platformLabel) throws Exception {
+    PlatformInfo platformInfo = fetchPlatformInfo(platformLabel);
     if (platformInfo == null) {
       return null;
     }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformInfoApiTest.java b/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformInfoApiTest.java
index f8c248c2d38a82..2e8b2498af8a58 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformInfoApiTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/platform/PlatformInfoApiTest.java
@@ -17,16 +17,16 @@
 import static com.google.common.truth.Truth.assertThat;
 
 import com.google.common.base.Strings;
-import com.google.common.collect.HashMultimap;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.platform.ConstraintSettingInfo;
 import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
 import com.google.devtools.build.lib.analysis.platform.PlatformProviderUtils;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
-import java.util.Map;
+import com.google.devtools.build.lib.cmdline.Label;
+import java.util.ArrayList;
+import java.util.List;
 import javax.annotation.Nullable;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -38,10 +38,11 @@ public class PlatformInfoApiTest extends BuildViewTestCase {
 
   @Test
   public void testPlatform() throws Exception {
-    platformBuilder().addConstraint("basic", "value1").build();
+    constraintBuilder("//foo:basic").addConstraintValue("value1").write();
+    platformBuilder("//foo:my_platform").addConstraint("value1").write();
     assertNoEvents();
 
-    PlatformInfo platformInfo = fetchPlatformInfo();
+    PlatformInfo platformInfo = fetchPlatformInfo("//foo:my_platform");
     assertThat(platformInfo).isNotNull();
     ConstraintSettingInfo constraintSetting =
         ConstraintSettingInfo.create(makeLabel("//foo:basic"));
@@ -53,91 +54,147 @@ public void testPlatform() throws Exception {
 
   @Test
   public void testPlatform_overlappingConstraintValueError() throws Exception {
+    List<String> lines =
+        new ImmutableList.Builder<String>()
+            .addAll(
+                constraintBuilder("//foo:basic")
+                    .addConstraintValue("value1")
+                    .addConstraintValue("value2")
+                    .lines())
+            .addAll(
+                platformBuilder("//foo:my_platform")
+                    .addConstraint("value1")
+                    .addConstraint("value2")
+                    .lines())
+            .build();
+
     checkError(
         "foo",
         "my_platform",
         "Duplicate constraint_values detected: "
             + "constraint_setting //foo:basic has [//foo:value1, //foo:value2]",
-        "constraint_setting(name = 'basic')",
-        "constraint_value(name = 'value1',",
-        "    constraint_setting = ':basic',",
-        ")",
-        "constraint_value(name = 'value2',",
-        "    constraint_setting = ':basic',",
-        ")",
-        "platform(name = 'my_platform',",
-        "    constraint_values = [",
-        "       ':value1',",
-        "       ':value2',",
-        "])");
+        lines.toArray(new String[] {}));
   }
 
   @Test
   public void testPlatform_remoteExecution() throws Exception {
-    platformBuilder().setRemoteExecutionProperties("foo: val1").build();
+    platformBuilder("//foo:my_platform").setRemoteExecutionProperties("foo: val1").write();
     assertNoEvents();
 
-    PlatformInfo platformInfo = fetchPlatformInfo();
+    PlatformInfo platformInfo = fetchPlatformInfo("//foo:my_platform");
     assertThat(platformInfo).isNotNull();
     assertThat(platformInfo.remoteExecutionProperties()).isEqualTo("foo: val1");
   }
 
-  PlatformBuilder platformBuilder() {
-    return new PlatformBuilder();
+  ConstraintBuilder constraintBuilder(String name) {
+    return new ConstraintBuilder(name);
   }
 
-  final class PlatformBuilder {
-    private final Multimap<String, String> constraints = HashMultimap.create();
-    private String remoteExecutionProperties = "";
+  final class ConstraintBuilder {
+    private final Label label;
+    private final List<String> constraintValues = new ArrayList<>();
+    private String defaultConstraintValue = null;
+
+    public ConstraintBuilder(String name) {
+      this.label = Label.parseAbsoluteUnchecked(name);
+    }
 
-    public PlatformBuilder addConstraint(String setting, String value) {
-      this.constraints.put(setting, value);
+    public ConstraintBuilder defaultConstraintValue(String defaultConstraintValue) {
+      this.defaultConstraintValue = defaultConstraintValue;
+      this.constraintValues.add(defaultConstraintValue);
       return this;
     }
 
-    public PlatformBuilder setRemoteExecutionProperties(String value) {
-      this.remoteExecutionProperties = value;
+    public ConstraintBuilder addConstraintValue(String constraintValue) {
+      this.constraintValues.add(constraintValue);
       return this;
     }
 
-    public void build() throws Exception {
+    public List<String> lines() {
       ImmutableList.Builder<String> lines = ImmutableList.builder();
 
-      // Add the constraint settings.
-      for (String name : constraints.keySet()) {
-        lines.add("constraint_setting(name = '" + name + "')");
+      // Add the constraint setting.
+      lines.add("constraint_setting(name = '" + label.getName() + "',");
+      if (!Strings.isNullOrEmpty(defaultConstraintValue)) {
+        lines.add("  default_constraint_value = ':" + defaultConstraintValue + "',");
       }
+      lines.add(")");
 
       // Add the constraint values.
-      for (Map.Entry<String, String> entry : constraints.entries()) {
+      for (String constraintValue : constraintValues) {
         lines.add(
             "constraint_value(",
-            "  name = '" + entry.getValue() + "',",
-            "  constraint_setting = ':" + entry.getKey() + "',",
+            "  name = '" + constraintValue + "',",
+            "  constraint_setting = ':" + label.getName() + "',",
             ")");
       }
 
-      // Add the platform.
-      lines.add("platform(", "  name = 'my_platform',");
-      if (!constraints.isEmpty()) {
-        lines.add("  constraint_values = [");
-        for (String name : constraints.values()) {
-          lines.add("    ':" + name + "',");
-        }
-        lines.add("  ],");
+      return lines.build();
+    }
+
+    public void write() throws Exception {
+      List<String> lines = lines();
+      String filename = label.getPackageFragment().getRelative("BUILD").getPathString();
+      scratch.appendFile(filename, lines.toArray(new String[] {}));
+    }
+  }
+
+  PlatformBuilder platformBuilder(String name) {
+    return new PlatformBuilder(name);
+  }
+
+  final class PlatformBuilder {
+    private final Label label;
+    private final List<String> constraintValues = new ArrayList<>();
+    private String remoteExecutionProperties = "";
+
+    public PlatformBuilder(String name) {
+      this.label = Label.parseAbsoluteUnchecked(name);
+    }
+
+    public PlatformBuilder addConstraint(String value) {
+      this.constraintValues.add(value);
+      return this;
+    }
+
+    public PlatformBuilder setRemoteExecutionProperties(String value) {
+      this.remoteExecutionProperties = value;
+      return this;
+    }
+
+    public List<String> lines() {
+      ImmutableList.Builder<String> lines = ImmutableList.builder();
+
+      lines.add("platform(", "  name = '" + label.getName() + "',");
+      lines.add("  constraint_values = [");
+      for (String name : constraintValues) {
+        lines.add("    ':" + name + "',");
       }
+      lines.add("  ],");
       if (!Strings.isNullOrEmpty(remoteExecutionProperties)) {
         lines.add("  remote_execution_properties = '" + remoteExecutionProperties + "',");
       }
       lines.add(")");
 
-      scratch.file("foo/BUILD", lines.build().toArray(new String[] {}));
+      return lines.build();
+    }
+
+    public void write() throws Exception {
+      List<String> lines = lines();
+      String filename = label.getPackageFragment().getRelative("BUILD").getPathString();
+      scratch.appendFile(filename, lines.toArray(new String[] {}));
     }
   }
 
   @Nullable
-  PlatformInfo fetchPlatformInfo() throws Exception {
-    ConfiguredTarget myRuleTarget = getConfiguredTarget("//foo:my_platform");
-    return PlatformProviderUtils.platform(myRuleTarget);
+  ConstraintSettingInfo fetchConstraintSettingInfo(String label) throws Exception {
+    ConfiguredTarget target = getConfiguredTarget(label);
+    return PlatformProviderUtils.constraintSetting(target);
+  }
+
+  @Nullable
+  PlatformInfo fetchPlatformInfo(String platformLabel) throws Exception {
+    ConfiguredTarget target = getConfiguredTarget(platformLabel);
+    return PlatformProviderUtils.platform(target);
   }
 }
diff --git a/src/test/shell/bazel/toolchain_test.sh b/src/test/shell/bazel/toolchain_test.sh
index 097e874641ca60..da8c639e36ca34 100755
--- a/src/test/shell/bazel/toolchain_test.sh
+++ b/src/test/shell/bazel/toolchain_test.sh
@@ -914,4 +914,88 @@ EOF
     expect_log "Selected execution platform //platforms:platform2_4"
 }
 
+function test_default_constraint_values {
+  # Add test constraints and platforms.
+  mkdir -p platforms
+  cat >> platforms/BUILD <<EOF
+package(default_visibility = ['//visibility:public'])
+constraint_setting(name = 'setting1', default_constraint_value = ':value_foo')
+constraint_value(name = 'value_foo', constraint_setting = ':setting1')
+constraint_value(name = 'value_bar', constraint_setting = ':setting1')
+
+platform(
+    name = 'platform_default',
+    constraint_values = [])
+platform(
+    name = 'platform_no_default',
+    constraint_values = [':value_bar'])
+EOF
+
+  # Add test toolchains using the constraints.
+  write_test_toolchain
+  cat >> BUILD <<EOF
+load('//toolchain:toolchain_test_toolchain.bzl', 'test_toolchain')
+
+# Define the toolchains.
+test_toolchain(
+    name = 'test_toolchain_impl_foo',
+    extra_str = 'foo',
+    visibility = ['//visibility:public'])
+
+test_toolchain(
+    name = 'test_toolchain_impl_bar',
+    extra_str = 'bar',
+    visibility = ['//visibility:public'])
+
+# Declare the toolchains.
+toolchain(
+    name = 'test_toolchain_foo',
+    toolchain_type = '//toolchain:test_toolchain',
+    exec_compatible_with = [],
+    target_compatible_with = [
+      # No constraint set, takes the default.
+    ],
+    toolchain = ':test_toolchain_impl_foo',
+    visibility = ['//visibility:public'])
+toolchain(
+    name = 'test_toolchain_bar',
+    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',
+    visibility = ['//visibility:public'])
+EOF
+
+  # Register the toolchains
+  cat >> WORKSPACE <<EOF
+register_toolchains('//:test_toolchain_foo', '//:test_toolchain_bar')
+EOF
+
+  write_test_rule
+  mkdir -p demo
+  cat >> demo/BUILD <<EOF
+load('//toolchain:rule_use_toolchain.bzl', 'use_toolchain')
+# Use the toolchain.
+use_toolchain(
+    name = 'use',
+    message = 'this is the rule')
+EOF
+
+  # Test some builds and verify which was used.
+  # This should use the default value.
+  bazel build \
+    --platforms=//platforms:platform_default \
+    //demo:use &> $TEST_log || fail "Build failed"
+  expect_log 'toolchain extra_str: "foo"'
+
+  # This should use the explicit value.
+  bazel build \
+    --platforms=//platforms:platform_no_default \
+    //demo:use &> $TEST_log || fail "Build failed"
+  expect_log 'toolchain extra_str: "bar"'
+}
+
 run_suite "toolchain tests"