diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index a70bc3ec50a0d2..146ce81357b59a 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -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", ], diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/BazelBuildApiGlobals.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/BazelBuildApiGlobals.java index 5e635c8aa53bd8..b5b2131db63bdc 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/BazelBuildApiGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/BazelBuildApiGlobals.java @@ -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; @@ -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 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); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java index fbd7fc0c6ee4ad..8611b8bc08cd01 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleExtensionEvalFunction.java @@ -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()); diff --git a/src/main/java/com/google/devtools/build/lib/packages/BzlInitThreadContext.java b/src/main/java/com/google/devtools/build/lib/packages/BzlInitThreadContext.java index 083565abaaa215..eea5105d5a3805 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BzlInitThreadContext.java +++ b/src/main/java/com/google/devtools/build/lib/packages/BzlInitThreadContext.java @@ -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> fragmentNameToClass, SymbolGenerator symbolGenerator, @@ -45,6 +48,7 @@ public BzlInitThreadContext( symbolGenerator, /*analysisRuleLabel=*/ null, networkAllowlistForTests); + this.bzlFile = bzlFile; } /** @@ -73,6 +77,17 @@ public static BzlInitThreadContext fromOrFailFunction(StarlarkThread thread, Str return (BzlInitThreadContext) ctx; } + /** + * Returns the label of the .bzl module being initialized. + * + *

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. */ diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index 83946fd1ff2d9f..ea8fd51614136a 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java @@ -125,6 +125,31 @@ public final class BuildLanguageOptions extends OptionsBase { + " key.") public List 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 experimentalBzlVisibilityAllowlist; + @Option( name = "experimental_cc_skylark_api_enabled_packages", converter = CommaSeparatedOptionListConverter.class, @@ -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( @@ -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"; @@ -693,6 +721,8 @@ public StarlarkSemantics toStarlarkSemantics() { new StarlarkSemantics.Key<>("experimental_builtins_bzl_path", "%bundled%"); public static final StarlarkSemantics.Key> EXPERIMENTAL_BUILTINS_INJECTION_OVERRIDE = new StarlarkSemantics.Key<>("experimental_builtins_injection_override", ImmutableList.of()); + public static final StarlarkSemantics.Key> EXPERIMENTAL_BZL_VISIBILITY_ALLOWLIST = + new StarlarkSemantics.Key<>("experimental_bzl_visibility_allowlist", ImmutableList.of()); public static final StarlarkSemantics.Key MAX_COMPUTATION_STEPS = new StarlarkSemantics.Key<>("max_computation_steps", 0L); public static final StarlarkSemantics.Key NESTED_SET_DEPTH_LIMIT = diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java index 7d4d9ed66ad0b0..c74e767a813148 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java @@ -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. @@ -741,7 +742,7 @@ private BzlLoadValue computeInternalWithCompiledBzl( } ImmutableList> programLoads = getLoadsFromProgram(prog); ImmutableList

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. + * + *

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". + * + *

{@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 loadValues, + List loadKeys, + List> 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. @@ -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); @@ -1390,6 +1455,18 @@ static BzlLoadFailedException builtinsFailed(Label file, BuiltinsFailedException cause, cause.getTransience()); } + + /** + * Returns an exception for bzl-visibility violations. + * + *

{@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 { diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java index 9df00d3c1de815..be9ca873c066fd 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java @@ -184,8 +184,10 @@ private ImmutableMap 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); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java index 0c2dd2d84e8f6f..dc17178a35c836 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.clock.BlazeClock; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.LabelConstants; +import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.cmdline.RepositoryMapping; import com.google.devtools.build.lib.collect.nestedset.NestedSet; @@ -595,17 +596,47 @@ private static FileValue getBuildFileValue(Environment env, RootedPath buildFile } /** - * Loads the .bzl modules whose names and load-locations are {@code programLoads} and whose - * corresponding Skyframe keys are {@code keys}. Returns a map from module name to module, or null - * for a Skyframe restart. The {@code packageId} is used only for error reporting. + * Loads the .bzl modules whose names and load-locations are {@code programLoads}, and whose + * corresponding Skyframe keys are {@code keys}. + * + *

Validates bzl-visibility of loaded modules. + * + *

Returns a map from module name to module, or null for a Skyframe restart. + * + *

The {@code packageId} is used only for error reporting. * *

This function is called for load statements in BUILD and WORKSPACE files. For loads in .bzl * files, see {@link BzlLoadFunction}. */ + /* + * TODO(b/237658764): This logic has several problems: + * + * - It is partly duplicated by loadPrelude() below. + * - The meaty computeBzlLoads* helpers are almost copies of BzlLoadFunction#computeBzlLoads*. + * - This function is called from WorkspaceFileFunction and BzlmodRepoRuleFunction (and morally + * probably should be called by SingleExtensionEvalFunction rather than requesting a BzlLoadKey + * directly). But the API is awkward for these callers. + * - InliningState is not shared across all callers within a BUILD file; see the comment in + * computeBzlLoadsWithInlining. + * + * To address these issues, we can instead make public the two BzlLoadFunction#computeBzlLoads* + * methods. Their programLoads parameter is only used for wrapping exceptions in + * BzlLoadFailedException#whileLoadingDep, but we can probably push that wrapping to the caller. + * If we fix PackageFunction to use a shared InliningState, then our computeBzlLoadsWithInlining + * method will take it as a param and its signature will then basically match the one in + * BzlLoadFunction. + * + * At that point we can eliminate our own computeBzlLoads* methods in favor of the BzlLoadFunction + * ones. We could factor out the piece of loadBzlModules that dispatches to computeBzlLoads* and + * translates the possible exception, and push the visibility checking and loadedModules map + * construction to the caller, so that loadPrelude can become just a call to the factored-out + * code. + */ @Nullable static ImmutableMap loadBzlModules( Environment env, PackageIdentifier packageId, + String requestingFileDescription, List> programLoads, List keys, @Nullable BzlLoadFunction bzlLoadFunctionForInlining) @@ -616,6 +647,13 @@ static ImmutableMap loadBzlModules( bzlLoadFunctionForInlining == null ? computeBzlLoadsNoInlining(env, keys) : computeBzlLoadsWithInlining(env, keys, bzlLoadFunctionForInlining); + if (bzlLoads == null) { + return null; // Skyframe deps unavailable + } + // Validate that the current BUILD/WORKSPACE file satisfies each loaded dependency's + // bzl-visibility. + BzlLoadFunction.checkLoadVisibilities( + packageId, requestingFileDescription, bzlLoads, keys, programLoads, env.getListener()); } catch (BzlLoadFailedException e) { Throwable rootCause = Throwables.getRootCause(e); throw PackageFunctionException.builder() @@ -626,9 +664,6 @@ static ImmutableMap loadBzlModules( .setPackageLoadingCode(PackageLoading.Code.IMPORT_STARLARK_FILE_ERROR) .buildCause(); } - if (bzlLoads == null) { - return null; // Skyframe deps unavailable - } // Build map of loaded modules. Map loadedModules = Maps.newLinkedHashMapWithExpectedSize(bzlLoads.size()); @@ -655,6 +690,7 @@ private static Module loadPrelude( if (loads == null) { return null; // skyframe restart } + // No need to validate visibility since we're processing an internal load on behalf of Bazel. return loads.get(0).getModule(); } catch (BzlLoadFailedException e) { @@ -1262,10 +1298,24 @@ private LoadedPackage loadPackage( } // Load .bzl modules in parallel. + Label buildFileLabel; + try { + buildFileLabel = + Label.create( + packageId, + packageLookupValue.getBuildFileName().getFilenameFragment().getPathString()); + } catch (LabelSyntaxException e) { + throw new IllegalStateException("Failed to construct label representing BUILD file", e); + } try { loadedModules = loadBzlModules( - env, packageId, programLoads, keys.build(), bzlLoadFunctionForInlining); + env, + packageId, + "file " + buildFileLabel.getCanonicalForm(), + programLoads, + keys.build(), + bzlLoadFunctionForInlining); } catch (NoSuchPackageException e) { throw new PackageFunctionException(e, Transience.PERSISTENT); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java index 37ddb85bcac376..cdadffdfc14232 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java @@ -302,7 +302,14 @@ public SkyValue compute(SkyKey skyKey, Environment env) try { loadedModules = PackageFunction.loadBzlModules( - env, rootPackage, programLoads, keys.build(), bzlLoadFunctionForInlining); + env, + rootPackage, + // In error messages, attribute the blame to "WORKSPACE content" since we're not sure + // at this point what the actual source of the content was. + "WORKSPACE content", + programLoads, + keys.build(), + bzlLoadFunctionForInlining); } catch (NoSuchPackageException e) { throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT); } diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkBuildApiGlobals.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkBuildApiGlobals.java index 83fa078446c0eb..821f0df2915639 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkBuildApiGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkBuildApiGlobals.java @@ -24,6 +24,50 @@ @DocumentMethods public interface StarlarkBuildApiGlobals { + @StarlarkMethod( + name = "visibility", + // TODO(b/22193153): Link to a concepts page for bzl-visibility. + doc = + "(Experimental; enabled by --experimental_bzl_visibility. This feature's" + + " API may change. Only packages that appear in" + + " --experimental_bzl_visibility_allowlist are permitted to call this" + + " function. Known issue: This feature currently may not work under bzlmod.)" + + "

Sets the bzl-visibility of the .bzl module currently being initialized." + + "

The bzl-visibility of a .bzl module (not to be confused with target visibility)" + + " governs whether or not a load() of that .bzl is permitted from" + + " within the BUILD and .bzl files of a particular package. There are currently two" + + " allowed values:" + + "

" + + "

Generally, visibility() is called at the top of the .bzl file," + + " immediately after its load() statements. (It is poor style to put" + + " this declaration later in the file or in a helper method.) It may not be called" + + " more than once per .bzl, or after the .bzl's top-level code has finished" + + " executing." + + "

Note that a .bzl module having a public bzl-visibility does not necessarily" + + " imply that its corresponding file target has public visibility. This means that" + + " it's possible to be able to load() a .bzl file without being able to" + + " depend on it in a filegroup or other target.", + parameters = { + @Param( + name = "value", + named = false, + doc = + "The bzl-visibility level to set. May be \"public\" or" + + " \"private\".") + }, + // Ordinarily we'd use enableOnlyWithFlag here to gate access on + // --experimental_bzl_visibility. However, the StarlarkSemantics isn't available at the point + // where the top-level environment is determined (see StarlarkModules#addPredeclared and + // notice that it relies on the overload of Starlark#addMethods that uses the default + // semantics). So instead we make this builtin unconditionally defined, but have it fail at + // call time if used without the flag. + useStarlarkThread = true) + void visibility(String value, StarlarkThread thread) throws EvalException; + @StarlarkMethod( name = "configuration_field", // TODO(cparsons): Provide a link to documentation for available StarlarkConfigurationFields. diff --git a/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java b/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java index 43ef35c11a084f..7c6c191e7b648f 100644 --- a/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java +++ b/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java @@ -183,8 +183,8 @@ public String getBaseName() { } /** - * Returns a {@link PathFragment} instance representing the relative path between this {@link - * PathFragment} and the given {@link PathFragment}. + * Returns a {@link PathFragment} instance formed by resolving {@code other} relative to this + * path. For example, if this path is "a" and other is "b", returns "a/b". * *

If the passed path is absolute it is returned untouched. This can be useful to resolve * symlinks. @@ -197,8 +197,8 @@ public PathFragment getRelative(PathFragment other) { } /** - * Returns a {@link PathFragment} instance representing the relative path between this {@link - * PathFragment} and the given path. + * Returns a {@link PathFragment} instance formed by resolving {@code other} relative to this + * path. For example, if this path is "a" and other is "b", returns "a/b". * *

See {@link #getRelative(PathFragment)} for details. */ diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeBuildApiGlobals.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeBuildApiGlobals.java index 2b3d4829ff35da..4ddc958a4b543f 100644 --- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeBuildApiGlobals.java +++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeBuildApiGlobals.java @@ -22,6 +22,9 @@ /** Fake implementation of {@link StarlarkBuildApiGlobals}. */ public class FakeBuildApiGlobals implements StarlarkBuildApiGlobals { + @Override + public void visibility(String value, StarlarkThread thread) throws EvalException {} + @Override public LateBoundDefaultApi configurationField(String fragment, String name, StarlarkThread thread) throws EvalException { diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto index df2d5534f64725..d8577eb8774e4c 100644 --- a/src/main/protobuf/failure_details.proto +++ b/src/main/protobuf/failure_details.proto @@ -1250,6 +1250,7 @@ message StarlarkLoading { IO_ERROR = 7 [(metadata) = { exit_code: 1 }]; LABEL_CROSSES_PACKAGE_BOUNDARY = 8 [(metadata) = { exit_code: 1 }]; BUILTINS_ERROR = 9 [(metadata) = { exit_code: 1 }]; + VISIBILITY_ERROR = 10 [(metadata) = { exit_code: 1 }]; } Code code = 1; diff --git a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java index bff2d6f3e6ce84..db884b20673f5d 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import com.google.common.collect.ImmutableClassToInstanceMap; +import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.skyframe.serialization.DeserializationContext; import com.google.devtools.build.lib.skyframe.serialization.DynamicCodec; import com.google.devtools.build.lib.skyframe.serialization.SerializationContext; @@ -123,6 +124,8 @@ private static BuildLanguageOptions buildRandomOptions(Random rand) throws Excep "--experimental_sibling_repository_layout=" + rand.nextBoolean(), "--experimental_builtins_bzl_path=" + rand.nextDouble(), "--experimental_builtins_dummy=" + rand.nextBoolean(), + "--experimental_bzl_visibility=" + rand.nextBoolean(), + "--experimental_bzl_visibility_allowlist=" + rand.nextDouble(), "--experimental_enable_android_migration_apis=" + rand.nextBoolean(), "--experimental_google_legacy_api=" + rand.nextBoolean(), "--experimental_platforms_api=" + rand.nextBoolean(), @@ -163,6 +166,10 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { .setBool(BuildLanguageOptions.EXPERIMENTAL_SIBLING_REPOSITORY_LAYOUT, rand.nextBoolean()) .set(BuildLanguageOptions.EXPERIMENTAL_BUILTINS_BZL_PATH, String.valueOf(rand.nextDouble())) .setBool(BuildLanguageOptions.EXPERIMENTAL_BUILTINS_DUMMY, rand.nextBoolean()) + .setBool(BuildLanguageOptions.EXPERIMENTAL_BZL_VISIBILITY, rand.nextBoolean()) + .set( + BuildLanguageOptions.EXPERIMENTAL_BZL_VISIBILITY_ALLOWLIST, + ImmutableList.of(String.valueOf(rand.nextDouble()))) .setBool( BuildLanguageOptions.EXPERIMENTAL_ENABLE_ANDROID_MIGRATION_APIS, rand.nextBoolean()) .setBool(BuildLanguageOptions.EXPERIMENTAL_GOOGLE_LEGACY_API, rand.nextBoolean()) diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java index 2acdaa26e0352f..ede7b150ea8f14 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java @@ -218,15 +218,34 @@ private static SkyKey key(String label) { return BzlLoadValue.keyForBuild(Label.parseAbsoluteUnchecked(label)); } - // Ensures that a Starlark file has been successfully processed by checking that the - // the label in its dependency set corresponds to the requested label. + /** Loads a .bzl with the given label and asserts success. */ private void checkSuccessfulLookup(String label) throws Exception { SkyKey skyKey = key(label); EvaluationResult result = get(skyKey); + // Ensure that the file has been processed by checking its Module for the label field. assertThat(label) .isEqualTo(BazelModuleContext.of(result.get(skyKey).getModule()).label().toString()); } + /* Loads a .bzl with the given label and asserts BzlLoadFailedException with the given message. */ + private void checkFailingLookup(String label, String expectedMessage) + throws InterruptedException { + SkyKey skyKey = key(label); + EvaluationResult result = + SkyframeExecutorTestUtils.evaluate( + getSkyframeExecutor(), skyKey, /*keepGoing=*/ false, reporter); + assertThat(result.hasError()).isTrue(); + assertThatEvaluationResult(result) + .hasErrorEntryForKeyThat(skyKey) + .hasExceptionThat() + .isInstanceOf(BzlLoadFailedException.class); + assertThatEvaluationResult(result) + .hasErrorEntryForKeyThat(skyKey) + .hasExceptionThat() + .hasMessageThat() + .contains(expectedMessage); + } + @Test public void testBzlLoadNoBuildFile() throws Exception { scratch.file("pkg/ext.bzl", ""); @@ -320,7 +339,7 @@ public void testLoadMustRespectPackageBoundary_ofSubpkg() throws Exception { scratch.file("a/a.bzl", "load('//a:b/b.bzl', 'b')"); scratch.file("a/b/BUILD", ""); scratch.file("a/b/b.bzl", "b = 42"); - checkStrayLabel( + checkFailingLookup( "//a:a.bzl", "Label '//a:b/b.bzl' is invalid because 'a/b' is a subpackage; perhaps you meant to" + " put the colon here: '//a/b:b.bzl'?"); @@ -332,7 +351,7 @@ public void testLoadMustRespectPackageBoundary_ofSubpkg_relative() throws Except scratch.file("a/a.bzl", "load('b/b.bzl', 'b')"); scratch.file("a/b/BUILD", ""); scratch.file("a/b/b.bzl", "b = 42"); - checkStrayLabel( + checkFailingLookup( "//a:a.bzl", "Label '//a:b/b.bzl' is invalid because 'a/b' is a subpackage; perhaps you meant to" + " put the colon here: '//a/b:b.bzl'?"); @@ -345,7 +364,7 @@ public void testLoadMustRespectPackageBoundary_ofIndirectSubpkg() throws Excepti scratch.file("a/b/BUILD", ""); scratch.file("a/b/c/BUILD", ""); scratch.file("a/b/c/c.bzl", "c = 42"); - checkStrayLabel( + checkFailingLookup( "//a:a.bzl", "Label '//a/b:c/c.bzl' is invalid because 'a/b/c' is a subpackage; perhaps you meant" + " to put the colon here: '//a/b/c:c.bzl'?"); @@ -357,29 +376,207 @@ public void testLoadMustRespectPackageBoundary_ofParentPkg() throws Exception { scratch.file("a/b/b.bzl", "load('//a/c:c/c.bzl', 'c')"); scratch.file("a/BUILD"); scratch.file("a/c/c/c.bzl", "c = 42"); - checkStrayLabel( + checkFailingLookup( "//a/b:b.bzl", "Label '//a/c:c/c.bzl' is invalid because 'a/c' is not a package; perhaps you meant to " + "put the colon here: '//a:c/c/c.bzl'?"); } - // checkStrayLabel checks that execution of target fails because - // the label of its load statement strays into a subpackage. - private void checkStrayLabel(String target, String expectedMessage) throws InterruptedException { - SkyKey skyKey = key(target); - EvaluationResult result = - SkyframeExecutorTestUtils.evaluate( - getSkyframeExecutor(), skyKey, /*keepGoing=*/ false, reporter); - assertThat(result.hasError()).isTrue(); - assertThatEvaluationResult(result) - .hasErrorEntryForKeyThat(skyKey) - .hasExceptionThat() - .isInstanceOf(BzlLoadFailedException.class); - assertThatEvaluationResult(result) - .hasErrorEntryForKeyThat(skyKey) - .hasExceptionThat() - .hasMessageThat() - .contains(expectedMessage); + @Test + public void testBzlVisibility_disabledWithoutFlag() throws Exception { + setBuildLanguageOptions("--experimental_bzl_visibility=false"); + + scratch.file("a/BUILD"); + scratch.file( + "a/foo.bzl", // + "load(\"//b:bar.bzl\", \"x\")"); + scratch.file("b/BUILD"); + scratch.file( + "b/bar.bzl", // + "visibility(\"private\")", + "x = 1"); + + reporter.removeHandler(failFastHandler); + checkFailingLookup("//a:foo.bzl", "initialization of module 'b/bar.bzl' failed"); + assertContainsEvent("Use of `visibility()` requires --experimental_bzl_visibility"); + } + + @Test + public void testBzlVisibility_disabledWithoutAllowlist() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", + // Put a in allowlist, but not b, to show that it's b we need and not a. + "--experimental_bzl_visibility_allowlist=a"); + + scratch.file("a/BUILD"); + scratch.file( + "a/foo.bzl", // + "load(\"//b:bar.bzl\", \"x\")"); + scratch.file("b/BUILD"); + scratch.file( + "b/bar.bzl", // + "visibility(\"private\")", + "x = 1"); + + reporter.removeHandler(failFastHandler); + checkFailingLookup("//a:foo.bzl", "initialization of module 'b/bar.bzl' failed"); + assertContainsEvent("`visibility() is not enabled for package //b"); + } + + @Test + public void testBzlVisibility_publicExplicit() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=b"); + + scratch.file("a/BUILD"); + scratch.file( + "a/foo.bzl", // + "load(\"//b:bar.bzl\", \"x\")"); + scratch.file("b/BUILD"); + scratch.file( + "b/bar.bzl", // + "visibility(\"public\")", + "x = 1"); + + checkSuccessfulLookup("//a:foo.bzl"); + assertNoEvents(); + } + + @Test + public void testBzlVisibility_publicImplicit() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=b"); + + scratch.file("a/BUILD"); + scratch.file( + "a/foo.bzl", // + "load(\"//b:bar.bzl\", \"x\")"); + scratch.file("b/BUILD"); + scratch.file( + "b/bar.bzl", + // No visibility() declaration, defaults to public. + "x = 1"); + + checkSuccessfulLookup("//a:foo.bzl"); + assertNoEvents(); + } + + @Test + public void testBzlVisibility_privateSamePackage() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=a"); + + scratch.file("a/BUILD"); + scratch.file( + "a/foo.bzl", // + "load(\"//a:bar.bzl\", \"x\")"); + scratch.file( + "a/bar.bzl", // + "visibility(\"public\")", + "x = 1"); + + checkSuccessfulLookup("//a:foo.bzl"); + assertNoEvents(); + } + + @Test + public void testBzlVisibility_privateDifferentPackage() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=b"); + + scratch.file("a/BUILD"); + scratch.file( + "a/foo.bzl", // + "load(\"//b:bar.bzl\", \"x\")"); + scratch.file("b/BUILD"); + scratch.file( + "b/bar.bzl", // + "visibility(\"private\")", + "x = 1"); + + reporter.removeHandler(failFastHandler); + checkFailingLookup( + "//a:foo.bzl", "module //a:foo.bzl contains .bzl load-visibility violations"); + assertContainsEvent("Starlark file //b:bar.bzl is not visible for loading from package //a."); + } + + @Test + public void testBzlVisibility_failureInDependency() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", + // While we're here, test that we can write either "pkg" or "//pkg" in the allowlist. + "--experimental_bzl_visibility_allowlist=b,//c"); + + scratch.file("a/BUILD"); + scratch.file( + "a/foo.bzl", // + "load(\"//b:bar.bzl\", \"x\")"); + scratch.file("b/BUILD"); + scratch.file( + "b/bar.bzl", // + "load(\"//c:baz.bzl\", \"y\")", + "visibility(\"public\")", + "x = y"); + scratch.file("c/BUILD"); + scratch.file( + "c/baz.bzl", // + "visibility(\"private\")", + "y = 1"); + + reporter.removeHandler(failFastHandler); + checkFailingLookup( + "//a:foo.bzl", + "at /workspace/a/foo.bzl:1:6: module //b:bar.bzl contains .bzl load-visibility violations"); + assertContainsEvent("Starlark file //c:baz.bzl is not visible for loading from package //b."); + } + + @Test + public void testBzlVisibility_setNonlocally() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=b"); + + // Checks a case where visibility() is called in a different package than the module that is + // actually being initialized. (This is bad style in practice, but it's semantically simpler to + // allow it than to go out of our way to ban it.) + scratch.file("a/BUILD"); + scratch.file( + "a/foo.bzl", // + "load(\"//b:bar.bzl\", \"x\")"); + scratch.file("b/BUILD"); + scratch.file( + "b/bar.bzl", // + "load(\"//c:helper.bzl\", \"helper\")", + "helper()", + "x = 1"); + scratch.file("c/BUILD"); + scratch.file( + "c/helper.bzl", // + // Should have a visibility("public") call here, but let's omit it and rely on the default + // being public. That way we can also test that c need not be in the allowlist just to call + // visibility() on behalf of b. + "def helper():", + " visibility(\"private\")"); + + reporter.removeHandler(failFastHandler); + checkFailingLookup( + "//a:foo.bzl", "module //a:foo.bzl contains .bzl load-visibility violations"); + assertContainsEvent("Starlark file //b:bar.bzl is not visible for loading from package //a."); + } + + @Test + public void testBzlVisibility_cannotBeSetTwice() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=a"); + + scratch.file("a/BUILD"); + scratch.file( + "a/foo.bzl", // + "visibility(\"public\")", + "visibility(\"public\")"); + + reporter.removeHandler(failFastHandler); + checkFailingLookup("//a:foo.bzl", "initialization of module 'a/foo.bzl' failed"); + assertContainsEvent(".bzl visibility may not be set more than once"); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java index 666476efa4384a..1cbb1d223c9ffa 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java @@ -769,6 +769,76 @@ public void testLoadWithoutBzlSuffix() throws Exception { assertContainsEvent("The label must reference a file with extension '.bzl'"); } + @Test + public void testBzlVisibilityViolation() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=b"); + + scratch.file( + "a/BUILD", // + "load(\"//b:foo.bzl\", \"x\")"); + scratch.file("b/BUILD"); + scratch.file( + "b/foo.bzl", // + "visibility(\"private\")", + "x = 1"); + + reporter.removeHandler(failFastHandler); + Exception ex = evaluatePackageToException("a"); + assertThat(ex) + .hasMessageThat() + .contains( + "error loading package 'a': file //a:BUILD contains .bzl load-visibility violations"); + assertDetailedExitCode( + ex, PackageLoading.Code.IMPORT_STARLARK_FILE_ERROR, ExitCode.BUILD_FAILURE); + assertContainsEvent("Starlark file //b:foo.bzl is not visible for loading from package //a."); + } + + @Test + public void testVisibilityCallableNotAvailableInBUILD() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=a"); + + scratch.file( + "a/BUILD", // + "visibility(\"public\")"); + + reporter.removeHandler(failFastHandler); + // The evaluation result ends up being null, probably due to the test framework swallowing + // exceptions (similar to b/26382502). So let's just look for the error event instead of + // asserting on the exception. + SkyframeExecutorTestUtils.evaluate( + getSkyframeExecutor(), + PackageValue.key(PackageIdentifier.createInMainRepo("a")), + /*keepGoing=*/ false, + reporter); + assertContainsEvent("name 'visibility' is not defined"); + } + + @Test + public void testVisibilityCallableErroneouslyInvokedInBUILD() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=a"); + + scratch.file( + "a/BUILD", // + "load(\":helper.bzl\", \"helper\")", + "helper()"); + scratch.file( + "a/helper.bzl", // + "def helper():", + " visibility(\"public\")"); + + reporter.removeHandler(failFastHandler); + SkyframeExecutorTestUtils.evaluate( + getSkyframeExecutor(), + PackageValue.key(PackageIdentifier.createInMainRepo("a")), + /*keepGoing=*/ false, + reporter); + assertContainsEvent( + "'visibility' can only be called during .bzl initialization (top-level evaluation)"); + } + @Test public void testBadWorkspaceFile() throws Exception { Path workspacePath = scratch.overwriteFile("WORKSPACE", "junk"); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java index f3d1d3f7b42d30..647f9bb9162f14 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java @@ -154,6 +154,30 @@ public void testRepositoryMappingInChunks() throws Exception { .containsEntry(b, ImmutableMap.of("x", y, "good", main)); } + @Test + public void testBzlVisibility() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=pkg"); + + createWorkspaceFile( + "workspace(name = 'foo')", // + "load('//pkg:a.bzl', 'a')"); + scratch.file("pkg/BUILD"); + scratch.file( + "pkg/a.bzl", // + "visibility('private')"); + + reporter.removeHandler(failFastHandler); + SkyKey key = ExternalPackageFunction.key(); + eval(key); + // The evaluation result ends up being null, probably due to the test framework swallowing + // exceptions (similar to b/26382502). So let's just look for the error event instead of + // asserting on the exception. + assertContainsEvent( + "Starlark file //pkg:a.bzl is not visible for loading from package //. Check the file's" + + " `visibility()` declaration."); + } + @Test public void testInvalidRepo() throws Exception { // Test intentionally introduces errors.