[7.0.0] Automatically add function transition allow list when needed (#…

…19855)

If the allowlist is already present its value is respected (The package
paths and label name are fixed/checked. Only repository name can be
changed).

Automatically add allowlist with tools repository as defined by rule
definition environment.

This fixes the incompatibility introduced in:
#19493 (because whilelist is
ignored and default allowlist is added)

Commit
bb7fb2d

PiperOrigin-RevId: 574226941
Change-Id: If90f78a610d7bd3c107dcd94a39902c7c939aa7b

Co-authored-by: Googler <[email protected]>

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java

@@ -37,6 +37,7 @@
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.analysis.Allowlist;
 import com.google.devtools.build.lib.analysis.BaseRuleClasses;
+import com.google.devtools.build.lib.analysis.PackageSpecificationProvider;
 import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment;
 import com.google.devtools.build.lib.analysis.TemplateVariableInfo;
 import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
@@ -502,7 +503,6 @@ public static StarlarkRuleFunction createRule(
     }
 
     boolean hasStarlarkDefinedTransition = false;
-    boolean hasFunctionTransitionAllowlist = false;
     for (Pair<String, StarlarkAttrModule.Descriptor> attribute : attributes) {
       String name = attribute.getFirst();
       StarlarkAttrModule.Descriptor descriptor = attribute.getSecond();
@@ -518,29 +518,6 @@ public static StarlarkRuleFunction createRule(
         }
         builder.setHasAnalysisTestTransition();
       }
