Use a non-side-effectful PackageLookupValue#packageExists implementation for //external package lookups, but keep the current (incorrect) semantics for unsuccessful //external package lookups.

Refactor some users of the old WorkspacePackageLookupValue.

--
MOS_MIGRATED_REVID=123034174

3 files changed, 57 insertions, 96 deletions

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 83e992c1a3..2400de7ee0 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
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelValidator;
@@ -57,7 +58,7 @@ public class PackageLookupFunction implements SkyFunction {
     if (!packageKey.getRepository().isMain()) {
       return computeExternalPackageLookupValue(skyKey, env, packageKey);
     } else if (packageKey.equals(Label.EXTERNAL_PACKAGE_IDENTIFIER)) {
-      return computeWorkspaceLookupValue(env, packageKey);
+      return computeWorkspacePackageLookupValue(env, pkgLocator.getPathEntries());
     }
 
     String packageNameErrorMsg = LabelValidator.validatePackageName(
@@ -84,17 +85,8 @@ public class PackageLookupFunction implements SkyFunction {
       }
     }
 
-    // TODO(bazel-team): The following is O(n^2) on the number of elements on the package path due
-    // to having restart the SkyFunction after every new dependency. However, if we try to batch
-    // the missing value keys, more dependencies than necessary will be declared. This wart can be
-    // fixed once we have nicer continuation support [skyframe-loading]
-    for (Path packagePathEntry : pkgLocator.getPathEntries()) {
-      PackageLookupValue value = getPackageLookupValue(env, packagePathEntry, packageKey);
-      if (value == null || value.packageExists()) {
-        return value;
-      }
-    }
-    return PackageLookupValue.NO_BUILD_FILE_VALUE;
+    return getPackageLookupValue(env, pkgLocator.getPathEntries(), packageKey,
+        packageKey.getPackageFragment().getChild("BUILD"));
   }
 
   @Nullable
