Skip to content

Commit

Permalink
Support execution constraints per exec group
Browse files Browse the repository at this point in the history
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
  • Loading branch information
quval authored and philwo committed Mar 15, 2021
1 parent a973c65 commit 704af8c
Show file tree
Hide file tree
Showing 3 changed files with 273 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -590,12 +591,31 @@ public ImmutableList<Artifact> 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<String, String> computeExecProperties(
Map<String, String> targetExecProperties, @Nullable PlatformInfo executionPlatform) {
Map<String, String> targetExecProperties,
@Nullable PlatformInfo executionPlatform,
Set<String> execGroups) {
Map<String, String> execProperties = new HashMap<>();

if (executionPlatform != null) {
execProperties.putAll(executionPlatform.execProperties());
Map<String, Map<String, String>> 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<String, Map<String, String>> 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
Expand All @@ -611,7 +631,8 @@ public static ActionOwner createActionOwner(
ImmutableList<AspectDescriptor> aspectDescriptors,
BuildConfiguration configuration,
Map<String, String> targetExecProperties,
@Nullable PlatformInfo executionPlatform) {
@Nullable PlatformInfo executionPlatform,
Set<String> execGroups) {
return ActionOwner.create(
rule.getLabel(),
aspectDescriptors,
Expand All @@ -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);
}

Expand Down Expand Up @@ -1258,20 +1279,18 @@ private ImmutableMap<String, ImmutableMap<String, String>> 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<String, ImmutableMap<String, String>> parseExecProperties(
Map<String, String> rawExecProperties, Set<String> execGroups)
throws InvalidExecGroupException {
private static Map<String, Map<String, String>> parseExecGroups(
Map<String, String> rawExecProperties) {
Map<String, Map<String, String>> consolidatedProperties = new HashMap<>();
consolidatedProperties.put(DEFAULT_EXEC_GROUP_NAME, new HashMap<>());
for (Map.Entry<String, String> execProperty : rawExecProperties.entrySet()) {
Expand All @@ -1284,14 +1303,30 @@ private static ImmutableMap<String, ImmutableMap<String, String>> 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<String, ImmutableMap<String, String>> parseExecProperties(
Map<String, String> rawExecProperties, @Nullable Set<String> execGroups)
throws InvalidExecGroupException {
Map<String, Map<String, String>> consolidatedProperties = parseExecGroups(rawExecProperties);
if (execGroups != null) {
for (Map.Entry<String, Map<String, String>> 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());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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");
}
}
167 changes: 167 additions & 0 deletions src/test/shell/bazel/platforms_test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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"

0 comments on commit 704af8c

Please sign in to comment.