Support execution constraints per exec group

When computing exec properties from the execution platform for an action, take into account only the properties that are relevant to the action's exec groups. In particular, allow setting exec properties for arbitrary exec groups on platforms. Previously, any such properties were rejected.

With this change, the following becomes possible:
```
cc_test(
    name = "my_test",
    ...,
    exec_properties = {
        "test.key": "value",
    },
)
```
This will apply `{"key": "value"}` for the test-runner action only (i.e., compilation and linkage won't be affected). The following also becomes possible:
```
platform(
    name = "test_platform",
    constraint_values = [":test_constraint"],
    exec_properties = {
        "test.key": "value",
    },
)

cc_test(
    name = "my_test",
    ...,
    exec_compatible_with = [":test_constraint"],
)
```
This achieves the same in a more succinct way.

For related discussion, see PR bazelbuild#12719 by @ulfjack.

Closes bazelbuild#13110.

PiperOrigin-RevId: 361167318

src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java

@@ -515,7 +515,8 @@ public ActionOwner getActionOwner(String execGroup) {
             aspectDescriptors,
             getConfiguration(),
             getExecProperties(execGroup, execProperties),
-            getExecutionPlatform(execGroup));
+            getExecutionPlatform(execGroup),
+            ImmutableSet.of(execGroup));
     actionOwners.put(execGroup, actionOwner);
     return actionOwner;
   }
@@ -622,12 +623,31 @@ public ImmutableList<Artifact> getBuildInfo(BuildInfoKey key) throws Interrupted
             AnalysisUtils.isStampingEnabled(this, getConfiguration()), key, getConfiguration());
   }
 
+  /**
+   * Computes a map of exec properties given the execution platform, taking only properties in exec
+   * groups that are applicable to this action. Properties for specific exec groups take precedence
+   * over properties that don't specify an exec group.
+   */
   private static ImmutableMap<String, String> computeExecProperties(
-      Map<String, String> targetExecProperties, @Nullable PlatformInfo executionPlatform) {
+      Map<String, String> targetExecProperties,
+      @Nullable PlatformInfo executionPlatform,
+      Set<String> execGroups) {
     Map<String, String> execProperties = new HashMap<>();
 
     if (executionPlatform != null) {
-      execProperties.putAll(executionPlatform.execProperties());
+      Map<String, Map<String, String>> execPropertiesPerGroup =
+          parseExecGroups(executionPlatform.execProperties());
+
+      if (execPropertiesPerGroup.containsKey(DEFAULT_EXEC_GROUP_NAME)) {
+        execProperties.putAll(execPropertiesPerGroup.get(DEFAULT_EXEC_GROUP_NAME));
+        execPropertiesPerGroup.remove(DEFAULT_EXEC_GROUP_NAME);
+      }
+
+      for (Map.Entry<String, Map<String, String>> execGroup : execPropertiesPerGroup.entrySet()) {
+        if (execGroups.contains(execGroup.getKey())) {
+          execProperties.putAll(execGroup.getValue());
+        }
+      }
     }
 
     // If the same key occurs both in the platform and in target-specific properties, the
@@ -643,7 +663,8 @@ public static ActionOwner createActionOwner(
       ImmutableList<AspectDescriptor> aspectDescriptors,
       BuildConfiguration configuration,
       Map<String, String> targetExecProperties,
-      @Nullable PlatformInfo executionPlatform) {
+      @Nullable PlatformInfo executionPlatform,
+      Set<String> execGroups) {
     return ActionOwner.create(
         rule.getLabel(),
         aspectDescriptors,
@@ -653,7 +674,7 @@ public static ActionOwner createActionOwner(
         configuration.checksum(),
         configuration.toBuildEvent(),
         configuration.isHostConfiguration() ? HOST_CONFIGURATION_PROGRESS_TAG : null,
-        computeExecProperties(targetExecProperties, executionPlatform),
+        computeExecProperties(targetExecProperties, executionPlatform, execGroups),
         executionPlatform);
   }
 
@@ -1389,20 +1410,18 @@ private ImmutableMap<String, ImmutableMap<String, String>> parseExecProperties(
       return ImmutableMap.of(DEFAULT_EXEC_GROUP_NAME, ImmutableMap.of());
     } else {
       return parseExecProperties(
-          execProperties,
-          toolchainContexts == null ? ImmutableSet.of() : toolchainContexts.getExecGroups());
+          execProperties, toolchainContexts == null ? null : toolchainContexts.getExecGroups());
     }
   }
 
   /**
    * Parse raw exec properties attribute value into a map of exec group names to their properties.
    * The raw map can have keys of two forms: (1) 'property' and (2) 'exec_group_name.property'. The
-   * former get parsed into the target's default exec group, the latter get parsed into their
-   * relevant exec groups.
+   * former get parsed into the default exec group, the latter get parsed into their relevant exec
+   * groups.
    */
