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 41af6c00a1662c..93a7b4695af2e8 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 @@ -17,6 +17,7 @@ import com.google.common.collect.ImmutableList; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.packages.BazelStarlarkContext; import com.google.devtools.build.lib.packages.BzlInitThreadContext; import com.google.devtools.build.lib.packages.BzlVisibility; @@ -57,34 +58,28 @@ public void visibility(Object value, StarlarkThread thread) throws EvalException throw Starlark.errorf(".bzl visibility may not be set more than once"); } - BzlVisibility bzlVisibility = null; - // `visibility("public")` and `visibility("private")` + RepositoryName repo = context.getBzlFile().getRepository(); + ImmutableList specs; if (value instanceof String) { - if (value.equals("public")) { - bzlVisibility = BzlVisibility.PUBLIC; - } else if (value.equals("private")) { - bzlVisibility = BzlVisibility.PRIVATE; - } - // `visibility(["//pkg1", "//pkg2", ...])` + // `visibility("public")`, `visibility("private")`, visibility("//pkg") + specs = + ImmutableList.of(PackageSpecification.fromStringForBzlVisibility(repo, (String) value)); } else if (value instanceof StarlarkList) { + // `visibility(["//pkg1", "//pkg2", ...])` List specStrings = Sequence.cast(value, String.class, "visibility list"); - ImmutableList.Builder specs = + ImmutableList.Builder specsBuilder = ImmutableList.builderWithExpectedSize(specStrings.size()); for (String specString : specStrings) { PackageSpecification spec = - PackageSpecification.fromStringForBzlVisibility( - context.getBzlFile().getRepository(), specString); - specs.add(spec); + PackageSpecification.fromStringForBzlVisibility(repo, specString); + specsBuilder.add(spec); } - bzlVisibility = new BzlVisibility.PackageListBzlVisibility(specs.build()); - } - if (bzlVisibility == null) { + specs = specsBuilder.build(); + } else { throw Starlark.errorf( - "Invalid bzl-visibility: got '%s', want \"public\", \"private\", or list of package" - + " specification strings", - Starlark.type(value)); + "Invalid bzl-visibility: got '%s', want string or list of strings", Starlark.type(value)); } - context.setBzlVisibility(bzlVisibility); + context.setBzlVisibility(BzlVisibility.of(specs)); } private void checkVisibilityAllowlist(PackageIdentifier pkgId, List allowlist) diff --git a/src/main/java/com/google/devtools/build/lib/packages/BzlVisibility.java b/src/main/java/com/google/devtools/build/lib/packages/BzlVisibility.java index a4746a5dafc534..c3d79f9bab8f1b 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BzlVisibility.java +++ b/src/main/java/com/google/devtools/build/lib/packages/BzlVisibility.java @@ -18,7 +18,19 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier; import java.util.List; -/** A visibility level governing the loading of a .bzl module. */ +/** + * A visibility level governing the loading of a .bzl module. + * + *

This is just a container for a list of PackageSpecifications; the scope of visibility is the + * union of all specifications. + */ +// TODO(brandjon): If we ever support negation then this class would become a bit fancier -- it'd +// have to assemble multiple distinct lists, a la PackageSpecificationProvider and +// PackageSpecification.PackageGroupContents. At the moment we don't anticipate that though. +// +// TODO(brandjon): If we have a large allowlist consumed by many .bzl files, we may end up with many +// copies of the allowlist embedded in different instances of this class. Consider more aggressive +// interning. public abstract class BzlVisibility { private BzlVisibility() {} @@ -30,6 +42,23 @@ private BzlVisibility() {} */ public abstract boolean allowsPackage(PackageIdentifier pkg); + /** + * Returns a (possibly interned) {@code BzlVisibility} that allows access as per the given package + * specifications. + */ + public static BzlVisibility of(List specs) { + if (specs.isEmpty() + || (specs.size() == 1 && specs.get(0) instanceof PackageSpecification.NoPackages)) { + return PRIVATE; + } + for (PackageSpecification spec : specs) { + if (spec instanceof PackageSpecification.AllPackages) { + return PUBLIC; + } + } + return new ListBzlVisibility(specs); + } + /** A visibility indicating that everyone may load the .bzl */ public static final BzlVisibility PUBLIC = new BzlVisibility() { @@ -53,16 +82,12 @@ public boolean allowsPackage(PackageIdentifier pkg) { /** * A visibility that decides whether a package's BUILD and .bzl files may load the .bzl by - * checking the package against a set of package specifications. - * - *

