diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java index 48347b94d11a74..d4343a531dc48f 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java @@ -42,7 +42,6 @@ import com.google.devtools.build.lib.analysis.test.TestProvider.TestParams; import com.google.devtools.build.lib.analysis.test.TestTagsProvider; import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; @@ -371,8 +370,7 @@ private void propagateTransitiveValidationOutputGroups() { // only allow native and builtins to override transitive validation propagation if (rdeLabel != null && BuiltinRestriction.isNotAllowed( - rdeLabel, - RepositoryMapping.EMPTY, + rdeLabel.getPackageIdentifier(), BuiltinRestriction.INTERNAL_STARLARK_API_ALLOWLIST)) { ruleContext.ruleError(rdeLabel + " cannot access the _transitive_validation private API"); return; 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 4b3f0f49bfb69d..548bdb6aa7c9ee 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 @@ -1836,9 +1836,7 @@ public Object call(StarlarkThread thread, Tuple args, Dict kwarg // allow setting private attributes from initializers in builtins Label definitionLabel = currentRuleClass.getRuleDefinitionEnvironmentLabel(); BuiltinRestriction.failIfLabelOutsideAllowlist( - definitionLabel, - targetDefinitionContext.getMainRepoMapping(), - ALLOWLIST_RULE_EXTENSION_API_EXPERIMENTAL); + definitionLabel, ALLOWLIST_RULE_EXTENSION_API_EXPERIMENTAL); } String nativeName = arg.startsWith("_") ? "$" + arg.substring(1) : arg; Attribute attr = diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuiltinRestriction.java b/src/main/java/com/google/devtools/build/lib/packages/BuiltinRestriction.java index f4778e45bbb744..4e4b822848490b 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BuiltinRestriction.java +++ b/src/main/java/com/google/devtools/build/lib/packages/BuiltinRestriction.java @@ -13,11 +13,14 @@ // limitations under the License. package com.google.devtools.build.lib.packages; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.LoadingCache; import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.cmdline.BazelModuleContext; import com.google.devtools.build.lib.cmdline.Label; -import com.google.devtools.build.lib.cmdline.RepositoryMapping; +import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.Collection; import net.starlark.java.eval.EvalException; @@ -47,7 +50,6 @@ public final class BuiltinRestriction { BuiltinRestriction.allowlistEntry("", "tools/build_defs/android"), BuiltinRestriction.allowlistEntry("", "third_party/bazel_rules/rules_android"), BuiltinRestriction.allowlistEntry("rules_android", ""), - BuiltinRestriction.allowlistEntry("build_bazel_rules_android", ""), // Apple rules BuiltinRestriction.allowlistEntry("", "third_party/apple_crosstool"), @@ -55,7 +57,6 @@ public final class BuiltinRestriction { "", "third_party/cpptoolchains/portable_llvm/build_defs"), BuiltinRestriction.allowlistEntry("", "third_party/bazel_rules/rules_apple"), BuiltinRestriction.allowlistEntry("rules_apple", ""), - BuiltinRestriction.allowlistEntry("build_bazel_rules_apple", ""), // Cc rules BuiltinRestriction.allowlistEntry("", "third_party/bazel_rules/rules_cc"), @@ -84,7 +85,6 @@ public final class BuiltinRestriction { // Proto rules BuiltinRestriction.allowlistEntry("", "third_party/protobuf"), BuiltinRestriction.allowlistEntry("protobuf", ""), - BuiltinRestriction.allowlistEntry("com_google_protobuf", ""), // Shell rules BuiltinRestriction.allowlistEntry("rules_shell", "")); @@ -120,9 +120,24 @@ static AllowlistEntry create(String apparentRepoName, PathFragment packagePrefix return new AutoValue_BuiltinRestriction_AllowlistEntry(apparentRepoName, packagePrefix); } - final boolean allows(Label label, RepositoryMapping repoMapping) { - return label.getRepository().equals(repoMapping.get(apparentRepoName())) - && label.getPackageFragment().startsWith(packagePrefix()); + final boolean allows(PackageIdentifier packageIdentifier) { + return reposMatch(apparentRepoName(), packageIdentifier.getRepository()) + && packageIdentifier.getPackageFragment().startsWith(packagePrefix()); + } + + private static boolean reposMatch(String allowedName, RepositoryName givenName) { + if (allowedName.equals(RepositoryName.MAIN.getName())) { + return givenName.equals(RepositoryName.MAIN); + } + if (allowedName.equals(RepositoryName.BAZEL_TOOLS.getName())) { + return givenName.equals(RepositoryName.BAZEL_TOOLS); + } + if (allowedName.equals(RepositoryName.BUILTINS.getName())) { + return givenName.equals(RepositoryName.BUILTINS); + } + // allowedName is a module name and givenName is a real canonical repo name, so it belongs to + // any version of that module if and only if it contains + as a prefix. + return givenName.getName().startsWith(allowedName + "+"); } } @@ -165,30 +180,41 @@ public static void failIfCalledOutsideDefaultAllowlist(StarlarkThread thread) */ public static void failIfModuleOutsideAllowlist( BazelModuleContext moduleContext, Collection allowlist) throws EvalException { - failIfLabelOutsideAllowlist(moduleContext.label(), moduleContext.repoMapping(), allowlist); + failIfLabelOutsideAllowlist(moduleContext.label(), allowlist); } /** * Throws {@code EvalException} if the given {@link Label} is not within either 1) the builtins * repository, or 2) a package or subpackage of an entry in the given allowlist. */ - public static void failIfLabelOutsideAllowlist( - Label label, RepositoryMapping repoMapping, Collection allowlist) + public static void failIfLabelOutsideAllowlist(Label label, Collection allowlist) throws EvalException { - if (isNotAllowed(label, repoMapping, allowlist)) { + if (isNotAllowed(label.getPackageIdentifier(), allowlist)) { throw Starlark.errorf("file '%s' cannot use private API", label.getCanonicalForm()); } } + private static final LoadingCache< + Collection, LoadingCache> + IS_NOT_ALLOWED_CACHE = + Caffeine.newBuilder() + .initialCapacity(5) + .weakKeys() + .build( + allowlist -> + Caffeine.newBuilder() + .weakKeys() + .build(pkgId -> allowlist.stream().noneMatch(e -> e.allows(pkgId)))); + /** - * Returns true if the given {@link Label} is not within both 1) the builtins repository, or 2) a - * package or subpackage of an entry in the given allowlist. + * Returns true if the given {@link PackageIdentifier} is not within both 1) the builtins + * repository, or 2) a package or subpackage of an entry in the given allowlist. */ public static boolean isNotAllowed( - Label label, RepositoryMapping repoMapping, Collection allowlist) { - if (label.getRepository().getName().equals("_builtins")) { + PackageIdentifier packageIdentifier, Collection allowlist) { + if (packageIdentifier.getRepository().getName().equals("_builtins")) { return false; } - return allowlist.stream().noneMatch(e -> e.allows(label, repoMapping)); + return IS_NOT_ALLOWED_CACHE.get(allowlist).get(packageIdentifier); } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java index b55cefe5419885..3842aefd1dbd0e 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcStarlarkInternal.java @@ -17,7 +17,10 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.devtools.build.lib.rules.cpp.CcModule.nullIfNone; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.LoadingCache; import com.google.common.base.Strings; +import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -63,6 +66,7 @@ import com.google.devtools.build.lib.starlarkbuildapi.NativeComputedDefaultApi; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.Objects; +import java.util.concurrent.CompletionException; import java.util.regex.Pattern; import java.util.regex.PatternSyntaxException; import javax.annotation.Nullable; @@ -89,6 +93,24 @@ public class CcStarlarkInternal implements StarlarkValue { public static final String NAME = "cc_internal"; + // Avoid repeated parsing of allowlists, which previously accounted a non-negligible percentage + // of Bazel analysis CPU time (~1%). Also enables caching by identity in BuiltinRestriction. + private static final LoadingCache> + ALLOWLIST_CACHE = + Caffeine.newBuilder() + .initialCapacity(5) + .weakKeys() + .build( + allowlistObject -> + Sequence.cast(allowlistObject, Tuple.class, "allowlist").stream() + // TODO(bazel-team): Avoid unchecked indexing and casts on values + // obtained from Starlark, even though it is allowlisted. + .map( + p -> + BuiltinRestriction.allowlistEntry( + (String) p.get(0), (String) p.get(1))) + .collect(toImmutableList())); + @StarlarkMethod( name = "check_private_api", documented = false, @@ -122,13 +144,14 @@ public void checkPrivateApi(Object allowlistObject, Object depth, StarlarkThread return; } BazelModuleContext bazelModuleContext = (BazelModuleContext) module.getClientData(); - ImmutableList allowlist = - Sequence.cast(allowlistObject, Tuple.class, "allowlist").stream() - // TODO(bazel-team): Avoid unchecked indexing and casts on values obtained from - // Starlark, even though it is allowlisted. - .map(p -> BuiltinRestriction.allowlistEntry((String) p.get(0), (String) p.get(1))) - .collect(toImmutableList()); - BuiltinRestriction.failIfModuleOutsideAllowlist(bazelModuleContext, allowlist); + try { + BuiltinRestriction.failIfModuleOutsideAllowlist( + bazelModuleContext, ALLOWLIST_CACHE.get(allowlistObject)); + } catch (CompletionException e) { + // Thrown by ALLOWLIST_CACHE. + Throwables.throwIfInstanceOf(e.getCause(), EvalException.class); + throw new IllegalStateException("Unexpected exception type", e); + } } /** Wraps a dictionary of build variables into CcToolchainVariables. */ diff --git a/src/main/starlark/builtins_bzl/common/cc/cc_common_bazel.bzl b/src/main/starlark/builtins_bzl/common/cc/cc_common_bazel.bzl index 1421c5fcd5f78a..d83d48239221c6 100644 --- a/src/main/starlark/builtins_bzl/common/cc/cc_common_bazel.bzl +++ b/src/main/starlark/builtins_bzl/common/cc/cc_common_bazel.bzl @@ -62,11 +62,13 @@ def _implementation_deps_allowed_by_allowlist(*, ctx): _cc_internal.check_private_api(allowlist = _PRIVATE_STARLARKIFICATION_ALLOWLIST) return _cc_common_internal.implementation_deps_allowed_by_allowlist(ctx = ctx) +_INTERNAL_EXPORTS_ALLOWLIST = [ + ("", "third_party/bazel_rules/rules_cc"), + ("rules_cc", ""), +] + def _internal_exports(): - _cc_internal.check_private_api(allowlist = [ - ("", "third_party/bazel_rules/rules_cc"), - ("rules_cc", ""), - ]) + _cc_internal.check_private_api(allowlist = _INTERNAL_EXPORTS_ALLOWLIST) return _cc_internal cc_common = struct( diff --git a/src/main/starlark/builtins_bzl/common/java/java_common.bzl b/src/main/starlark/builtins_bzl/common/java/java_common.bzl index 10b62c45ef5335..9712ae084f35d9 100644 --- a/src/main/starlark/builtins_bzl/common/java/java_common.bzl +++ b/src/main/starlark/builtins_bzl/common/java/java_common.bzl @@ -16,12 +16,14 @@ _java_common_internal = _builtins.internal.java_common_internal_do_not_use +_INTERNAL_EXPORTS_ALLOWLIST = [ + ("", "javatests/com/google/devtools/grok/kythe/analyzers/build/testdata/pkg"), + ("", "third_party/bazel_rules/rules_java"), + ("rules_java", ""), +] + def _internal_exports(): - _builtins.internal.cc_internal.check_private_api(allowlist = [ - ("", "javatests/com/google/devtools/grok/kythe/analyzers/build/testdata/pkg"), - ("", "third_party/bazel_rules/rules_java"), - ("rules_java", ""), - ]) + _builtins.internal.cc_internal.check_private_api(allowlist = _INTERNAL_EXPORTS_ALLOWLIST) return struct( create_compilation_action = _java_common_internal.create_compilation_action, create_header_compilation_action = _java_common_internal.create_header_compilation_action,