Automatically add function transition allow list when needed
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: https://github.com/bazelbuild/bazel/issues/19493 (because whilelist is ignored and default allowlist is added)
PiperOrigin-RevId: 574226941
Change-Id: If90f78a610d7bd3c107dcd94a39902c7c939aa7b
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
index efb3946..81b43f9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
+++ b/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 @@
}
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 @@
}
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 @@
}
}
- // 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);
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/FunctionSplitTransitionAllowlist.java b/src/main/java/com/google/devtools/build/lib/packages/FunctionSplitTransitionAllowlist.java
index 6a6b0ab..00a4291 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/FunctionSplitTransitionAllowlist.java
+++ b/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() {}
}
diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 2d29dcd..88dd1a2 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -1303,6 +1303,10 @@
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());
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
index 48920e7..0cd0b77 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/StarlarkRuleTransitionProviderTest.java
@@ -985,9 +985,10 @@
}
@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 @@
reporter.removeHandler(failFastHandler);
getConfiguredTarget("//test");
- assertContainsEvent("Use of Starlark transition without allowlist");
+ assertContainsEvent("Non-allowlisted use of Starlark transition");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
index c2f72a4..d1f3826 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
+++ b/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 @@
}
@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 @@
"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