Package specifications may have the form {@code //foo/bar} or {@code //foo/bar/...}. Negated - * package specifications are not yet supported. + * checking the package against a set of {@link PackageSpecifications}s. */ - // TODO(b/22193153): Negation. - public static class PackageListBzlVisibility extends BzlVisibility { + private static class ListBzlVisibility extends BzlVisibility { private final ImmutableList specs; - public PackageListBzlVisibility(List specs) { + public ListBzlVisibility(List specs) { this.specs = ImmutableList.copyOf(specs); } 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 2e2a1d38e98112..b45a4ed96effdd 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 @@ -33,36 +33,45 @@ public interface StarlarkBuildApiGlobals { + " --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. Allowed values include:" - + "

    " - + "
  • \"public\" (default): the .bzl can be loaded anywhere." - + "
  • \"private\": the .bzl can only be loaded by files in the same" - + " package (subpackages are excluded)." - + "
  • a list of package specifications (e.g. [\"//pkg1\"," - + "\"//pkg2/subpkg/...\"]): the .bzl can be loaded by files in any package" - + " matching one of the listed specifications. Package specifications may be package" - + " paths, or package paths with a trailing \"/...\" to include all" - + " subpackages; negated patterns are not currently supported. All package" - + " specifications are within the current repository; the \"@\" syntax is disallowed." - + "
" + + "

The bzl-visibility of a module governs whether or not other BUILD and .bzl" + + " files may load it. (This is distinct from the target visibility of the underlying" + + " .bzl source file, which governs whether the file may appear as a dependency of" + + " other targets.) Bzl-visibility works at the level of packages: To load a" + + " module, the file doing the loading must live in a package that has been granted" + + " visibility to the module. A module can always be loaded within its own package," + + " regardless of its visibility." + "

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.", + + " executing.", parameters = { @Param( name = "value", named = false, doc = - "The bzl-visibility level to set. May be \"public\"," - + " \"private\", or a list of packages.") + "A list of package specification strings, or a single package specification string." + + "

Package specifications follow the same format as for" + + " package_group," + + " except that negative package specifications are not permitted. That is, a" + + " specification may have the forms:" + + "

    " + + "
  • \"//foo\": the package //foo" // + + "
  • \"//foo/...\": the package //foo and all of" + + " its subpackages." // + + "
  • \"public\" or \"private\": all packages or no" + + " packages, respectively" + + "
" + + "

The \"@\" syntax is not allowed; all specifications are interpreted" + + " relative to the current module's repository." + + "

If value is a list of strings, the set of packages granted" + + " visibility to this module is the union of the packages represented by each" + + " specification. (An empty list has the same effect as private.)" + + " If value is a single string, it is treated as if it were the" + + " singleton list [value]." + + "

Note that the specification \"//...\" is always interpreted" + + " as \"all packages in the current repository\", regardless of the value of" + + " the --incompatible_fix_package_group_reporoot_syntax flag.") }, // Ordinarily we'd use enableOnlyWithFlag here to gate access on // --experimental_bzl_visibility. However, the StarlarkSemantics isn't available at the point 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 c6c712a92c535c..c5532c8ee65ff1 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 @@ -600,6 +600,27 @@ public void testBzlVisibility_privateDifferentPackage() throws Exception { assertContainsEvent("Starlark file //b:bar.bzl is not visible for loading from package //a."); } + @Test + public void testBzlVisibility_emptyListMeansPrivate() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=everyone"); + + scratch.file("a/BUILD"); + scratch.file( + "a/foo.bzl", // + "load(\"//b:bar.bzl\", \"x\")"); + scratch.file("b/BUILD"); + scratch.file( + "b/bar.bzl", // + "visibility([])", + "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_publicListElement() throws Exception { setBuildLanguageOptions( @@ -753,6 +774,35 @@ public void testBzlVisibility_enumeratedPackages() throws Exception { assertContainsEvent("Starlark file //b:bar.bzl is not visible for loading from package //a2."); } + @Test + public void testBzlVisibility_singleEnumeratedPackageAsString() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=everyone"); + + scratch.file("a1/BUILD"); + scratch.file( + "a1/foo1.bzl", // + "load(\"//b:bar.bzl\", \"x\")"); + scratch.file("a2/BUILD"); + scratch.file( + "a2/foo2.bzl", // + "load(\"//b:bar.bzl\", \"x\")"); + scratch.file("b/BUILD"); + scratch.file( + "b/bar.bzl", // + // Note: "//a1", not ["//a1"] + "visibility(\"//a1\")", + "x = 1"); + + checkSuccessfulLookup("//a1:foo1.bzl"); + assertNoEvents(); + + reporter.removeHandler(failFastHandler); + checkFailingLookup( + "//a2:foo2.bzl", "module //a2:foo2.bzl contains .bzl load-visibility violations"); + assertContainsEvent("Starlark file //b:bar.bzl is not visible for loading from package //a2."); + } + @Test public void testBzlVisibility_enumeratedPackagesMultipleRepos() throws Exception { setBuildLanguageOptions( @@ -918,9 +968,7 @@ public void testBzlVisibility_invalid_badType() throws Exception { reporter.removeHandler(failFastHandler); checkFailingLookup("//a:foo.bzl", "initialization of module 'a/foo.bzl' failed"); - assertContainsEvent( - "Invalid bzl-visibility: got 'int', want \"public\", \"private\", or list of package" - + " specification strings"); + assertContainsEvent("Invalid bzl-visibility: got 'int', want string or list of strings"); } @Test @@ -953,6 +1001,21 @@ public void testBzlVisibility_invalid_packageOutsideRepo() throws Exception { assertContainsEvent("invalid package name '@repo//b': must start with '//'"); } + @Test + public void testBzlVisibility_invalid_negationNotSupported() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=everyone"); + + scratch.file("a/BUILD"); + scratch.file( + "a/foo.bzl", // + "visibility([\"-//a\"])"); + + reporter.removeHandler(failFastHandler); + checkFailingLookup("//a:foo.bzl", "initialization of module 'a/foo.bzl' failed"); + assertContainsEvent("Cannot use negative package patterns here"); + } + @Test public void testLoadFromNonExistentRepository_producesMeaningfulError() throws Exception { scratch.file("BUILD", "load(\"@repository//dir:file.bzl\", \"foo\")");