diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FileSystemValueCheckerInferringAncestors.java b/src/main/java/com/google/devtools/build/lib/skyframe/FileSystemValueCheckerInferringAncestors.java index 6588860a20b12a..18a86fc4a2d868 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FileSystemValueCheckerInferringAncestors.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FileSystemValueCheckerInferringAncestors.java @@ -48,6 +48,7 @@ import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.TreeSet; import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -73,6 +74,7 @@ public final class FileSystemValueCheckerInferringAncestors { private final SyscallCache syscallCache; private final SkyValueDirtinessChecker skyValueDirtinessChecker; + private final Set deletedDirectories = Sets.newConcurrentHashSet(); private final Set valuesToInvalidate = Sets.newConcurrentHashSet(); private final ConcurrentMap valuesToInject = new ConcurrentHashMap<>(); @@ -165,7 +167,7 @@ private static Map makeNodeVisitStates( // list of deleted children and listing. It is possible for the diff to report a deleted // directory without listing all of the files under it as deleted. if (existingState == null) { - state = new NodeVisitState(/*collectMaybeDeletedChildren=*/ path != top); + state = new NodeVisitState(/* collectMaybeDeletedChildren= */ !path.equals(top)); nodeStates.put(path, state); } else { state = existingState; @@ -223,11 +225,51 @@ private ImmutableDiff processEntries(int nThreads) } } + // If any directory was deleted, invalidate all FSVs and DLSVs under it since the diff may only + // report the root of the deleted subtree. + if (!deletedDirectories.isEmpty()) { + var treesToInvalidate = new TreeSet<>(deletedDirectories); + // Optimize the walk over all keys below by trimming those trees that are subtrees of other + // trees to be invalidated. This allows for O(log r) lookup instead of O(n) for each key + // where r is the number of deleted directories that aren't transitive subdirectories of other + // deleted directories and n is the number of deleted directories. + RootedPath lastTree = null; + for (var treeIterator = treesToInvalidate.iterator(); treeIterator.hasNext(); ) { + var tree = treeIterator.next(); + if (lastTree != null && tree.asPath().startsWith(lastTree.asPath())) { + treeIterator.remove(); + } else { + lastTree = tree; + } + } + + // FSVs and DLSVs do not track their parents, so we need to look at all keys. + inMemoryGraph.parallelForEach( + entry -> { + var key = entry.getKey(); + RootedPath path; + if (key instanceof FileStateKey fsk) { + path = fsk.argument(); + } else if (key instanceof DirectoryListingStateValue.Key dlsk) { + path = dlsk.argument(); + } else { + return; + } + var floorPath = treesToInvalidate.floor(path); + if (floorPath != null + && path.asPath().startsWith(floorPath.asPath()) + && !valuesToInject.containsKey(key)) { + valuesToInvalidate.add(key); + } + }); + } + return new ImmutableDiff(valuesToInvalidate, valuesToInject); } private void processEntry(RootedPath path, NodeVisitState state) throws StatFailedException { - NodeVisitState rootParentSentinel = new NodeVisitState(/*collectMaybeDeletedChildren=*/ false); + NodeVisitState rootParentSentinel = + new NodeVisitState(/* collectMaybeDeletedChildren= */ false); while (state != rootParentSentinel) { RootedPath parentPath = path.getParentDirectory(); @@ -299,6 +341,9 @@ && listingHasEntriesOutsideOf(path, maybeDeletedChildren))) { boolean typeChanged = newFsv.getType() != oldFsv.getType(); if (typeChanged) { parentListingKey(path).ifPresent(valuesToInvalidate::add); + if (oldFsv.getType().isDirectory() && !newFsv.getType().exists()) { + deletedDirectories.add(path); + } } return typeChanged; } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileSystemValueCheckerInferringAncestorsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileSystemValueCheckerInferringAncestorsTest.java index d5b8ae406d0bbb..ea72207ace05c7 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileSystemValueCheckerInferringAncestorsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileSystemValueCheckerInferringAncestorsTest.java @@ -624,8 +624,14 @@ public void getDiffWithInferredAncestors_deleteDirReportDirOnly_returnsDir() thr FileStateValue file1Value = fileStateValue("dir/file1"); FileStateKey file2Key = fileStateValueKey("dir/file2"); FileStateValue file2Value = fileStateValue("dir/file2"); + FileStateKey subdirFileKey = fileStateValueKey("dir/subdir/file"); + FileStateValue subdirFileValue = fileStateValue("dir/subdir/file"); FileStateKey dirKey = fileStateValueKey("dir"); FileStateValue dirValue = fileStateValue("dir"); + FileStateKey subdirKey = fileStateValueKey("dir/subdir"); + FileStateValue subdirValue = fileStateValue("dir/subdir"); + SkyKey subdirListingKey = directoryListingStateValueKey("dir/subdir"); + DirectoryListingStateValue subdirListingValue = directoryListingStateValue(file("file")); file1.getParentDirectory().deleteTree(); addDoneNodesAndThenMarkChanged( ImmutableMap.of( @@ -633,8 +639,14 @@ public void getDiffWithInferredAncestors_deleteDirReportDirOnly_returnsDir() thr file1Value, file2Key, file2Value, + subdirFileKey, + subdirFileValue, dirKey, dirValue, + subdirKey, + subdirValue, + subdirListingKey, + subdirListingValue, fileStateValueKey(""), fileStateValue(""))); @@ -650,7 +662,13 @@ public void getDiffWithInferredAncestors_deleteDirReportDirOnly_returnsDir() thr assertThat(diff.changedKeysWithNewValues()) .containsExactly(dirKey, NONEXISTENT_FILE_STATE_NODE_DELTA); assertThat(diff.changedKeysWithoutNewValues()) - .containsExactly(directoryListingStateValueKey("")); + .containsExactly( + directoryListingStateValueKey(""), + file1Key, + file2Key, + subdirKey, + subdirListingKey, + subdirFileKey); assertThat(statedPaths).containsExactly("dir", ""); } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java index 47e75802b924f0..39e2fda5fc6f0c 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessIntegrationTest.java @@ -22,6 +22,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.eventbus.Subscribe; +import com.google.devtools.build.lib.actions.BuildFailedException; import com.google.devtools.build.lib.actions.ChangedFilesMessage; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.buildtool.util.SkyframeIntegrationTestBase; @@ -187,6 +188,97 @@ public void changedFile_statFails_throwsError() throws Exception { assertThat(e).hasCauseThat().hasCauseThat().hasCauseThat().isInstanceOf(IOException.class); } + @Test + public void renameDirectory_thenRenameBackWithRemovedFile_glob() throws Exception { + write( + "foo/BUILD", + """ + genrule( + name = "foo", + srcs = glob(["*dir*/**"]), + outs = ["out"], + cmd = "echo $(SRCS) > $@", + ) + """); + Path dir = write("foo/dir/file1.txt").getParentDirectory(); + write("foo/dir/file2.txt"); + buildTarget("//foo"); + assertContents("foo/dir/file1.txt foo/dir/file2.txt", "//foo"); + + Path newDir = dir.getParentDirectory().getChild("new_dir"); + dir.renameTo(newDir); + buildTargetWithRetryUntilSeesChange("//foo", "foo/dir/file1.txt"); + assertContents("foo/new_dir/file1.txt foo/new_dir/file2.txt", "//foo"); + + newDir.getChild("file2.txt").delete(); + newDir.renameTo(dir); + buildTargetWithRetryUntilSeesChange("//foo", "foo/new_dir/file1.txt"); + assertContents("foo/dir/file1.txt", "//foo"); + } + + @Test + public void renameDirectory_thenRenameBackWithRemovedFile_inputs() throws Exception { + write( + "foo/BUILD", + """ + genrule( + name = "foo", + srcs = ["dir/file1.txt", "dir/file2.txt"], + outs = ["out"], + cmd = "echo $(SRCS) > $@", + ) + """); + Path dir = write("foo/dir/file1.txt").getParentDirectory(); + write("foo/dir/file2.txt"); + buildTarget("//foo"); + assertContents("foo/dir/file1.txt foo/dir/file2.txt", "//foo"); + + Path newDir = dir.getParentDirectory().getChild("new_dir"); + dir.renameTo(newDir); + write( + "foo/BUILD", + """ + genrule( + name = "foo", + srcs = ["new_dir/file1.txt", "new_dir/file2.txt"], + outs = ["out"], + cmd = "echo $(SRCS) > $@", + ) + """); + buildTargetWithRetryUntilSeesChange("//foo", "foo/dir/file1.txt"); + assertContents("foo/new_dir/file1.txt foo/new_dir/file2.txt", "//foo"); + + newDir.getChild("file2.txt").delete(); + newDir.renameTo(dir); + write( + "foo/BUILD", + """ + genrule( + name = "foo", + srcs = ["dir/file1.txt"], + outs = ["out"], + cmd = "echo $(SRCS) > $@", + ) + """); + buildTargetWithRetryUntilSeesChange("//foo", "foo/new_dir/file1.txt"); + assertContents("foo/dir/file1.txt", "//foo"); + + write( + "foo/BUILD", + """ + genrule( + name = "foo", + srcs = ["dir/file2.txt"], + outs = ["out"], + cmd = "echo $(SRCS) > $@", + ) + """); + var e = assertThrows(BuildFailedException.class, () -> buildTarget("//foo")); + // It is important that the error message says "do not exist", not "are in error". The latter + // would indicate that the corresponding FileStateValue still considers the file to exist. + assertThat(e).hasMessageThat().contains("1 input file(s) do not exist"); + } + /** * Runs {@link #buildTarget(String...)} repeatedly until we observe a change for the given path. * diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessTest.java index 551f8d67134d2c..74946f68946ddc 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/LocalDiffAwarenessTest.java @@ -38,14 +38,13 @@ import org.junit.After; import org.junit.Assume; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; import org.junit.rules.TestName; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -/** - * Integration tests for LocalDiffAwareness. - */ +/** Integration tests for LocalDiffAwareness. */ @RunWith(JUnit4.class) public class LocalDiffAwarenessTest extends BuildIntegrationTestCase { @@ -58,11 +57,10 @@ public class LocalDiffAwarenessTest extends BuildIntegrationTestCase { private Path testCaseRoot; private Path testCaseIgnoredDir; - @org.junit.Rule - public TestName name = new TestName(); + @Rule public TestName name = new TestName(); @Before - public final void initializeSettings() throws Exception { + public final void initializeSettings() throws Exception { LocalDiffAwareness.Factory factory = new LocalDiffAwareness.Factory(ImmutableList.of()); // Make sure all test functions have their own directory to test testCaseRoot = testRoot.getChild(name.getMethodName()); @@ -197,11 +195,27 @@ public void testRemoveDirectory() throws Exception { touch("equestria/foo.txt"); new ModifiedFileSetChecker().modify("equestria").modify("equestria/foo.txt").check(); - rm("equestria"); new ModifiedFileSetChecker().modify("equestria").modify("equestria/foo.txt").check(); } + @Test + public void testMoveDirectory() throws Exception { + captureFirstView(watchFsEnabledProvider); + + mkdir("equestria"); + touch("equestria/foo.txt"); + new ModifiedFileSetChecker().modify("equestria").modify("equestria/foo.txt").check(); + + testCaseRoot.getRelative("equestria").renameTo(testCaseRoot.getRelative("equestria2")); + // The contents of a moved directory are *not* reported as modified. + new ModifiedFileSetChecker() + .modify("equestria") + .modify("equestria2") + .modify("equestria2/foo.txt") + .check(); + } + @Test public void testLotsOfChanges() throws Exception { captureFirstView(watchFsEnabledProvider); @@ -220,12 +234,12 @@ public void testLotsOfChanges() throws Exception { mkdir("a/b"); touch("a/b/b1.txt"); new ModifiedFileSetChecker() - .modify("a") - .modify("a/a1.txt") - .modify("a/a2.txt") - .modify("a/b") - .modify("a/b/b1.txt") - .check(); + .modify("a") + .modify("a/a1.txt") + .modify("a/a2.txt") + .modify("a/b") + .modify("a/b/b1.txt") + .check(); rm("a/b/b1.txt"); touch("a/b/b2.txt"); @@ -235,11 +249,11 @@ public void testLotsOfChanges() throws Exception { mkdir("a/b"); touch("a/b/b3.txt"); new ModifiedFileSetChecker() - .modify("a/b") - .modify("a/b/b1.txt") - .modify("a/b/b2.txt") - .modify("a/b/b3.txt") - .check(); + .modify("a/b") + .modify("a/b/b1.txt") + .modify("a/b/b2.txt") + .modify("a/b/b3.txt") + .check(); rm("a/b/b3.txt"); new ModifiedFileSetChecker().modify("a/b/b3.txt").check(); @@ -288,12 +302,12 @@ public void modifiedPathIsntUnderWatchRoot() { View oldView = new LocalDiffAwareness.SequentialView( - localDiff, /*position=*/ 0, /*modifiedAbsolutePaths=*/ ImmutableSet.of()); + localDiff, /* position= */ 0, /* modifiedAbsolutePaths= */ ImmutableSet.of()); View newView = new LocalDiffAwareness.SequentialView( localDiff, - /*position=*/ 1, - /*modifiedAbsolutePaths=*/ ImmutableSet.of( + /* position= */ 1, + /* modifiedAbsolutePaths= */ ImmutableSet.of( otherRootDirectoryNioPath.resolve("foo.txt"))); Throwable throwable = assertThrows(BrokenDiffAwarenessException.class, () -> localDiff.getDiff(oldView, newView));