-      // Check for existence of the function transition allowlist attribute.
-      // TODO(b/121385274): remove when we stop allowlisting starlark transitions
-      if (name.equals(FunctionSplitTransitionAllowlist.ATTRIBUTE_NAME)) {
-        if (!BuildType.isLabelType(attr.getType())) {
-          throw Starlark.errorf("_allowlist_function_transition attribute must be a label type");
-        }
-        if (attr.getDefaultValueUnchecked() == null) {
-          throw Starlark.errorf(
-              "_allowlist_function_transition attribute must have a default value");
-        }
-        Label defaultLabel = (Label) attr.getDefaultValueUnchecked();
-        // Check the label value for package and target name, to make sure this works properly
-        // in Bazel where it is expected to be found under @bazel_tools.
-        if (!(defaultLabel
-                    .getPackageName()
-                    .equals(FunctionSplitTransitionAllowlist.LABEL.getPackageName())
-                && defaultLabel.getName().equals(FunctionSplitTransitionAllowlist.LABEL.getName()))) {
-          throw Starlark.errorf(
-              "_allowlist_function_transition attribute (%s) does not have the expected value %s",
-              defaultLabel, FunctionSplitTransitionAllowlist.LABEL);
-        }
-        hasFunctionTransitionAllowlist = true;
-      }
 
       try {
         if (builder.contains(attr.getName())) {
@@ -653,15 +630,40 @@ public static StarlarkRuleFunction createRule(
       }
     }
 
-    // TODO(b/121385274): remove when we stop allowlisting starlark transitions
+    boolean hasFunctionTransitionAllowlist = false;
+    // Check for existence of the function transition allowlist attribute.
+    if (builder.contains(FunctionSplitTransitionAllowlist.ATTRIBUTE_NAME)) {
+      Attribute attr = builder.getAttribute(FunctionSplitTransitionAllowlist.ATTRIBUTE_NAME);
+      if (!BuildType.isLabelType(attr.getType())) {
+        throw Starlark.errorf("_allowlist_function_transition attribute must be a label type");
+      }
+      if (attr.getDefaultValueUnchecked() == null) {
+        throw Starlark.errorf("_allowlist_function_transition attribute must have a default value");
+      }
+      Label defaultLabel = (Label) attr.getDefaultValueUnchecked();
+      // Check the label value for package and target name, to make sure this works properly
+      // in Bazel where it is expected to be found under @bazel_tools.
+      if (!(defaultLabel
+              .getPackageName()
+              .equals(FunctionSplitTransitionAllowlist.LABEL.getPackageName())
+          && defaultLabel.getName().equals(FunctionSplitTransitionAllowlist.LABEL.getName()))) {
+        throw Starlark.errorf(
+            "_allowlist_function_transition attribute (%s) does not have the expected value %s",
+            defaultLabel, FunctionSplitTransitionAllowlist.LABEL);
+      }
+      hasFunctionTransitionAllowlist = true;
+    }
     if (hasStarlarkDefinedTransition) {
       if (!bzlFile.getRepository().getNameWithAt().equals("@_builtins")) {
         if (!hasFunctionTransitionAllowlist) {
-          throw Starlark.errorf(
-              "Use of Starlark transition without allowlist attribute"
-                  + " '_allowlist_function_transition'. See Starlark transitions documentation"
-                  + " for details and usage: %s %s",
-              builder.getRuleDefinitionEnvironmentLabel(), builder.getType());
+          // add the allowlist automatically
+          builder.add(
+              attr(FunctionSplitTransitionAllowlist.ATTRIBUTE_NAME, LABEL)
+                  .cfg(ExecutionTransitionFactory.createFactory())
+                  .mandatoryBuiltinProviders(ImmutableList.of(PackageSpecificationProvider.class))
+                  .value(
+                      ruleDefinitionEnvironment.getToolsLabel(
+                          FunctionSplitTransitionAllowlist.LABEL_STR)));
         }
         builder.addAllowlistChecker(FUNCTION_TRANSITION_ALLOWLIST_CHECKER);
       }

src/main/java/com/google/devtools/build/lib/packages/FunctionSplitTransitionAllowlist.java

@@ -16,14 +16,12 @@
 
 import com.google.devtools.build.lib.cmdline.Label;
 
-/**
- * This class provides constants associated with the function split transition allowlist.
- */
+/** This class provides constants associated with the function split transition allowlist. */
 public class FunctionSplitTransitionAllowlist {
   public static final String NAME = "function_transition";
   public static final String ATTRIBUTE_NAME = "$allowlist_function_transition";
-  public static final Label LABEL =
-      Label.parseCanonicalUnchecked("//tools/allowlists/function_transition_allowlist");
+  public static final String LABEL_STR = "//tools/allowlists/function_transition_allowlist";
+  public static final Label LABEL = Label.parseCanonicalUnchecked(LABEL_STR);
 
   private FunctionSplitTransitionAllowlist() {}
 }

src/main/java/com/google/devtools/build/lib/packages/RuleClass.java

@@ -1303,6 +1303,10 @@ public boolean contains(String name) {
       return attributes.containsKey(name);
     }
 
+    public Attribute getAttribute(String name) {
+      return attributes.get(name);
+    }
+
     /** Returns a list of all attributes added to this Builder so far. */
     public ImmutableList<Attribute> getAttributes() {
       return ImmutableList.copyOf(attributes.values());

src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java

@@ -985,9 +985,10 @@ public void testCannotTransitionOnExperimentalFlag() throws Exception {
   }
 
   @Test
-  public void testCannotTransitionWithoutAllowlist() throws Exception {
+  public void testTransitionIsCheckedAgainstDefaultAllowlist() throws Exception {
     scratch.overwriteFile(
-        "tools/allowlists/function_transition_allowlist/BUILD",
+        TestConstants.TOOLS_REPOSITORY_SCRATCH
+            + "tools/allowlists/function_transition_allowlist/BUILD",
         "package_group(",
         "    name = 'function_transition_allowlist',",
         "    packages = [],",
@@ -1013,7 +1014,7 @@ public void testCannotTransitionWithoutAllowlist() throws Exception {
 
     reporter.removeHandler(failFastHandler);
     getConfiguredTarget("//test");
-    assertContainsEvent("Use of Starlark transition without allowlist");
+    assertContainsEvent("Non-allowlisted use of Starlark transition");
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java

@@ -58,6 +58,7 @@
 import com.google.devtools.build.lib.rules.objc.ObjcProvider;
 import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
 import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
+import com.google.devtools.build.lib.testutil.TestConstants;
 import com.google.devtools.build.lib.testutil.TestRuleClassProvider;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -2852,18 +2853,20 @@ public void testBadAnalysisTestRule_notAnalysisTest() throws Exception {
   }
 
   @Test
-  public void testBadAllowlistTransition_noAllowlist() throws Exception {
+  public void testBadAllowlistTransition_automaticAllowlist() throws Exception {
     scratch.overwriteFile(
-        "tools/allowlists/function_transition_allowlist/BUILD",
+        TestConstants.TOOLS_REPOSITORY_SCRATCH
+            + "tools/allowlists/function_transition_allowlist/BUILD",
         "package_group(",
         "    name = 'function_transition_allowlist',",
         "    packages = [",
-        "        '//test/...',",
+        // cross-repo allowlists don't work well
+        analysisMock.isThisBazel() ? "'public'," : "'//test/...',",
         "    ],",
         ")");
     scratch.file(
         "test/rules.bzl",
-        "def transition_func(settings):",
+        "def transition_func(settings, attr):",
         "  return {'t0': {'//command_line_option:cpu': 'k8'}}",
         "my_transition = transition(implementation = transition_func, inputs = [],",
         "  outputs = ['//command_line_option:cpu'])",
@@ -2887,9 +2890,8 @@ public void testBadAllowlistTransition_noAllowlist() throws Exception {
         "my_rule(name = 'my_rule', dep = ':dep')",
         "simple_rule(name = 'dep')");
 
-    reporter.removeHandler(failFastHandler);
     getConfiguredTarget("//test:my_rule");
-    assertContainsEvent("Use of Starlark transition without allowlist");
+    assertNoEvents();
   }
 
   @Test