Bring PackageSpecification docs up-to-date, clean up its interface
Focuses on documenting the Strings that PackageSpecifications can be
translated from and to.
--
MOS_MIGRATED_REVID=128195540
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index e7c4ac4..e3cf5f6 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -80,7 +80,6 @@
import com.google.devtools.build.skyframe.WalkableGraph;
import com.google.devtools.common.options.Option;
import com.google.devtools.common.options.OptionsBase;
-
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashMap;
@@ -91,7 +90,6 @@
import java.util.Map;
import java.util.Set;
import java.util.logging.Logger;
-
import javax.annotation.Nullable;
/**
@@ -970,15 +968,20 @@
throws EvalException, InterruptedException {
BuildConfiguration targetConfig = target.getConfiguration();
return new RuleContext.Builder(
- env, (Rule) target.getTarget(), null, targetConfig, configurations.getHostConfiguration(),
- ruleClassProvider.getPrerequisiteValidator(),
- ((Rule) target.getTarget()).getRuleClassObject().getConfigurationFragmentPolicy())
- .setVisibility(NestedSetBuilder.<PackageSpecification>create(
- Order.STABLE_ORDER, PackageSpecification.EVERYTHING))
- .setPrerequisites(getPrerequisiteMapForTesting(eventHandler, target, configurations))
- .setConfigConditions(ImmutableMap.<Label, ConfigMatchingProvider>of())
- .setUniversalFragment(ruleClassProvider.getUniversalFragment())
- .build();
+ env,
+ (Rule) target.getTarget(),
+ null,
+ targetConfig,
+ configurations.getHostConfiguration(),
+ ruleClassProvider.getPrerequisiteValidator(),
+ ((Rule) target.getTarget()).getRuleClassObject().getConfigurationFragmentPolicy())
+ .setVisibility(
+ NestedSetBuilder.<PackageSpecification>create(
+ Order.STABLE_ORDER, PackageSpecification.everything()))
+ .setPrerequisites(getPrerequisiteMapForTesting(eventHandler, target, configurations))
+ .setConfigConditions(ImmutableMap.<Label, ConfigMatchingProvider>of())
+ .setUniversalFragment(ruleClassProvider.getUniversalFragment())
+ .build();
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
index 0597e0b..c6a0f4f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -53,12 +53,10 @@
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.vfs.PathFragment;
-
import java.util.ArrayList;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;
-
import javax.annotation.Nullable;
/**
@@ -87,7 +85,7 @@
if (ruleVisibility instanceof ConstantRuleVisibility) {
return ((ConstantRuleVisibility) ruleVisibility).isPubliclyVisible()
? NestedSetBuilder.<PackageSpecification>create(
- Order.STABLE_ORDER, PackageSpecification.EVERYTHING)
+ Order.STABLE_ORDER, PackageSpecification.everything())
: NestedSetBuilder.<PackageSpecification>emptySet(Order.STABLE_ORDER);
} else if (ruleVisibility instanceof PackageGroupsRuleVisibility) {
PackageGroupsRuleVisibility packageGroupsVisibility =
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 67ea4ed..bffd929 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
@@ -21,16 +21,15 @@
import com.google.devtools.build.lib.events.EventHandler;
import com.google.devtools.build.lib.events.Location;
import com.google.devtools.build.lib.packages.License.DistributionType;
-
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Set;
/**
- * This class represents a package group. It has a name and a set of packages
- * and can be asked if a specific package is included in it. The package set is
- * represented as a list of PathFragments.
+ * This class represents a package group BUILD target. It has a name, a list of {@link
+ * PackageSpecification}s, a list of {@link Label}s of other package groups this one includes, and
+ * can be asked if a specific package is included in it.
*/
public class PackageGroup implements Target {
private boolean containsErrors;
@@ -40,18 +39,25 @@
private final List<PackageSpecification> packageSpecifications;
private final List<Label> includes;
- public PackageGroup(Label label, Package pkg, Collection<String> packages,
- Collection<Label> includes, EventHandler eventHandler, Location location) {
+ public PackageGroup(
+ Label label,
+ Package pkg,
+ Collection<String> packageSpecifications,
+ Collection<Label> includes,
+ EventHandler eventHandler,
+ Location location) {
this.label = label;
this.location = location;
this.containingPackage = pkg;
this.includes = ImmutableList.copyOf(includes);
ImmutableList.Builder<PackageSpecification> packagesBuilder = ImmutableList.builder();
- for (String containedPackage : packages) {
+ for (String packageSpecification : packageSpecifications) {
PackageSpecification specification = null;
try {
- specification = PackageSpecification.fromString(label, containedPackage);
+ specification =
+ PackageSpecification.fromString(
+ label.getPackageIdentifier().getRepository(), packageSpecification);
} catch (PackageSpecification.InvalidPackageSpecificationException e) {
containsErrors = true;
eventHandler.handle(Event.error(location, e.getMessage()));
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 7d37f10..a50a810 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
@@ -17,62 +17,65 @@
import com.google.devtools.build.lib.cmdline.Label;
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.vfs.PathFragment;
/**
- * A class that represents some packages that are included in the visibility list of a rule.
+ * Represents one of the following:
+ *
+ * <ul>
+ * <li>A single package (e.g. "//foo/bar")
+ * <li>All transitive subpackages of a package, inclusive (e.g. "//foo/bar/...", which includes
+ * "//foo/bar")
+ * <li>All packages (i.e. "//...")
+ * </ul>
+ *
+ * <p>Typically (exclusively?) used for package visibility, as part of a {@link PackageGroup}
+ * target.
+ *
+ * <p>A package specification is specific to a single {@link RepositoryName} unless it is the "all
+ * packages" specification.
*/
public abstract class PackageSpecification {
private static final String PACKAGE_LABEL = "__pkg__";
private static final String SUBTREE_LABEL = "__subpackages__";
private static final String ALL_BENEATH_SUFFIX = "/...";
- public static final PackageSpecification EVERYTHING =
- new PackageSpecification() {
- @Override
- public boolean containsPackage(PackageIdentifier packageName) {
- return true;
- }
- @Override
- protected String asString() {
- return "//...";
- }
- };
-
+ /** Returns {@code true} if the package spec includes the provided {@code packageName}. */
public abstract boolean containsPackage(PackageIdentifier packageName);
- @Override
- public int hashCode() {
- return asString().hashCode();
- }
-
- @Override
- public final String toString() {
- return asString();
- }
-
- protected abstract String asString();
-
- @Override
- public boolean equals(Object that) {
- if (this == that) {
- return true;
- }
-
- if (!(that instanceof PackageSpecification)) {
- return false;
- }
-
- return this.asString().equals(((PackageSpecification) that).asString());
- }
+ /**
+ * Returns a {@link String} representation of the {@link PackageSpecification} of the same format
+ * accepted by {@link #fromString}.
+ *
+ * <p>The returned {@link String} is insensitive to the {@link RepositoryName} associated with the
+ * {@link PackageSpecification}.
+ */
+ public abstract String toStringWithoutRepository();
/**
- * Parses a string as a visibility specification.
- * Throws {@link InvalidPackageSpecificationException} if the label cannot be parsed.
+ * Parses the provided {@link String} into a {@link PackageSpecification}.
*
- * <p>Note that these strings are different from what {@link #fromLabel} understands.
+ * <p>The {@link String} must have one of 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>
+ *
+ * <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 "//...").
+ *
+ * <p>Throws {@link InvalidPackageSpecificationException} if the {@link String} cannot be parsed.
*/
- public static PackageSpecification fromString(Label context, final String spec)
+ public static PackageSpecification fromString(RepositoryName repositoryName, String spec)
throws InvalidPackageSpecificationException {
String result = spec;
boolean allBeneath = false;
@@ -80,14 +83,14 @@
allBeneath = true;
result = result.substring(0, result.length() - ALL_BENEATH_SUFFIX.length());
if (result.equals("/")) {
- // Special case: //... will not end in /...
- return EVERYTHING;
+ // spec was "//...".
+ return AllPackages.EVERYTHING;
}
}
if (!spec.startsWith("//")) {
- throw new InvalidPackageSpecificationException("invalid package name '" + spec
- + "': must start with '//'");
+ throw new InvalidPackageSpecificationException(
+ String.format("invalid package name '%s': must start with '//'", spec));
}
PackageIdentifier packageId;
@@ -95,24 +98,33 @@
packageId = PackageIdentifier.parse(result);
} catch (LabelSyntaxException e) {
throw new InvalidPackageSpecificationException(
- "invalid package name '" + spec + "': " + e.getMessage());
+ String.format("invalid package name '%s': %s", spec, e.getMessage()));
}
-
Verify.verify(packageId.getRepository().isDefault());
- packageId = PackageIdentifier.create(
- context.getPackageIdentifier().getRepository(), packageId.getPackageFragment());
- return allBeneath ?
- new AllPackagesBeneath(packageId) :
- new SinglePackage(packageId);
+ PackageIdentifier packageIdForSpecifiedRepository =
+ PackageIdentifier.create(repositoryName, packageId.getPackageFragment());
+ return allBeneath
+ ? new AllPackagesBeneath(packageIdForSpecifiedRepository)
+ : new SinglePackage(packageIdForSpecifiedRepository);
}
/**
- * Parses a label as a visibility specification. returns null if the label cannot be parsed.
+ * Parses the provided {@link Label} into a {@link PackageSpecification} specific to the {@link
+ * RepositoryName} associated with the label.
*
- * <p>Note that these strings are different from what {@link #fromString} understands.
+ * <p>If {@code label.getName.equals("__pkg__")} then this results in a {@link
+ * PackageSpecification} that contains exactly the named package.
+ *
+ * <p>If {@code label.getName.equals("__subpackages__")} then this results in a {@link
+ * PackageSpecification} that contains all transitive subpackages of that package, inclusive.
+ *
+ * <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, "//...")}).
*/
- public static PackageSpecification fromLabel(Label label) {
+ static PackageSpecification fromLabel(Label label) {
if (label.getName().equals(PACKAGE_LABEL)) {
return new SinglePackage(label.getPackageIdentifier());
} else if (label.getName().equals(SUBTREE_LABEL)) {
@@ -122,10 +134,14 @@
}
}
+ public static PackageSpecification everything() {
+ return AllPackages.EVERYTHING;
+ }
+
private static class SinglePackage extends PackageSpecification {
private PackageIdentifier singlePackageName;
- public SinglePackage(PackageIdentifier packageName) {
+ private SinglePackage(PackageIdentifier packageName) {
this.singlePackageName = packageName;
}
@@ -135,15 +151,37 @@
}
@Override
- protected String asString() {
+ public String toStringWithoutRepository() {
+ return "//" + singlePackageName.getPackageFragment().getPathString();
+ }
+
+ @Override
+ public String toString() {
return singlePackageName.toString();
}
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (!(o instanceof SinglePackage)) {
+ return false;
+ }
+ SinglePackage that = (SinglePackage) o;
+ return singlePackageName.equals(that.singlePackageName);
+ }
+
+ @Override
+ public int hashCode() {
+ return singlePackageName.hashCode();
+ }
}
private static class AllPackagesBeneath extends PackageSpecification {
private PackageIdentifier prefix;
- public AllPackagesBeneath(PackageIdentifier prefix) {
+ private AllPackagesBeneath(PackageIdentifier prefix) {
this.prefix = prefix;
}
@@ -154,16 +192,69 @@
}
@Override
- protected String asString() {
- return prefix.equals(new PathFragment("")) ? "..." : prefix + "/...";
+ public String toStringWithoutRepository() {
+ return "//" + prefix.getPackageFragment().getPathString() + ALL_BENEATH_SUFFIX;
+ }
+
+ @Override
+ public String toString() {
+ if (prefix.getPackageFragment().equals(PathFragment.EMPTY_FRAGMENT)) {
+ return "//...";
+ }
+ return prefix + "/...";
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (!(o instanceof AllPackagesBeneath)) {
+ return false;
+ }
+ AllPackagesBeneath that = (AllPackagesBeneath) o;
+ return prefix.equals(that.prefix);
+ }
+
+ @Override
+ public int hashCode() {
+ return prefix.hashCode();
}
}
- /**
- * Exception class to be thrown when a specification cannot be parsed.
- */
- public static class InvalidPackageSpecificationException extends Exception {
- public InvalidPackageSpecificationException(String message) {
+ private static class AllPackages extends PackageSpecification {
+
+ private static final PackageSpecification EVERYTHING = new AllPackages();
+
+ @Override
+ public boolean containsPackage(PackageIdentifier packageName) {
+ return true;
+ }
+
+ @Override
+ public String toStringWithoutRepository() {
+ return "//...";
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ return o instanceof AllPackages;
+ }
+
+ @Override
+ public int hashCode() {
+ return "//...".hashCode();
+ }
+
+ @Override
+ public String toString() {
+ return "//...";
+ }
+ }
+
+ /** Exception class to be thrown when a specification cannot be parsed. */
+ static class InvalidPackageSpecificationException extends Exception {
+ private InvalidPackageSpecificationException(String message) {
super(message);
}
}
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 8f993ab..404b7aa 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
@@ -16,18 +16,17 @@
import static org.junit.Assert.assertFalse;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.events.util.EventCollectionApparatus;
import com.google.devtools.build.lib.packages.util.PackageFactoryApparatus;
import com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
-
+import java.util.concurrent.SynchronousQueue;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-import java.util.concurrent.SynchronousQueue;
-
/**
* Checks against a class initialization deadlock. "query sometimes hangs".
*
@@ -51,8 +50,11 @@
@Override
public void run() {
try {
- groupQueue.put(PackageSpecification.fromString(
- Label.parseAbsoluteUnchecked("//context"), "//fruits/..."));
+ RepositoryName defaultRepoName =
+ Label.parseAbsoluteUnchecked("//context")
+ .getPackageIdentifier()
+ .getRepository();
+ groupQueue.put(PackageSpecification.fromString(defaultRepoName, "//fruits/..."));
} 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 24002f2..50ca268 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
@@ -23,7 +23,6 @@
import com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
-
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
@@ -151,7 +150,7 @@
scratch.file("fruits/BUILD", "package_group(name = 'mango', packages = ['//...'])");
PackageGroup packageGroup = getPackageGroup("fruits", "mango");
assertThat(packageGroup.getPackageSpecifications())
- .containsExactlyElementsIn(ImmutableList.of(PackageSpecification.EVERYTHING));
+ .containsExactlyElementsIn(ImmutableList.of(PackageSpecification.everything()));
}
private Package getPackage(String packageName) throws Exception {
