From 2022ad876a991609984e42030453602bac9f2882 Mon Sep 17 00:00:00 2001 From: Nathan Harmata Date: Mon, 22 Feb 2016 23:04:14 +0000 Subject: Fix blatant bug with Skyframe globbing where we incorrectly allow dangling symlinks to match a glob pattern. This bug has/had two consequences: (1) Change pruning will incorrectly cut off changes to GlobValues that ought to now match more files (say, if a dangling symlink comes into existence), causing a package to be incorrectly incrementally not re-loaded. (2) After a recent change to PackageFunction where we use a fancy hybrid globbing approach, we use skyframe globbing on incremental package loading. So if a re-loaded package has the same glob pattern but this glob pattern incorrectly matches a dangling symlink, the re-loaded package will incorrectly have a target for the dangling symlink path. -- MOS_MIGRATED_REVID=115274842 --- .../devtools/build/lib/skyframe/GlobFunction.java | 3 ++ .../build/lib/skyframe/GlobFunctionTest.java | 9 ++++ .../build/lib/skyframe/PackageFunctionTest.java | 57 ++++++++++++++++++++++ 3 files changed, 69 insertions(+) (limited to 'src') 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 50b7c294c7..e1c877f455 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 @@ -146,6 +146,9 @@ public final class GlobFunction implements SkyFunction { "readdir and stat disagree about whether " + symlinkRootedPath.asPath() + " is a symlink."), Transience.TRANSIENT); } + if (!symlinkFileValue.exists()) { + continue; + } isDirectory = symlinkFileValue.isDirectory(); } 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 32dffabba7..0246f864e2 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 @@ -676,6 +676,15 @@ public abstract class GlobFunctionTest { assertThat(errorInfo.getException().getMessage()).contains(expectedMessage); } + @Test + public void testSymlinks() throws Exception { + FileSystemUtils.createDirectoryAndParents(pkgPath.getRelative("symlinks")); + FileSystemUtils.ensureSymbolicLink(pkgPath.getRelative("symlinks/dangling.txt"), "nope"); + FileSystemUtils.createEmptyFile(pkgPath.getRelative("symlinks/yup")); + FileSystemUtils.ensureSymbolicLink(pkgPath.getRelative("symlinks/existing.txt"), "yup"); + assertGlobMatches("symlinks/*.txt", "symlinks/existing.txt"); + } + private class CustomInMemoryFs extends InMemoryFileSystem { private Map stubbedStats = Maps.newHashMap(); 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 5a5d7cc913..8cd8003f26 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 @@ -29,6 +29,7 @@ import com.google.devtools.build.lib.analysis.util.BuildViewTestCase; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; import com.google.devtools.build.lib.packages.ConstantRuleVisibility; +import com.google.devtools.build.lib.packages.NoSuchTargetException; import com.google.devtools.build.lib.packages.Preprocessor; import com.google.devtools.build.lib.packages.util.SubincludePreprocessor; import com.google.devtools.build.lib.pkgcache.PathPackageLocator; @@ -513,6 +514,62 @@ public class PackageFunctionTest extends BuildViewTestCase { assertTrue(result.get(skyKey).getPackage().containsErrors()); } + // Regression test for the two ugly consequences of a bug where GlobFunction incorrectly matched + // dangling symlinks. + @Test + public void testIncrementalSkyframeHybridGlobbingOnDanglingSymlink() throws Exception { + Path packageDirPath = scratch.file("foo/BUILD", + "exports_files(glob(['*.txt']))").getParentDirectory(); + scratch.file("foo/existing.txt"); + FileSystemUtils.ensureSymbolicLink(packageDirPath.getChild("dangling.txt"), "nope"); + + getSkyframeExecutor().preparePackageLoading( + new PathPackageLocator(outputBase, ImmutableList.of(rootDirectory)), + ConstantRuleVisibility.PUBLIC, true, + 7, "", UUID.randomUUID()); + + SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("foo")); + PackageValue value = validPackage(skyKey); + assertFalse(value.getPackage().containsErrors()); + assertThat(value.getPackage().getTarget("existing.txt").getName()).isEqualTo("existing.txt"); + try { + value.getPackage().getTarget("dangling.txt"); + fail(); + } catch (NoSuchTargetException expected) { + } + + scratch.overwriteFile("foo/BUILD", + "exports_files(glob(['*.txt'])),", + "#some-irrelevant-comment"); + + getSkyframeExecutor().invalidateFilesUnderPathForTesting(reporter, + ModifiedFileSet.builder().modify(new PathFragment("foo/BUILD")).build(), rootDirectory); + + value = validPackage(skyKey); + assertFalse(value.getPackage().containsErrors()); + assertThat(value.getPackage().getTarget("existing.txt").getName()).isEqualTo("existing.txt"); + try { + value.getPackage().getTarget("dangling.txt"); + fail(); + } catch (NoSuchTargetException expected) { + // One consequence of the bug was that dangling symlinks were matched by globs evaluated by + // Skyframe globbing, meaning there would incorrectly be corresponding targets in packages + // that had skyframe cache hits during skyframe hybrid globbing. + } + + scratch.file("foo/nope"); + getSkyframeExecutor().invalidateFilesUnderPathForTesting(reporter, + ModifiedFileSet.builder().modify(new PathFragment("foo/nope")).build(), rootDirectory); + + PackageValue newValue = validPackage(skyKey); + assertFalse(newValue.getPackage().containsErrors()); + assertThat(newValue.getPackage().getTarget("existing.txt").getName()).isEqualTo("existing.txt"); + // Another consequence of the bug is that change pruning would incorrectly cut off changes that + // caused a dangling symlink potentially matched by a glob to come into existence. + assertThat(newValue.getPackage().getTarget("dangling.txt").getName()).isEqualTo("dangling.txt"); + assertThat(newValue.getPackage()).isNotSameAs(value.getPackage()); + } + private static class CustomInMemoryFs extends InMemoryFileSystem { private abstract static class FileStatusOrException { abstract FileStatus get() throws IOException; -- cgit v1.2.3