Revert "Support execution constraints per exec group"

This reverts commit cbefbd9.

Signed-off-by: Philipp Wollermann <[email protected]>

Author: philwo

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

@@ -483,8 +483,7 @@ public ActionOwner getActionOwner(String execGroup) {
             aspectDescriptors,
             getConfiguration(),
             getExecProperties(execGroup, execProperties),
-            getExecutionPlatform(execGroup),
-            ImmutableSet.of(execGroup));
+            getExecutionPlatform(execGroup));
     actionOwners.put(execGroup, actionOwner);
     return actionOwner;
   }
@@ -591,31 +590,12 @@ 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,
-      Set<String> execGroups) {
+      Map<String, String> targetExecProperties, @Nullable PlatformInfo executionPlatform) {
     Map<String, String> execProperties = new HashMap<>();
 
     if (executionPlatform != null) {
-      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());
-        }
-      }
+      execProperties.putAll(executionPlatform.execProperties());
     }
 
     // If the same key occurs both in the platform and in target-specific properties, the
@@ -631,8 +611,7 @@ public static ActionOwner createActionOwner(
       ImmutableList<AspectDescriptor> aspectDescriptors,
       BuildConfiguration configuration,
       Map<String, String> targetExecProperties,
-      @Nullable PlatformInfo executionPlatform,
-      Set<String> execGroups) {
+      @Nullable PlatformInfo executionPlatform) {
     return ActionOwner.create(
         rule.getLabel(),
         aspectDescriptors,
@@ -642,7 +621,7 @@ public static ActionOwner createActionOwner(
         configuration.checksum(),
         configuration.toBuildEvent(),
         configuration.isHostConfiguration() ? HOST_CONFIGURATION_PROGRESS_TAG : null,
-        computeExecProperties(targetExecProperties, executionPlatform, execGroups),
+        computeExecProperties(targetExecProperties, executionPlatform),
         executionPlatform);
   }
 
@@ -1279,18 +1258,20 @@ private ImmutableMap<String, ImmutableMap<String, String>> parseExecProperties(
       return ImmutableMap.of(DEFAULT_EXEC_GROUP_NAME, ImmutableMap.of());
     } else {
       return parseExecProperties(
-          execProperties, toolchainContexts == null ? null : toolchainContexts.getExecGroups());
+          execProperties,
+          toolchainContexts == null ? ImmutableSet.of() : 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 default exec group, the latter get parsed into their relevant exec
-   * groups.
+   * former get parsed into the target's default exec group, the latter get parsed into their
+   * relevant exec groups.
    */
-  private static Map<String, Map<String, String>> parseExecGroups(
-      Map<String, String> rawExecProperties) {
+  private static ImmutableMap<String, ImmutableMap<String, String>> parseExecProperties(
+      Map<String, String> rawExecProperties, Set<String> execGroups)
+      throws InvalidExecGroupException {
     Map<String, Map<String, String>> consolidatedProperties = new HashMap<>();
     consolidatedProperties.put(DEFAULT_EXEC_GROUP_NAME, new HashMap<>());
     for (Map.Entry<String, String> execProperty : rawExecProperties.entrySet()) {
@@ -1303,30 +1284,14 @@ private static Map<String, Map<String, String>> parseExecGroups(
       } else {
         String execGroup = rawProperty.substring(0, delimiterIndex);
         String property = rawProperty.substring(delimiterIndex + 1);
-        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)) {
+        if (!execGroups.contains(execGroup)) {
           throw new InvalidExecGroupException(
               String.format(
-                  "Tried to set properties for non-existent exec group '%s'.", execGroupName));
+                  "Tried to set exec property '%s' for non-existent exec group '%s'.",
+                  property, execGroup));
         }
+        consolidatedProperties.putIfAbsent(execGroup, new HashMap<>());
+        consolidatedProperties.get(execGroup).put(property, execProperty.getValue());
       }
     }
 

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

@@ -105,10 +105,6 @@ private void createToolchainsAndPlatforms() throws Exception {
         "platform(",
         "    name = 'platform_2',",
         "    constraint_values = [':constraint_2'],",
-        "    exec_properties = {",
-        "        'watermelon.ripeness': 'unripe',",
-        "        'watermelon.color': 'red',",
-        "    },",
         ")");
 
     useConfiguration(
@@ -395,54 +391,4 @@ 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,172 +299,5 @@ 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"