From cbefbd9e5453c084069219ef89403db36b591675 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 8c8251b501b499..29fc484bad7c71 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 @@ -483,7 +483,8 @@ public ActionOwner getActionOwner(String execGroup) { aspectDescriptors, getConfiguration(), getExecProperties(execGroup, execProperties), - getExecutionPlatform(execGroup)); + getExecutionPlatform(execGroup), + ImmutableSet.of(execGroup)); actionOwners.put(execGroup, actionOwner); return actionOwner; } @@ -590,12 +591,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 @@ -611,7 +631,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, @@ -621,7 +642,7 @@ public static ActionOwner createActionOwner( configuration.checksum(), configuration.toBuildEvent(), configuration.isHostConfiguration() ? HOST_CONFIGURATION_PROGRESS_TAG : null, - computeExecProperties(targetExecProperties, executionPlatform), + computeExecProperties(targetExecProperties, executionPlatform, execGroups), executionPlatform); } @@ -1258,20 +1279,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()) { @@ -1284,14 +1303,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 b9185be8a4eba9..bac2cc908ab4cc 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 8319e22d011e52..36d5b6f31f4bbe 100755 --- a/src/test/shell/bazel/platforms_test.sh +++ b/src/test/shell/bazel/platforms_test.sh @@ -297,5 +297,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"