-  private static ImmutableMap<String, ImmutableMap<String, String>> parseExecProperties(
-      Map<String, String> rawExecProperties, Set<String> execGroups)
-      throws InvalidExecGroupException {
+  private static Map<String, Map<String, String>> parseExecGroups(
+      Map<String, String> rawExecProperties) {
     Map<String, Map<String, String>> consolidatedProperties = new HashMap<>();
     consolidatedProperties.put(DEFAULT_EXEC_GROUP_NAME, new HashMap<>());
     for (Map.Entry<String, String> execProperty : rawExecProperties.entrySet()) {
@@ -1415,14 +1434,30 @@ private static ImmutableMap<String, ImmutableMap<String, String>> parseExecPrope
       } else {
         String execGroup = rawProperty.substring(0, delimiterIndex);
         String property = rawProperty.substring(delimiterIndex + 1);
-        if (!execGroups.contains(execGroup)) {
+        consolidatedProperties.putIfAbsent(execGroup, new HashMap<>());
+        consolidatedProperties.get(execGroup).put(property, execProperty.getValue());
+      }
+    }
+    return consolidatedProperties;
+  }
+
+  /**
+   * Parse raw exec properties attribute value into a map of exec group names to their properties.
+   * If given a set of exec groups, validates all the exec groups in the map are applicable to the
+   * action.
+   */
+  private static ImmutableMap<String, ImmutableMap<String, String>> parseExecProperties(
+      Map<String, String> rawExecProperties, @Nullable Set<String> execGroups)
+      throws InvalidExecGroupException {
+    Map<String, Map<String, String>> consolidatedProperties = parseExecGroups(rawExecProperties);
+    if (execGroups != null) {
+      for (Map.Entry<String, Map<String, String>> execGroup : consolidatedProperties.entrySet()) {
+        String execGroupName = execGroup.getKey();
+        if (!execGroupName.equals(DEFAULT_EXEC_GROUP_NAME) && !execGroups.contains(execGroupName)) {
           throw new InvalidExecGroupException(
               String.format(
-                  "Tried to set exec property '%s' for non-existent exec group '%s'.",
-                  property, execGroup));
+                  "Tried to set properties for non-existent exec group '%s'.", execGroupName));
         }
-        consolidatedProperties.putIfAbsent(execGroup, new HashMap<>());
-        consolidatedProperties.get(execGroup).put(property, execProperty.getValue());
       }
     }
 

src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java

@@ -105,6 +105,10 @@ private void createToolchainsAndPlatforms() throws Exception {
         "platform(",
         "    name = 'platform_2',",
         "    constraint_values = [':constraint_2'],",
+        "    exec_properties = {",
+        "        'watermelon.ripeness': 'unripe',",
+        "        'watermelon.color': 'red',",
+        "    },",
         ")");
 
     useConfiguration(
@@ -391,4 +395,54 @@ public void testInheritsRuleRequirements() throws Exception {
                 ImmutableSet.of(Label.parseAbsoluteUnchecked("//rule:toolchain_type_1")),
                 ImmutableSet.of(Label.parseAbsoluteUnchecked("//platform:constraint_1"))));
   }
