Make globs work in remote repositories.

This involved quite a few changes, mainly changing a bunch of places where we refer to packages by a PathFragment to PackageIdentifier. 

The only wart is the code in PathPackageLocator: ideally, it would just call into PackageLookupFunction. Unfortunately, it is (through globbing and Parser.include) called from within a Skyframe function, and we don't want to have two eval() calls going on at the same time, so we cannot use that.

There is a potential correctness issue there: PathPackageLocator now assumes where external repositories are put and assumes that they are there when it gets control, but my understanding is that the associated RepositoryValue is always evaluated before, so it works out okay.

--
MOS_MIGRATED_REVID=97751539

9 files changed, 41 insertions, 33 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
index 9ea62103fe..ac5b6a2f1f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/EnvironmentBackedRecursivePackageProvider.java
@@ -64,9 +64,9 @@ public final class EnvironmentBackedRecursivePackageProvider implements Recursiv
   }
 
   @Override
-  public boolean isPackage(EventHandler eventHandler, String packageName)
+  public boolean isPackage(EventHandler eventHandler, PackageIdentifier packageId)
       throws MissingDepException {
-    SkyKey packageLookupKey = PackageLookupValue.key(new PathFragment(packageName));
+    SkyKey packageLookupKey = PackageLookupValue.key(packageId);
     try {
       PackageLookupValue packageLookupValue =
           (PackageLookupValue) env.getValueOrThrow(packageLookupKey, NoSuchPackageException.class,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
index d1ee81cb07..942a201770 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java
@@ -18,6 +18,7 @@ import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.packages.PackageIdentifier;
 import com.google.devtools.build.lib.vfs.Dirent;
 import com.google.devtools.build.lib.vfs.Dirent.Type;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -60,8 +61,9 @@ final class GlobFunction implements SkyFunction {
     PathFragment globSubdir = glob.getSubdir();
     if (!globSubdir.equals(PathFragment.EMPTY_FRAGMENT)) {
       PackageLookupValue globSubdirPkgLookupValue = (PackageLookupValue) env.getValue(
-          PackageLookupValue.key(glob.getPackageId().getPackageFragment()
-              .getRelative(globSubdir)));
+          PackageLookupValue.key(new PackageIdentifier(
+              glob.getPackageId().getRepository(),
+              glob.getPackageId().getPackageFragment().getRelative(globSubdir))));
       if (globSubdirPkgLookupValue == null) {
         return null;
       }
@@ -220,7 +222,8 @@ final class GlobFunction implements SkyFunction {
         PathFragment directory = glob.getPackageId().getPackageFragment()
             .getRelative(glob.getSubdir()).getRelative(fileName);
         PackageLookupValue pkgLookupValue = (PackageLookupValue)
-            env.getValue(PackageLookupValue.key(directory));
+            env.getValue(PackageLookupValue.key(new PackageIdentifier(
+                glob.getPackageId().getRepository(), directory)));
         if (pkgLookupValue == null) {
           return;
         }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
index c4e8c490af..c029a1ddc1 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GraphBackedRecursivePackageProvider.java
@@ -79,8 +79,8 @@ public final class GraphBackedRecursivePackageProvider implements RecursivePacka
   }
 
   @Override
-  public boolean isPackage(EventHandler eventHandler, String packageName) {
-    SkyKey packageLookupKey = PackageLookupValue.key(new PathFragment(packageName));
+  public boolean isPackage(EventHandler eventHandler, PackageIdentifier packageName) {
+    SkyKey packageLookupKey = PackageLookupValue.key(packageName);
     if (!graph.exists(packageLookupKey)) {
       // If the package lookup key does not exist in the graph, then it must not correspond to any
       // package, because the SkyQuery environment has already loaded the universe.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
index db3e7c18bb..2c8cc8e83c 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupFunction.java
@@ -41,9 +41,9 @@ import javax.annotation.Nullable;
  */
 class PackageLookupFunction implements SkyFunction {
 
-  private final AtomicReference<ImmutableSet<String>> deletedPackages;
+  private final AtomicReference<ImmutableSet<PackageIdentifier>> deletedPackages;
 
-  PackageLookupFunction(AtomicReference<ImmutableSet<String>> deletedPackages) {
+  PackageLookupFunction(AtomicReference<ImmutableSet<PackageIdentifier>> deletedPackages) {
     this.deletedPackages = deletedPackages;
   }
 
@@ -64,7 +64,7 @@ class PackageLookupFunction implements SkyFunction {
           + packageNameErrorMsg);
     }
 
-    if (deletedPackages.get().contains(packageKey.getPackageFragment().toString())) {
+    if (deletedPackages.get().contains(packageKey)) {
       return PackageLookupValue.deletedPackage();
     }
 
@@ -142,7 +142,7 @@ class PackageLookupFunction implements SkyFunction {
       throws PackageLookupFunctionException {
     PackageIdentifier id = (PackageIdentifier) skyKey.argument();
     SkyKey repositoryKey = RepositoryValue.key(id.getRepository());
-    RepositoryValue repositoryValue = null;
+    RepositoryValue repositoryValue;
     try {
       repositoryValue = (RepositoryValue) env.getValueOrThrow(
           repositoryKey, NoSuchPackageException.class, IOException.class, EvalException.class);
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 5230d09779..6fbfe574b1 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
@@ -193,7 +193,10 @@ public class PrepareDepsOfPatternFunction implements SkyFunction {
 
     @Override
     public boolean isPackage(String packageName) {
-      return packageProvider.isPackage(env.getListener(), 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));
     }
 
     @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 942ed83fcb..b91362834e 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
@@ -132,7 +132,10 @@ public class RecursivePackageProviderBackedTargetPatternResolver
 
   @Override
   public boolean isPackage(String packageName) {
-    return recursivePackageProvider.isPackage(eventHandler, 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));
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
index ef22ab993b..a1ac8e2116 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SequencedSkyframeExecutor.java
@@ -211,11 +211,12 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
   }
 
   @Override
-  public void sync(PackageCacheOptions packageCacheOptions, Path workingDirectory,
+  public void sync(PackageCacheOptions packageCacheOptions, Path outputBase, Path workingDirectory,
                    String defaultsPackageContents, UUID commandId)
       throws InterruptedException, AbruptExitException {
     this.valueCacheEvictionLimit = packageCacheOptions.minLoadedPkgCountForCtNodeEviction;
-    super.sync(packageCacheOptions, workingDirectory, defaultsPackageContents, commandId);
+    super.sync(
+        packageCacheOptions, outputBase, workingDirectory, defaultsPackageContents, commandId);
     handleDiffs();
   }
 
@@ -243,11 +244,10 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
     recordingDiffer.invalidate(Iterables.filter(memoizingEvaluator.getValues().keySet(), pred));
   }
 
-  private void invalidateDeletedPackages(Iterable<String> deletedPackages) {
+  private void invalidateDeletedPackages(Iterable<PackageIdentifier> deletedPackages) {
     ArrayList<SkyKey> packagesToInvalidate = Lists.newArrayList();
-    for (String deletedPackage : deletedPackages) {
-      PathFragment pathFragment = new PathFragment(deletedPackage);
-      packagesToInvalidate.add(PackageLookupValue.key(pathFragment));
+    for (PackageIdentifier deletedPackage : deletedPackages) {
+      packagesToInvalidate.add(PackageLookupValue.key(deletedPackage));
     }
     recordingDiffer.invalidate(packagesToInvalidate);
   }
@@ -257,7 +257,7 @@ public final class SequencedSkyframeExecutor extends SkyframeExecutor {
    */
   @Override
   @VisibleForTesting  // productionVisibility = Visibility.PRIVATE
-  public void setDeletedPackages(Iterable<String> pkgs) {
+  public void setDeletedPackages(Iterable<PackageIdentifier> pkgs) {
     // Invalidate the old deletedPackages as they may exist now.
     invalidateDeletedPackages(deletedPackages.get());
     deletedPackages.set(ImmutableSet.copyOf(pkgs));
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 96c39ef9f5..601d7ec6bd 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -180,8 +180,8 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       new AtomicReference<>(UnixGlob.DEFAULT_SYSCALLS);
   protected final AtomicReference<PathPackageLocator> pkgLocator =
       new AtomicReference<>();
-  protected final AtomicReference<ImmutableSet<String>> deletedPackages =
-      new AtomicReference<>(ImmutableSet.<String>of());
+  protected final AtomicReference<ImmutableSet<PackageIdentifier>> deletedPackages =
+      new AtomicReference<>(ImmutableSet.<PackageIdentifier>of());
   private final AtomicReference<EventBus> eventBus = new AtomicReference<>();
 
   private final ImmutableList<BuildInfoFactory> buildInfoFactories;
@@ -753,7 +753,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
    * Sets the packages that should be treated as deleted and ignored.
    */
   @VisibleForTesting  // productionVisibility = Visibility.PRIVATE
-  public abstract void setDeletedPackages(Iterable<String> pkgs);
+  public abstract void setDeletedPackages(Iterable<PackageIdentifier> pkgs);
 
   /**
    * Prepares the evaluator for loading.
@@ -1299,7 +1299,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     /**
      * Returns whether the given package should be consider deleted and thus should be ignored.
      */
-    public boolean isPackageDeleted(String packageName) {
+    public boolean isPackageDeleted(PackageIdentifier packageName) {
       return deletedPackages.get().contains(packageName);
     }
 
@@ -1349,12 +1349,13 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   @ThreadCompatible
   public abstract void updateLoadedPackageSet(Set<PackageIdentifier> loadedPackages);
 
-  public void sync(PackageCacheOptions packageCacheOptions, Path workingDirectory,
+  public void sync(PackageCacheOptions packageCacheOptions, Path outputBase, Path workingDirectory,
       String defaultsPackageContents, UUID commandId) throws InterruptedException,
       AbruptExitException{
 
     preparePackageLoading(
-        createPackageLocator(packageCacheOptions, directories.getWorkspace(), workingDirectory),
+        createPackageLocator(
+            packageCacheOptions, outputBase, directories.getWorkspace(), workingDirectory),
         packageCacheOptions.defaultVisibility, packageCacheOptions.showLoadingProgress,
         packageCacheOptions.globbingThreads, defaultsPackageContents, commandId);
     setDeletedPackages(ImmutableSet.copyOf(packageCacheOptions.deletedPackages));
@@ -1364,9 +1365,9 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   }
 
   protected PathPackageLocator createPackageLocator(PackageCacheOptions packageCacheOptions,
-      Path workspace, Path workingDirectory) throws AbruptExitException{
+      Path outputBase, Path workspace, Path workingDirectory) throws AbruptExitException {
     return PathPackageLocator.create(
-        packageCacheOptions.packagePath, getReporter(), workspace, workingDirectory);
+        outputBase, packageCacheOptions.packagePath, getReporter(), workspace, workingDirectory);
   }
 
   private CyclesReporter createCyclesReporter() {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
index 3e9b6cad1f..7ffb79ce89 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframePackageManager.java
@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
-import com.google.devtools.build.lib.cmdline.LabelValidator;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
@@ -136,7 +135,7 @@ class SkyframePackageManager implements PackageManager {
   }
 
   @Override
-  public boolean isPackage(EventHandler eventHandler, String packageName) {
+  public boolean isPackage(EventHandler eventHandler, PackageIdentifier packageName) {
     return getBuildFileForPackage(packageName) != null;
   }
 
@@ -147,11 +146,10 @@ class SkyframePackageManager implements PackageManager {
 
   @ThreadSafe
   @Override
-  public Path getBuildFileForPackage(String packageName) {
+  public Path getBuildFileForPackage(PackageIdentifier packageName) {
     // Note that this method needs to be thread-safe, as it is currently used concurrently by
     // legacy blaze code.
-    if (packageLoader.isPackageDeleted(packageName)
-        || LabelValidator.validatePackageName(packageName) != null) {
+    if (packageLoader.isPackageDeleted(packageName)) {
       return null;
     }
     // TODO(bazel-team): Use a PackageLookupValue here [skyframe-loading]