From 62d10511df7cb16713f761e1dbb7bf0a7a47ba29 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 6 Oct 2022 11:33:12 -0700 Subject: [PATCH] Simplify visibility() arg parsing logic, support visibility("//foo") BazelBuildApiGlobals: - An arg of "some_string" is treated as syntactic sugar for ["some_string"]. This supports args of form "//foo". - Deleted special casing for "public" and "private". These are now handled by the sugar and by the new PackageSpecification syntax added in unknown commit. BzlLoadFunctionTest: - Add tests for `visibility([])`, `visibility("//foo")`, and the unsupported case of negated package specs BzlVisibility: - Add interning of special public/private cases; lift construction to factory method. - Rename PackageListBzlVisibility since it's really package specifications, not packages being listed. Make private since users don't need direct access to it. (Kept PUBLIC/PRIVATE singletons as public since it's reasonable for client code to directly refer to those.) Since it's likely we won't support negation, we could even eliminate the BzlVisibility class altogether in favor of lists of PackageSpecifications. This would simplify interning of public/private (but complicate any hypothetical future interning of non-trivial allowlists). But having a separate class is more convenient/readable for the bzl machinery. StarlarkBuildApiGlobals: - Move description of valid visibility() arg values to the arg's doc. - Condense the mention of the distinction between bzl-visibility and target visibility. - Mention behavior relative to --incompatible_fix_package_group_reporoot_syntax. Work toward #11261. PiperOrigin-RevId: 479369691 Change-Id: Ib9b2844abc03b0596399cc3fe09e9b89c6416e90 --- .../starlark/BazelBuildApiGlobals.java | 33 ++++----- .../build/lib/packages/BzlVisibility.java | 41 ++++++++--- .../StarlarkBuildApiGlobals.java | 51 ++++++++------ .../lib/skyframe/BzlLoadFunctionTest.java | 69 ++++++++++++++++++- 4 files changed, 143 insertions(+), 51 deletions(-) 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\")");