Track empty directories with BAZEL_TRACK_SOURCE_DIRECTORIES
RecursiveFilesystemTraversalFunction did not emit a ResolvedFile for
empty directories and thus didn't track their existence at all since
they have no children that would track it implicitly.
This commit adds the tracking of empty directories for
DirectoryArtifactTraversalRequest only, which is used to implement the
BAZEL_TRACK_SOURCE_DIRECTORIES flag.
Also adds a basic test for the source directory tracking functionality.
Closes #15774.
PiperOrigin-RevId: 468804794
Change-Id: I2c06c05b335781234d4fa90f3d8c1e7fe6dd184a
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
index 8e3adbe..326e331 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
@@ -637,6 +637,11 @@
}
@Override
+ protected boolean emitEmptyDirectoryNodes() {
+ return true;
+ }
+
+ @Override
protected String errorInfo() {
return "Directory artifact " + artifact.prettyPrint();
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesetTraversalRequest.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesetTraversalRequest.java
index 650c21b..e98ff06 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesetTraversalRequest.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesetTraversalRequest.java
@@ -65,6 +65,11 @@
}
@Override
+ protected boolean emitEmptyDirectoryNodes() {
+ return false;
+ }
+
+ @Override
protected final String errorInfo() {
return String.format(
"Fileset '%s' traversing %s '%s'",
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
index a50cc3f..de37dd5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunction.java
@@ -620,6 +620,9 @@
paths = NestedSetBuilder.<ResolvedFile>stableOrder().addTransitive(children).add(root);
} else {
root = ResolvedFileFactory.directory(rootInfo.realPath);
+ if (traversal.emitEmptyDirectoryNodes() && paths.isEmpty()) {
+ paths.add(root);
+ }
}
return RecursiveFilesystemTraversalValue.of(root, paths.build());
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TraversalRequest.java b/src/main/java/com/google/devtools/build/lib/skyframe/TraversalRequest.java
index 55871f1..2073b72 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/TraversalRequest.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/TraversalRequest.java
@@ -53,6 +53,14 @@
protected abstract boolean skipTestingForSubpackage();
/**
+ * Whether to emit nodes for empty directories.
+ *
+ * <p>If this returns false, empty directories will not be represented in the result of the
+ * traversal.
+ */
+ protected abstract boolean emitEmptyDirectoryNodes();
+
+ /**
* Returns information to be attached to any error messages that may be reported.
*
* <p>This is purely informational and is not considered in equality.
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
index b6806de..716067f 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -20,6 +20,7 @@
"SkyframeErrorProcessorTest.java",
"BuildDriverFunctionTest.java",
"SkyFunctionEnvironmentForTesting.java",
+ "TrackSourceDirectoriesIntegrationTest.java",
]
java_library(
@@ -433,3 +434,22 @@
"//third_party:truth",
],
)
+
+java_test(
+ name = "TrackSourceDirectoriesIntegrationTest",
+ srcs = ["TrackSourceDirectoriesIntegrationTest.java"],
+ jvm_flags = ["-DBAZEL_TRACK_SOURCE_DIRECTORIES=1"],
+ deps = [
+ "//src/main/java/com/google/devtools/build/lib/events",
+ "//src/main/java/com/google/devtools/build/lib/skyframe:track_source_directories_flag",
+ "//src/main/java/com/google/devtools/build/lib/vfs",
+ "//src/test/java/com/google/devtools/build/lib/buildtool/util",
+ "//src/test/java/com/google/devtools/build/lib/events:testutil",
+ "//src/test/java/com/google/devtools/build/lib/testutil:JunitUtils",
+ "//src/test/java/com/google/devtools/build/skyframe:testutil",
+ "//third_party:guava",
+ "//third_party:junit4",
+ "//third_party:truth",
+ "@com_google_testparameterinjector//:testparameterinjector",
+ ],
+)
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
index 166ca3a..2275716 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalFunctionTest.java
@@ -64,6 +64,7 @@
import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalFunction.DanglingSymlinkException;
import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalFunction.RecursiveFilesystemTraversalException;
import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalValue.ResolvedFile;
+import com.google.devtools.build.lib.skyframe.RecursiveFilesystemTraversalValue.ResolvedFileFactory;
import com.google.devtools.build.lib.testutil.FoundationTestCase;
import com.google.devtools.build.lib.testutil.TimestampGranularityUtils;
import com.google.devtools.build.lib.util.io.OutErr;
@@ -304,13 +305,22 @@
}
private static TraversalRequest fileLikeRoot(
- Artifact file, PackageBoundaryMode pkgBoundaryMode, boolean strictOutput) {
+ Artifact file,
+ PackageBoundaryMode pkgBoundaryMode,
+ boolean strictOutput,
+ boolean emitEmptyDirectoryNodes) {
return new AutoValue_RecursiveFilesystemTraversalFunctionTest_BasicTraversalRequest(
DirectTraversalRoot.forFileOrDirectory(file),
/*isRootGenerated=*/ !file.isSourceArtifact(),
pkgBoundaryMode,
strictOutput,
- /*skipTestingForSubpackage=*/ false);
+ /*skipTestingForSubpackage=*/ false,
+ emitEmptyDirectoryNodes);
+ }
+
+ private static TraversalRequest fileLikeRoot(
+ Artifact file, PackageBoundaryMode pkgBoundaryMode, boolean strictOutput) {
+ return fileLikeRoot(file, pkgBoundaryMode, strictOutput, /*emitEmptyDirectoryNodes=*/ false);
}
private static TraversalRequest fileLikeRoot(Artifact file, PackageBoundaryMode pkgBoundaryMode) {
@@ -324,7 +334,8 @@
/*isRootGenerated=*/ false,
pkgBoundaryMode,
/*strictOutputFiles=*/ false,
- /*skipTestingForSubpackage=*/ true);
+ /*skipTestingForSubpackage=*/ true,
+ /*emitEmptyDirectoryNodes=*/ false);
}
@AutoValue
@@ -343,7 +354,8 @@
isRootGenerated(),
crossPkgBoundaries(),
strictOutputFiles(),
- skipTestingForSubpackage);
+ skipTestingForSubpackage,
+ emitEmptyDirectoryNodes());
}
}
@@ -671,9 +683,29 @@
appendToFile(someFile, "not all changes are treated equal");
RecursiveFilesystemTraversalValue v4 =
traverseAndAssertFiles(traversalRoot, expected1, expected2, expected3);
- assertThat(v4).isEqualTo(v3);
+ assertThat(v4).isSameInstanceAs(v3);
assertTraversalRootHashesAreEqual(v3, v4);
assertThat(progressReceiver.invalidations).doesNotContain(traversalRoot);
+
+ // Add a new empty subdirectory to the directory and see that the value is rebuilt, but results
+ // in the collection of files.
+ // TODO(#15901): Empty directories currently aren't representable as tree artifact contents and
+ // thus aren't tested here.
+ if (!directoryArtifact.isTreeArtifact()) {
+ childOf(directoryArtifact, "empty_dir").asPath().createDirectory();
+ if (directoryArtifact.isSourceArtifact()) {
+ invalidateDirectory(directoryArtifact);
+ } else {
+ invalidateOutputArtifact(directoryArtifact);
+ }
+
+ RecursiveFilesystemTraversalValue v5 =
+ traverseAndAssertFiles(traversalRoot, expected1, expected2, expected3);
+ assertThat(v5.getResolvedRoot()).isEqualTo(v4.getResolvedRoot());
+ assertThat(v5.getTransitiveFiles().toList())
+ .containsExactlyElementsIn(v4.getTransitiveFiles().toList());
+ assertThat(progressReceiver.invalidations).contains(traversalRoot);
+ }
}
@Test
@@ -694,6 +726,43 @@
}
@Test
+ public void testTraversalOfSourceDirectoryWithEmptyDirectoryNodes() throws Exception {
+ Artifact directoryArtifact = sourceArtifact("dir");
+ directoryArtifact.getPath().createDirectoryAndParents();
+
+ TraversalRequest traversalRoot =
+ fileLikeRoot(
+ directoryArtifact,
+ DONT_CROSS,
+ /*strictOutput=*/ false,
+ /*emitEmptyDirectoryNodes=*/ true);
+
+ // Assert that the SkyValue is built and looks right.
+ ResolvedFile rootNode =
+ ResolvedFileFactory.directory(
+ RootedPath.toRootedPath(
+ directoryArtifact.getRoot().getRoot(), directoryArtifact.getRootRelativePath()));
+ RecursiveFilesystemTraversalValue v1 = traverseAndAssertFiles(traversalRoot, rootNode);
+ assertThat(progressReceiver.invalidations).isEmpty();
+ assertThat(progressReceiver.evaluations).contains(traversalRoot);
+ progressReceiver.clear();
+
+ // Add a new file to the directory and see that the value is rebuilt.
+ RootedPath emptyDir = childOf(directoryArtifact, "empty_dir");
+ emptyDir.asPath().createDirectory();
+ ResolvedFile emptyDirNode = ResolvedFileFactory.directory(emptyDir);
+ invalidateDirectory(directoryArtifact);
+
+ // The value only contains nodes for empty directories - the root dir is no longer empty at this
+ // point and thus not represented as a node.
+ RecursiveFilesystemTraversalValue v2 = traverseAndAssertFiles(traversalRoot, emptyDirNode);
+ assertThat(v2).isNotEqualTo(v1);
+ assertTraversalRootHashesAreEqual(v1, v2);
+ assertThat(progressReceiver.invalidations).contains(traversalRoot);
+ assertThat(progressReceiver.evaluations).contains(traversalRoot);
+ }
+
+ @Test
public void testTraversalOfTransitiveSymlinkToDirectory() throws Exception {
Artifact directLinkArtifact = sourceArtifact("direct/dir.sym");
Artifact transitiveLinkArtifact = sourceArtifact("transitive/sym.sym");
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
new file mode 100644
index 0000000..f9534f4
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TrackSourceDirectoriesIntegrationTest.java
@@ -0,0 +1,139 @@
+// Copyright 2022 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.skyframe;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.assertContainsEvent;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.assertDoesNotContainEvent;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase;
+import com.google.devtools.build.lib.events.EventKind;
+import com.google.devtools.build.lib.events.util.EventCollectionApparatus;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.testing.junit.testparameterinjector.TestParameter;
+import com.google.testing.junit.testparameterinjector.TestParameterInjector;
+import java.io.IOException;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+/** Tests for {@link TrackSourceDirectoriesFlag}. */
+@RunWith(TestParameterInjector.class)
+public final class TrackSourceDirectoriesIntegrationTest extends BuildIntegrationTestCase {
+
+ @Override
+ protected EventCollectionApparatus createEvents() {
+ return new EventCollectionApparatus(ImmutableSet.of(EventKind.FINISH));
+ }
+
+ @BeforeClass
+ public static void checkTrackSourceDirectoriesFlag() {
+ // Enabled via jvm_flags.
+ assertThat(TrackSourceDirectoriesFlag.trackSourceDirectories()).isTrue();
+ }
+
+ @Test
+ public void build_unchangedSourceDirectory_doesNotRebuild() throws Exception {
+ getWorkspace().getRelative("pkg/dir/empty_dir").createDirectoryAndParents();
+ write("pkg/dir/file", "foo");
+ write(
+ "pkg/BUILD",
+ "genrule(",
+ " name = 'a',",
+ " srcs = ['dir'],",
+ " outs = ['out'],",
+ " cmd = 'touch $@',",
+ ")");
+
+ String testTarget = "//pkg:a";
+ String testTargetRebuildsEvent = "Executing genrule " + testTarget;
+
+ // Initial build
+ buildTarget(testTarget);
+ assertContainsEvent(events.collector(), testTargetRebuildsEvent);
+ events.collector().clear();
+
+ // Verify that the target doesn't rebuild without changes.
+ buildTarget(testTarget);
+
+ assertDoesNotContainEvent(events.collector(), testTargetRebuildsEvent);
+ }
+
+ private enum Change {
+ CREATE_EMPTY_DIRECTORY {
+ @Override
+ void apply(Path sourceDirectory) throws IOException {
+ sourceDirectory.getRelative("empty_dir/nested_empty_dir").createDirectory();
+ }
+ },
+ CREATE_EMPTY_FILE_IN_EMPTY_DIRECTORY {
+ @Override
+ void apply(Path sourceDirectory) throws IOException {
+ FileSystemUtils.createEmptyFile(sourceDirectory.getRelative("empty_dir/file"));
+ }
+ },
+ REMOVE_EMPTY_DIRECTORY {
+ @Override
+ void apply(Path sourceDirectory) throws IOException {
+ sourceDirectory.getChild("empty_dir").deleteTree();
+ }
+ },
+ REPLACE_EMPTY_DIRECTORY_WITH_EMPTY_FILE {
+ @Override
+ void apply(Path sourceDirectory) throws IOException {
+ sourceDirectory.getChild("empty_dir").deleteTree();
+ FileSystemUtils.createEmptyFile(sourceDirectory.getChild("empty_dir"));
+ }
+ },
+ CHANGE_FILE_CONTENT {
+ @Override
+ void apply(Path sourceDirectory) throws IOException {
+ FileSystemUtils.writeContentAsLatin1(sourceDirectory.getChild("file"), "changed");
+ }
+ };
+
+ abstract void apply(Path sourceDirectory) throws IOException;
+ }
+
+ @Test
+ public void build_changedSourceDirectory_rebuildsTarget(@TestParameter Change change)
+ throws Exception {
+ getWorkspace().getRelative("pkg/dir/empty_dir").createDirectoryAndParents();
+ write("pkg/dir/file", "foo");
+ write(
+ "pkg/BUILD",
+ "genrule(",
+ " name = 'a',",
+ " srcs = ['dir'],",
+ " outs = ['out'],",
+ " cmd = 'touch $@',",
+ ")");
+
+ String testTarget = "//pkg:a";
+ String testTargetRebuildsEvent = "Executing genrule " + testTarget;
+
+ // Initial build
+ buildTarget(testTarget);
+ assertContainsEvent(events.collector(), testTargetRebuildsEvent);
+ events.collector().clear();
+
+ // Change source directory and verify that the target is rebuilt as expected.
+ change.apply(getWorkspace().getRelative("pkg/dir"));
+ buildTarget(testTarget);
+
+ assertContainsEvent(events.collector(), testTargetRebuildsEvent);
+ }
+}
