From 62ce436c8376f20691c097af3067a60e19924a5d Mon Sep 17 00:00:00 2001 From: Yuval Date: Thu, 24 Dec 2020 10:53:12 +0200 Subject: [PATCH 1/6] 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. --- .../build/lib/analysis/RuleContext.java | 61 ++++++++++++++----- 1 file changed, 45 insertions(+), 16 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..1a1fe0559e5bd7 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,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 @@ -643,7 +656,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 +667,7 @@ public static ActionOwner createActionOwner( configuration.checksum(), configuration.toBuildEvent(), configuration.isHostConfiguration() ? HOST_CONFIGURATION_PROGRESS_TAG : null, - computeExecProperties(targetExecProperties, executionPlatform), + computeExecProperties(targetExecProperties, executionPlatform, execGroups), executionPlatform); } @@ -1390,19 +1404,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()) { @@ -1415,14 +1428,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()); } } From bb997b3b2096ece83ef100e465de083a97309db6 Mon Sep 17 00:00:00 2001 From: Yuval Date: Thu, 25 Feb 2021 22:23:22 +0200 Subject: [PATCH 2/6] First apply the properties for the default exec group, then for others. --- .../devtools/build/lib/analysis/RuleContext.java | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 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 1a1fe0559e5bd7..468c5efc0af7be 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 @@ -634,10 +634,16 @@ private static ImmutableMap computeExecProperties( Map execProperties = new HashMap<>(); if (executionPlatform != null) { - for (Map.Entry> execGroup : - parseExecGroups(executionPlatform.execProperties()).entrySet()) { - if (execGroup.getKey().equals(DEFAULT_EXEC_GROUP_NAME) - || execGroups.contains(execGroup.getKey())) { + 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()); } } From da8952da43c24b122c3fa444fc262c00727b027e Mon Sep 17 00:00:00 2001 From: Yuval Date: Thu, 25 Feb 2021 22:39:12 +0200 Subject: [PATCH 3/6] Doc behaviour when applying properties for exec groups: properties for specific groups take precedence. --- .../com/google/devtools/build/lib/analysis/RuleContext.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 468c5efc0af7be..8ca4000bc3e304 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 @@ -625,7 +625,8 @@ public ImmutableList getBuildInfo(BuildInfoKey key) throws Interrupted /** * Computes a map of exec properties given the execution platform, taking only properties in exec - * groups that are applicable to this action. + * 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, From 903343e956b296f102cd4004e6076a7017ba9058 Mon Sep 17 00:00:00 2001 From: Yuval Date: Thu, 4 Mar 2021 01:23:42 +0200 Subject: [PATCH 4/6] Add test cases. --- .../lib/analysis/StarlarkExecGroupTest.java | 54 ++++++++ src/test/shell/bazel/platforms_test.sh | 125 ++++++++++++++++++ 2 files changed, 179 insertions(+) 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..090c36a70a6eb6 100755 --- a/src/test/shell/bazel/platforms_test.sh +++ b/src/test/shell/bazel/platforms_test.sh @@ -299,5 +299,130 @@ 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" +} + run_suite "platform mapping test" From 2ecd6e0e2242a2859e5a5da7ddfd1d339ef45ef4 Mon Sep 17 00:00:00 2001 From: Yuval Date: Thu, 4 Mar 2021 11:34:38 +0200 Subject: [PATCH 5/6] Test cases: execution properties for unknown execgroups --- src/test/shell/bazel/platforms_test.sh | 49 ++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/src/test/shell/bazel/platforms_test.sh b/src/test/shell/bazel/platforms_test.sh index 090c36a70a6eb6..9a02cc8b76adae 100755 --- a/src/test/shell/bazel/platforms_test.sh +++ b/src/test/shell/bazel/platforms_test.sh @@ -321,7 +321,7 @@ 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" + (! 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" @@ -353,7 +353,7 @@ 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" + (! 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" @@ -417,12 +417,55 @@ 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" + (! 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" + (! grep "unknown_value" out.txt) || fail "Used the value for an unknown execgroup" +} + +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" From d6a443856c6961fc34cbbac11a6264457b644792 Mon Sep 17 00:00:00 2001 From: Yuval Date: Thu, 4 Mar 2021 18:11:52 +0200 Subject: [PATCH 6/6] Align style with other tests --- src/test/shell/bazel/platforms_test.sh | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/test/shell/bazel/platforms_test.sh b/src/test/shell/bazel/platforms_test.sh index 9a02cc8b76adae..de2067176e7c9d 100755 --- a/src/test/shell/bazel/platforms_test.sh +++ b/src/test/shell/bazel/platforms_test.sh @@ -321,7 +321,7 @@ 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" + 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" @@ -353,7 +353,7 @@ 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" + 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" @@ -417,7 +417,7 @@ 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" + 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" @@ -446,7 +446,6 @@ 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" - (! grep "unknown_value" out.txt) || fail "Used the value for an unknown execgroup" } function test_cannot_set_properties_for_irrelevant_execgroup_on_target_cc_test() { @@ -463,7 +462,7 @@ cc_test( } ) EOF - (! bazel test :a >& $TEST_log) || fail "Build passed when we expected an error" + 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" }