package_group: Add "public"/"private" constants, fix "//..." meaning This CL does three things: 1. It adds the special package specifications "public" and "private, guarded behind the flag `--incompatible_package_group_has_public_syntax`. 2. It fixes the behavior of package specification "//..." to mean "all packages in the current repo" rather than "all packages in any repo", guarded behind the flag `--incompatible_fix_package_group_reporoot_syntax`. 3. It fixes the behavior of `bazel query --output=proto` (and `--output=xml`) so that the `packages` attribute is serialized without dropping the leading `//` from package names -- it is now `"//foo/bar/..."` instead of `"foo/bar/..."`. This behavioral change is guarded behind `--incompatible_package_group_includes_double_slash`. The .bzl `visibility()` builtin always acts as if the first two flags are enabled. Work toward #11261. Work toward #16323. Work toward #16355. Work toward #16391. RELNOTES: Added three `package_group`-related flags: `--incompatible_package_group_includes_double_slash` (#16391), `--incompatible_package_group_has_public_syntax` (#16355), and `--incompatible_fix_package_group_reporoot_syntax` (#16323). With these flags, `package_group` can now easily specify "all packages", "no packages", and "all packages in the current repo". In PackageSpecification: - fromString(), fromStringPositive(): rewrote docstring, added bool args for flags, renamed or removed vars for simplicity, added handling of public/private and flag behavior - AllPackagesBeneath(): fixed asStringWithoutRepository() for case of //..., which wasn't likely to arise before. - normalize order of equals(), hashCode(), etc. - AllPackages: change hash, and change str representation to "public" - Add NoPackages, symmetric to AllPackages - NegativePackageSpecification: don't reuse the delegate's hash as our own hash verbatim In PackageGroupTest: - Move `testEverythingSpecificationWorks` to above `testNegativeEverything`, and made it use "public" syntax in place of "//...". - Add test for "private" syntax. - Add test for flag guarding "public"/"private". - Add tests for old and new "//..." behavior. - Add tests that you can't negate "public" or "private". - Add test for illegal combination of flags. - Add assertions for stringification of "public"/"private". Other changes: - Update XmlOutputFormatterTest and ProtoOutputFormatterTest with test cases for the flag. In the latter case, parameterize a helper to allow setting per-test-case flags. - Add test cases to BzlLoadFunctionTest for bzl visibility accepting the list forms `["public"]`/`["private"]`, and for its behavior being unaffected by the new flags. PiperOrigin-RevId: 479347358 Change-Id: I124ca5406bff15615adaa1256e2d7bef69b9d9a5
diff --git a/site/en/concepts/visibility.md b/site/en/concepts/visibility.md index 89efcfb..596e838 100644 --- a/site/en/concepts/visibility.md +++ b/site/en/concepts/visibility.md
@@ -45,7 +45,8 @@ * Package groups use a different syntax for specifying packages. Within a package group, the forms `"//foo/bar:__pkg__"` and `"//foo/bar:__subpackages__"` are respectively replaced by `"//foo/bar"` - and `"//foo/bar/..."`. + and `"//foo/bar/..."`. Likewise, `"//visibility:public"` and + `"//visibility:private"` are just `"public"` and `"private"`. For example, if `//some/package:mytarget` has its `visibility` set to `[":__subpackages__", "//tests:__pkg__"]`, then it could be used by any target
diff --git a/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm b/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm index 164ece7..c8c64fd 100644 --- a/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm +++ b/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm
@@ -178,13 +178,18 @@ <li>As above, but with a trailing <code>/...</code>. For example, <code> //foo/...</code> specifies the set of <code>//foo</code> and all its - subpackages. As a special case, <code>//...</code> specifies all - packages in any repository (in other words, public). + subpackages. <code>//...</code> specifies all packages in the current + repository. + + <li>The strings <code>public</code> or <code>private</code>, which + respectively specify every package or no package. (This form requires + the flag <code>--incompatible_package_group_has_public_syntax</code> to + be set.) </ol> - <p>In addition, a package specification may also be prefixed with - <code>-</code> to indicate that it is negated.</p> + <p>In addition, the first two kinds of package specifications may also + be prefixed with <code>-</code> to indicate that they are negated.</p> <p>The package group contains any package that matches at least one of its positive specifications and none of its negative specifications @@ -193,11 +198,24 @@ subpackages of <code>//foo/tests</code>. (<code>//foo</code> itself is included while </code>//foo/tests</code> itself is not.)</p> - <p>Aside from <code>//...</code>, there is no way to directly specify + <p>Aside from public visibility, there is no way to directly specify packages outside the current repository.</p> <p>If this attribute is missing, it is the same as setting it to an - empty list (in other words, private). + empty list, which is also the same as setting it to a list containing + only <code>private</code>. + + <p><i>Note:</i> The specification <code>//...</code> has a legacy behavior + of being the same as <code>public</code>. This behavior is disabled when + <code>--incompatible_fix_package_group_reporoot_syntax</code> is + enabled.</p> + + <p><i>Note:</i> As a legacy behavior, when this attribute is serialized as + part of <code>bazel query --output=proto</code> (or + <code>--output=xml</code>), the leading slashes are omitted if + <code>--incompatible_package_group_includes_double_slash</code> is not + enabled. For instance, <code>//pkg/foo/...</code> will output as + <code>\"pkg/foo/...\"</code>.</p> </td> </tr> <tr>
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java index f9dec2a..a73a3d3 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Package.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -1652,14 +1652,26 @@ return Label.create(pkg.getPackageIdentifier(), targetName); } - /** - * Adds a package group to the package. - */ - void addPackageGroup(String name, Collection<String> packages, Collection<Label> includes, - EventHandler eventHandler, Location location) + /** Adds a package group to the package. */ + void addPackageGroup( + String name, + Collection<String> packages, + Collection<Label> includes, + boolean allowPublicPrivate, + boolean repoRootMeansCurrentRepo, + EventHandler eventHandler, + Location location) throws NameConflictException, LabelSyntaxException { PackageGroup group = - new PackageGroup(createLabel(name), pkg, packages, includes, eventHandler, location); + new PackageGroup( + createLabel(name), + pkg, + packages, + includes, + allowPublicPrivate, + repoRootMeansCurrentRepo, + eventHandler, + location); Target existing = targets.get(group.getName()); if (existing != null) { throw nameConflict(group, existing);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java b/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java index ebad8d7..f9f9a79 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java
@@ -47,6 +47,8 @@ Package pkg, Collection<String> packageSpecifications, Collection<Label> includes, + boolean allowPublicPrivate, + boolean repoRootMeansCurrentRepo, EventHandler eventHandler, Location location) { this.label = label; @@ -61,7 +63,11 @@ PackageSpecification specification = null; try { specification = - PackageSpecification.fromString(label.getRepository(), packageSpecification); + PackageSpecification.fromString( + label.getRepository(), + packageSpecification, + allowPublicPrivate, + repoRootMeansCurrentRepo); } catch (PackageSpecification.InvalidPackageSpecificationException e) { errorsFound = true; eventHandler.handle(
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java b/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java index 8c5433d..51696ae 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageSpecification.java
@@ -47,11 +47,15 @@ * packages" specification. */ public abstract class PackageSpecification { - private static final String PACKAGE_LABEL = "__pkg__"; - private static final String SUBTREE_LABEL = "__subpackages__"; + private static final String PUBLIC_VISIBILITY = "public"; + private static final String PRIVATE_VISIBILITY = "private"; private static final String ALL_BENEATH_SUFFIX = "/..."; private static final String NEGATIVE_PREFIX = "-"; + // Used for interpreting `visibility` labels. + private static final String PACKAGE_LABEL = "__pkg__"; + private static final String SUBTREE_LABEL = "__subpackages__"; + /** Returns {@code true} if the package spec includes the provided {@code packageName}. */ protected abstract boolean containsPackage(PackageIdentifier packageName); @@ -82,9 +86,13 @@ * additional context. But, for instance, if interpreted with respect to a {@code package_group}'s * {@code packages} attribute, the strings always have the same repository as the package group. */ - // TODO(brandjon): This method's main benefit is that it's round-trippable. We could eliminate - // it in favor of asString() if we provided a public variant of fromString() that tolerates - // repositories. + // TODO(brandjon): This method's main benefit is that it's round-trippable. But we can already + // achieve the same thing with asString(), if the caller parses out the repo to pass to + // fromString() as a separate arg. It'd be nice to eliminate this method in favor of asString() + // and make a version of fromString() that can parse repo names in the label. We'd just have to + // mimic the Label parsing of repo (we can't reuse Label parsing directly since it won't like the + // `/...` syntax). Repo remapping shouldn't come up, since the names we get from stringification + // ought to already be canonical/absolute. protected abstract String asStringWithoutRepository(); @Override @@ -93,71 +101,124 @@ } /** - * Parses the provided {@link String} into a {@link PackageSpecification}. + * Parses the string {@code spec} into a {@link PackageSpecification}, within the context of the + * given repository name. * - * <p>The {@link String} must have one of the following forms: + * <p>{@code spec} may have the following forms: * - * <ul> - * <li>The full name of a single package, without repository qualification, prefixed with "//" - * (e.g. "//foo/bar"). This results in a {@link PackageSpecification} that contains exactly - * the named package. - * <li>The full name of a single package, without repository qualification, prefixed with "//", - * and suffixed with "/..." (e.g. "//foo/bar/...") This results in a {@link - * PackageSpecification} that contains all transitive subpackages of that package, inclusive. - * <li>Exactly "//...". This results in a {@link PackageSpecification} that contains all packages. - * </ul> + * <ol> + * <li>The full name of a single package, without repository qualification, prefixed with "//" + * (e.g. "//foo/bar"). The resulting specification contains exactly that package. + * <li>The same, but suffixed with "/..." for a non-root package ("//foo/bar/...") or "..." for + * the root package ("//..."). The resulting specification contains that package and all its + * subpackages. + * <li>The string constants "public" or "private". The resulting specification contains either + * all packages or no packages, respectively. + * </ol> * - * <p>If and only if the {@link String} is one of the first two forms, the returned {@link - * PackageSpecification} is specific to the provided {@link RepositoryName}. Note that it is not - * possible to construct a repository-specific {@link PackageSpecification} for all transitive - * subpackages of the root package (i.e. a repository-specific "//..."). + * In the first two cases, the repository of the given package name is taken to be {@code + * repositoryName}. In the third case the repository name is ignored. * - * <p>Throws {@link InvalidPackageSpecificationException} if the {@link String} cannot be parsed. + * <p>In the first two cases, {@code spec} may also be prefixed by a "-". The resulting + * specification contains the same set of packages but is marked as being negated. (Negation logic + * is applied at the level of {@link PackageGroupContents}.) + * + * <p>Setting {@code allowPublicPrivate} to false disallows the string constants "public" and + * "private". Note that if {@link #asString} is called with {@code includeDoubleSlash} set to + * false, the stringification of "public" and "//public" is ambiguous (likewise for private), + * hence why it might be appropriate to prohibit these forms. + * + * <p>Setting {@code repoRootMeansCurrentRepo} to false restores the following legacy behavior: In + * the specific case where {@code spec} is "//..." (or its negation), the package specification + * contains <i>all</i> packages (possibly marked as negated) rather than just those packages in + * {@code repositoryName}. In other words, "//..." behaves the same as "public". However, "//" + * still represents just the root package of {@code repositoryName}. + * + * <p>To protect against requiring users to update to a disallowed syntax, it is illegal to + * specify {@code repoRootMeansCurrentRepo} without also specifying {@code allowPublicPrivate}. + * + * @throws InvalidPackageSpecificationException if the string does not fit one of these forms */ - public static PackageSpecification fromString(RepositoryName repositoryName, String spec) + // TODO(#16365): Remove allowPublicPrivate. + // TODO(#16324): Remove legacy behavior and repoRootMeansCurrentRepo param. + public static PackageSpecification fromString( + RepositoryName repositoryName, + String spec, + boolean allowPublicPrivate, + boolean repoRootMeansCurrentRepo) throws InvalidPackageSpecificationException { - String result = spec; - boolean negative = false; - if (result.startsWith(NEGATIVE_PREFIX)) { - negative = true; - result = result.substring(NEGATIVE_PREFIX.length()); + if (repoRootMeansCurrentRepo && !allowPublicPrivate) { + throw new InvalidPackageSpecificationException( + "Cannot use new \"//...\" meaning without allowing new \"public\" syntax. Try enabling" + + " --incompatible_package_group_has_public_syntax or disabling" + + " --incompatible_fix_package_group_reporoot_syntax."); } - PackageSpecification packageSpecification = fromStringPositive(repositoryName, result); + if (!allowPublicPrivate + && (spec.equals(PUBLIC_VISIBILITY) || spec.equals(PRIVATE_VISIBILITY))) { + throw new InvalidPackageSpecificationException( + String.format( + "Use of \"%s\" package specification requires enabling" + + " --incompatible_package_group_has_public_syntax", + spec)); + } + boolean negative = false; + if (spec.startsWith(NEGATIVE_PREFIX)) { + negative = true; + spec = spec.substring(NEGATIVE_PREFIX.length()); + if (spec.equals(PUBLIC_VISIBILITY) || spec.equals(PRIVATE_VISIBILITY)) { + throw new InvalidPackageSpecificationException( + String.format("Cannot negate \"%s\" package specification", spec)); + } + } + PackageSpecification packageSpecification = + fromStringPositive(repositoryName, spec, repoRootMeansCurrentRepo); return negative ? new NegativePackageSpecification(packageSpecification) : packageSpecification; } - private static PackageSpecification fromStringPositive(RepositoryName repositoryName, String spec) + private static PackageSpecification fromStringPositive( + RepositoryName repositoryName, String spec, boolean repoRootMeansCurrentRepo) throws InvalidPackageSpecificationException { - String result = spec; - boolean allBeneath = false; - if (result.endsWith(ALL_BENEATH_SUFFIX)) { - allBeneath = true; - result = result.substring(0, result.length() - ALL_BENEATH_SUFFIX.length()); - if (result.equals("/")) { - // spec was "//...". - return AllPackages.EVERYTHING; - } + if (spec.equals(PUBLIC_VISIBILITY)) { + return AllPackages.INSTANCE; + } else if (spec.equals(PRIVATE_VISIBILITY)) { + return NoPackages.INSTANCE; } - if (!spec.startsWith("//")) { throw new InvalidPackageSpecificationException( - String.format("invalid package name '%s': must start with '//'", spec)); + String.format( + "invalid package name '%s': must start with '//' or be 'public' or 'private'", spec)); } - PackageIdentifier packageId; + String pkgPath; + boolean allBeneath = false; + if (spec.endsWith(ALL_BENEATH_SUFFIX)) { + allBeneath = true; + pkgPath = spec.substring(0, spec.length() - ALL_BENEATH_SUFFIX.length()); + if (pkgPath.equals("/")) { + // spec was "//...". + if (repoRootMeansCurrentRepo) { + pkgPath = "//"; + } else { + // Legacy behavior: //... is "public". + return AllPackages.INSTANCE; + } + } + } else { + pkgPath = spec; + } + + PackageIdentifier unqualifiedPkgId; try { - packageId = PackageIdentifier.parse(result); + unqualifiedPkgId = PackageIdentifier.parse(pkgPath); } catch (LabelSyntaxException e) { throw new InvalidPackageSpecificationException( String.format("invalid package name '%s': %s", spec, e.getMessage())); } - Verify.verify(packageId.getRepository().isMain()); + Verify.verify(unqualifiedPkgId.getRepository().isMain()); - PackageIdentifier packageIdForSpecifiedRepository = - PackageIdentifier.create(repositoryName, packageId.getPackageFragment()); - return allBeneath - ? new AllPackagesBeneath(packageIdForSpecifiedRepository) - : new SinglePackage(packageIdForSpecifiedRepository); + PackageIdentifier pkgId = + PackageIdentifier.create(repositoryName, unqualifiedPkgId.getPackageFragment()); + return allBeneath ? new AllPackagesBeneath(pkgId) : new SinglePackage(pkgId); } /** @@ -165,13 +226,21 @@ * * <p>This rejects negative package patterns, and translates the exception type into {@code * EvalException}. + * + * <p>Note that .bzl visibility package specifications always behave as if {@code + * --incompatible_package_group_has_public_syntax} and {@code + * --incompatible_fix_package_group_reporoot_syntax} are enabled. */ - // TODO(b/22193153): Support negatives too. public static PackageSpecification fromStringForBzlVisibility( RepositoryName repositoryName, String spec) throws EvalException { PackageSpecification result; try { - result = fromString(repositoryName, spec); + result = + fromString( + repositoryName, + spec, + /*allowPublicPrivate=*/ true, + /*repoRootMeansCurrentRepo=*/ true); } catch (InvalidPackageSpecificationException e) { throw new EvalException(e.getMessage()); } @@ -193,8 +262,8 @@ * * <p>If the label's name is neither "__pkg__" nor "__subpackages__", this returns {@code null}. * - * <p>Note that there is no {@link Label} associated with the {@link RepositoryName}-agnostic "all - * packages" specification (corresponding to {@code #fromString(null, "//...")}). + * <p>Note that there is no {@link Label} associated with the {@link RepositoryName}-agnostic + * "public" specification ("//..." under legacy semantics). */ @Nullable static PackageSpecification fromLabel(Label label) { @@ -208,7 +277,11 @@ } public static PackageSpecification everything() { - return AllPackages.EVERYTHING; + return AllPackages.INSTANCE; + } + + public static PackageSpecification nothing() { + return NoPackages.INSTANCE; } private static final class SinglePackage extends PackageSpecification { @@ -291,7 +364,12 @@ @Override protected String asStringWithoutRepository() { - return "//" + prefix.getPackageFragment().getPathString() + ALL_BENEATH_SUFFIX; + if (prefix.getPackageFragment().equals(PathFragment.EMPTY_FRAGMENT)) { + // Special case: Emit "//..." rather than suffixing "/...", which would yield "///...". + return "//..."; + } else { + return "//" + prefix.getPackageFragment().getPathString() + ALL_BENEATH_SUFFIX; + } } @Override @@ -336,11 +414,6 @@ } @Override - public int hashCode() { - return delegate.hashCode(); - } - - @Override public boolean equals(Object obj) { if (this == obj) { return true; @@ -348,12 +421,17 @@ return obj instanceof NegativePackageSpecification && delegate.equals(((NegativePackageSpecification) obj).delegate); } + + @Override + public int hashCode() { + return NegativePackageSpecification.class.hashCode() ^ delegate.hashCode(); + } } @VisibleForSerialization static final class AllPackages extends PackageSpecification { @SerializationConstant @VisibleForSerialization - static final PackageSpecification EVERYTHING = new AllPackages(); + static final PackageSpecification INSTANCE = new AllPackages(); @Override protected boolean containsPackage(PackageIdentifier packageName) { @@ -362,14 +440,15 @@ @Override protected String asString(boolean includeDoubleSlash) { - // Note that "//..." is the desired result, not "...", even under the legacy behavior of - // includeDoubleSlash=false. - return "//..."; + // Under legacy formatting rules, use legacy syntax. This avoids ambiguity between "public" + // and "//public", and ensures that AllPackages is round-trippable when the value of + // includeDoubleSlash matches allowPublicPrivate. + return includeDoubleSlash ? "public" : "//..."; } @Override protected String asStringWithoutRepository() { - return "//..."; + return "public"; } @Override @@ -379,7 +458,38 @@ @Override public int hashCode() { - return "//...".hashCode(); + return AllPackages.class.hashCode(); + } + } + + @VisibleForSerialization + static final class NoPackages extends PackageSpecification { + @SerializationConstant @VisibleForSerialization + static final PackageSpecification INSTANCE = new NoPackages(); + + @Override + protected boolean containsPackage(PackageIdentifier packageName) { + return false; + } + + @Override + protected String asString(boolean includeDoubleSlash) { + return "private"; + } + + @Override + protected String asStringWithoutRepository() { + return "private"; + } + + @Override + public boolean equals(Object o) { + return o instanceof NoPackages; + } + + @Override + public int hashCode() { + return NoPackages.class.hashCode(); } } @@ -437,7 +547,11 @@ for (PackageSpecification spec : packageSpecifications) { if (spec instanceof SinglePackage) { singlePositives.put(((SinglePackage) spec).singlePackageName, spec); - } else if (spec instanceof AllPackages || spec instanceof AllPackagesBeneath) { + } else if (spec instanceof AllPackages + || spec instanceof AllPackagesBeneath + // We can't drop NoPackages because it still needs to be serialized, e.g. in bazel query + // output. + || spec instanceof NoPackages) { otherPositives.add(spec); } else if (spec instanceof NegativePackageSpecification) { negatives.add(spec); @@ -483,6 +597,10 @@ * * <p>Note that strings for specs that cross repositories can't be reparsed using {@link * PackageSpecification#fromString}. + * + * <p>The special public constant will serialize as {@code "public"} if {@code + * includeDoubleSlash} is true, and {@code "//..."} otherwise. The private constant will always + * serialize as {@code "private"}, */ public Stream<String> streamPackageStrings(boolean includeDoubleSlash) { return streamSpecs().map(spec -> spec.asString(includeDoubleSlash)); @@ -492,6 +610,9 @@ * Maps {@link PackageSpecification#asStringWithoutRepository} to the component package specs. * * <p>Note that this is ambiguous w.r.t. specs that reference other repositories. + * + * <p>The special public and private constants will serialize as {@code "public"} and {@code + * "private"} respectively. */ public Stream<String> streamPackageStringsWithoutRepository() { return streamSpecs().map(PackageSpecification::asStringWithoutRepository);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java index f45c882..703b9de 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java +++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java
@@ -542,7 +542,18 @@ Location loc = thread.getCallerLocation(); try { - context.pkgBuilder.addPackageGroup(name, packages, includes, context.eventHandler, loc); + context.pkgBuilder.addPackageGroup( + name, + packages, + includes, + /*allowPublicPrivate=*/ thread + .getSemantics() + .getBool(BuildLanguageOptions.INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX), + /*repoRootMeansCurrentRepo=*/ thread + .getSemantics() + .getBool(BuildLanguageOptions.INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX), + context.eventHandler, + loc); return Starlark.NONE; } catch (LabelSyntaxException e) { throw Starlark.errorf("package group has invalid name: %s: %s", name, e.getMessage());
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 a3518ad..4605061 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
@@ -443,6 +443,31 @@ public boolean incompatibleDisallowStructProviderSyntax; @Option( + name = "incompatible_package_group_has_public_syntax", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = + "In package_group's `packages` attribute, allows writing \"public\" or \"private\" to" + + " refer to all packages or no packages respectively.") + public boolean incompatiblePackageGroupHasPublicSyntax; + + @Option( + name = "incompatible_fix_package_group_reporoot_syntax", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = + "In package_group's `packages` attribute, changes the meaning of the value \"//...\" to" + + " refer to all packages in the current repository instead of all packages in any" + + " repository. You can use the special value \"public\" in place of \"//...\" to" + + " obtain the old behavior. This flag requires" + + " that --incompatible_package_group_has_public_syntax also be enabled.") + public boolean incompatibleFixPackageGroupReporootSyntax; + + @Option( name = "incompatible_visibility_private_attributes_at_definition", defaultValue = "false", documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, @@ -675,6 +700,12 @@ .setBool( INCOMPATIBLE_DISALLOW_STRUCT_PROVIDER_SYNTAX, incompatibleDisallowStructProviderSyntax) + .setBool( + INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX, + incompatiblePackageGroupHasPublicSyntax) + .setBool( + INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX, + incompatibleFixPackageGroupReporootSyntax) .setBool(INCOMPATIBLE_JAVA_COMMON_PARAMETERS, incompatibleJavaCommonParameters) .setBool(INCOMPATIBLE_NEW_ACTIONS_API, incompatibleNewActionsApi) .setBool(INCOMPATIBLE_NO_ATTR_LICENSE, incompatibleNoAttrLicense) @@ -761,6 +792,10 @@ public static final String INCOMPATIBLE_DISALLOW_EMPTY_GLOB = "-incompatible_disallow_empty_glob"; public static final String INCOMPATIBLE_DISALLOW_STRUCT_PROVIDER_SYNTAX = "-incompatible_disallow_struct_provider_syntax"; + public static final String INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX = + "-incompatible_package_group_has_public_syntax"; + public static final String INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX = + "-incompatible_fix_package_group_reporoot_syntax"; public static final String INCOMPATIBLE_DO_NOT_SPLIT_LINKING_CMDLINE = "+incompatible_do_not_split_linking_cmdline"; public static final String INCOMPATIBLE_JAVA_COMMON_PARAMETERS =
diff --git a/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java b/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java index 5bd8261..eb44883 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java +++ b/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java
@@ -137,6 +137,17 @@ + "query: no-op (aspects are always followed).") public boolean useAspects; + @Option( + name = "incompatible_package_group_includes_double_slash", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.QUERY, + effectTags = {OptionEffectTag.TERMINAL_OUTPUT}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = + "If enabled, when outputting package_group's `packages` attribute, the leading `//`" + + " will not be omitted.") + public boolean incompatiblePackageGroupIncludesDoubleSlash; + /** Return the current options as a set of QueryEnvironment settings. */ public Set<Setting> toSettings() { Set<Setting> settings = EnumSet.noneOf(Setting.class);
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java index d3f85d8..3c1eb27 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java +++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/ProtoOutputFormatter.java
@@ -101,6 +101,7 @@ private AspectResolver aspectResolver; private DependencyFilter dependencyFilter; + private boolean packageGroupIncludesDoubleSlash; private boolean relativeLocations; private boolean displaySourceFileLocation; private boolean includeDefaultValues = true; @@ -126,6 +127,7 @@ super.setOptions(options, aspectResolver, hashFunction); this.aspectResolver = aspectResolver; this.dependencyFilter = FormatUtils.getDependencyFilter(options); + this.packageGroupIncludesDoubleSlash = options.incompatiblePackageGroupIncludesDoubleSlash; this.relativeLocations = options.relativeLocations; this.displaySourceFileLocation = options.displaySourceFileLocation; this.includeDefaultValues = options.protoIncludeDefaultValues; @@ -344,9 +346,8 @@ PackageGroup packageGroup = (PackageGroup) target; Build.PackageGroup.Builder packageGroupPb = Build.PackageGroup.newBuilder().setName(packageGroup.getLabel().toString()); - // TODO(b/77598306): Migrate to format with leading double slash for (String containedPackage : - packageGroup.getContainedPackages(/*includeDoubleSlash=*/ false)) { + packageGroup.getContainedPackages(packageGroupIncludesDoubleSlash)) { packageGroupPb.addContainedPackage(containedPackage); } for (Label include : packageGroup.getIncludes()) {
diff --git a/src/main/java/com/google/devtools/build/lib/query2/query/output/XmlOutputFormatter.java b/src/main/java/com/google/devtools/build/lib/query2/query/output/XmlOutputFormatter.java index bcac1c6..1fff552 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/query/output/XmlOutputFormatter.java +++ b/src/main/java/com/google/devtools/build/lib/query2/query/output/XmlOutputFormatter.java
@@ -61,6 +61,7 @@ private AspectResolver aspectResolver; private DependencyFilter dependencyFilter; + private boolean packageGroupIncludesDoubleSlash; private boolean relativeLocations; private boolean displaySourceFileLocation; private QueryOptions queryOptions; @@ -83,6 +84,7 @@ super.setOptions(options, aspectResolver, hashFunction); this.aspectResolver = aspectResolver; this.dependencyFilter = FormatUtils.getDependencyFilter(options); + this.packageGroupIncludesDoubleSlash = options.incompatiblePackageGroupIncludesDoubleSlash; this.relativeLocations = options.relativeLocations; this.displaySourceFileLocation = options.displaySourceFileLocation; @@ -208,8 +210,7 @@ createValueElement( doc, Type.STRING_LIST, - // TODO(b/77598306): Migrate to format with leading double slash - packageGroup.getContainedPackages(/*includeDoubleSlash=*/ false)); + packageGroup.getContainedPackages(packageGroupIncludesDoubleSlash)); packages.setAttribute("name", "packages"); elem.appendChild(packages); } else if (target instanceof OutputFile) {
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java index ed83559..07c9a77 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupStaticInitializationTest.java
@@ -43,7 +43,12 @@ try { RepositoryName defaultRepoName = Label.parseAbsoluteUnchecked("//context").getRepository(); - groupQueue.put(PackageSpecification.fromString(defaultRepoName, "//fruits/...")); + groupQueue.put( + PackageSpecification.fromString( + defaultRepoName, + "//fruits/...", + /*allowPublicPrivate=*/ true, + /*repoRootMeansCurrentRepo=*/ true)); } catch (Exception e) { // Can't throw from Runnable, but this will cause the test to timeout // when the consumer can't take the object.
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java index b6baeaa..f91a849 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java
@@ -218,7 +218,133 @@ } @Test - public void testNegative_everything() throws Exception { + public void testEverythingSpecificationWorks() throws Exception { + setBuildLanguageOptions("--incompatible_package_group_has_public_syntax=true"); + + scratch.file( + "fruits/BUILD", // + "package_group(", + " name = 'mango',", + " packages = ['public'],", + ")"); + PackageGroup grp = getPackageGroup("fruits", "mango"); + + // Assert that we're using the right package spec. + assertThat(grp.getContainedPackages(/*includeDoubleSlash=*/ true)).containsExactly("public"); + // Assert that this package spec contains packages from both inside and outside the main repo. + assertThat(grp.contains(pkgId("pkg"))).isTrue(); + assertThat(grp.contains(pkgId("somerepo", "pkg"))).isTrue(); + } + + @Test + public void testNothingSpecificationWorks() throws Exception { + setBuildLanguageOptions("--incompatible_package_group_has_public_syntax=true"); + + scratch.file( + "fruits/BUILD", // + "package_group(", + " name = 'mango',", + " packages = ['private'],", + ")"); + PackageGroup grp = getPackageGroup("fruits", "mango"); + + // Assert that we're using the right package spec. + assertThat(grp.getContainedPackages(/*includeDoubleSlash=*/ true)).containsExactly("private"); + assertThat(grp.contains(pkgId("anything"))).isFalse(); + } + + @Test + public void testPublicPrivateAreNotAccessibleWithoutFlag() throws Exception { + setBuildLanguageOptions("--incompatible_package_group_has_public_syntax=false"); + + scratch.file( + "foo/BUILD", // + "package_group(", + " name = 'grp1',", + " packages = ['public'],", + ")"); + scratch.file( + "bar/BUILD", // + "package_group(", + " name = 'grp2',", + " packages = ['private'],", + ")"); + + reporter.removeHandler(failFastHandler); + getPackageGroup("foo", "grp1"); + assertContainsEvent( + "Use of \"public\" package specification requires enabling" + + " --incompatible_package_group_has_public_syntax"); + getPackageGroup("bar", "grp2"); + assertContainsEvent( + "Use of \"private\" package specification requires enabling" + + " --incompatible_package_group_has_public_syntax"); + } + + @Test + public void testRepoRootSubpackagesIsPublic_withoutFlag() throws Exception { + setBuildLanguageOptions("--incompatible_fix_package_group_reporoot_syntax=false"); + + scratch.file( + "fruits/BUILD", // + "package_group(", + " name = 'mango',", + " packages = ['//...'],", + ")"); + PackageGroup grp = getPackageGroup("fruits", "mango"); + + // Use includeDoubleSlash=true to make package spec stringification distinguish AllPackages from + // AllPackagesBeneath with empty package path. + assertThat(grp.getContainedPackages(/*includeDoubleSlash=*/ true)) + // Assert that "//..." gave us AllPackages. + .containsExactly("public"); + assertThat(grp.contains(pkgId("pkg"))).isTrue(); + assertThat(grp.contains(pkgId("somerepo", "pkg"))).isTrue(); + } + + @Test + public void testRepoRootSubpackagesIsNotPublic_withFlag() throws Exception { + setBuildLanguageOptions( + "--incompatible_package_group_has_public_syntax=true", + "--incompatible_fix_package_group_reporoot_syntax=true"); + + scratch.file( + "fruits/BUILD", // + "package_group(", + " name = 'mango',", + " packages = ['//...'],", + ")"); + PackageGroup grp = getPackageGroup("fruits", "mango"); + + // Use includeDoubleSlash=true to make package spec stringification distinguish AllPackages from + // AllPackagesBeneath with empty package path. + assertThat(grp.getContainedPackages(/*includeDoubleSlash=*/ true)) + // Assert that "//..." gave us AllPackagesBeneath. + .containsExactly("//..."); + assertThat(grp.contains(pkgId("pkg"))).isTrue(); + assertThat(grp.contains(pkgId("somerepo", "pkg"))).isFalse(); + } + + @Test + public void testCannotUseNewRepoRootSyntaxWithoutPublicSyntax() throws Exception { + setBuildLanguageOptions( + "--incompatible_package_group_has_public_syntax=false", + "--incompatible_fix_package_group_reporoot_syntax=true"); + + scratch.file( + "fruits/BUILD", // + "package_group(", + " name = 'mango',", + " packages = ['//something'],", + ")"); + + reporter.removeHandler(failFastHandler); + getPackageGroup("fruits", "mango"); + assertContainsEvent("Cannot use new \"//...\" meaning without allowing new \"public\" syntax."); + } + + @Test + public void testNegative_repoRootSubpackages() throws Exception { scratch.file( "test/BUILD", "package_group(", @@ -236,20 +362,35 @@ } @Test - public void testEverythingSpecificationWorks() throws Exception { - scratch.file( - "fruits/BUILD", // - "package_group(", - " name = 'mango',", - " packages = ['//...'],", - ")"); - PackageGroup grp = getPackageGroup("fruits", "mango"); + public void testNegative_public() throws Exception { + setBuildLanguageOptions("--incompatible_package_group_has_public_syntax=true"); - // Assert that we're actually using the "everything" package specification, and assert that this - // means we include packages from the main repo but also from other repos. - assertThat(grp.getContainedPackages(/*includeDoubleSlash=*/ false)).containsExactly("//..."); - assertThat(grp.contains(pkgId("pkg"))).isTrue(); - assertThat(grp.contains(pkgId("somerepo", "pkg"))).isTrue(); + scratch.file( + "fruits/BUILD", + "package_group(", + " name = 'apple',", + " packages = ['-public'],", + ")"); + + reporter.removeHandler(failFastHandler); + getPackageGroup("fruits", "apple"); + assertContainsEvent("Cannot negate \"public\" package specification"); + } + + @Test + public void testNegative_private() throws Exception { + setBuildLanguageOptions("--incompatible_package_group_has_public_syntax=true"); + + scratch.file( + "fruits/BUILD", + "package_group(", + " name = 'apple',", + " packages = ['-private'],", + ")"); + + reporter.removeHandler(failFastHandler); + getPackageGroup("fruits", "apple"); + assertContainsEvent("Cannot negate \"private\" package specification"); } @Test @@ -275,16 +416,18 @@ PackageGroupContents contents = PackageGroupContents.create( ImmutableList.of( - PackageSpecification.fromString(main, "//a"), - PackageSpecification.fromString(main, "//a/b/..."), - PackageSpecification.fromString(main, "-//c"), - PackageSpecification.fromString(main, "-//c/d/..."), - PackageSpecification.fromString(main, "//..."), - PackageSpecification.fromString(main, "-//..."), - PackageSpecification.fromString(main, "//"), - PackageSpecification.fromString(main, "-//"), - PackageSpecification.fromString(other, "//z"), - PackageSpecification.fromString(other, "//..."))); + pkgSpec(main, "//a"), + pkgSpec(main, "//a/b/..."), + pkgSpec(main, "-//c"), + pkgSpec(main, "-//c/d/..."), + pkgSpec(main, "//..."), + pkgSpec(main, "-//..."), + pkgSpec(main, "//"), + pkgSpec(main, "-//"), + pkgSpec(other, "//z"), + pkgSpec(other, "//..."), + pkgSpec(main, "public"), + pkgSpec(main, "private"))); assertThat( contents.streamPackageStrings(/*includeDoubleSlash=*/ false).collect(toImmutableList())) .containsExactly( @@ -297,8 +440,9 @@ "", "-", "@other//z", - // TODO(#16323): When parsing is fixed, change this "//..." to "@other//...". - "//..."); + "@other//...", + "//...", // legacy syntax for public + "private"); assertThat( contents.streamPackageStrings(/*includeDoubleSlash=*/ true).collect(toImmutableList())) .containsExactly( @@ -311,8 +455,9 @@ "//", "-//", "@other//z", - // TODO(#16323): When parsing is fixed, change this "//..." to "@other//...". - "//..."); + "@other//...", + "public", + "private"); assertThat(contents.streamPackageStringsWithoutRepository().collect(toImmutableList())) .containsExactly( "//a", @@ -324,7 +469,15 @@ "//", "-//", "//z", - "//..."); + "//...", + "public", + "private"); + } + + /** Convenience method for obtaining a PackageSpecification. */ + private PackageSpecification pkgSpec(RepositoryName repository, String spec) throws Exception { + return PackageSpecification.fromString( + repository, spec, /*allowPublicPrivate=*/ true, /*repoRootMeansCurrentRepo=*/ true); } /** Convenience method for obtaining a PackageIdentifier. */
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 f0ac79e..c6c712a 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
@@ -601,6 +601,54 @@ } @Test + public void testBzlVisibility_publicListElement() 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", // + // Tests "public" as a list item, and alongside other list items. + "visibility([\"public\", \"//c\"])", + "x = 1"); + + checkSuccessfulLookup("//a:foo.bzl"); + assertNoEvents(); + } + + @Test + public void testBzlVisibility_privateListElement() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=everyone"); + + scratch.file("a1/BUILD"); + scratch.file( + "a1/foo.bzl", // + "load(\"//b:bar.bzl\", \"x\")"); + scratch.file("a2/BUILD"); + scratch.file( + "a2/foo.bzl", // + "load(\"//b:bar.bzl\", \"x\")"); + scratch.file("b/BUILD"); + scratch.file( + "b/bar.bzl", // + // Tests "private" as a list item, and alongside other list items. + "visibility([\"private\", \"//a1\"])", + "x = 1"); + + checkSuccessfulLookup("//a1:foo.bzl"); + assertNoEvents(); + reporter.removeHandler(failFastHandler); + checkFailingLookup( + "//a2:foo.bzl", "module //a2:foo.bzl contains .bzl load-visibility violations"); + assertContainsEvent("Starlark file //b:bar.bzl is not visible for loading from package //a2."); + } + + @Test public void testBzlVisibility_failureInDependency() throws Exception { setBuildLanguageOptions( "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=everyone"); @@ -743,6 +791,65 @@ "Starlark file @repo//lib:bar.bzl is not visible for loading from package //pkg."); } + // TODO(#16365): This test case can be deleted once --incompatible_package_group_has_public_syntax + // is deleted (not just flipped). + @Test + public void testBzlVisibility_canUsePublicPrivate_regardlessOfFlag() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", + "--experimental_bzl_visibility_allowlist=everyone", + // Test that we can use "public" and "private" visibility for .bzl files even when the + // incompatible flag is disabled. + "--incompatible_package_group_has_public_syntax=false"); + + scratch.file("a/BUILD"); + scratch.file( + "a/foo1.bzl", // + "visibility(\"public\")"); + scratch.file( + "a/foo2.bzl", // + "visibility(\"private\")"); + + checkSuccessfulLookup("//a:foo1.bzl"); + checkSuccessfulLookup("//a:foo2.bzl"); + assertNoEvents(); + } + + // TODO(#16324): Once --incompatible_fix_package_group_reporoot_syntax is deleted (not just + // flipped), this test case will be redundant with tests for //... in PackageGroupTest. At that + // point we'll just delete this test case. + @Test + public void testBzlVisibility_repoRootSubpackagesIsNotPublic_regardlessOfFlag() throws Exception { + setBuildLanguageOptions( + "--experimental_bzl_visibility=true", + "--experimental_bzl_visibility_allowlist=everyone", + // Test that we get the fixed behavior even when the incompatible flag is disabled. + "--incompatible_fix_package_group_reporoot_syntax=false"); + + scratch.overwriteFile( + "WORKSPACE", // + "local_repository(", + " name = 'repo',", + " path = 'repo'", + ")"); + scratch.file("repo/WORKSPACE"); + scratch.file("repo/a/BUILD"); + scratch.file( + "repo/a/foo.bzl", // + "load(\"@//b:bar.bzl\", \"x\")"); + scratch.file("b/BUILD"); + scratch.file( + "b/bar.bzl", // + "visibility([\"//...\"])", + "x = 1"); + + reporter.removeHandler(failFastHandler); + checkFailingLookup( + "@repo//a:foo.bzl", "module @repo//a:foo.bzl contains .bzl load-visibility violations"); + assertContainsEvent( + "Starlark file //b:bar.bzl is not visible for loading from package @repo//a."); + } + @Test public void testBzlVisibility_disallowsSubpackagesWithoutWildcard() throws Exception { setBuildLanguageOptions(