Skip to content

Avoid repo mapping lookup to check builtin restrictions #28173

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
fmeum wants to merge 1 commit into from
Closed

Avoid repo mapping lookup to check builtin restrictions #28173

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/main/java/com/google/devtools/build/lib/analysis/RuleConfiguredTargetBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -372,8 +371,7 @@ private void propagateTransitiveValidationOutputGroups() {
if (rdeLabel != null
&& BuiltinRestriction.isNotAllowed(
rdeLabel,
RepositoryMapping.EMPTY,
BuiltinRestriction.INTERNAL_STARLARK_API_ALLOWLIST)) {
BuiltinRestriction.INTERNAL_STARLARK_API_ALLOWLIST)) {
ruleContext.ruleError(rdeLabel + " cannot access the _transitive_validation private API");
return;
}
Expand Down
4 changes: 1 addition & 3 deletions ...main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1836,9 +1836,7 @@ public Object call(StarlarkThread thread, Tuple args, Dict<String, Object> 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 =
Expand Down
36 changes: 23 additions & 13 deletions src/main/java/com/google/devtools/build/lib/packages/BuiltinRestriction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
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.RepositoryName;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Collection;
import net.starlark.java.eval.EvalException;
Expand Down Expand Up @@ -47,15 +47,13 @@ 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"),
BuiltinRestriction.allowlistEntry(
"", "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"),
Expand Down Expand Up @@ -84,7 +82,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", ""));
Expand Down Expand Up @@ -120,10 +117,25 @@ 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()))
final boolean allows(Label label) {
return reposMatch(apparentRepoName(), label.getRepository())
&& label.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 <allowedName>+ as a prefix.
return givenName.getName().startsWith(allowedName + "+");
}
}

/**
Expand Down Expand Up @@ -165,17 +177,16 @@ public static void failIfCalledOutsideDefaultAllowlist(StarlarkThread thread)
*/
public static void failIfModuleOutsideAllowlist(
BazelModuleContext moduleContext, Collection<AllowlistEntry> 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<AllowlistEntry> allowlist)
public static void failIfLabelOutsideAllowlist(Label label, Collection<AllowlistEntry> allowlist)
throws EvalException {
if (isNotAllowed(label, repoMapping, allowlist)) {
if (isNotAllowed(label, allowlist)) {
throw Starlark.errorf("file '%s' cannot use private API", label.getCanonicalForm());
}
}
Expand All @@ -184,11 +195,10 @@ public static void failIfLabelOutsideAllowlist(
* 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.
*/
public static boolean isNotAllowed(
Label label, RepositoryMapping repoMapping, Collection<AllowlistEntry> allowlist) {
public static boolean isNotAllowed(Label label, Collection<AllowlistEntry> allowlist) {
if (label.getRepository().getName().equals("_builtins")) {
return false;
}
return allowlist.stream().noneMatch(e -> e.allows(label, repoMapping));
return allowlist.stream().noneMatch(e -> e.allows(label));
}
}
Loading