Have GlobFunction make use of the assumption that the glob's package exists by having it not declare a dep on the PackageLookupValue for the package. This optimization means that a BUILD file edit doesn't (necessarily) invalidate all the globs in the package; the PackageLookupValue node would get change-pruned but we still pay the very small cost of invalidating unnecessarily.
Also slightly improve variable naming in GlobFunctionTest.
--
MOS_MIGRATED_REVID=113799936
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GlobDescriptor.java b/src/main/java/com/google/devtools/build/lib/skyframe/GlobDescriptor.java
index ec59544..1687a81 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GlobDescriptor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GlobDescriptor.java
@@ -17,6 +17,7 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.lib.util.StringCanonicalizer;
+import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.Serializable;
@@ -32,6 +33,7 @@
@ThreadSafe
public final class GlobDescriptor implements Serializable {
final PackageIdentifier packageId;
+ final Path packageRoot;
final PathFragment subdir;
final String pattern;
final boolean excludeDirs;
@@ -40,15 +42,17 @@
* Constructs a GlobDescriptor.
*
* @param packageId the name of the owner package (must be an existing package)
+ * @param packageRoot the package root of {@code packageId}
* @param subdir the subdirectory being looked at (must exist and must be a directory. It's
* assumed that there are no other packages between {@code packageName} and
* {@code subdir}.
* @param pattern a valid glob pattern
* @param excludeDirs true if directories should be excluded from results
*/
- public GlobDescriptor(PackageIdentifier packageId, PathFragment subdir, String pattern,
- boolean excludeDirs) {
+ public GlobDescriptor(PackageIdentifier packageId, Path packageRoot, PathFragment subdir,
+ String pattern, boolean excludeDirs) {
this.packageId = Preconditions.checkNotNull(packageId);
+ this.packageRoot = Preconditions.checkNotNull(packageRoot);
this.subdir = Preconditions.checkNotNull(subdir);
this.pattern = Preconditions.checkNotNull(StringCanonicalizer.intern(pattern));
this.excludeDirs = excludeDirs;
@@ -56,8 +60,9 @@
@Override
public String toString() {
- return String.format("<GlobDescriptor packageName=%s subdir=%s pattern=%s excludeDirs=%s>",
- packageId, subdir, pattern, excludeDirs);
+ return String.format(
+ "<GlobDescriptor packageName=%s packageRoot=%s subdir=%s pattern=%s excludeDirs=%s>",
+ packageId, packageRoot, subdir, pattern, excludeDirs);
}
/**
@@ -70,6 +75,13 @@
}
/**
+ * Returns the package root of {@code getPackageId()}.
+ */
+ public Path getPackageRoot() {
+ return packageRoot;
+ }
+
+ /**
* Returns the subdirectory of the package under consideration.
*/
public PathFragment getSubdir() {
@@ -107,7 +119,8 @@
return false;
}
GlobDescriptor other = (GlobDescriptor) obj;
- return packageId.equals(other.packageId) && subdir.equals(other.subdir)
- && pattern.equals(other.pattern) && excludeDirs == other.excludeDirs;
+ return packageId.equals(other.packageId) && packageRoot.equals(other.packageRoot)
+ && subdir.equals(other.subdir) && pattern.equals(other.pattern)
+ && excludeDirs == other.excludeDirs;
}
}
\ No newline at end of file
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 5520c23..50b7c29 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,7 +18,6 @@
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
-import com.google.devtools.build.lib.util.Preconditions;
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;
@@ -54,16 +53,8 @@
public SkyValue compute(SkyKey skyKey, Environment env) throws GlobFunctionException {
GlobDescriptor glob = (GlobDescriptor) skyKey.argument();
- PackageLookupValue globPkgLookupValue = (PackageLookupValue)
- env.getValue(PackageLookupValue.key(glob.getPackageId()));
- if (globPkgLookupValue == null) {
- return null;
- }
- Preconditions.checkState(globPkgLookupValue.packageExists(), "%s isn't an existing package",
- glob.getPackageId());
- // Note that this implies that the package's BUILD file exists which implies that the
- // package's directory exists.
-
+ // Note that the glob's package is assumed to exist which implies that the package's BUILD file
+ // exists which implies that the package's directory exists.
PathFragment globSubdir = glob.getSubdir();
if (!globSubdir.equals(PathFragment.EMPTY_FRAGMENT)) {
PackageLookupValue globSubdirPkgLookupValue = (PackageLookupValue) env.getValue(
@@ -103,8 +94,8 @@
matches.add(globSubdir);
}
} else {
- SkyKey globKey = GlobValue.internalKey(
- glob.getPackageId(), globSubdir, patternTail, glob.excludeDirs());
+ SkyKey globKey = GlobValue.internalKey(glob.getPackageId(), glob.getPackageRoot(),
+ globSubdir, patternTail, glob.excludeDirs());
GlobValue globValue = (GlobValue) env.getValue(globKey);
if (globValue == null) {
return null;
@@ -114,8 +105,7 @@
}
PathFragment dirPathFragment = glob.getPackageId().getPackageFragment().getRelative(globSubdir);
- RootedPath dirRootedPath = RootedPath.toRootedPath(globPkgLookupValue.getRoot(),
- dirPathFragment);
+ RootedPath dirRootedPath = RootedPath.toRootedPath(glob.getPackageRoot(), dirPathFragment);
if (alwaysUseDirListing || containsGlobs(patternHead)) {
// Pattern contains globs, so a directory listing is required.
//
@@ -145,7 +135,7 @@
// For symlinks, look up the corresponding FileValue. This ensures that if the symlink
// changes and "switches types" (say, from a file to a directory), this value will be
// invalidated.
- RootedPath symlinkRootedPath = RootedPath.toRootedPath(globPkgLookupValue.getRoot(),
+ RootedPath symlinkRootedPath = RootedPath.toRootedPath(glob.getPackageRoot(),
dirPathFragment.getRelative(fileName));
FileValue symlinkFileValue = (FileValue) env.getValue(FileValue.key(symlinkRootedPath));
if (symlinkFileValue == null) {
@@ -166,7 +156,7 @@
} else {
// Pattern does not contain globs, so a direct stat is enough.
String fileName = patternHead;
- RootedPath fileRootedPath = RootedPath.toRootedPath(globPkgLookupValue.getRoot(),
+ RootedPath fileRootedPath = RootedPath.toRootedPath(glob.getPackageRoot(),
dirPathFragment.getRelative(fileName));
FileValue fileValue = (FileValue) env.getValue(FileValue.key(fileRootedPath));
if (fileValue == null) {
@@ -212,7 +202,7 @@
Environment env) {
if (isDirectory && subdirPattern != null) {
// This is a directory, and the pattern covers files under that directory.
- SkyKey subdirGlobKey = GlobValue.internalKey(glob.getPackageId(),
+ SkyKey subdirGlobKey = GlobValue.internalKey(glob.getPackageId(), glob.getPackageRoot(),
glob.getSubdir().getRelative(fileName), subdirPattern, glob.excludeDirs());
GlobValue subdirGlob = (GlobValue) env.getValue(subdirGlobKey);
if (subdirGlob == null) {
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java
index 127db0c..5e877bd 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/GlobValue.java
@@ -20,6 +20,7 @@
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
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.UnixGlob;
import com.google.devtools.build.skyframe.SkyKey;
@@ -79,9 +80,8 @@
* @throws InvalidGlobPatternException if the pattern is not valid.
*/
@ThreadSafe
- public static SkyKey key(PackageIdentifier packageId, String pattern, boolean excludeDirs,
- PathFragment subdir)
- throws InvalidGlobPatternException {
+ public static SkyKey key(PackageIdentifier packageId, Path packageRoot, String pattern,
+ boolean excludeDirs, PathFragment subdir) throws InvalidGlobPatternException {
if (pattern.indexOf('?') != -1) {
throw new InvalidGlobPatternException(pattern, "wildcard ? forbidden");
}
@@ -91,7 +91,7 @@
throw new InvalidGlobPatternException(pattern, error);
}
- return internalKey(packageId, subdir, pattern, excludeDirs);
+ return internalKey(packageId, packageRoot, subdir, pattern, excludeDirs);
}
/**
@@ -100,10 +100,10 @@
* <p>Do not use outside {@code GlobFunction}.
*/
@ThreadSafe
- static SkyKey internalKey(PackageIdentifier packageId, PathFragment subdir, String pattern,
- boolean excludeDirs) {
+ static SkyKey internalKey(PackageIdentifier packageId, Path packageRoot, PathFragment subdir,
+ String pattern, boolean excludeDirs) {
return new SkyKey(SkyFunctions.GLOB,
- new GlobDescriptor(packageId, subdir, pattern, excludeDirs));
+ new GlobDescriptor(packageId, packageRoot, subdir, pattern, excludeDirs));
}
/**
@@ -113,7 +113,7 @@
*/
@ThreadSafe
static SkyKey internalKey(GlobDescriptor glob, String subdirName) {
- return internalKey(glob.packageId, glob.subdir.getRelative(subdirName),
+ return internalKey(glob.packageId, glob.packageRoot, glob.subdir.getRelative(subdirName),
glob.pattern, glob.excludeDirs);
}
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 d77ef79..03b0f5f 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
@@ -232,6 +232,7 @@
Collection<Pair<String, Boolean>> globPatterns,
Map<Label, Path> subincludes,
PackageIdentifier packageIdentifier,
+ Path packageRoot,
boolean containsErrors)
throws InternalInconsistentFilesystemException {
boolean packageShouldBeInError = containsErrors;
@@ -312,8 +313,8 @@
boolean excludeDirs = globPattern.getSecond();
SkyKey globSkyKey;
try {
- globSkyKey =
- GlobValue.key(packageIdentifier, pattern, excludeDirs, PathFragment.EMPTY_FRAGMENT);
+ globSkyKey = GlobValue.key(packageIdentifier, packageRoot, pattern, excludeDirs,
+ PathFragment.EMPTY_FRAGMENT);
} catch (InvalidGlobPatternException e) {
// Globs that make it to pkg.getGlobPatterns() should already be filtered for errors.
throw new IllegalStateException(e);
@@ -483,7 +484,8 @@
try {
packageShouldBeConsideredInError =
markDependenciesAndPropagateInconsistentFilesystemExceptions(
- env, globPatterns, subincludes, packageId, legacyPkgBuilder.containsErrors());
+ env, globPatterns, subincludes, packageId, packageLookupValue.getRoot(),
+ legacyPkgBuilder.containsErrors());
} catch (InternalInconsistentFilesystemException e) {
packageFunctionCache.invalidate(packageId);
throw new PackageFunctionException(e,
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
index cb55822..32dffab 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java
@@ -96,7 +96,7 @@
private AtomicReference<PathPackageLocator> pkgLocator;
private TimestampGranularityMonitor tsgm;
- private static final PackageIdentifier PKG_PATH_ID = PackageIdentifier.createInDefaultRepo("pkg");
+ private static final PackageIdentifier PKG_ID = PackageIdentifier.createInDefaultRepo("pkg");
@Before
public final void setUp() throws Exception {
@@ -104,7 +104,7 @@
root = fs.getRootDirectory().getRelative("root/workspace");
writableRoot = fs.getRootDirectory().getRelative("writableRoot/workspace");
outputBase = fs.getRootDirectory().getRelative("output_base");
- pkgPath = root.getRelative(PKG_PATH_ID.getPackageFragment());
+ pkgPath = root.getRelative(PKG_ID.getPackageFragment());
pkgLocator =
new AtomicReference<>(
@@ -267,33 +267,6 @@
}
@Test
- public void testGlobMissingPackage() throws Exception {
- // This is a malformed value key, because "missing" is not a package. Nevertheless, we have a
- // sanity check that building the corresponding GlobValue fails loudly. The test depends on
- // implementation details of ParallelEvaluator and GlobFunction.
- SkyKey skyKey =
- GlobValue.key(
- PackageIdentifier.createInDefaultRepo("missing"),
- "foo",
- false,
- PathFragment.EMPTY_FRAGMENT);
- try {
- driver.evaluate(
- ImmutableList.of(skyKey),
- false,
- SkyframeExecutor.DEFAULT_THREAD_COUNT,
- NullEventHandler.INSTANCE);
- fail();
- } catch (RuntimeException e) {
- assertThat(e.getMessage())
- .contains("Unrecoverable error while evaluating node '" + skyKey + "'");
- Throwable cause = e.getCause();
- assertThat(cause).isInstanceOf(IllegalStateException.class);
- assertThat(cause.getMessage()).contains("isn't an existing package");
- }
- }
-
- @Test
public void testGlobDoesNotCrossPackageBoundary() throws Exception {
FileSystemUtils.createEmptyFile(pkgPath.getRelative("foo/BUILD"));
// "foo/bar" should not be in the results because foo is a separate package.
@@ -360,7 +333,7 @@
}
private GlobValue runGlob(boolean excludeDirs, String pattern) throws Exception {
- SkyKey skyKey = GlobValue.key(PKG_PATH_ID, pattern, excludeDirs, PathFragment.EMPTY_FRAGMENT);
+ SkyKey skyKey = GlobValue.key(PKG_ID, root, pattern, excludeDirs, PathFragment.EMPTY_FRAGMENT);
EvaluationResult<SkyValue> result =
driver.evaluate(
ImmutableList.of(skyKey),
@@ -444,7 +417,7 @@
private void assertIllegalPattern(String pattern) {
try {
- GlobValue.key(PKG_PATH_ID, pattern, false, PathFragment.EMPTY_FRAGMENT);
+ GlobValue.key(PKG_ID, root, pattern, false, PathFragment.EMPTY_FRAGMENT);
fail("invalid pattern not detected: " + pattern);
} catch (InvalidGlobPatternException e) {
// Expected.
@@ -582,7 +555,6 @@
/** Regression test for b/13319874: Directory listing crash. */
@Test
public void testResilienceToFilesystemInconsistencies_DirectoryExistence() throws Exception {
- long nodeId = pkgPath.getRelative("BUILD").stat().getNodeId();
// Our custom filesystem says "pkgPath/BUILD" exists but "pkgPath" does not exist.
fs.stubStat(pkgPath, null);
RootedPath pkgRootedPath = RootedPath.toRootedPath(root, pkgPath);
@@ -590,13 +562,8 @@
FileValue pkgDirValue =
FileValue.value(pkgRootedPath, pkgDirFileStateValue, pkgRootedPath, pkgDirFileStateValue);
differencer.inject(ImmutableMap.of(FileValue.key(pkgRootedPath), pkgDirValue));
- String expectedMessage =
- "Some filesystem operations implied /root/workspace/pkg/BUILD was a "
- + "regular file with size of 0 and mtime of 0 and nodeId of "
- + nodeId
- + " and mtime of 0 "
- + "but others made us think it was a nonexistent path";
- SkyKey skyKey = GlobValue.key(PKG_PATH_ID, "*/foo", false, PathFragment.EMPTY_FRAGMENT);
+ String expectedMessage = "/root/workspace/pkg is no longer an existing directory";
+ SkyKey skyKey = GlobValue.key(PKG_ID, root, "*/foo", false, PathFragment.EMPTY_FRAGMENT);
EvaluationResult<GlobValue> result =
driver.evaluate(
ImmutableList.of(skyKey),
@@ -623,7 +590,7 @@
ImmutableMap.of(
DirectoryListingStateValue.key(fooBarDirRootedPath), fooBarDirListingValue));
String expectedMessage = "/root/workspace/pkg/foo/bar/wiz is no longer an existing directory.";
- SkyKey skyKey = GlobValue.key(PKG_PATH_ID, "**/wiz", false, PathFragment.EMPTY_FRAGMENT);
+ SkyKey skyKey = GlobValue.key(PKG_ID, root, "**/wiz", false, PathFragment.EMPTY_FRAGMENT);
EvaluationResult<GlobValue> result =
driver.evaluate(
ImmutableList.of(skyKey),
@@ -695,7 +662,8 @@
ImmutableMap.of(DirectoryListingStateValue.key(wizRootedPath), wizDirListingValue));
String expectedMessage =
"readdir and stat disagree about whether " + fileRootedPath.asPath() + " is a symlink";
- SkyKey skyKey = GlobValue.key(PKG_PATH_ID, "foo/bar/wiz/*", false, PathFragment.EMPTY_FRAGMENT);
+ SkyKey skyKey = GlobValue.key(PKG_ID, root, "foo/bar/wiz/*", false,
+ PathFragment.EMPTY_FRAGMENT);
EvaluationResult<GlobValue> result =
driver.evaluate(
ImmutableList.of(skyKey),
