From d0676693310215407224c1b8e8aea9e3eddc183d Mon Sep 17 00:00:00 2001 From: Yuval Date: Fri, 5 Mar 2021 09:53:44 -0800 Subject: [PATCH] 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 #12719 by @ulfjack. Closes #13110. PiperOrigin-RevId: 361167318 --- .../build/lib/analysis/RuleContext.java | 69 ++++++-- .../lib/analysis/StarlarkExecGroupTest.java | 54 ++++++ src/test/shell/bazel/platforms_test.sh | 167 ++++++++++++++++++ 3 files changed, 273 insertions(+), 17 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 3c79f0afeac919..359182081364fe 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 @@ -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 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 computeExecProperties( - Map targetExecProperties, @Nullable PlatformInfo executionPlatform) { + Map targetExecProperties, + @Nullable PlatformInfo executionPlatform, + Set execGroups) { Map execProperties = new HashMap<>(); if (executionPlatform != null) { - execProperties.putAll(executionPlatform.execProperties()); + Map> 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> 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 aspectDescriptors, BuildConfiguration configuration, Map targetExecProperties, - @Nullable PlatformInfo executionPlatform) { + @Nullable PlatformInfo executionPlatform, + Set 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> 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> 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()) { @@ -1415,14 +1434,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/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java index 6c14dc63313ebc..b5ff8bc4c6c5af 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java +++ b/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"); + } } diff --git a/src/test/shell/bazel/platforms_test.sh b/src/test/shell/bazel/platforms_test.sh index e372f41b174684..de2067176e7c9d 100755 --- a/src/test/shell/bazel/platforms_test.sh +++ b/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"