aboutsummaryrefslogtreecommitdiffhomepage
path: root/src
diff options
context:
space:
mode:
authorGravatar Nathan Harmata <nharmata@google.com>2016-02-22 23:04:14 +0000
committerGravatar Damien Martin-Guillerez <dmarting@google.com>2016-02-23 13:08:38 +0000
commit2022ad876a991609984e42030453602bac9f2882 (patch)
tree05db5b6a99d858b96d0afa134b04f76368daa74a /src
parent3f8fe0ded2a05b8e971c68c18cf820faba01fa1f (diff)
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
Diffstat (limited to 'src')
-rw-r--r--src/main/java/com/google/devtools/build/lib/skyframe/GlobFunction.java3
-rw-r--r--src/test/java/com/google/devtools/build/lib/skyframe/GlobFunctionTest.java9
-rw-r--r--src/test/java/com/google/devtools/build/lib/skyframe/PackageFunctionTest.java57
3 files changed, 69 insertions, 0 deletions
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<Path, FileStatus> 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;