Add an enum representing the specific build file name (WORKSPACE, BUILD) to the PackageLookupValue to reduce the number of references to the filename "BUILD".

--
MOS_MIGRATED_REVID=129203257

7 files changed, 134 insertions, 65 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
index 043bd0f0df..98e4ad9e71 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/repository/RepositoryFunction.java
@@ -47,11 +47,9 @@ import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
-
 import java.io.IOException;
 import java.nio.charset.Charset;
 import java.util.Map;
-
 import javax.annotation.Nullable;
 
 /**
@@ -262,21 +260,19 @@ public abstract class RepositoryFunction {
 
   /**
    * Uses a remote repository name to fetch the corresponding Rule describing how to get it.
-   * 
-   * This should be the unique entry point for resolving a remote repository function.
+   *
+   * <p>This should be the unique entry point for resolving a remote repository function.
    */
   @Nullable
   public static Rule getRule(String repository, Environment env)
       throws RepositoryFunctionException {
 
     SkyKey packageLookupKey = PackageLookupValue.key(Label.EXTERNAL_PACKAGE_IDENTIFIER);
-    PackageLookupValue packageLookupValue;
-    packageLookupValue = (PackageLookupValue) env.getValue(packageLookupKey);
+    PackageLookupValue packageLookupValue = (PackageLookupValue) env.getValue(packageLookupKey);
     if (packageLookupValue == null) {
       return null;
     }
-    RootedPath workspacePath =
-        RootedPath.toRootedPath(packageLookupValue.getRoot(), new PathFragment("WORKSPACE"));
+    RootedPath workspacePath = packageLookupValue.getRootedPath(Label.EXTERNAL_PACKAGE_IDENTIFIER);
 
     SkyKey workspaceKey = WorkspaceFileValue.key(workspacePath);
     do {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index 00d0300336..186232aeff 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -64,7 +64,6 @@ import com.google.devtools.build.skyframe.SkyValue;
 import com.google.devtools.build.skyframe.ValueOrException2;
 import com.google.devtools.build.skyframe.ValueOrException3;
 import com.google.devtools.build.skyframe.ValueOrException4;
-
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Collection;
@@ -77,7 +76,6 @@ import java.util.Map.Entry;
 import java.util.Set;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
-
 import javax.annotation.Nullable;
 
 /**
@@ -414,7 +412,6 @@ public class PackageFunction implements SkyFunction {
   public SkyValue compute(SkyKey key, Environment env) throws PackageFunctionException,
       InterruptedException {
     PackageIdentifier packageId = (PackageIdentifier) key.argument();
-    PathFragment packageNameFragment = packageId.getPackageFragment();
 
     SkyKey packageLookupKey = PackageLookupValue.key(packageId);
     PackageLookupValue packageLookupValue;
@@ -463,9 +460,7 @@ public class PackageFunction implements SkyFunction {
           Transience.PERSISTENT);
     }
 
-    PathFragment buildFileFragment = packageNameFragment.getChild("BUILD");
-    RootedPath buildFileRootedPath = RootedPath.toRootedPath(packageLookupValue.getRoot(),
-        buildFileFragment);
+    RootedPath buildFileRootedPath = packageLookupValue.getRootedPath(packageId);
     FileValue buildFileValue = null;
     Path buildFilePath = buildFileRootedPath.asPath();
     String replacementContents = null;
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 2400de7ee0..638f2eee8d 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
@@ -21,6 +21,7 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.packages.BuildFileNotFoundException;
 import com.google.devtools.build.lib.packages.NoSuchPackageException;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
+import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName;
 import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -30,10 +31,8 @@ import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
-
 import java.io.IOException;
 import java.util.concurrent.atomic.AtomicReference;
-
 import javax.annotation.Nullable;
 
 /**
@@ -52,7 +51,7 @@ public class PackageLookupFunction implements SkyFunction {
     PathPackageLocator pkgLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env);
     PackageIdentifier packageKey = (PackageIdentifier) skyKey.argument();
     if (PackageFunction.isDefaultsPackage(packageKey)) {
-      return PackageLookupValue.success(pkgLocator.getPathEntries().get(0));
+      return PackageLookupValue.success(pkgLocator.getPathEntries().get(0), BuildFileName.BUILD);
     }
 
     if (!packageKey.getRepository().isMain()) {
@@ -85,8 +84,7 @@ public class PackageLookupFunction implements SkyFunction {
       }
     }
 
-    return getPackageLookupValue(env, pkgLocator.getPathEntries(), packageKey,
-        packageKey.getPackageFragment().getChild("BUILD"));
+    return getPackageLookupValue(env, pkgLocator.getPathEntries(), packageKey, BuildFileName.BUILD);
   }
 
   @Nullable
@@ -124,14 +122,18 @@ public class PackageLookupFunction implements SkyFunction {
     return fileValue;
   }
 
-  private PackageLookupValue getPackageLookupValue(Environment env,
-      ImmutableList<Path> packagePathEntries, PackageIdentifier packageIdentifier,
-      PathFragment buildFileFragment) throws PackageLookupFunctionException {
+  private PackageLookupValue getPackageLookupValue(
+      Environment env,
+      ImmutableList<Path> packagePathEntries,
+      PackageIdentifier packageIdentifier,
+      BuildFileName buildFileName)
+      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) {
+      PathFragment buildFileFragment = buildFileName.getBuildFileFragment(packageIdentifier);
       RootedPath buildFileRootedPath = RootedPath.toRootedPath(packagePathEntry,
           buildFileFragment);
       FileValue fileValue = getFileValue(buildFileRootedPath, env, packageIdentifier);
@@ -139,17 +141,18 @@ public class PackageLookupFunction implements SkyFunction {
         return null;
       }
       if (fileValue.isFile()) {
-        return PackageLookupValue.success(buildFileRootedPath.getRoot());
+        return PackageLookupValue.success(buildFileRootedPath.getRoot(), buildFileName);
       }
     }
     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"));
+  private PackageLookupValue computeWorkspacePackageLookupValue(
+      Environment env, ImmutableList<Path> packagePathEntries)
+      throws PackageLookupFunctionException {
+    PackageLookupValue result =
+        getPackageLookupValue(
+            env, packagePathEntries, Label.EXTERNAL_PACKAGE_IDENTIFIER, BuildFileName.WORKSPACE);
     if (result == null) {
       return null;
     }
@@ -171,7 +174,7 @@ public class PackageLookupFunction implements SkyFunction {
       return null;
     }
     return lastPackagePackagePathFileValue.exists()
-        ? PackageLookupValue.success(lastPackagePath)
+        ? PackageLookupValue.success(lastPackagePath, BuildFileName.WORKSPACE)
         : PackageLookupValue.NO_BUILD_FILE_VALUE;
   }
 
@@ -197,7 +200,8 @@ public class PackageLookupFunction implements SkyFunction {
       throw new PackageLookupFunctionException(new BuildFileNotFoundException(id, e.getMessage()),
           Transience.PERSISTENT);
     }
-    PathFragment buildFileFragment = id.getPackageFragment().getChild("BUILD");
+    BuildFileName buildFileName = BuildFileName.BUILD;
+    PathFragment buildFileFragment = id.getPackageFragment().getChild(buildFileName.getFilename());
     RootedPath buildFileRootedPath = RootedPath.toRootedPath(repositoryValue.getPath(),
         buildFileFragment);
     FileValue fileValue = getFileValue(buildFileRootedPath, env, packageIdentifier);
@@ -206,7 +210,7 @@ public class PackageLookupFunction implements SkyFunction {
     }
 
     if (fileValue.isFile()) {
-      return PackageLookupValue.success(repositoryValue.getPath());
+      return PackageLookupValue.success(repositoryValue.getPath(), buildFileName);
     }
 
     return PackageLookupValue.NO_BUILD_FILE_VALUE;
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 ce006cc3c5..a9861b727a 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
@@ -13,10 +13,13 @@
 // limitations under the License.
 package com.google.devtools.build.lib.skyframe;
 
+import com.google.common.base.Objects;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
+import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
@@ -33,6 +36,48 @@ import com.google.devtools.build.skyframe.SkyValue;
  */
 public abstract class PackageLookupValue implements SkyValue {
 
+  /**
+   * The file (BUILD, WORKSPACE, etc.) that defines this package, referred to as the "build file".
+   */
+  public enum BuildFileName {
+    WORKSPACE("WORKSPACE") {
+      @Override
+      public PathFragment getBuildFileFragment(PackageIdentifier packageIdentifier) {
+        return new PathFragment(BuildFileName.WORKSPACE.getFilename());
+      }
+    },
+    BUILD("BUILD") {
+      @Override
+      public PathFragment getBuildFileFragment(PackageIdentifier packageIdentifier) {
+        return packageIdentifier.getPackageFragment().getChild(getFilename());
+      }
+    };
+
+    private static final BuildFileName[] VALUES = BuildFileName.values();
+
+    private final String filename;
+
+    private BuildFileName(String filename) {
+      this.filename = filename;
+    }
+
+    public String getFilename() {
+      return filename;
+    }
+
+    /**
+     * Returns a {@link PathFragment} to the build file that defines the package.
+     *
+     * @param packageIdentifier the identifier for this package, which the caller should already
+     *     know (since it was in the {@link SkyKey} used to get the {@link PackageLookupValue}.
+     */
+    public abstract PathFragment getBuildFileFragment(PackageIdentifier packageIdentifier);
+
+    public static BuildFileName lookupByOrdinal(int ordinal) {
+      return VALUES[ordinal];
+    }
+  }
+
   public static final NoBuildFilePackageLookupValue NO_BUILD_FILE_VALUE =
       new NoBuildFilePackageLookupValue();
   public static final DeletedPackageLookupValue DELETED_PACKAGE_VALUE =
@@ -52,8 +97,8 @@ public abstract class PackageLookupValue implements SkyValue {
   protected PackageLookupValue() {
   }
 
-  public static PackageLookupValue success(Path root) {
-    return new SuccessfulPackageLookupValue(root);
+  public static PackageLookupValue success(Path root, BuildFileName buildFileName) {
+    return new SuccessfulPackageLookupValue(root, buildFileName);
   }
 
   public static PackageLookupValue invalidPackageName(String errorMsg) {
@@ -66,14 +111,24 @@ public abstract class PackageLookupValue implements SkyValue {
    */
   public abstract Path getRoot();
 
+  /** For a successful package lookup, returns the build file name that the package uses. */
+  public abstract BuildFileName getBuildFileName();
+
+  /** Returns whether the package lookup was successful. */
+  public abstract boolean packageExists();
+
   /**
-   * Returns whether the package lookup was successful.
+   * For a successful package lookup, returns the {@link RootedPath} for the build file that defines
+   * the package.
    */
-  public abstract boolean packageExists();
+  public RootedPath getRootedPath(PackageIdentifier packageIdentifier) {
+    return RootedPath.toRootedPath(
+        getRoot(), getBuildFileName().getBuildFileFragment(packageIdentifier));
+  }
 
   /**
-   * For an unsuccessful package lookup, gets the reason why {@link #packageExists} returns
-   * {@code false}.
+   * For an unsuccessful package lookup, gets the reason why {@link #packageExists} returns {@code
+   * false}.
    */
   abstract ErrorReason getErrorReason();
 
@@ -97,9 +152,11 @@ public abstract class PackageLookupValue implements SkyValue {
   public static class SuccessfulPackageLookupValue extends PackageLookupValue {
 
     private final Path root;
+    private final BuildFileName buildFileName;
 
-    private SuccessfulPackageLookupValue(Path root) {
+    private SuccessfulPackageLookupValue(Path root, BuildFileName buildFileName) {
       this.root = root;
+      this.buildFileName = buildFileName;
     }
 
     @Override
@@ -113,6 +170,11 @@ public abstract class PackageLookupValue implements SkyValue {
     }
 
     @Override
+    public BuildFileName getBuildFileName() {
+      return buildFileName;
+    }
+
+    @Override
     ErrorReason getErrorReason() {
       throw new IllegalStateException();
     }
@@ -128,12 +190,12 @@ public abstract class PackageLookupValue implements SkyValue {
         return false;
       }
       SuccessfulPackageLookupValue other = (SuccessfulPackageLookupValue) obj;
-      return root.equals(other.root);
+      return root.equals(other.root) && buildFileName == other.buildFileName;
     }
 
     @Override
     public int hashCode() {
-      return root.hashCode();
+      return Objects.hashCode(root.hashCode(), buildFileName.hashCode());
     }
   }
 
@@ -148,6 +210,11 @@ public abstract class PackageLookupValue implements SkyValue {
     public Path getRoot() {
       throw new IllegalStateException();
     }
+
+    @Override
+    public BuildFileName getBuildFileName() {
+      throw new IllegalStateException();
+    }
   }
 
   /** Marker value for no build file found. */
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
index be334649f4..0269305288 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileFunctionTest.java
@@ -65,12 +65,6 @@ import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
-
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
@@ -85,8 +79,11 @@ import java.util.Set;
 import java.util.UUID;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
-
 import javax.annotation.Nullable;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 /** Tests for {@link FileFunction}. */
 @RunWith(JUnit4.class)
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
index 119205af5d..e243a82e45 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java
@@ -51,18 +51,15 @@ import com.google.devtools.build.skyframe.EvaluationResult;
 import com.google.devtools.build.skyframe.RecordingDifferencer;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
-
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
 import java.io.IOException;
 import java.util.Collection;
 import java.util.Map;
 import java.util.Set;
 import java.util.UUID;
-
 import javax.annotation.Nullable;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 /**
  * Unit tests of specific functionality of PackageFunction. Note that it's already tested
@@ -102,6 +99,12 @@ public class PackageFunctionTest extends BuildViewTestCase {
   }
 
   @Test
+  public void testValidPackage() throws Exception {
+    scratch.file("pkg/BUILD");
+    validPackage(PackageValue.key(PackageIdentifier.parse("@//pkg")));
+  }
+
+  @Test
   public void testInconsistentNewPackage() throws Exception {
     scratch.file("pkg/BUILD", "subinclude('//foo:sub')");
     scratch.file("foo/sub");
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 2b35fa1495..b04b79868c 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
@@ -29,6 +29,7 @@ import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.events.NullEventHandler;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
 import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
+import com.google.devtools.build.lib.skyframe.PackageLookupValue.BuildFileName;
 import com.google.devtools.build.lib.skyframe.PackageLookupValue.ErrorReason;
 import com.google.devtools.build.lib.testutil.FoundationTestCase;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
@@ -42,16 +43,14 @@ import com.google.devtools.build.skyframe.SequentialBuildDriver;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
-
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
 import java.util.HashMap;
 import java.util.Map;
 import java.util.UUID;
 import java.util.concurrent.atomic.AtomicReference;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
 
 /**
  * Tests for {@link PackageLookupFunction}.
@@ -207,6 +206,7 @@ public class PackageLookupFunctionTest extends FoundationTestCase {
     PackageLookupValue packageLookupValue = lookupPackage("parentpackage/everythinggood");
     assertTrue(packageLookupValue.packageExists());
     assertEquals(rootDirectory, packageLookupValue.getRoot());
+    assertEquals(BuildFileName.BUILD, packageLookupValue.getBuildFileName());
   }
 
   @Test
@@ -215,6 +215,7 @@ public class PackageLookupFunctionTest extends FoundationTestCase {
     PackageLookupValue packageLookupValue = lookupPackage("");
     assertTrue(packageLookupValue.packageExists());
     assertEquals(rootDirectory, packageLookupValue.getRoot());
+    assertEquals(BuildFileName.BUILD, packageLookupValue.getBuildFileName());
   }
 
   @Test
@@ -225,7 +226,7 @@ public class PackageLookupFunctionTest extends FoundationTestCase {
     assertTrue(packageLookupValue.packageExists());
     assertEquals(rootDirectory, packageLookupValue.getRoot());
   }
-  
+
   @Test
   public void testPackageLookupValueHashCodeAndEqualsContract() throws Exception {
     Path root1 = rootDirectory.getRelative("root1");
@@ -234,16 +235,22 @@ public class PackageLookupFunctionTest extends FoundationTestCase {
     // PackageLookupValue are supposed to have reference equality semantics, and some are supposed
     // to have logical equality semantics.
     new EqualsTester()
-        .addEqualityGroup(PackageLookupValue.success(root1), PackageLookupValue.success(root1))
-        .addEqualityGroup(PackageLookupValue.success(root2), PackageLookupValue.success(root2))
+        .addEqualityGroup(
+            PackageLookupValue.success(root1, BuildFileName.BUILD),
+            PackageLookupValue.success(root1, BuildFileName.BUILD))
+        .addEqualityGroup(
+            PackageLookupValue.success(root2, BuildFileName.BUILD),
+            PackageLookupValue.success(root2, BuildFileName.BUILD))
         .addEqualityGroup(
             PackageLookupValue.NO_BUILD_FILE_VALUE, PackageLookupValue.NO_BUILD_FILE_VALUE)
         .addEqualityGroup(
             PackageLookupValue.DELETED_PACKAGE_VALUE, PackageLookupValue.DELETED_PACKAGE_VALUE)
-        .addEqualityGroup(PackageLookupValue.invalidPackageName("nope1"),
+        .addEqualityGroup(
+            PackageLookupValue.invalidPackageName("nope1"),
             PackageLookupValue.invalidPackageName("nope1"))
-        .addEqualityGroup(PackageLookupValue.invalidPackageName("nope2"),
-             PackageLookupValue.invalidPackageName("nope2"))
+        .addEqualityGroup(
+            PackageLookupValue.invalidPackageName("nope2"),
+            PackageLookupValue.invalidPackageName("nope2"))
         .testEquals();
   }
 }