Support setting properties under exec groups on platforms.

quval · quval · commit af02b716c73d · 2020-12-24T10:53:12.000+02:00
Previously, exec groups were rejected on platforms. This change allows setting arbitrary exec groups on platforms, and applying only the relevant ones to actions after platform resolution. Also fixes a bug where the default execution platform for the "test" execution group did not take into account execution constraints set on targets. Builds upon a change by @ulfjack: ulfjack@342543f
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -475,14 +475,21 @@ public ActionOwner getActionOwner() {
    * rather than the host platform. Note that the value is not cached (on the assumption that this
    * method is only called once).
    */
-  public ActionOwner getTestActionOwner() {
+  public ActionOwner getTestActionOwner(boolean useTargetPlatform) {
+    PlatformInfo executionPlatform = null;
+    if (toolchainContexts != null) {
+      executionPlatform = useTargetPlatform
+          ? toolchainContexts.getTargetPlatform()
+          : getExecutionPlatform();
+    }
     ActionOwner actionOwner =
         createActionOwner(
             rule,
             aspectDescriptors,
             getConfiguration(),
             getExecProperties(TEST_RUNNER_EXEC_GROUP, execProperties),
-            toolchainContexts == null ? null : toolchainContexts.getTargetPlatform());
+            executionPlatform,
+            ImmutableSet.of(TEST_RUNNER_EXEC_GROUP));
     return actionOwner;
   }
 
@@ -501,7 +508,8 @@ public ActionOwner getActionOwner(String execGroup) {
             aspectDescriptors,
             getConfiguration(),
             getExecProperties(execGroup, execProperties),
-            getExecutionPlatform(execGroup));
+            getExecutionPlatform(execGroup),
+            ImmutableSet.of(execGroup));
     actionOwners.put(execGroup, actionOwner);
     return actionOwner;
   }
@@ -608,12 +616,24 @@ 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.
+   */
   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());
+      for (Map.Entry<String, Map<String, String>> execGroup :
+          parseExecGroups(executionPlatform.execProperties()).entrySet()) {
+        if (execGroup.getKey().equals(DEFAULT_EXEC_GROUP_NAME)
+                || execGroups.contains(execGroup.getKey())) {
+          execProperties.putAll(execGroup.getValue());
+        }
+      }
     }
 
     // If the same key occurs both in the platform and in target-specific properties, the
@@ -629,7 +649,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,
@@ -639,7 +660,7 @@ public static ActionOwner createActionOwner(
         configuration.checksum(),
         configuration.toBuildEvent(),
         configuration.isHostConfiguration() ? HOST_CONFIGURATION_PROGRESS_TAG : null,
-        computeExecProperties(targetExecProperties, executionPlatform),
+        computeExecProperties(targetExecProperties, executionPlatform, execGroups),
         executionPlatform);
   }
 
@@ -1277,19 +1298,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()) {
@@ -1302,14 +1322,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());
       }
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestActionBuilder.java
@@ -331,12 +331,9 @@ private TestParams createTestAction(int shards) throws InterruptedException {
     ImmutableList.Builder<Artifact> coverageArtifacts = ImmutableList.builder();
     ImmutableList.Builder<ActionInput> testOutputs = ImmutableList.builder();
 
-    ActionOwner actionOwner = testConfiguration.useTargetPlatformForTests()
-        ? ruleContext.getTestActionOwner()
-        : ruleContext.getActionOwner(TEST_RUNNER_EXEC_GROUP);
-    if (actionOwner == null) {
-      actionOwner = ruleContext.getActionOwner();
-    }
+    ActionOwner actionOwner = ruleContext.getTestActionOwner(
+        testConfiguration.useTargetPlatformForTests());
+
     // Use 1-based indices for user friendliness.
     for (int shard = 0; shard < shardRuns; shard++) {
       String shardDir = shardRuns > 1 ? String.format("shard_%d_of_%d", shard + 1, shards) : null;
