Make package names in the package_group.packages attribute refer to the repository where the package group is.

There is currently no way to refer to packages in other repositories and that doesn't seem to be useful, because visibility currently checks the repository name in the label and that can be changed in the main WORKSPACE file. If needed, it'd be pretty easy to implement, though.

As a drive-by fix, made the parsing of the package name call into the same logic implemented in the cmdline package because code duplication is bad, mmmkay?

Fixes #767.

--
MOS_MIGRATED_REVID=112032508

5 files changed, 48 insertions, 41 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index cc9951458f..0282489405 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -1215,7 +1215,7 @@ public final class RuleContext extends TargetContext
     // Check visibility attribute
     for (PackageSpecification specification :
       prerequisite.getProvider(VisibilityProvider.class).getVisibility()) {
-      if (specification.containsPackage(rule.getLabel().getPackageFragment())) {
+      if (specification.containsPackage(rule.getLabel().getPackageIdentifier())) {
         return true;
       }
     }
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
index 64851d6e6b..13f795e160 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/PackageIdentifier.java
@@ -301,7 +301,7 @@ public final class PackageIdentifier implements Comparable<PackageIdentifier>, S
       repo = input.substring(0, packageStartPos);
       packageName = input.substring(packageStartPos + 2);
     } else if (input.startsWith("@")) {
-      throw new LabelSyntaxException("invalid package name '" + input + "'");
+      throw new LabelSyntaxException("starts with a '@' but does not contain '//'");
     } else if (packageStartPos == 0) {
       repo = PackageIdentifier.DEFAULT_REPOSITORY;
       packageName = input.substring(2);
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 3695e367d4..67ea4edb4d 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
@@ -51,7 +51,7 @@ public class PackageGroup implements Target {
     for (String containedPackage : packages) {
       PackageSpecification specification = null;
       try {
-        specification = PackageSpecification.fromString(containedPackage);
+        specification = PackageSpecification.fromString(label, containedPackage);
       } catch (PackageSpecification.InvalidPackageSpecificationException e) {
         containsErrors = true;
         eventHandler.handle(Event.error(location, e.getMessage()));
@@ -74,7 +74,7 @@ public class PackageGroup implements Target {
 
   public boolean contains(Package pkg) {
     for (PackageSpecification specification : packageSpecifications) {
-      if (specification.containsPackage(pkg.getNameFragment())) {
+      if (specification.containsPackage(pkg.getPackageIdentifier())) {
         return true;
       }
     }
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 c81123a88e..fb2073e600 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
@@ -13,8 +13,10 @@
 // limitations under the License.
 package com.google.devtools.build.lib.packages;
 
+import com.google.common.base.Verify;
 import com.google.devtools.build.lib.cmdline.Label;
-import com.google.devtools.build.lib.cmdline.LabelValidator;
+import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
 /**
@@ -24,10 +26,14 @@ 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 AllPackagesBeneath(new PathFragment(""));
+  public static final PackageSpecification EVERYTHING = new PackageSpecification() {
+    @Override
+    public boolean containsPackage(PackageIdentifier packageName) {
+      return true;
+    }
+  };
 
-  public abstract boolean containsPackage(PathFragment packageName);
+  public abstract boolean containsPackage(PackageIdentifier packageName);
 
   @Override
   public int hashCode() {
@@ -53,39 +59,39 @@ public abstract class PackageSpecification {
    *
    * <p>Note that these strings are different from what {@link #fromLabel} understands.
    */
-  public static PackageSpecification fromString(final String spec)
+  public static PackageSpecification fromString(Label context, final String spec)
       throws InvalidPackageSpecificationException {
     String result = spec;
     boolean allBeneath = false;
-
-    if (result.startsWith("//")) {
-      result = spec.substring(2);
-    } else {
-      throw new InvalidPackageSpecificationException("invalid package label: " + spec);
+    if (result.endsWith(ALL_BENEATH_SUFFIX)) {
+      allBeneath = true;
+      result = result.substring(0, result.length() - ALL_BENEATH_SUFFIX.length());
+      if (result.equals("/")) {
+        // Special case: //... will not end in /...
+        return EVERYTHING;
+      }
     }
 
-    if (result.indexOf(':') >= 0) {
-      throw new InvalidPackageSpecificationException("invalid package label: " + spec);
+    if (!spec.startsWith("//")) {
+      throw new InvalidPackageSpecificationException("invalid package name '" + spec
+          + "': must start with '//'");
     }
 
-    if (result.equals("...")) {
-      // Special case: //... will not end in /...
-      return EVERYTHING;
+    PackageIdentifier packageId;
+    try {
+      packageId = PackageIdentifier.parse(result);
+    } catch (LabelSyntaxException e) {
+      throw new InvalidPackageSpecificationException(
+          "invalid package name '" + spec + "': " + e.getMessage());
     }
 
-    if (result.endsWith(ALL_BENEATH_SUFFIX)) {
-      allBeneath = true;
-      result = result.substring(0, result.length() - ALL_BENEATH_SUFFIX.length());
-    }
+    Verify.verify(packageId.getRepository().isDefault());
+    packageId = PackageIdentifier.create(
+        context.getPackageIdentifier().getRepository(), packageId.getPackageFragment());
 
-    String errorMessage = LabelValidator.validatePackageName(result);
-    if (errorMessage == null) {
-      return allBeneath ?
-          new AllPackagesBeneath(new PathFragment(result)) :
-          new SinglePackage(new PathFragment(result));
-    } else {
-      throw new InvalidPackageSpecificationException(errorMessage);
-    }
+    return allBeneath ?
+        new AllPackagesBeneath(packageId) :
+        new SinglePackage(packageId);
   }
 
   /**
@@ -95,23 +101,23 @@ public abstract class PackageSpecification {
    */
   public static PackageSpecification fromLabel(Label label) {
     if (label.getName().equals(PACKAGE_LABEL)) {
-      return new SinglePackage(label.getPackageFragment());
+      return new SinglePackage(label.getPackageIdentifier());
     } else if (label.getName().equals(SUBTREE_LABEL)) {
-      return new AllPackagesBeneath(label.getPackageFragment());
+      return new AllPackagesBeneath(label.getPackageIdentifier());
     } else {
       return null;
     }
   }
 
   private static class SinglePackage extends PackageSpecification {
-    private PathFragment singlePackageName;
+    private PackageIdentifier singlePackageName;
 
-    public SinglePackage(PathFragment packageName) {
+    public SinglePackage(PackageIdentifier packageName) {
       this.singlePackageName = packageName;
     }
 
     @Override
-    public boolean containsPackage(PathFragment packageName) {
+    public boolean containsPackage(PackageIdentifier packageName) {
       return this.singlePackageName.equals(packageName);
     }
 
@@ -122,15 +128,16 @@ public abstract class PackageSpecification {
   }
 
   private static class AllPackagesBeneath extends PackageSpecification {
-    private PathFragment prefix;
+    private PackageIdentifier prefix;
 
-    public AllPackagesBeneath(PathFragment prefix) {
+    public AllPackagesBeneath(PackageIdentifier prefix) {
       this.prefix = prefix;
     }
 
     @Override
-    public boolean containsPackage(PathFragment packageName) {
-      return packageName.startsWith(prefix);
+    public boolean containsPackage(PackageIdentifier packageName) {
+      return packageName.getRepository().equals(prefix.getRepository())
+          && packageName.getPackageFragment().startsWith(prefix.getPackageFragment());
     }
 
     @Override
diff --git a/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryVisibility.java b/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryVisibility.java
index 92be0321cf..427ea8483b 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryVisibility.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/BlazeQueryVisibility.java
@@ -29,7 +29,7 @@ public class BlazeQueryVisibility extends QueryVisibility<Target> {
 
   @Override
   public boolean contains(Target target) {
-    return packageSpecification.containsPackage(target.getPackage().getNameFragment());
+    return packageSpecification.containsPackage(target.getLabel().getPackageIdentifier());
   }
 
   @Override