From 51fb9e13a864f4f704ae378ea632433bae7ddc31 Mon Sep 17 00:00:00 2001 From: Philipp Wollermann Date: Thu, 20 May 2021 12:03:40 +0200 Subject: [PATCH] Revert "Support execution constraints per exec group" This reverts commit cbefbd9e5453c084069219ef89403db36b591675. Signed-off-by: Philipp Wollermann --- .../build/lib/analysis/RuleContext.java | 69 ++------ .../lib/analysis/StarlarkExecGroupTest.java | 54 ------ src/test/shell/bazel/platforms_test.sh | 167 ------------------ 3 files changed, 17 insertions(+), 273 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 29fc484bad7c71..8c8251b501b499 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,8 +483,7 @@ public ActionOwner getActionOwner(String execGroup) { aspectDescriptors, getConfiguration(), getExecProperties(execGroup, execProperties), - getExecutionPlatform(execGroup), - ImmutableSet.of(execGroup)); + getExecutionPlatform(execGroup)); actionOwners.put(execGroup, actionOwner); return actionOwner; } @@ -591,31 +590,12 @@ 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, - Set execGroups) { + Map targetExecProperties, @Nullable PlatformInfo executionPlatform) { Map execProperties = new HashMap<>(); if (executionPlatform != null) { - 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()); - } - } + execProperties.putAll(executionPlatform.execProperties()); } // If the same key occurs both in the platform and in target-specific properties, the @@ -631,8 +611,7 @@ public static ActionOwner createActionOwner( ImmutableList aspectDescriptors, BuildConfiguration configuration, Map targetExecProperties, - @Nullable PlatformInfo executionPlatform, - Set execGroups) { + @Nullable PlatformInfo executionPlatform) { return ActionOwner.create( rule.getLabel(), aspectDescriptors, @@ -642,7 +621,7 @@ public static ActionOwner createActionOwner( configuration.checksum(), configuration.toBuildEvent(), configuration.isHostConfiguration() ? HOST_CONFIGURATION_PROGRESS_TAG : null, - computeExecProperties(targetExecProperties, executionPlatform, execGroups), + computeExecProperties(targetExecProperties, executionPlatform), executionPlatform); } @@ -1279,18 +1258,20 @@ private ImmutableMap> parseExecProperties( return ImmutableMap.of(DEFAULT_EXEC_GROUP_NAME, ImmutableMap.of()); } else { return parseExecProperties( - execProperties, toolchainContexts == null ? null : toolchainContexts.getExecGroups()); + execProperties, + toolchainContexts == null ? ImmutableSet.of() : 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 default exec group, the latter get parsed into their relevant exec - * groups. + * former get parsed into the target's default exec group, the latter get parsed into their + * relevant exec groups. */ - private static Map> parseExecGroups( - Map rawExecProperties) { + private static ImmutableMap> parseExecProperties( + Map rawExecProperties, Set execGroups) + throws InvalidExecGroupException { Map> consolidatedProperties = new HashMap<>(); consolidatedProperties.put(DEFAULT_EXEC_GROUP_NAME, new HashMap<>()); for (Map.Entry execProperty : rawExecProperties.entrySet()) { @@ -1303,30 +1284,14 @@ private static Map> parseExecGroups( } else { String execGroup = rawProperty.substring(0, delimiterIndex); String property = rawProperty.substring(delimiterIndex + 1); - 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)) { + if (!execGroups.contains(execGroup)) { throw new InvalidExecGroupException( String.format( - "Tried to set properties for non-existent exec group '%s'.", execGroupName)); + "Tried to set exec property '%s' for non-existent exec group '%s'.", + property, execGroup)); } + 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 bac2cc908ab4cc..b9185be8a4eba9 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,10 +105,6 @@ private void createToolchainsAndPlatforms() throws Exception { "platform(", " name = 'platform_2',", " constraint_values = [':constraint_2'],", - " exec_properties = {", - " 'watermelon.ripeness': 'unripe',", - " 'watermelon.color': 'red',", - " },", ")"); useConfiguration( @@ -395,54 +391,4 @@ 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 de2067176e7c9d..e372f41b174684 100755 --- a/src/test/shell/bazel/platforms_test.sh +++ b/src/test/shell/bazel/platforms_test.sh @@ -299,172 +299,5 @@ 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"