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..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 @@ -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); } @@ -1390,19 +1411,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 +1435,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"