Skip to content

Commit

Permalink
BEGIN_PUBLIC
Browse files Browse the repository at this point in the history 
Deprecate `copy_from_rule` from exec_groups.

`test_platform_execgroup_properties_test_inherits_default` test checks if exec_group inherits `exec_compatible_with` from target. Removing `copy_from_rule` we need to specify toolchains and exec constraints directly inside the exec_group. We don't know which constraints the target will set, but those constraints should be relevant only to the rule itself, not to its exec groups. Moreover, seems like this inheritance from target to exec groups is not been used in practice.

RELNOTES[INC]: `copy_from_rule` is exec_groups is deprecated (bazelbuild#17668).

END_PUBLIC

PiperOrigin-RevId: 516521869
Change-Id: I9ac998783e7ccd2ecac53122a21a159c59255816
  • Loading branch information
@kotlaja @fweikert
kotlaja authored and fweikert committed May 25, 2023
1 parent 11ceadd commit 81e9531
Show file tree
Hide file tree
Showing 7 changed files with 59 additions and 158 deletions.
2 changes: 1 addition & 1 deletion src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ def _make_binary_rule(implementation, attrs, executable = False, test = False):
"deploysrcjar": "%{name}_deploy-src.jar",
},
exec_groups = {
"cpp_link": exec_group(copy_from_rule = True),
"cpp_link": exec_group(toolchains = cc_helper.use_cpp_toolchain()),
},
)

Expand Down
2 changes: 1 addition & 1 deletion src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -904,7 +904,7 @@ def make_cc_binary(cc_binary_attrs, **kwargs):
},
fragments = ["google_cpp", "cpp"],
exec_groups = {
"cpp_link": exec_group(copy_from_rule = True),
"cpp_link": exec_group(toolchains = cc_helper.use_cpp_toolchain()),
},
toolchains = cc_helper.use_cpp_toolchain() +
semantics.get_runtimes_toolchain(),
Expand Down
2 changes: 1 addition & 1 deletion src/main/starlark/builtins_bzl/common/cc/cc_library.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,7 @@ cc_library = rule(
incompatible_use_toolchain_transition = True,
provides = [CcInfo],
exec_groups = {
"cpp_link": exec_group(copy_from_rule = True),
"cpp_link": exec_group(toolchains = cc_helper.use_cpp_toolchain()),
},
compile_one_filetype = [".cc", ".h", ".c"],
)
2 changes: 1 addition & 1 deletion src/main/starlark/builtins_bzl/common/cc/cc_test.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ def make_cc_test(with_linkstatic = False, with_aspects = False):
},
fragments = ["google_cpp", "cpp", "coverage"],
exec_groups = {
"cpp_link": exec_group(copy_from_rule = True),
"cpp_link": exec_group(toolchains = cc_helper.use_cpp_toolchain()),
},
toolchains = cc_helper.use_cpp_toolchain() +
semantics.get_runtimes_toolchain(),
Expand Down
159 changes: 44 additions & 115 deletions src/test/java/com/google/devtools/build/lib/analysis/AutoExecGroupsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -867,6 +867,50 @@ public void customExecGroupsAndToolchain(String action) throws Exception {
.isEqualTo(Label.parseCanonical("//platforms:platform_1"));
}

