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"