Implement an allowlist for dormant dependencies and materializers. Mostly modeled after Starlark function transitions. RELNOTES: None. PiperOrigin-RevId: 675999961 Change-Id: I67936eec693f5e2c2b054342777b20533cbec6b6
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DormantDependency.java b/src/main/java/com/google/devtools/build/lib/analysis/DormantDependency.java index 3dcbaa1..53d6ec7 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/DormantDependency.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/DormantDependency.java
@@ -27,6 +27,12 @@ * by rules in the reverse transitive closure. */ public record DormantDependency(Label label) implements StarlarkValue { + public static final String NAME = "dormant_dependency"; + public static final String ALLOWLIST_ATTRIBUTE_NAME = "$allowlist_dormant_dependency"; + public static final String ALLOWLIST_LABEL_STR = + "//tools/allowlists/dormant_dependency_allowlist"; + public static final Label ALLOWLIST_LABEL = Label.parseCanonicalUnchecked(ALLOWLIST_LABEL_STR); + @Override public void repr(Printer printer) { printer.append("<dormant dependency label='");
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java index a912831..1466da1 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -48,6 +48,7 @@ import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider; import com.google.devtools.build.lib.analysis.config.FeatureSet; import com.google.devtools.build.lib.analysis.config.Fragment; +import com.google.devtools.build.lib.analysis.configuredtargets.PackageGroupConfiguredTarget; import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo; import com.google.devtools.build.lib.analysis.platform.PlatformInfo; import com.google.devtools.build.lib.analysis.platform.ToolchainInfo; @@ -1745,7 +1746,9 @@ } if (attribute.isForDependencyResolution()) { - if (!configuredTarget.isForDependencyResolution()) { + if (!configuredTarget.isForDependencyResolution() + && !(configuredTarget.getConfiguredTarget() + instanceof PackageGroupConfiguredTarget)) { attributeError( attribute.getName(), String.format(
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 512984a..8162d63 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
@@ -41,6 +41,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.DormantDependency; import com.google.devtools.build.lib.analysis.PackageSpecificationProvider; import com.google.devtools.build.lib.analysis.RuleDefinitionEnvironment; import com.google.devtools.build.lib.analysis.TemplateVariableInfo; @@ -122,6 +123,7 @@ import java.util.Map.Entry; import java.util.Objects; import java.util.Optional; +import java.util.function.Function; import java.util.stream.Collectors; import javax.annotation.Nullable; import net.starlark.java.eval.Dict; @@ -702,6 +704,7 @@ boolean hasStarlarkDefinedTransition = false; boolean propagatesAspects = false; + boolean hasMaterializers = false; List<String> dormantAttributes = new ArrayList<>(); for (Pair<String, StarlarkAttrModule.Descriptor> attribute : attributes) { @@ -768,6 +771,11 @@ dormantAttributes.add(name); } + if (attr.isLateBound() + && attr.getLateBoundDefault() instanceof StarlarkMaterializingLateBoundDefault<?>) { + hasMaterializers = true; + } + try { if (builder.contains(attr.getName())) { builder.override(attr); @@ -866,50 +874,27 @@ hasStarlarkDefinedTransition |= visitor.hasStarlarkDefinedTransition; builder.cfg(transitionFactory); - 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().getName().equals("_builtins")) { - if (!hasFunctionTransitionAllowlist) { - // 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); - } - } else { - if (hasFunctionTransitionAllowlist) { - throw Starlark.errorf( - "Unused function-based split transition allowlist: %s %s", - builder.getRuleDefinitionEnvironmentLabel(), builder.getType()); - } - } + checkAndAddAllowlistIfNecessary( + builder, + ruleDefinitionEnvironment, + dependencyResolutionRule || hasMaterializers, + bzlFile, + DORMANT_DEPENDENCY_ALLOWLIST_CHECKER, + "dormant dependency", + StarlarkRuleClassFunctions::createDormantDependencyAllowlistAttribute, + DormantDependency.ALLOWLIST_ATTRIBUTE_NAME, + DormantDependency.ALLOWLIST_LABEL); + + checkAndAddAllowlistIfNecessary( + builder, + ruleDefinitionEnvironment, + hasStarlarkDefinedTransition, + bzlFile, + FUNCTION_TRANSITION_ALLOWLIST_CHECKER, + "function-based split transition", + StarlarkRuleClassFunctions::createStarlarkFunctionTransitionAllowlistAttribute, + FunctionSplitTransitionAllowlist.ATTRIBUTE_NAME, + FunctionSplitTransitionAllowlist.LABEL); for (Object o : providesArg) { if (!StarlarkAttrModule.isProvider(o)) { @@ -960,6 +945,79 @@ builder, thread.getCallerLocation(), thread.getNextIdentityToken()); } + private static Attribute.Builder<Label> createStarlarkFunctionTransitionAllowlistAttribute( + RuleDefinitionEnvironment env) { + return attr(FunctionSplitTransitionAllowlist.ATTRIBUTE_NAME, LABEL) + .cfg(ExecutionTransitionFactory.createFactory()) + .mandatoryBuiltinProviders(ImmutableList.of(PackageSpecificationProvider.class)) + .value(env.getToolsLabel(FunctionSplitTransitionAllowlist.LABEL_STR)); + } + + private static Attribute.Builder<Label> createDormantDependencyAllowlistAttribute( + RuleDefinitionEnvironment env) { + try { + return attr(DormantDependency.ALLOWLIST_ATTRIBUTE_NAME, LABEL) + .cfg(ExecutionTransitionFactory.createFactory()) + .mandatoryBuiltinProviders(ImmutableList.of(PackageSpecificationProvider.class)) + .setPropertyFlag("FOR_DEPENDENCY_RESOLUTION") + .setPropertyFlag("FOR_DEPENDENCY_RESOLUTION_EXPLICITLY_SET") + .value(env.getToolsLabel(DormantDependency.ALLOWLIST_LABEL_STR)); + } catch (EvalException e) { + throw new IllegalStateException(e); + } + } + + private static void checkAndAddAllowlistIfNecessary( + RuleClass.Builder builder, + RuleDefinitionEnvironment ruleDefinitionEnvironment, + boolean usesFunctionality, + Label bzlFileLabel, + AllowlistChecker allowlistChecker, + String description, + Function<RuleDefinitionEnvironment, Attribute.Builder<Label>> attributeFactory, + String attributeName, + Label label) + throws EvalException { + boolean hasAllowlist = false; + // Check for existence of the allowlist attribute. + if (builder.contains(attributeName)) { + Attribute attr = builder.getAttribute(attributeName); + if (!BuildType.isLabelType(attr.getType())) { + throw Starlark.errorf( + "%s attribute must be a label type", Attribute.getStarlarkName(attributeName)); + } + if (attr.getDefaultValueUnchecked() == null) { + throw Starlark.errorf( + "%s attribute must have a default value", Attribute.getStarlarkName(attributeName)); + } + 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(label.getPackageName()) + && defaultLabel.getName().equals(label.getName()))) { + throw Starlark.errorf( + "%s attribute (%s) does not have the expected value %s", + Attribute.getStarlarkName(attributeName), defaultLabel, label); + } + hasAllowlist = true; + } + if (usesFunctionality) { + if (!bzlFileLabel.getRepository().getName().equals("_builtins")) { + if (!hasAllowlist) { + // add the allowlist automatically + builder.add(attributeFactory.apply(ruleDefinitionEnvironment)); + } + builder.addAllowlistChecker(allowlistChecker); + } + } else { + if (hasAllowlist) { + throw Starlark.errorf( + "Unused %s allowlist: %s %s", + description, builder.getRuleDefinitionEnvironmentLabel(), builder.getType()); + } + } + } + private static TransitionFactory<RuleTransitionData> convertConfig(@Nullable Object cfg) throws EvalException { if (cfg.equals(Starlark.NONE)) { @@ -1764,6 +1822,14 @@ .build(); @SerializationConstant + static final AllowlistChecker DORMANT_DEPENDENCY_ALLOWLIST_CHECKER = + AllowlistChecker.builder() + .setAllowlistAttr(DormantDependency.NAME) + .setErrorMessage("Non-allowlisted use of dormant dependencies") + .setLocationCheck(AllowlistChecker.LocationCheck.DEFINITION) + .build(); + + @SerializationConstant static final AllowlistChecker EXTEND_RULE_ALLOWLIST_CHECKER = AllowlistChecker.builder() .setAllowlistAttr("extend_rule")
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/DormantDependencyTest.java b/src/test/java/com/google/devtools/build/lib/analysis/DormantDependencyTest.java index d05e82a..5330662 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/DormantDependencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/DormantDependencyTest.java
@@ -21,6 +21,7 @@ import com.google.devtools.build.lib.cmdline.TargetParsingException; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.Rule; +import com.google.devtools.build.lib.testutil.TestConstants; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -608,4 +609,51 @@ assertThrows(ViewCreationFailedException.class, () -> update("//a:r")); assertContainsEvent("shouldn't have actions"); } + + @Test + public void testAllowlistForDormantAttributes() throws Exception { + scratch.overwriteFile( + TestConstants.TOOLS_REPOSITORY_SCRATCH + + "tools/allowlists/dormant_dependency_allowlist/BUILD", + """ + package_group( + name = 'dormant_dependency_allowlist', + # This rule is in @bazel_tools but must reference a package in the main repository. + # (the value of packages= can't cross repositories at the moment) + includes = ['@@//pkg:pkg']) + """); + + scratch.file( + "pkg/BUILD", + """ + package_group(name='pkg', packages=['//ok/...']) + """); + + String dormantRule = + """ + def _impl(ctx): + return [DefaultInfo()] + r = rule( + implementation = _impl, + dependency_resolution_rule = True, + attrs={"dormant": attr.dormant_label()}) + """; + + String dormantBuildFile = + """ + load(":r.bzl", "r") + filegroup(name="dep") + r(name="r", dormant=":dep") + """; + scratch.file("ok/r.bzl", dormantRule); + scratch.file("ok/BUILD", dormantBuildFile); + scratch.file("bad/r.bzl", dormantRule); + scratch.file("bad/BUILD", dormantBuildFile); + + update("//ok:r"); + + reporter.removeHandler(failFastHandler); + assertThrows(ViewCreationFailedException.class, () -> update("//bad:r")); + assertContainsEvent("Non-allowlisted use of dormant dependencies"); + } }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java index acec4a7..64e9b01 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java
@@ -734,6 +734,15 @@ ) """); + config.create( + "embedded_tools/tools/allowlists/dormant_dependency_allowlist/BUILD", + """ + package_group( + name = "dormant_dependency_allowlist", + packages = ["public"], + ) + """); + MockPlatformSupport.setup(config); ccSupport().setup(config); javaSupport().setupRulesJava(config, runfiles::rlocation);
diff --git a/tools/allowlists/BUILD b/tools/allowlists/BUILD index d0d22bb..2eee22a 100644 --- a/tools/allowlists/BUILD +++ b/tools/allowlists/BUILD
@@ -6,6 +6,7 @@ "BUILD", "//tools/allowlists/android_binary_allowlist:srcs", "//tools/allowlists/config_feature_flag:srcs", + "//tools/allowlists/dormant_dependency_allowlist:srcs", "//tools/allowlists/extend_rule_allowlist:srcs", "//tools/allowlists/function_transition_allowlist:srcs", "//tools/allowlists/initializer_allowlist:srcs",
diff --git a/tools/allowlists/dormant_dependency_allowlist/BUILD b/tools/allowlists/dormant_dependency_allowlist/BUILD new file mode 100644 index 0000000..1951624 --- /dev/null +++ b/tools/allowlists/dormant_dependency_allowlist/BUILD
@@ -0,0 +1,7 @@ +# Package group restricting access to dormant dependencies. + +filegroup( + name = "srcs", + srcs = glob(["**"]), + visibility = ["//tools/allowlists:__pkg__"], +)
diff --git a/tools/allowlists/dormant_dependency_allowlist/BUILD.tools b/tools/allowlists/dormant_dependency_allowlist/BUILD.tools new file mode 100644 index 0000000..f4b3052 --- /dev/null +++ b/tools/allowlists/dormant_dependency_allowlist/BUILD.tools
@@ -0,0 +1,12 @@ +# Package group restricting access to dormant dependencies. + +filegroup( + name = "srcs", + srcs = glob(["**"]), + visibility = ["//tools/allowlists:__pkg__"], +) + +package_group( + name = "dormant_dependency_allowlist", + packages = ["public"], +) \ No newline at end of file