@Test
@TestParameters({
"{action: ctx.actions.run}",
"{action: ctx.actions.run_shell}",
})
public void customExecGroups_execCompatibleWith(String action) throws Exception {
String customExecGroups =
" 'custom_exec_group': exec_group(\n"
+ " exec_compatible_with = ['//platforms:constraint_1'],\n"
+ " toolchains = ['//rule:toolchain_type_1'],\n"
+ " ),\n";
String executable =
action.equals("ctx.actions.run") ? "executable = ctx.executable._tool," : "";
String execCompatibleWith = " exec_compatible_with = ['//platforms:constraint_1'],";
createCustomRule(
/* action= */ action,
/* actionParameters= */ "exec_group = 'custom_exec_group'," + executable,
/* extraAttributes= */ "",
/* toolchains= */ "['//rule:toolchain_type_1']",
/* execGroups= */ customExecGroups,
/* execCompatibleWith= */ execCompatibleWith);
scratch.overwriteFile(
"test/BUILD",
"load('//test:defs.bzl', 'custom_rule')",
"custom_rule(",
" name = 'custom_rule_name',",
" exec_properties = {'custom_exec_group.mem': '64'}",
")");
useConfiguration("--incompatible_auto_exec_groups");

ConfiguredTarget target = getConfiguredTarget("//test:custom_rule_name");
ImmutableMap<String, ExecGroup> execGroups =
getRuleContext(target).getExecGroups().execGroups();
Action ruleAction = (Action) ((RuleConfiguredTarget) target).getActions().get(0);

assertThat(execGroups.keySet()).containsExactly("custom_exec_group", "//rule:toolchain_type_1");
assertThat(execGroups.get("custom_exec_group").toolchainTypes())
.containsExactly(
ToolchainTypeRequirement.create(Label.parseCanonical("//rule:toolchain_type_1")));
assertThat(execGroups.get("custom_exec_group").execCompatibleWith())
.isEqualTo(ImmutableSet.of(Label.parseCanonical("//platforms:constraint_1")));
assertThat(ruleAction.getOwner().getExecProperties()).containsExactly("mem", "64");
}