+
+  @Test
+  public void testInheritsPlatformExecGroupExecProperty() throws Exception {
+    createToolchainsAndPlatforms();
+    writeRuleWithActionsAndWatermelonExecGroup();
+
+    scratch.file(
+        "test/BUILD",
+        "load('//test:defs.bzl', 'with_actions')",
+        "with_actions(",
+        "  name = 'papaya',",
+        "  output = 'out.txt',",
+        "  watermelon_output = 'watermelon_out.txt',",
+        ")");
+
+    ConfiguredTarget target = getConfiguredTarget("//test:papaya");
+
+    assertThat(
+            getGeneratingAction(target, "test/watermelon_out.txt").getOwner().getExecProperties())
+        .containsExactly("ripeness", "unripe", "color", "red");
+    assertThat(getGeneratingAction(target, "test/out.txt").getOwner().getExecProperties())
+        .containsExactly();
+  }
+
+  @Test
+  public void testOverridePlatformExecGroupExecProperty() throws Exception {
+    createToolchainsAndPlatforms();
+    writeRuleWithActionsAndWatermelonExecGroup();
+
+    scratch.file(
+        "test/BUILD",
+        "load('//test:defs.bzl', 'with_actions')",
+        "with_actions(",
+        "  name = 'papaya',",
+        "  output = 'out.txt',",
+        "  watermelon_output = 'watermelon_out.txt',",
+        "  exec_properties = {",
+        "    'watermelon.ripeness': 'ripe',",
+        "    'ripeness': 'unknown',",
+        "  },",
+        ")");
+
+    ConfiguredTarget target = getConfiguredTarget("//test:papaya");
+
+    assertThat(
+            getGeneratingAction(target, "test/watermelon_out.txt").getOwner().getExecProperties())
+        .containsExactly("ripeness", "ripe", "color", "red");
+    assertThat(getGeneratingAction(target, "test/out.txt").getOwner().getExecProperties())
+        .containsExactly("ripeness", "unknown");
+  }
 }

src/test/shell/bazel/platforms_test.sh

@@ -299,5 +299,172 @@ EOF
   grep "child_value" out.txt || fail "Did not find the overriding value"
 }
 
