Skip to content

Commit

Permalink
Add basic bzl visibility functionality
Browse files Browse the repository at this point in the history
This adds a `visibility()` callable to .bzl files, to be used during top-level evaluation (and ideally, immediately after `load()` statements at the top of the file). `visibility("public")` declares the .bzl to be loadable by anyone (the default), while `visibility("private")` ensures it can only be loaded by BUILD/.bzl files in the same package. A follow-up CL will add the ability to allowlist package paths.

This API is not final, and is guarded behind two experimental flags: --experimental_bzl_visibility to guard the feature overall, and --experimental_bzl_visibility_allowlist to only permit certain packages to declare a visibility. (For technical reasons the `visibility()` builtin itself is available unconditionally even though it can only be successfully called when those flag checks are satisfied.)

The allowlist implementation does not handle repository remapping correctly and therefore may not work in bzlmod. Our plan is to prototype the feature within Google and then remove allowlisting altogether, rather than fix the allowlisting to accommodate repository remapping.

Work towards #11261.

PiperOrigin-RevId: 458528142
Change-Id: I3d15d902edf10f6676eac08ebb1e02f6451f5c26
  • Loading branch information
brandjon authored and Copybara-Service committed Jul 1, 2022
1 parent ccebb62 commit 24a4941
Show file tree
Hide file tree
Showing 17 changed files with 640 additions and 45 deletions.
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Expand Up @@ -2210,7 +2210,9 @@ java_library(
srcs = ["starlark/BazelBuildApiGlobals.java"],
deps = [
":starlark/starlark_late_bound_default",
"//src/main/java/com/google/devtools/build/lib/cmdline:cmdline-primitives",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/packages/semantics",
"//src/main/java/com/google/devtools/build/lib/starlarkbuildapi",
"//src/main/java/net/starlark/java/eval",
],
Expand Down
Expand Up @@ -14,8 +14,14 @@

package com.google.devtools.build.lib.analysis.starlark;

import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.packages.BazelStarlarkContext;
import com.google.devtools.build.lib.packages.BzlInitThreadContext;
import com.google.devtools.build.lib.packages.BzlVisibility;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.starlarkbuildapi.StarlarkBuildApiGlobals;
import java.util.List;
import net.starlark.java.eval.EvalException;
import net.starlark.java.eval.Starlark;
import net.starlark.java.eval.StarlarkThread;
Expand All @@ -24,19 +30,75 @@
* Bazel implementation of {@link StarlarkBuildApiGlobals}: a collection of global Starlark build
* API functions that belong in the global namespace.
*/
// TODO(brandjon): This should probably be refactored into a StarlarkLibrary#BZL field, analogous to
// StarlarkLibrary#COMMON and StarlarkLibrary#BUILD.
public class BazelBuildApiGlobals implements StarlarkBuildApiGlobals {

@Override
public void visibility(String value, StarlarkThread thread) throws EvalException {
// Manually check the experimental flag because enableOnlyWithFlag doesn't work for top-level
// builtins.
if (!thread.getSemantics().getBool(BuildLanguageOptions.EXPERIMENTAL_BZL_VISIBILITY)) {
throw Starlark.errorf("Use of `visibility()` requires --experimental_bzl_visibility");
}

BzlInitThreadContext context = BzlInitThreadContext.fromOrFailFunction(thread, "visibility");
if (context.getBzlVisibility() != null) {
throw Starlark.errorf(".bzl visibility may not be set more than once");
}

// Check the currently initializing .bzl file's package against this experimental feature's
// allowlist. BuildLanguageOptions isn't allowed to depend on Label, etc., so this is
// represented as a list of strings. For simplicity we convert the strings to PackageIdentifiers
// here, at linear cost and redundantly for each call to `visibility()`. This is ok because the
// allowlist is temporary, expected to remain small, and calls to visibility() are relatively
// infrequent.
List<String> allowlist =
thread.getSemantics().get(BuildLanguageOptions.EXPERIMENTAL_BZL_VISIBILITY_ALLOWLIST);
PackageIdentifier pkgId = context.getBzlFile().getPackageIdentifier();
boolean foundMatch = false;
for (String allowedPkgString : allowlist) {
// TODO(b/22193153): This seems incorrect since parse doesn't take into account any repository
// map. (This shouldn't matter within Google's monorepo, which doesn't use a repo map.)
try {
PackageIdentifier allowedPkgId = PackageIdentifier.parse(allowedPkgString);
if (pkgId.equals(allowedPkgId)) {
foundMatch = true;
break;
}
} catch (LabelSyntaxException ex) {
throw new EvalException("Invalid bzl visibility allowlist", ex);
}
}
if (!foundMatch) {
throw Starlark.errorf(
"`visibility() is not enabled for package %s; consider adding it to "
+ "--experimental_bzl_visibility_allowlist",
pkgId.getCanonicalForm());
}

BzlVisibility bzlVisibility;
if (value.equals("public")) {
bzlVisibility = BzlVisibility.PUBLIC;
} else if (value.equals("private")) {
bzlVisibility = BzlVisibility.PRIVATE;
} else {
throw Starlark.errorf("Invalid .bzl visibility: '%s'", value);
}
context.setBzlVisibility(bzlVisibility);
}

@Override
public StarlarkLateBoundDefault<?> configurationField(
String fragment, String name, StarlarkThread thread) throws EvalException {
BazelStarlarkContext bazelContext = BazelStarlarkContext.from(thread);
Class<?> fragmentClass = bazelContext.getFragmentNameToClass().get(fragment);
BazelStarlarkContext context = BazelStarlarkContext.from(thread);
Class<?> fragmentClass = context.getFragmentNameToClass().get(fragment);
if (fragmentClass == null) {
throw Starlark.errorf("invalid configuration fragment name '%s'", fragment);
}
try {
return StarlarkLateBoundDefault.forConfigurationField(
fragmentClass, name, bazelContext.getToolsRepository());
fragmentClass, name, context.getToolsRepository());
} catch (StarlarkLateBoundDefault.InvalidConfigurationFieldException exception) {
throw new EvalException(exception);
}
Expand Down
Expand Up @@ -135,6 +135,10 @@ public SkyValue compute(SkyKey skyKey, Environment env)
if (bzlLoadValue == null) {
return null;
}
// TODO(wyv): Consider whether there's a need to check bzl-visibility
// (BzlLoadFunction#checkLoadVisibilities).
// TODO(wyv): Consider refactoring to use PackageFunction#loadBzlModules, or the simpler API
// that may be created by b/237658764.

// Check that the .bzl file actually exports a module extension by our name.
Object exported = bzlLoadValue.getModule().getGlobal(extensionId.getExtensionName());
Expand Down
Expand Up @@ -29,11 +29,14 @@
*/
public final class BzlInitThreadContext extends BazelStarlarkContext {

private final Label bzlFile;

// For storing the result of calling `visibility()`.
@Nullable private BzlVisibility bzlVisibility;

// TODO(b/236456122): Are all these arguments needed for .bzl initialization?
public BzlInitThreadContext(
Label bzlFile,
@Nullable RepositoryName toolsRepository,
@Nullable ImmutableMap<String, Class<?>> fragmentNameToClass,
SymbolGenerator<?> symbolGenerator,
Expand All @@ -45,6 +48,7 @@ public BzlInitThreadContext(
symbolGenerator,
/*analysisRuleLabel=*/ null,
networkAllowlistForTests);
this.bzlFile = bzlFile;
}

/**
Expand Down Expand Up @@ -73,6 +77,17 @@ public static BzlInitThreadContext fromOrFailFunction(StarlarkThread thread, Str
return (BzlInitThreadContext) ctx;
}

/**
* Returns the label of the .bzl module being initialized.
*
* <p>Note that this is not necessarily the same as the module of the innermost stack frame (i.e.,
* {@code BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread)).label()}),
* since the module may call helper functions loaded from elsewhere.
*/
public Label getBzlFile() {
return bzlFile;
}

/**
* Returns the saved BzlVisibility that was declared for the currently initializing .bzl module.
*/
Expand Down
Expand Up @@ -125,6 +125,31 @@ public final class BuildLanguageOptions extends OptionsBase {
+ " key.")
public List<String> experimentalBuiltinsInjectionOverride;

@Option(
name = "experimental_bzl_visibility",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
metadataTags = {OptionMetadataTag.EXPERIMENTAL},
help =
"If enabled, adds a `visibility()` function that .bzl files may call during top-level"
+ " evaluation to set their visibility for the purpose of load() statements.")
public boolean experimentalBzlVisibility;

@Option(
name = "experimental_bzl_visibility_allowlist",
converter = CommaSeparatedOptionListConverter.class,
defaultValue = "",
documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
metadataTags = {OptionMetadataTag.EXPERIMENTAL},
help =
"A comma-separated list of packages (sans \"//\") which, if --experimental_bzl_visibility"
+ " is enabled, are permitted to contain .bzl files that set a bzl-visibility by"
+ " calling the `visibility()` function. (Known issue: This flag may not work"
+ " correctly in the presence of repository remapping, which is used by bzlmod.)")
public List<String> experimentalBzlVisibilityAllowlist;

@Option(
name = "experimental_cc_skylark_api_enabled_packages",
converter = CommaSeparatedOptionListConverter.class,
Expand Down Expand Up @@ -565,6 +590,8 @@ public StarlarkSemantics toStarlarkSemantics() {
.set(EXPERIMENTAL_BUILTINS_BZL_PATH, experimentalBuiltinsBzlPath)
.setBool(EXPERIMENTAL_BUILTINS_DUMMY, experimentalBuiltinsDummy)
.set(EXPERIMENTAL_BUILTINS_INJECTION_OVERRIDE, experimentalBuiltinsInjectionOverride)
.setBool(EXPERIMENTAL_BZL_VISIBILITY, experimentalBzlVisibility)
.set(EXPERIMENTAL_BZL_VISIBILITY_ALLOWLIST, experimentalBzlVisibilityAllowlist)
.setBool(
EXPERIMENTAL_ENABLE_ANDROID_MIGRATION_APIS, experimentalEnableAndroidMigrationApis)
.setBool(
Expand Down Expand Up @@ -633,6 +660,7 @@ public StarlarkSemantics toStarlarkSemantics() {
public static final String EXPERIMENTAL_ALLOW_TAGS_PROPAGATION =
"-experimental_allow_tags_propagation";
public static final String EXPERIMENTAL_BUILTINS_DUMMY = "-experimental_builtins_dummy";
public static final String EXPERIMENTAL_BZL_VISIBILITY = "-experimental_bzl_visibility";
public static final String EXPERIMENTAL_CC_SHARED_LIBRARY = "-experimental_cc_shared_library";
public static final String EXPERIMENTAL_DISABLE_EXTERNAL_PACKAGE =
"-experimental_disable_external_package";
Expand Down Expand Up @@ -693,6 +721,8 @@ public StarlarkSemantics toStarlarkSemantics() {
new StarlarkSemantics.Key<>("experimental_builtins_bzl_path", "%bundled%");
public static final StarlarkSemantics.Key<List<String>> EXPERIMENTAL_BUILTINS_INJECTION_OVERRIDE =
new StarlarkSemantics.Key<>("experimental_builtins_injection_override", ImmutableList.of());
public static final StarlarkSemantics.Key<List<String>> EXPERIMENTAL_BZL_VISIBILITY_ALLOWLIST =
new StarlarkSemantics.Key<>("experimental_bzl_visibility_allowlist", ImmutableList.of());
public static final StarlarkSemantics.Key<Long> MAX_COMPUTATION_STEPS =
new StarlarkSemantics.Key<>("max_computation_steps", 0L);
public static final StarlarkSemantics.Key<Integer> NESTED_SET_DEPTH_LIMIT =
Expand Down
Expand Up @@ -725,13 +725,14 @@ private BzlLoadValue computeInternalWithCompiledBzl(
Environment env,
@Nullable InliningState inliningState)
throws BzlLoadFailedException, InterruptedException {
Label label = key.getLabel();
// Ensure the .bzl exists and passes static checks (parsing, resolving).
// (A missing prelude file still returns a valid but empty BzlCompileValue.)
if (!compileValue.lookupSuccessful()) {
throw new BzlLoadFailedException(compileValue.getError(), Code.COMPILE_ERROR);
}
Program prog = compileValue.getProgram();
Label label = key.getLabel();
PackageIdentifier pkg = label.getPackageIdentifier();

// Determine dependency BzlLoadValue keys for the load statements in this bzl.
// Labels are resolved relative to the current repo mapping.
Expand All @@ -741,7 +742,7 @@ private BzlLoadValue computeInternalWithCompiledBzl(
}
ImmutableList<Pair<String, Location>> programLoads = getLoadsFromProgram(prog);
ImmutableList<Label> loadLabels =
getLoadLabels(env.getListener(), programLoads, label.getPackageIdentifier(), repoMapping);
getLoadLabels(env.getListener(), programLoads, pkg, repoMapping);
if (loadLabels == null) {
throw new BzlLoadFailedException(
String.format(
Expand All @@ -750,22 +751,34 @@ private BzlLoadValue computeInternalWithCompiledBzl(
StarlarkBuiltinsValue.isBuiltinsRepo(label.getRepository()) ? " (internal)" : ""),
Code.PARSE_ERROR);
}
ImmutableList.Builder<BzlLoadValue.Key> loadKeys =
ImmutableList.Builder<BzlLoadValue.Key> loadKeysBuilder =
ImmutableList.builderWithExpectedSize(loadLabels.size());
for (Label loadLabel : loadLabels) {
loadKeys.add(key.getKeyForLoad(loadLabel));
loadKeysBuilder.add(key.getKeyForLoad(loadLabel));
}
ImmutableList<BzlLoadValue.Key> loadKeys = loadKeysBuilder.build();

// Load .bzl modules.
// When not using bzl inlining, this is done in parallel for all loads.
List<BzlLoadValue> loadValues =
inliningState == null
? computeBzlLoadsWithSkyframe(env, loadKeys.build(), programLoads)
: computeBzlLoadsWithInlining(env, loadKeys.build(), programLoads, inliningState);
? computeBzlLoadsWithSkyframe(env, loadKeys, programLoads)
: computeBzlLoadsWithInlining(env, loadKeys, programLoads, inliningState);
if (loadValues == null) {
return null; // Skyframe deps unavailable
}

// Validate that the current .bzl file satisfies each loaded dependency's bzl-visibility.
// Violations are reported as error events (since there can be more than one in a single file)
// and also trigger a BzlLoadFailedException.
checkLoadVisibilities(
pkg,
"module " + label.getCanonicalForm(),
loadValues,
loadKeys,
programLoads,
env.getListener());

// Accumulate a transitive digest of the bzl file, the digests of its direct loads, and the
// digest of the @_builtins pseudo-repository (if applicable).
Fingerprint fp = new Fingerprint();
Expand Down Expand Up @@ -807,6 +820,7 @@ private BzlLoadValue computeInternalWithCompiledBzl(
RuleClassProvider ruleClassProvider = packageFactory.getRuleClassProvider();
BzlInitThreadContext context =
new BzlInitThreadContext(
label,
ruleClassProvider.getToolsRepository(),
ruleClassProvider.getConfigurationFragmentMap(),
new SymbolGenerator<>(label),
Expand Down Expand Up @@ -1067,6 +1081,56 @@ private List<BzlLoadValue> computeBzlLoadsWithInlining(
return valuesMissing ? null : bzlLoads;
}

/**
* Checks that all (directly) requested loads are visible to the requesting file's package.
*
* <p>Each load that is not visible is reported as an error on the event handler. If there is at
* least one error, {@link BzlLoadFailedException} is thrown.
*
* <p>The requesting file may be a .bzl file or another Starlark file (BUILD, WORKSPACE, etc.).
* {@code requestingPackage} is its logical containing package used for visibility validation,
* while {@code requestingFileDescription} is a piece of text for error messages, e.g. "module
* foo.bzl".
*
* <p>{@code loadValues}, {@code loadKeys}, and {@code programLoads} are all ordered corresponding
* to the load statements of the requesting bzl.
*/
// TODO(brandjon): It'd be nice to pass in a single Label argument that unifies requestingPackage
// and requestingFileDescription. But some callers of PackageFunction#loadBzlModules don't have
// such a label handy. Ex: Workspace logic has multiple possible sources of workspace file
// content.
static void checkLoadVisibilities(
PackageIdentifier requestingPackage,
String requestingFileDescription,
List<BzlLoadValue> loadValues,
List<BzlLoadValue.Key> loadKeys,
List<Pair<String, Location>> programLoads,
EventHandler handler)
throws BzlLoadFailedException {
boolean ok = true;
for (int i = 0; i < loadValues.size(); i++) {
BzlVisibility loadVisibility = loadValues.get(i).getBzlVisibility();
Label loadLabel = loadKeys.get(i).getLabel();
PackageIdentifier loadPackage = loadLabel.getPackageIdentifier();
if (loadVisibility.equals(BzlVisibility.PRIVATE) && !requestingPackage.equals(loadPackage)) {
handler.handle(
Event.error(
programLoads.get(i).second,
String.format(
// TODO(brandjon): Consider whether we should try to report error messages (here
// and elsewhere) using the literal text of the load() rather than the (already
// repo-remapped) label.
"Starlark file %s is not visible for loading from package %s. Check the"
+ " file's `visibility()` declaration.",
loadLabel, requestingPackage.getCanonicalForm())));
ok = false;
}
}
if (!ok) {
throw BzlLoadFailedException.visibilityViolation(requestingFileDescription);
}
}

/**
* Obtains the predeclared environment for a .bzl file, based on the type of .bzl and (if
* applicable) the injected builtins.
Expand Down Expand Up @@ -1326,6 +1390,7 @@ static BzlLoadFailedException executionFailed(Label label) {
return new BzlLoadFailedException(
String.format(
"initialization of module '%s'%s failed",
// TODO(brandjon): This error message drops the repo part of the label.
label.toPathFragment(),
StarlarkBuiltinsValue.isBuiltinsRepo(label.getRepository()) ? " (internal)" : ""),
Code.EVAL_ERROR);
Expand Down Expand Up @@ -1390,6 +1455,18 @@ static BzlLoadFailedException builtinsFailed(Label file, BuiltinsFailedException
cause,
cause.getTransience());
}

/**
* Returns an exception for bzl-visibility violations.
*
* <p>{@code fileDescription} is a string like {@code "module //pkg:foo.bzl"} or {@code "file
* //pkg:BUILD"}.
*/
static BzlLoadFailedException visibilityViolation(String fileDescription) {
return new BzlLoadFailedException(
String.format("%s contains .bzl load-visibility violations", fileDescription),
Code.VISIBILITY_ERROR);
}
}

private static final class BzlLoadFunctionException extends SkyFunctionException {
Expand Down
Expand Up @@ -184,8 +184,10 @@ private ImmutableMap<String, Module> loadBzlModules(Environment env, String bzlF

// Load the .bzl module.
try {
// TODO(b/22193153, wyv): Determine whether bzl-visibility should apply at all to this type of
// bzl load. As it stands, this call checks that bzlFile is visible to package @//.
return PackageFunction.loadBzlModules(
env, PackageIdentifier.EMPTY_PACKAGE_ID, programLoads, keys, null);
env, PackageIdentifier.EMPTY_PACKAGE_ID, "Bzlmod system", programLoads, keys, null);
} catch (NoSuchPackageException e) {
throw new BzlmodRepoRuleFunctionException(e, Transience.PERSISTENT);
}
Expand Down

0 comments on commit 24a4941

Please sign in to comment.