includes,
+ boolean allowPublicPrivate,
+ boolean repoRootMeansCurrentRepo,
EventHandler eventHandler,
Location location) {
this.label = label;
@@ -61,7 +63,11 @@ public PackageGroup(
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 8c5433d1280ebf..51696ae8ea8f3d 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 @@ public abstract class PackageSpecification {
* 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 @@ public String toString() {
}
/**
- * 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.
+ *
+ * {@code spec} may have the following forms:
+ *
+ *
+ * The full name of a single package, without repository qualification, prefixed with "//"
+ * (e.g. "//foo/bar"). The resulting specification contains exactly that package.
+ * 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.
+ * The string constants "public" or "private". The resulting specification contains either
+ * all packages or no packages, respectively.
+ *
+ *
+ * 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.
+ *
+ * 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}.)
*
- *
The {@link String} must have one of the following forms:
+ *
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.
*
- *
- * 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.
- * 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.
- * Exactly "//...". This results in a {@link PackageSpecification} that contains all packages.
- *
+ * 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 all 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}.
*
- *
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 "//...").
+ *
To protect against requiring users to update to a disallowed syntax, it is illegal to
+ * specify {@code repoRootMeansCurrentRepo} without also specifying {@code allowPublicPrivate}.
*
- *
Throws {@link InvalidPackageSpecificationException} if the {@link String} cannot be parsed.
+ * @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;
+ 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.");
+ }
+ 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 (result.startsWith(NEGATIVE_PREFIX)) {
+ if (spec.startsWith(NEGATIVE_PREFIX)) {
negative = true;
- result = result.substring(NEGATIVE_PREFIX.length());
+ 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, result);
+ 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;
+ 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 '//' or be 'public' or 'private'", spec));
+ }
+
+ String pkgPath;
boolean allBeneath = false;
- if (result.endsWith(ALL_BENEATH_SUFFIX)) {
+ if (spec.endsWith(ALL_BENEATH_SUFFIX)) {
allBeneath = true;
- result = result.substring(0, result.length() - ALL_BENEATH_SUFFIX.length());
- if (result.equals("/")) {
+ pkgPath = spec.substring(0, spec.length() - ALL_BENEATH_SUFFIX.length());
+ if (pkgPath.equals("/")) {
// spec was "//...".
- return AllPackages.EVERYTHING;
+ if (repoRootMeansCurrentRepo) {
+ pkgPath = "//";
+ } else {
+ // Legacy behavior: //... is "public".
+ return AllPackages.INSTANCE;
+ }
}
+ } else {
+ pkgPath = spec;
}
- if (!spec.startsWith("//")) {
- throw new InvalidPackageSpecificationException(
- String.format("invalid package name '%s': must start with '//'", spec));
- }
-
- PackageIdentifier packageId;
+ 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 @@ private static PackageSpecification fromStringPositive(RepositoryName repository
*
*
This rejects negative package patterns, and translates the exception type into {@code
* EvalException}.
+ *
+ *
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 @@ public static PackageSpecification fromStringForBzlVisibility(
*
*
If the label's name is neither "__pkg__" nor "__subpackages__", this returns {@code null}.
*
- *
Note that there is no {@link Label} associated with the {@link RepositoryName}-agnostic "all
- * packages" specification (corresponding to {@code #fromString(null, "//...")}).
+ *
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 @@ static PackageSpecification fromLabel(Label label) {
}
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 @@ protected String asString(boolean includeDoubleSlash) {
@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
@@ -335,11 +413,6 @@ protected String asStringWithoutRepository() {
return "-" + delegate.asStringWithoutRepository();
}
- @Override
- public int hashCode() {
- return delegate.hashCode();
- }
-
@Override
public boolean equals(Object obj) {
if (this == obj) {
@@ -348,12 +421,17 @@ public boolean equals(Object obj) {
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 @@ protected boolean containsPackage(PackageIdentifier packageName) {
@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 @@ public boolean equals(Object o) {
@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 @@ public static PackageGroupContents create(
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 @@ public boolean containsPackage(PackageIdentifier packageIdentifier) {
*
*
Note that strings for specs that cross repositories can't be reparsed using {@link
* PackageSpecification#fromString}.
+ *
+ *
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 streamPackageStrings(boolean includeDoubleSlash) {
return streamSpecs().map(spec -> spec.asString(includeDoubleSlash));
@@ -492,6 +610,9 @@ public Stream streamPackageStrings(boolean includeDoubleSlash) {
* Maps {@link PackageSpecification#asStringWithoutRepository} to the component package specs.
*
* Note that this is ambiguous w.r.t. specs that reference other repositories.
+ *
+ *
The special public and private constants will serialize as {@code "public"} and {@code
+ * "private"} respectively.
*/
public Stream 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 f45c882c1edad8..703b9deb329d0a 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 @@ public NoneType packageGroup(
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 a3518ad3c1cf64..460506127e3fef 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
@@ -442,6 +442,31 @@ public final class BuildLanguageOptions extends OptionsBase {
+ "instead return a list of provider instances.")
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",
@@ -675,6 +700,12 @@ public StarlarkSemantics toStarlarkSemantics() {
.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 StarlarkSemantics toStarlarkSemantics() {
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 5bd8261fa9a61a..eb44883d47703f 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 @@ public String getLineTerminator() {
+ "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 toSettings() {
Set 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 d3f85d8af73a82..3c1eb27e152b7a 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 @@ public class ProtoOutputFormatter extends AbstractUnorderedFormatter {
private AspectResolver aspectResolver;
private DependencyFilter dependencyFilter;
+ private boolean packageGroupIncludesDoubleSlash;
private boolean relativeLocations;
private boolean displaySourceFileLocation;
private boolean includeDefaultValues = true;
@@ -126,6 +127,7 @@ public void setOptions(
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 @@ public Build.Target toTargetProtoBuffer(Target target, Object extraDataForAttrHa
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 bcac1c684b0af2..1fff552a1a61ce 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 @@ class XmlOutputFormatter extends AbstractUnorderedFormatter {
private AspectResolver aspectResolver;
private DependencyFilter dependencyFilter;
+ private boolean packageGroupIncludesDoubleSlash;
private boolean relativeLocations;
private boolean displaySourceFileLocation;
private QueryOptions queryOptions;
@@ -83,6 +84,7 @@ public void setOptions(
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 @@ private Element createTargetElement(Document doc, Target target)
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 ed83559fdaea06..07c9a770ae030d 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 @@ public void testNoDeadlockOnPackageGroupCreation() throws Exception {
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 b6baeaa59e8498..f91a8492252171 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 @@ public void testNegative_subpackages() throws Exception {
}
@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 @@ public void testNegative_everything() throws Exception {
}
@Test
- public void testEverythingSpecificationWorks() throws Exception {
+ public void testNegative_public() throws Exception {
+ setBuildLanguageOptions("--incompatible_package_group_has_public_syntax=true");
+
scratch.file(
- "fruits/BUILD", //
+ "fruits/BUILD",
"package_group(",
- " name = 'mango',",
- " packages = ['//...'],",
+ " name = 'apple',",
+ " packages = ['-public'],",
")");
- PackageGroup grp = getPackageGroup("fruits", "mango");
- // 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();
+ 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 @@ public void testStringification() throws Exception {
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 @@ public void testStringification() throws Exception {
"",
"-",
"@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 @@ public void testStringification() throws Exception {
"//",
"-//",
"@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 @@ public void testStringification() throws Exception {
"//",
"-//",
"//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 f0ac79ea294c65..c6c712a92c535c 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,54 @@ public void testBzlVisibility_privateDifferentPackage() throws Exception {
assertContainsEvent("Starlark file //b:bar.bzl is not visible for loading from package //a.");
}
+ @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(
@@ -743,6 +791,65 @@ public void testBzlVisibility_enumeratedPackagesMultipleRepos() throws Exception
"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(