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
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 043bd0f..98e4ad9 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.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 @@
/**
* 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 00d0300..186232a 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.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.Set;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicInteger;
-
import javax.annotation.Nullable;
/**
@@ -414,7 +412,6 @@
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 @@
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 2400de7..638f2ee 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.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.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 @@
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 @@
}
}
- return getPackageLookupValue(env, pkgLocator.getPathEntries(), packageKey,
- packageKey.getPackageFragment().getChild("BUILD"));
+ return getPackageLookupValue(env, pkgLocator.getPathEntries(), packageKey, BuildFileName.BUILD);
}
@Nullable
@@ -124,14 +122,18 @@
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 @@
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 @@
return null;
}
return lastPackagePackagePathFileValue.exists()
- ? PackageLookupValue.success(lastPackagePath)
+ ? PackageLookupValue.success(lastPackagePath, BuildFileName.WORKSPACE)
: PackageLookupValue.NO_BUILD_FILE_VALUE;
}
@@ -197,7 +200,8 @@
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 @@
}
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 ce006cc..a9861b7 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 @@
*/
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 @@
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 Path getRoot();
- /**
- * Returns whether the package lookup was successful.
- */
+ /** 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();
/**
- * For an unsuccessful package lookup, gets the reason why {@link #packageExists} returns
- * {@code false}.
+ * For a successful package lookup, returns the {@link RootedPath} for the build file that defines
+ * the package.
+ */
+ 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}.
*/
abstract ErrorReason getErrorReason();
@@ -97,9 +152,11 @@
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 @@
}
@Override
+ public BuildFileName getBuildFileName() {
+ return buildFileName;
+ }
+
+ @Override
ErrorReason getErrorReason() {
throw new IllegalStateException();
}
@@ -128,12 +190,12 @@
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 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 be33464..0269305 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.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.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 119205a..e243a82 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.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 @@
}
@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 2b35fa1..b04b798 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.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.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 @@
PackageLookupValue packageLookupValue = lookupPackage("parentpackage/everythinggood");
assertTrue(packageLookupValue.packageExists());
assertEquals(rootDirectory, packageLookupValue.getRoot());
+ assertEquals(BuildFileName.BUILD, packageLookupValue.getBuildFileName());
}
@Test
@@ -215,6 +215,7 @@
PackageLookupValue packageLookupValue = lookupPackage("");
assertTrue(packageLookupValue.packageExists());
assertEquals(rootDirectory, packageLookupValue.getRoot());
+ assertEquals(BuildFileName.BUILD, packageLookupValue.getBuildFileName());
}
@Test
@@ -225,7 +226,7 @@
assertTrue(packageLookupValue.packageExists());
assertEquals(rootDirectory, packageLookupValue.getRoot());
}
-
+
@Test
public void testPackageLookupValueHashCodeAndEqualsContract() throws Exception {
Path root1 = rootDirectory.getRelative("root1");
@@ -234,16 +235,22 @@
// 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();
}
}