+function test_platform_execgroup_properties_cc_test() {
+  cat > a.cc <<'EOF'
+int main() {}
+EOF
+  cat > BUILD <<'EOF'
+cc_test(
+  name = "a",
+  srcs = ["a.cc"],
+)
+
+platform(
+  name = "my_platform",
+  parents = ["@local_config_platform//:host"],
+  exec_properties = {
+    "platform_key": "default_value",
+    "test.platform_key": "test_value",
+  }
+)
+EOF
+  bazel build --extra_execution_platforms=":my_platform" :a --execution_log_json_file out.txt || fail "Build failed"
+  grep "platform_key" out.txt || fail "Did not find the platform key"
+  grep "default_value" out.txt || fail "Did not find the default value"
+  grep "test_value" out.txt && fail "Used the test-action value when not testing"
+
+  bazel test --extra_execution_platforms=":my_platform" :a --execution_log_json_file out.txt || fail "Test failed"
+  grep "platform_key" out.txt || fail "Did not find the platform key"
+  grep "test_value" out.txt || fail "Did not find the test-action value"
+}
+
+function test_platform_execgroup_properties_nongroup_override_cc_test() {
+  cat > a.cc <<'EOF'
+int main() {}
+EOF
+  cat > BUILD <<'EOF'
+cc_test(
+  name = "a",
+  srcs = ["a.cc"],
+  exec_properties = {
+    "platform_key": "override_value",
+  },
+)
+
+platform(
+  name = "my_platform",
+  parents = ["@local_config_platform//:host"],
+  exec_properties = {
+    "platform_key": "default_value",
+    "test.platform_key": "test_value",
+  }
+)
+EOF
+  bazel build --extra_execution_platforms=":my_platform" :a --execution_log_json_file out.txt || fail "Build failed"
+  grep "platform_key" out.txt || fail "Did not find the platform key"
+  grep "override_value" out.txt || fail "Did not find the overriding value"
+  grep "default_value" out.txt && fail "Used the default value"
+
+  bazel test --extra_execution_platforms=":my_platform" :a --execution_log_json_file out.txt || fail "Test failed"
+  grep "platform_key" out.txt || fail "Did not find the platform key"
+  grep "override_value" out.txt || fail "Did not find the overriding value"
+}
+
+function test_platform_execgroup_properties_group_override_cc_test() {
+  cat > a.cc <<'EOF'
+int main() {}
+EOF
+  cat > BUILD <<'EOF'
+cc_test(
+  name = "a",
+  srcs = ["a.cc"],
+  exec_properties = {
+    "test.platform_key": "test_override",
+  },
+)
+
+platform(
+  name = "my_platform",
+  parents = ["@local_config_platform//:host"],
+  exec_properties = {
+    "platform_key": "default_value",
+    "test.platform_key": "test_value",
+  }
+)
+EOF
+  bazel build --extra_execution_platforms=":my_platform" :a --execution_log_json_file out.txt || fail "Build failed"
+  grep "platform_key" out.txt || fail "Did not find the platform key"
+  grep "default_value" out.txt || fail "Used the default value"
+
+  bazel test --extra_execution_platforms=":my_platform" :a --execution_log_json_file out.txt || fail "Test failed"
+  grep "platform_key" out.txt || fail "Did not find the platform key"
+  grep "test_override" out.txt || fail "Did not find the overriding test-action value"
+}
+
+function test_platform_execgroup_properties_override_group_and_default_cc_test() {
+  cat > a.cc <<'EOF'
+int main() {}
+EOF
+  cat > BUILD <<'EOF'
+cc_test(
+  name = "a",
+  srcs = ["a.cc"],
+  exec_properties = {
+    "platform_key": "override_value",
+    "test.platform_key": "test_override",
+  },
+)
+
+platform(
+  name = "my_platform",
+  parents = ["@local_config_platform//:host"],
+  exec_properties = {
+    "platform_key": "default_value",
+    "test.platform_key": "test_value",
+  }
+)
+EOF
+  bazel build --extra_execution_platforms=":my_platform" :a --execution_log_json_file out.txt || fail "Build failed"
+  grep "platform_key" out.txt || fail "Did not find the platform key"
+  grep "override_value" out.txt || fail "Did not find the overriding value"
+  grep "default_value" out.txt && fail "Used the default value"
+
+  bazel test --extra_execution_platforms=":my_platform" :a --execution_log_json_file out.txt || fail "Test failed"
+  grep "platform_key" out.txt || fail "Did not find the platform key"
+  grep "test_override" out.txt || fail "Did not find the overriding test-action value"
+}
+
+function test_platform_properties_only_applied_for_relevant_execgroups_cc_test() {
+  cat > a.cc <<'EOF'
+int main() {}
+EOF
+  cat > BUILD <<'EOF'
+cc_test(
+  name = "a",
+  srcs = ["a.cc"],
+)
+
+platform(
+  name = "my_platform",
+  parents = ["@local_config_platform//:host"],
+  exec_properties = {
+    "platform_key": "default_value",
+    "unknown.platform_key": "unknown_value",
+  }
+)
+EOF
+  bazel test --extra_execution_platforms=":my_platform" :a --execution_log_json_file out.txt || fail "Build failed"
+  grep "platform_key" out.txt || fail "Did not find the platform key"
+  grep "default_value" out.txt || fail "Did not find the default value"
+}
+
+function test_cannot_set_properties_for_irrelevant_execgroup_on_target_cc_test() {
+  cat > a.cc <<'EOF'
+int main() {}
+EOF
+  cat > BUILD <<'EOF'
+cc_test(
+  name = "a",
+  srcs = ["a.cc"],
+  exec_properties = {
+    "platform_key": "default_value",
+    "unknown.platform_key": "unknown_value",
+  }
+)
+EOF
+  bazel test :a &> $TEST_log && fail "Build passed when we expected an error"
+  grep "Tried to set properties for non-existent exec group" $TEST_log || fail "Did not complain about unknown exec group"
+}
+
 run_suite "platform mapping test"