Skip to content

Commit

Permalink
Revert "Support execution constraints per exec group"
Browse files Browse the repository at this point in the history 
This reverts commit cbefbd9.

Signed-off-by: Philipp Wollermann <[email protected]>
  • Loading branch information
@philwo
philwo committed May 20, 2021
1 parent a165baa commit 51fb9e1
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 273 deletions.
69 changes: 17 additions & 52 deletions src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -591,31 +590,12 @@ 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,
Set<String> execGroups) {
Map<String, String> targetExecProperties, @Nullable PlatformInfo executionPlatform) {
Map<String, String> execProperties = new HashMap<>();

if (executionPlatform != null) {
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());
}
}
execProperties.putAll(executionPlatform.execProperties());
}

// If the same key occurs both in the platform and in target-specific properties, the
Expand All @@ -631,8 +611,7 @@ public static ActionOwner createActionOwner(
ImmutableList<AspectDescriptor> aspectDescriptors,
BuildConfiguration configuration,
Map<String, String> targetExecProperties,
@Nullable PlatformInfo executionPlatform,
Set<String> execGroups) {
@Nullable PlatformInfo executionPlatform) {
return ActionOwner.create(
rule.getLabel(),
aspectDescriptors,
Expand All @@ -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);
}

Expand Down Expand Up @@ -1279,18 +1258,20 @@ private ImmutableMap<String, ImmutableMap<String, String>> 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<String, Map<String, String>> parseExecGroups(
Map<String, String> rawExecProperties) {
private static ImmutableMap<String, ImmutableMap<String, String>> parseExecProperties(
Map<String, String> rawExecProperties, Set<String> execGroups)
throws InvalidExecGroupException {
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 @@ -1303,30 +1284,14 @@ private static Map<String, Map<String, String>> 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<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)) {
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());
}
}

Expand Down
54 changes: 0 additions & 54 deletions src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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");
}
}
167 changes: 0 additions & 167 deletions src/test/shell/bazel/platforms_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"

0 comments on commit 51fb9e1

Please sign in to comment.