Prefer rethrowing Skyframe globbing errors rather than legacy globbing errors.
Skyframe globbing errors are more useful to the user (e.g. in the case of a symlink cycle, the error message contains the cycle) and are also persistent. Contrast with legacy globbing errors which are more opaque and are conservatively assumed to be transient.
Unsurprisingly, this change fixes a long-standing discrepancy between incremental and non-incremental package loading. The new, self-consistent behavior, is that PackageFunction will throw a NoSuchPackageException if any glob evaluation encounters a symlink cycle, regardless if glob evaluations were cached via Skyframe globbing. The discrepancy with the old code was that legacy globbing ignores some symlink issues (see UnixGlob.GlobVisitor#processSymlink), and so on non-incremental package loading with a symlink ignored by legacy globbing, PackageFunction wouldn't throw a NoSuchPackageException.
RELNOTES: Package loading now consistently fails if package loading had a glob evaluation that encountered a symlink cycle or symlink infinite expansion. Previously, such package loading with such glob evaluations would fail only in some cases.
PiperOrigin-RevId: 280298226
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 1fd71cd..9bd8252 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
@@ -53,6 +53,7 @@
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
+import com.google.devtools.build.lib.vfs.Symlinks;
import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
import com.google.devtools.build.skyframe.ErrorInfo;
import com.google.devtools.build.skyframe.EvaluationResult;
@@ -1117,6 +1118,89 @@
.isEqualTo(new Pair<String, String>("1 packages loaded", ""));
}
+ @Test
+ public void testLegacyGlobbingEncountersSymlinkCycleAndThrowsIOException() throws Exception {
+ reporter.removeHandler(failFastHandler);
+ getSkyframeExecutor().turnOffSyscallCacheForTesting();
+
+ // When a package's BUILD file and the relevant filesystem state is such that legacy globbing
+ // will encounter an IOException due to a directory symlink cycle,
+ Path fooBUILDPath = scratch.file("foo/BUILD", "glob(['cycle/**/foo.txt'])");
+ Path fooCyclePath = fooBUILDPath.getParentDirectory().getChild("cycle");
+ FileSystemUtils.ensureSymbolicLink(fooCyclePath, fooCyclePath);
+ IOException ioExnFromFS =
+ assertThrows(IOException.class, () -> fooCyclePath.statIfFound(Symlinks.FOLLOW));
+ // And it is indeed the case that the FileSystem throws an IOException when the cycle's Path is
+ // stat'd (following symlinks, as legacy globbing does).
+ assertThat(ioExnFromFS).hasMessageThat().contains("Too many levels of symbolic links");
+
+ // Then, when we evaluate the PackageValue node for the Package in keepGoing mode,
+ SkyKey pkgKey = PackageValue.key(PackageIdentifier.parse("@//foo"));
+ EvaluationResult<PackageValue> result =
+ SkyframeExecutorTestUtils.evaluate(
+ getSkyframeExecutor(), pkgKey, /*keepGoing=*/ true, reporter);
+ // The result is a *non-transient* Skyframe error.
+ assertThatEvaluationResult(result).hasErrorEntryForKeyThat(pkgKey).isNotTransient();
+ // And that error is a NoSuchPackageException
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(pkgKey)
+ .hasExceptionThat()
+ .isInstanceOf(NoSuchPackageException.class);
+ // With a useful error message,
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(pkgKey)
+ .hasExceptionThat()
+ .hasMessageThat()
+ .contains("Symlink cycle: /workspace/foo/cycle");
+ // And appropriate Skyframe root cause (N.B. since we want PackageFunction to rethrow in
+ // situations like this, we want the PackageValue node to be its own root cause).
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(pkgKey)
+ .rootCauseOfExceptionIs(pkgKey);
+
+ // Then, when we modify the BUILD file so as to force package loading,
+ scratch.overwriteFile(
+ "foo/BUILD", "glob(['cycle/**/foo.txt']) # dummy comment to force package loading");
+ // But we don't make any filesystem changes that would invalidate the GlobValues, meaning that
+ // PackageFunction will observe cache hits from Skyframe globbing,
+ //
+ // And we also have our filesystem blow up if the directory symlink cycle is encountered (thus,
+ // the absence of a crash indicates the lack of legacy globbing),
+ fs.stubStatError(
+ fooCyclePath,
+ new IOException() {
+ @Override
+ public String getMessage() {
+ throw new IllegalStateException("should't get here!");
+ }
+ });
+ // And we evaluate the PackageValue node for the Package in keepGoing mode,
+ getSkyframeExecutor()
+ .invalidateFilesUnderPathForTesting(
+ reporter,
+ ModifiedFileSet.builder().modify(PathFragment.create("foo/BUILD")).build(),
+ Root.fromPath(rootDirectory));
+ // The results are exactly the same as before,
+ result =
+ SkyframeExecutorTestUtils.evaluate(
+ getSkyframeExecutor(), pkgKey, /*keepGoing=*/ true, reporter);
+ assertThatEvaluationResult(result).hasErrorEntryForKeyThat(pkgKey).isNotTransient();
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(pkgKey)
+ .hasExceptionThat()
+ .isInstanceOf(NoSuchPackageException.class);
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(pkgKey)
+ .hasExceptionThat()
+ .hasMessageThat()
+ .contains("Symlink cycle: /workspace/foo/cycle");
+ assertThatEvaluationResult(result)
+ .hasErrorEntryForKeyThat(pkgKey)
+ .rootCauseOfExceptionIs(pkgKey);
+ // Thus showing that clean and incremental package loading have the same semantics in the
+ // presence of a symlink cycle encountered during glob evaluation.
+ }
+
private static class CustomInMemoryFs extends InMemoryFileSystem {
private abstract static class FileStatusOrException {
abstract FileStatus get() throws IOException;