Support execution constraints per exec group

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);
   }
 
@@ -1390,19 +1411,18 @@ private ImmutableMap<String, ImmutableMap<String, String>> parseExecProperties(
     } else {
       return parseExecProperties(
           execProperties,
-          toolchainContexts == null ? ImmutableSet.of() : toolchainContexts.getExecGroups());
+          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 +1435,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"