From af02b716c73d45c1494a468817e829ac23ddc152 Mon Sep 17 00:00:00 2001 From: Yuval Date: Thu, 24 Dec 2020 10:53:12 +0200 Subject: [PATCH] Support setting properties under exec groups on platforms. 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: https://github.com/ulfjack/bazel/commit/342543fac3f3e2dae48dd1daf7fbd51e0f103261 --- .../build/lib/analysis/RuleContext.java | 72 ++++++++++++++----- .../lib/analysis/test/TestActionBuilder.java | 9 +-- 2 files changed, 57 insertions(+), 24 deletions(-) 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 index bcc7dae69d84ba..a40fbb1e810abc 100644 --- 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 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 computeExecProperties( - Map targetExecProperties, @Nullable PlatformInfo executionPlatform) { + Map targetExecProperties, + @Nullable PlatformInfo executionPlatform, + Set execGroups) { Map execProperties = new HashMap<>(); if (executionPlatform != null) { - execProperties.putAll(executionPlatform.execProperties()); + for (Map.Entry> 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 aspectDescriptors, BuildConfiguration configuration, Map targetExecProperties, - @Nullable PlatformInfo executionPlatform) { + @Nullable PlatformInfo executionPlatform, + Set 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> 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> parseExecProperties( - Map rawExecProperties, Set execGroups) - throws InvalidExecGroupException { + private static Map> parseExecGroups( + Map rawExecProperties) { Map> consolidatedProperties = new HashMap<>(); consolidatedProperties.put(DEFAULT_EXEC_GROUP_NAME, new HashMap<>()); for (Map.Entry execProperty : rawExecProperties.entrySet()) { @@ -1302,14 +1322,30 @@ private static ImmutableMap> 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> parseExecProperties( + Map rawExecProperties, @Nullable Set execGroups) + throws InvalidExecGroupException { + Map> consolidatedProperties = parseExecGroups(rawExecProperties); + if (execGroups != null) { + for (Map.Entry> 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 index 83385b729cf3a2..a78d31d0cdc34a 100644 --- 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 coverageArtifacts = ImmutableList.builder(); ImmutableList.Builder 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;