BEGIN_PUBLIC

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 (#17668). END_PUBLIC PiperOrigin-RevId: 516521869 Change-Id: I9ac998783e7ccd2ecac53122a21a159c59255816
bazelbuild · Mar 14, 2023 · 9eadedb · 9eadedb
1 parent 49a9502
commit 9eadedb
Show file tree

Hide file tree

Showing 7 changed files with 59 additions and 158 deletions.
diff --git a/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl b/src/main/starlark/builtins_bzl/bazel/java/bazel_java_binary.bzl
@@ -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()),
         },
     )
 

diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_binary.bzl
@@ -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(),

diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_library.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_library.bzl
@@ -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"],
 )
diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_test.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_test.bzl
@@ -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(),

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AutoExecGroupsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AutoExecGroupsTest.java
@@ -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}",
@@ -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(

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
@@ -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;
@@ -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();

diff --git a/src/test/shell/integration/exec_group_test.sh b/src/test/shell/integration/exec_group_test.sh
@@ -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"
 }
 
@@ -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'],
@@ -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'],
@@ -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'],
 )