@@ -105,10 +97,10 @@ public class PackageLookupFunction implements SkyFunction {
 
   @Nullable
   private FileValue getFileValue(
-      RootedPath buildFileRootedPath, Environment env, PackageIdentifier packageIdentifier)
+      RootedPath fileRootedPath, Environment env, PackageIdentifier packageIdentifier)
       throws PackageLookupFunctionException {
-    String basename = buildFileRootedPath.asPath().getBaseName();
-    SkyKey fileSkyKey = FileValue.key(buildFileRootedPath);
+    String basename = fileRootedPath.asPath().getBaseName();
+    SkyKey fileSkyKey = FileValue.key(fileRootedPath);
     FileValue fileValue = null;
     try {
       fileValue = (FileValue) env.getValueOrThrow(fileSkyKey, IOException.class,
@@ -118,12 +110,12 @@ public class PackageLookupFunction implements SkyFunction {
       // BuildFileNotFoundException.
       throw new PackageLookupFunctionException(new BuildFileNotFoundException(packageIdentifier,
           "IO errors while looking for " + basename + " file reading "
-              + buildFileRootedPath.asPath() + ": " + e.getMessage(), e),
+              + fileRootedPath.asPath() + ": " + e.getMessage(), e),
           Transience.PERSISTENT);
     } catch (FileSymlinkException e) {
       throw new PackageLookupFunctionException(new BuildFileNotFoundException(packageIdentifier,
           "Symlink cycle detected while trying to find " + basename + " file "
-              + buildFileRootedPath.asPath()),
+              + fileRootedPath.asPath()),
           Transience.PERSISTENT);
     } catch (InconsistentFilesystemException e) {
       // This error is not transient from the perspective of the PackageLookupFunction.
@@ -132,19 +124,55 @@ public class PackageLookupFunction implements SkyFunction {
     return fileValue;
   }
 
-  private PackageLookupValue getPackageLookupValue(Environment env, Path packagePathEntry,
-      PackageIdentifier packageIdentifier) throws PackageLookupFunctionException {
-    PathFragment buildFileFragment = packageIdentifier.getPackageFragment().getChild("BUILD");
-    RootedPath buildFileRootedPath = RootedPath.toRootedPath(packagePathEntry,
-        buildFileFragment);
-    FileValue fileValue = getFileValue(buildFileRootedPath, env, packageIdentifier);
-    if (fileValue == null) {
+  private PackageLookupValue getPackageLookupValue(Environment env,
+      ImmutableList<Path> packagePathEntries, PackageIdentifier packageIdentifier,
+      PathFragment buildFileFragment) throws PackageLookupFunctionException {
+    // TODO(bazel-team): The following is O(n^2) on the number of elements on the package path due
+    // to having restart the SkyFunction after every new dependency. However, if we try to batch
+    // the missing value keys, more dependencies than necessary will be declared. This wart can be
+    // fixed once we have nicer continuation support [skyframe-loading]
+    for (Path packagePathEntry : packagePathEntries) {
+      RootedPath buildFileRootedPath = RootedPath.toRootedPath(packagePathEntry,
+          buildFileFragment);
+      FileValue fileValue = getFileValue(buildFileRootedPath, env, packageIdentifier);
+      if (fileValue == null) {
+        return null;
+      }
+      if (fileValue.isFile()) {
+        return PackageLookupValue.success(buildFileRootedPath.getRoot());
+      }
+    }
+    return PackageLookupValue.NO_BUILD_FILE_VALUE;
+  }
+
+  private PackageLookupValue computeWorkspacePackageLookupValue(Environment env,
+      ImmutableList<Path> packagePathEntries)
+          throws PackageLookupFunctionException{
+    PackageLookupValue result = getPackageLookupValue(
+        env, packagePathEntries, Label.EXTERNAL_PACKAGE_IDENTIFIER, new PathFragment("WORKSPACE"));
+    if (result == null) {
       return null;
     }
-    if (fileValue.isFile()) {
-      return PackageLookupValue.success(buildFileRootedPath.getRoot());
+    if (result.packageExists()) {
+      return result;
     }
-    return PackageLookupValue.NO_BUILD_FILE_VALUE;
+    // Fall back on the last package path entry if there were any and nothing else worked.
+    // TODO(kchodorow): get rid of this, the semantics are wrong (successful package lookup should
+    // mean the package exists). a bunch of tests need to be rewritten first though.
+    if (packagePathEntries.isEmpty()) {
+      return PackageLookupValue.NO_BUILD_FILE_VALUE;
+    }
+    Path lastPackagePath = packagePathEntries.get(packagePathEntries.size() - 1);
+    FileValue lastPackagePackagePathFileValue = getFileValue(
+        RootedPath.toRootedPath(lastPackagePath, PathFragment.EMPTY_FRAGMENT),
+        env,
+        Label.EXTERNAL_PACKAGE_IDENTIFIER);
+    if (lastPackagePackagePathFileValue == null) {
+      return null;
+    }
+    return lastPackagePackagePathFileValue.exists()
+        ? PackageLookupValue.success(lastPackagePath)
+        : PackageLookupValue.NO_BUILD_FILE_VALUE;
   }
 
   /**
@@ -178,39 +206,13 @@ public class PackageLookupFunction implements SkyFunction {
     }
 
     if (fileValue.isFile()) {
-        return PackageLookupValue.success(repositoryValue.getPath());
+      return PackageLookupValue.success(repositoryValue.getPath());
     }
 
     return PackageLookupValue.NO_BUILD_FILE_VALUE;
   }
 
   /**
-   * Look for a WORKSPACE file on each package path.  If none is found, use the last package path
-   * and pretend it was found there.
-   */
-  private SkyValue computeWorkspaceLookupValue(
-      Environment env, PackageIdentifier packageIdentifier)
-      throws PackageLookupFunctionException {
-    PathPackageLocator pkgLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env);
-    Path lastPackagePath = null;
-    for (Path packagePathEntry : pkgLocator.getPathEntries()) {
-      lastPackagePath = packagePathEntry;
-      RootedPath workspacePath = RootedPath.toRootedPath(
-          packagePathEntry, Label.EXTERNAL_PACKAGE_FILE_NAME);
-      FileValue value = getFileValue(workspacePath, env, packageIdentifier);
-      if (value == null) {
-        return null;
-      }
-      if (value.exists()) {
-        return PackageLookupValue.workspace(packagePathEntry);
-      }
-    }
-
-    // Fall back on the last package path entry if there were any and nothing else worked.
-    return PackageLookupValue.workspace(lastPackagePath);
-  }
-
-  /**
    * Used to declare all the exception types that can be wrapped in the exception thrown by
    * {@link PackageLookupFunction#compute}.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java
index e9d5bf5100..ce006cc3c5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageLookupValue.java
@@ -56,18 +56,10 @@ public abstract class PackageLookupValue implements SkyValue {
     return new SuccessfulPackageLookupValue(root);
   }
 
-  public static PackageLookupValue workspace(Path root) {
-    return new WorkspacePackageLookupValue(root);
-  }
-
   public static PackageLookupValue invalidPackageName(String errorMsg) {
     return new InvalidNamePackageLookupValue(errorMsg);
   }
 
-  public boolean isExternalPackage() {
-    return false;
-  }
-
   /**
    * For a successful package lookup, returns the root (package path entry) that the package
    * resides in.
@@ -145,28 +137,6 @@ public abstract class PackageLookupValue implements SkyValue {
     }
   }
 
-  // TODO(kchodorow): fix these semantics.  This class should not exist, WORKSPACE lookup should
-  // just return success/failure like a "normal" package.
-  /** Successful workspace package lookup value. */
-  public static class WorkspacePackageLookupValue extends SuccessfulPackageLookupValue {
-
-    private WorkspacePackageLookupValue(Path root) {
-      super(root);
-    }
-
-    // TODO(kchodorow): get rid of this, the semantics are wrong (successful package lookup should
-    // mean the package exists).
-    @Override
-    public boolean packageExists() {
-      return getRoot().exists();
-    }
-
-    @Override
-    public boolean isExternalPackage() {
-      return true;
-    }
-  }
-
   private abstract static class UnsuccessfulPackageLookupValue extends PackageLookupValue {
 
     @Override
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java
index db6209e7b1..336a826437 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageLookupFunctionTest.java
@@ -222,18 +222,7 @@ public class PackageLookupFunctionTest extends FoundationTestCase {
     assertTrue(packageLookupValue.packageExists());
     assertEquals(rootDirectory, packageLookupValue.getRoot());
   }
-
-  // TODO(kchodorow): Clean this up (see TODOs in PackageLookupValue).
-  @Test
-  public void testExternalPackageLookupSemantics() {
-    PackageLookupValue existing = PackageLookupValue.workspace(rootDirectory);
-    assertTrue(existing.isExternalPackage());
-    assertTrue(existing.packageExists());
-    PackageLookupValue nonExistent = PackageLookupValue.workspace(rootDirectory.getRelative("x/y"));
-    assertTrue(nonExistent.isExternalPackage());
-    assertFalse(nonExistent.packageExists());
-  }
-
+  
   @Test
   public void testPackageLookupValueHashCodeAndEqualsContract() throws Exception {
     Path root1 = rootDirectory.getRelative("root1");