From bb7fb2d32f055f2a70a5ab394cf5aef29bc74b2e Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 17 Oct 2023 12:10:04 -0700 Subject: [PATCH] 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 --- .../starlark/StarlarkRuleClassFunctions.java | 62 ++++++++++--------- .../FunctionSplitTransitionAllowlist.java | 8 +-- .../build/lib/packages/RuleClass.java | 4 ++ .../StarlarkRuleTransitionProviderTest.java | 7 ++- .../lib/starlark/StarlarkIntegrationTest.java | 14 +++-- 5 files changed, 51 insertions(+), 44 deletions(-) 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 6c53a63debc39f..be9d98bff4dafd 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; @@ -495,7 +496,6 @@ public static StarlarkRuleFunction createRule( } boolean hasStarlarkDefinedTransition = false; - boolean hasFunctionTransitionAllowlist = false; for (Pair attribute : attributes) { String name = attribute.getFirst(); StarlarkAttrModule.Descriptor descriptor = attribute.getSecond(); @@ -511,29 +511,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())) { @@ -646,15 +623,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); } 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 6a6b0ab83435fa..00a429153441d0 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 2d29dcd5d1311e..88dd1a2500d2a0 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 @@ 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 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 48920e79647d3a..0cd0b772330fec 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 @@ 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 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 c2f72a49d1c3b8..d1f38268e07add 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 @@ 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