@Test
@TestParameters({
"{action: ctx.actions.run}",
Expand Down Expand Up @@ -1703,121 +1747,6 @@ public void ccCommonLink_linkstampCompileActionExecutesOnFirstPlatform() throws
.isEqualTo(Label.parseCanonical("//platforms:platform_1"));
}

@Test
@TestParameters({
"{action: ctx.actions.run}",
"{action: ctx.actions.run_shell}",
})
public void customExecGroups_copyFromRuleSetToTrue(String action) throws Exception {
String executable =
action.equals("ctx.actions.run") ? "executable = ctx.executable._tool," : "";
String customExecGroups = " 'custom_exec_group': exec_group(copy_from_rule = True),\n";
String execCompatibleWith = " exec_compatible_with = ['//platforms:constraint_1'],";
createCustomRule(
/* action= */ action,
/* actionParameters= */ "exec_group = 'custom_exec_group'," + executable,
/* extraAttributes= */ "",
/* toolchains= */ "['//rule:toolchain_type_1']",
/* execGroups= */ customExecGroups,
/* execCompatibleWith= */ execCompatibleWith);
scratch.overwriteFile(
"test/BUILD",
"load('//test:defs.bzl', 'custom_rule')",
"custom_rule(",
" name = 'custom_rule_name',",
" exec_properties = {'custom_exec_group.mem': '64'}",
")");
useConfiguration("--incompatible_auto_exec_groups");

ConfiguredTarget target = getConfiguredTarget("//test:custom_rule_name");
ImmutableMap<String, ExecGroup> execGroups =
getRuleContext(target).getExecGroups().execGroups();
Action ruleAction = (Action) ((RuleConfiguredTarget) target).getActions().get(0);

assertThat(execGroups.keySet()).containsExactly("custom_exec_group", "//rule:toolchain_type_1");
assertThat(execGroups.get("custom_exec_group").toolchainTypes())
.containsExactly(
ToolchainTypeRequirement.create(Label.parseCanonical("//rule:toolchain_type_1")));
assertThat(execGroups.get("custom_exec_group").execCompatibleWith())
.isEqualTo(ImmutableSet.of(Label.parseCanonical("//platforms:constraint_1")));
assertThat(ruleAction.getOwner().getExecProperties()).containsExactly("mem", "64");
}

@Test
@TestParameters({
"{action: ctx.actions.run}",
"{action: ctx.actions.run_shell}",
})
public void customExecGroups_copyFromRuleSetToFalse(String action) throws Exception {
String executable =
action.equals("ctx.actions.run") ? "executable = ctx.executable._tool," : "";
String customExecGroups = " 'custom_exec_group': exec_group(copy_from_rule = False),\n";
String execCompatibleWith = " exec_compatible_with = ['//platforms:constraint_1'],";
createCustomRule(
/* action= */ action,
/* actionParameters= */ "exec_group = 'custom_exec_group'," + executable,
/* extraAttributes= */ "",
/* toolchains= */ "['//rule:toolchain_type_1']",
/* execGroups= */ customExecGroups,
/* execCompatibleWith= */ execCompatibleWith);
scratch.overwriteFile(
"test/BUILD",
"load('//test:defs.bzl', 'custom_rule')",
"custom_rule(",
" name = 'custom_rule_name',",
" exec_properties = {'custom_exec_group.mem': '64'}",
")");
useConfiguration("--incompatible_auto_exec_groups");

ConfiguredTarget target = getConfiguredTarget("//test:custom_rule_name");
ImmutableMap<String, ExecGroup> execGroups =
getRuleContext(target).getExecGroups().execGroups();
Action ruleAction = (Action) ((RuleConfiguredTarget) target).getActions().get(0);

assertThat(execGroups.keySet()).containsExactly("custom_exec_group", "//rule:toolchain_type_1");
assertThat(execGroups.get("custom_exec_group").toolchainTypes()).isEmpty();
assertThat(execGroups.get("custom_exec_group").execCompatibleWith()).isEmpty();
assertThat(ruleAction.getOwner().getExecProperties()).containsExactly("mem", "64");
}

@Test
@TestParameters({
"{action: ctx.actions.run}",
"{action: ctx.actions.run_shell}",
})
public void customExecGroups_copyFromRuleSetToTrue_twoToolchainsError(String action)
throws Exception {
String executable =
action.equals("ctx.actions.run") ? "executable = ctx.executable._tool," : "";
String customExecGroups = " 'custom_exec_group': exec_group(copy_from_rule = True),\n";
createCustomRule(
/* action= */ action,
/* actionParameters= */ "exec_group = 'custom_exec_group'," + executable,
/* extraAttributes= */ "",
/* toolchains= */ "['//rule:toolchain_type_1', '//rule:toolchain_type_2']",
/* execGroups= */ customExecGroups,
/* execCompatibleWith= */ "");
scratch.overwriteFile(
"test/BUILD",
"load('//test:defs.bzl', 'custom_rule')",
"custom_rule(",
" name = 'custom_rule_name',",
" exec_properties = {'custom_exec_group.mem': '64'}",
")");
useConfiguration("--incompatible_auto_exec_groups");

reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test:custom_rule_name");

assertContainsEvent(
Pattern.compile(
"Unable to find an execution platform for toolchains \\[(//rule:toolchain_type_1,"
+ " //rule:toolchain_type_2)|(//rule:toolchain_type_2, //rule:toolchain_type_1)\\]"
+ " and target platform //platforms:platform_1 from available execution platforms"
+ " \\[//platforms:platform_1, //platforms:platform_2,"
+ " //third_party/local_config_platform:host\\]"));
}

@Test
public void ccCommonCompile_cppCompileActionExecutesOnFirstPlatform() throws Exception {
scratch.file(
Expand Down
32 changes: 0 additions & 32 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 @@ -15,7 +15,6 @@
package com.google.devtools.build.lib.analysis;

import static com.google.common.truth.Truth.assertThat;
import static com.google.devtools.build.lib.analysis.testing.ExecGroupCollectionSubject.assertThat;
import static com.google.devtools.build.lib.packages.ExecGroup.DEFAULT_EXEC_GROUP_NAME;

import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
Expand Down Expand Up @@ -507,37 +506,6 @@ public void testSetUnknownExecGroup() throws Exception {
assertContainsEvent("errors encountered while analyzing target '//test:papaya'");
}

@Test
public void ruleInheritsRuleRequirements() throws Exception {
createToolchainsAndPlatforms();
scratch.file(
"test/defs.bzl",
"MyInfo = provider()",
"def _impl(ctx):",
" return []",
"my_rule = rule(",
" implementation = _impl,",
" exec_groups = {",
" 'watermelon': exec_group(copy_from_rule = True),",
" },",
" exec_compatible_with = ['//platform:constraint_1'],",
" toolchains = ['//rule:toolchain_type_1'],",
")");
scratch.file("test/BUILD", "load('//test:defs.bzl', 'my_rule')", "my_rule(name = 'papaya')");

ConfiguredTarget ct = getConfiguredTarget("//test:papaya");
ExecGroupCollection execGroups = getRuleContext(ct).getExecGroups();
assertThat(execGroups).isNotNull();
assertThat(execGroups).hasExecGroup("watermelon");
// TODO(https://github.com/bazelbuild/bazel/issues/14726): Add tests of optional toolchains.
assertThat(execGroups).execGroup("watermelon").hasToolchainType("//rule:toolchain_type_1");
assertThat(execGroups)
.execGroup("watermelon")
.toolchainType("//rule:toolchain_type_1")
.isMandatory();
assertThat(execGroups).execGroup("watermelon").hasExecCompatibleWith("//platform:constraint_1");
}

@Test
public void ruleInheritsPlatformExecGroupExecProperty() throws Exception {
createToolchainsAndPlatforms();
Expand Down
18 changes: 11 additions & 7 deletions src/test/shell/integration/exec_group_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,6 @@ EOF

bazel test --extra_execution_platforms="${pkg}:platform_no_constraint,${pkg}:platform_with_constraint" ${pkg}:a --execution_log_json_file out.txt || fail "Test failed"
grep --after=4 "platform" out.txt | grep "exec_property" || fail "Did not find the property key"
grep --after=4 "platform" out.txt | grep "no_constraint" && fail "Found the wrong property."
grep --after=4 "platform" out.txt | grep "requires_test_constraint" || fail "Did not find the property value"
}

Expand Down Expand Up @@ -703,8 +702,10 @@ def _impl(target, ctx):
sample_aspect = aspect(
implementation = _impl,
exec_groups = {
# extra should inherit both the exec constraint and the toolchain.
'extra': exec_group(copy_from_rule = True),
'extra': exec_group(
exec_compatible_with = ['//${pkg}/platform:value_foo'],
toolchains = ['//${pkg}/platform:toolchain_type']
),
},
exec_compatible_with = ['//${pkg}/platform:value_foo'],
toolchains = ['//${pkg}/platform:toolchain_type'],
Expand Down Expand Up @@ -856,8 +857,11 @@ def _impl(ctx):
sample_rule = rule(
implementation = _impl,
exec_groups = {
# extra should inherit both the exec constraint and the toolchain.
'extra': exec_group(copy_from_rule = True),
# extra should contain both the exec constraint and the toolchain.
'extra': exec_group(
exec_compatible_with = ['//${pkg}/platform:value_foo'],
toolchains = ['//${pkg}/platform:toolchain_type']
),
},
exec_compatible_with = ['//${pkg}/platform:value_foo'],
toolchains = ['//${pkg}/platform:toolchain_type'],
Expand Down Expand Up @@ -913,8 +917,8 @@ def _impl(ctx):
sample_rule = rule(
implementation = _impl,
exec_groups = {
# extra should inherit the toolchain, and the exec constraint from the target.
'extra': exec_group(copy_from_rule = True),
# extra should contain the toolchain, and the exec constraint from the target.
'extra': exec_group(toolchains = ['//${pkg}/platform:toolchain_type']),
},
toolchains = ['//${pkg}/platform:toolchain_type'],
)
Expand Down

0 comments on commit 81e9531

Please sign in to comment.