Make TargetPatternResolver#{isPackage, getTargetsInPackage} take a PackageIdentifier instead of a String.

This remarkably fiddly CL is a step towards making wildcards pattern work with remote repositories. I originally wanted to refactor findTargetsBeneathDirectory(), too, but it turns out that it's a much more complicated affair.

--
MOS_MIGRATED_REVID=103622420

5 files changed, 40 insertions, 35 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
index b127009d91..6dc23857ad 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPattern.java
@@ -249,7 +249,7 @@ public abstract class TargetPattern implements Serializable {
       Preconditions.checkArgument(excludedSubdirectories.isEmpty(),
           "Target pattern \"%s\" of type %s cannot be evaluated with excluded subdirectories: %s.",
           getOriginalPattern(), getType(), excludedSubdirectories);
-      if (resolver.isPackage(path)) {
+      if (resolver.isPackage(PackageIdentifier.createInDefaultRepo(path))) {
         // User has specified a package name. lookout for default target.
         return resolver.getExplicitTarget("//" + path);
       }
@@ -260,7 +260,7 @@ public abstract class TargetPattern implements Serializable {
       // first BUILD file is found (i.e. longest prefix match).
       for (int i = pieces.size() - 1; i > 0; i--) {
         String packageName = SLASH_JOINER.join(pieces.subList(0, i));
-        if (resolver.isPackage(packageName)) {
+        if (resolver.isPackage(PackageIdentifier.createInDefaultRepo(packageName))) {
           String targetName = SLASH_JOINER.join(pieces.subList(i, pieces.size()));
           return resolver.getExplicitTarget("//" + packageName + ":" + targetName);
         }
@@ -334,8 +334,18 @@ public abstract class TargetPattern implements Serializable {
           return targets;
         }
       }
-      return resolver.getTargetsInPackage(getOriginalPattern(), removeSuffix(pattern, suffix),
-          rulesOnly);
+
+      String packageName = removeSuffix(pattern, suffix);
+
+      try {
+        PackageIdentifier.parse(packageName);
+      } catch (LabelSyntaxException e) {
+        throw new TargetParsingException(
+            "Invalid package name '" + packageName + "': " + e.getMessage());
+      }
+
+      return resolver.getTargetsInPackage(getOriginalPattern(),
+          PackageIdentifier.createInDefaultRepo(packageName), rulesOnly);
     }
 
     @Override
diff --git a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java
index 4234ad5e10..d106692d9f 100644
--- a/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/cmdline/TargetPatternResolver.java
@@ -49,11 +49,12 @@ public interface TargetPatternResolver<T> {
    * only return rules in the given package.
    *
    * @param originalPattern the original target pattern for error reporting purposes
-   * @param packageName the name of the package
+   * @param packageIdentifier the identifier of the package
    * @param rulesOnly whether to return rules only
    */
-  ResolvedTargets<T> getTargetsInPackage(String originalPattern, String packageName,
-      boolean rulesOnly) throws TargetParsingException, InterruptedException;
+  ResolvedTargets<T> getTargetsInPackage(String originalPattern,
+      PackageIdentifier packageIdentifier, boolean rulesOnly)
+      throws TargetParsingException, InterruptedException;
 
   /**
    * Returns the set containing the targets found below the given {@code directory}. Conceptually,
@@ -85,7 +86,7 @@ public interface TargetPatternResolver<T> {
    * Returns true, if and only if the given name corresponds to a package, i.e., a file with the
    * name {@code packageName/BUILD} exists.
    */
-  boolean isPackage(String packageName);
+  boolean isPackage(PackageIdentifier packageIdentifier);
 
   /**
    * Returns the target kind of the given target, for example {@code cc_library rule}.
diff --git a/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternResolverUtil.java b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternResolverUtil.java
index 49b7548404..60a0d64f8a 100644
--- a/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternResolverUtil.java
+++ b/src/main/java/com/google/devtools/build/lib/pkgcache/TargetPatternResolverUtil.java
@@ -17,6 +17,7 @@ import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.cmdline.LabelValidator;
+import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.ResolvedTargets;
 import com.google.devtools.build.lib.cmdline.TargetParsingException;
 import com.google.devtools.build.lib.cmdline.TargetPatternResolver;
@@ -83,7 +84,7 @@ public final class TargetPatternResolverUtil {
     if (LabelValidator.validatePackageName(packageName) != null) {
       throw new TargetParsingException("'" + packageName + "' is not a valid package name");
     }
-    if (!resolver.isPackage(packageName)) {
+    if (!resolver.isPackage(PackageIdentifier.createInDefaultRepo(packageName))) {
       throw new TargetParsingException(
           TargetPatternResolverUtil.getParsingErrorMessage(
               "no such package '" + packageName + "': BUILD file not found on package path",
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
index b116595dae..a48117f757 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrepareDepsOfPatternFunction.java
@@ -160,20 +160,18 @@ public class PrepareDepsOfPatternFunction implements SkyFunction {
     }
 
     @Override
-    public ResolvedTargets<Void> getTargetsInPackage(String originalPattern, String packageName,
-        boolean rulesOnly) throws TargetParsingException {
+    public ResolvedTargets<Void> getTargetsInPackage(String originalPattern,
+        PackageIdentifier packageIdentifier, boolean rulesOnly) throws TargetParsingException {
       FilteringPolicy policy =
           rulesOnly ? FilteringPolicies.RULES_ONLY : FilteringPolicies.NO_FILTER;
-      return getTargetsInPackage(originalPattern, new PathFragment(packageName), policy);
+      return getTargetsInPackage(originalPattern, packageIdentifier, policy);
     }
 
     private ResolvedTargets<Void> getTargetsInPackage(String originalPattern,
-        PathFragment packageNameFragment, FilteringPolicy policy)
+        PackageIdentifier packageIdentifier, FilteringPolicy policy)
         throws TargetParsingException {
-      TargetPatternResolverUtil.validatePatternPackage(originalPattern, packageNameFragment, this);
       try {
-        PackageIdentifier packageId = PackageIdentifier.createInDefaultRepo(packageNameFragment);
-        Package pkg = packageProvider.getPackage(env.getListener(), packageId);
+        Package pkg = packageProvider.getPackage(env.getListener(), packageIdentifier);
         ResolvedTargets<Target> packageTargets =
             TargetPatternResolverUtil.resolvePackageTargets(pkg, policy);
         ImmutableList.Builder<SkyKey> builder = ImmutableList.builder();
@@ -194,11 +192,8 @@ public class PrepareDepsOfPatternFunction implements SkyFunction {
     }
 
     @Override
-    public boolean isPackage(String packageName) {
-      // TODO(bazel-team): this should get the whole PackageIdentifier. Using only the package name
-      // makes it impossible to use wildcards to refer to targets in remote repositories.
-      return packageProvider.isPackage(env.getListener(),
-          PackageIdentifier.createInDefaultRepo(packageName));
+    public boolean isPackage(PackageIdentifier packageIdentifier) {
+      return packageProvider.isPackage(env.getListener(), packageIdentifier);
     }
 
     @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
index 69cf3baf70..147a4e7046 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursivePackageProviderBackedTargetPatternResolver.java
@@ -76,7 +76,7 @@ public class RecursivePackageProviderBackedTargetPatternResolver
   public Target getTargetOrNull(String targetName) throws InterruptedException {
     try {
       Label label = Label.parseAbsolute(targetName);
-      if (!isPackage(label.getPackageName())) {
+      if (!isPackage(label.getPackageIdentifier())) {
         return null;
       }
       return recursivePackageProvider.getTarget(eventHandler, label);
@@ -100,35 +100,31 @@ public class RecursivePackageProviderBackedTargetPatternResolver
   }
 
   @Override
-  public ResolvedTargets<Target> getTargetsInPackage(String originalPattern, String packageName,
-                                                     boolean rulesOnly)
+  public ResolvedTargets<Target> getTargetsInPackage(
+      String originalPattern, PackageIdentifier packageIdentifier, boolean rulesOnly)
       throws TargetParsingException, InterruptedException {
     FilteringPolicy actualPolicy = rulesOnly
         ? FilteringPolicies.and(FilteringPolicies.RULES_ONLY, policy)
         : policy;
-    return getTargetsInPackage(originalPattern, new PathFragment(packageName), actualPolicy);
+    return getTargetsInPackage(originalPattern, packageIdentifier, actualPolicy);
   }
 
   private ResolvedTargets<Target> getTargetsInPackage(String originalPattern,
-      PathFragment packageNameFragment, FilteringPolicy policy)
+      PackageIdentifier packageIdentifier, FilteringPolicy policy)
       throws TargetParsingException, InterruptedException {
-    TargetPatternResolverUtil.validatePatternPackage(originalPattern, packageNameFragment, this);
     try {
-      Package pkg = getPackage(PackageIdentifier.createInDefaultRepo(packageNameFragment));
+      Package pkg = getPackage(packageIdentifier);
       return TargetPatternResolverUtil.resolvePackageTargets(pkg, policy);
     } catch (NoSuchThingException e) {
       String message = TargetPatternResolverUtil.getParsingErrorMessage(
-          "package contains errors", originalPattern);
+          e.getMessage(), originalPattern);
       throw new TargetParsingException(message, e);
     }
   }
 
   @Override
-  public boolean isPackage(String packageName) {
-    // TODO(bazel-team): this should get the whole PackageIdentifier. Using only the package name
-    // makes it impossible to use the //... wildcard to refer to targets in remote repositories.
-    return recursivePackageProvider.isPackage(
-        eventHandler, PackageIdentifier.createInDefaultRepo(packageName));
+  public boolean isPackage(PackageIdentifier packageIdentifier) {
+    return recursivePackageProvider.isPackage(eventHandler, packageIdentifier);
   }
 
   @Override
@@ -156,7 +152,9 @@ public class RecursivePackageProviderBackedTargetPatternResolver
           recursivePackageProvider.getPackagesUnderDirectory(
               repository, rootedPath, excludedPathFragments);
       for (PathFragment pkg : packagesUnderDirectory) {
-        targetBuilder.merge(getTargetsInPackage(originalPattern, pkg, FilteringPolicies.NO_FILTER));
+        targetBuilder.merge(getTargetsInPackage(originalPattern,
+            PackageIdentifier.createInDefaultRepo(pkg),
+            FilteringPolicies.NO_FILTER));
       }
     }