From 3aae02447b36dd2292348b4f1d699fb6bbaac901 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 15 Apr 2025 14:19:42 +0200 Subject: [PATCH 1/3] Add test for source directory invalidation --- .../google/devtools/build/lib/skyframe/BUILD | 19 ++ .../SourceDirectoryIntegrationTest.java | 232 ++++++++++++++++++ 2 files changed, 251 insertions(+) create mode 100644 src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java 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 32b758740e9a83..58d734feacb8dc 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD @@ -1921,6 +1921,25 @@ java_test( ], ) +java_test( + name = "SourceDirectoryIntegrationTest", + srcs = ["SourceDirectoryIntegrationTest.java"], + jvm_flags = [ + "-DBAZEL_TRACK_SOURCE_DIRECTORIES=1", + ], + deps = [ + "//src/main/java/com/google/devtools/build/lib:runtime", + "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/events", + "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + "//src/test/java/com/google/devtools/build/lib/buildtool/util", + "//third_party:guava", + "//third_party:junit4", + "//third_party:truth", + ], +) + java_test( name = "FileArtifactValueTest", timeout = "short", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java new file mode 100644 index 00000000000000..f6b101be45d21c --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java @@ -0,0 +1,232 @@ +// Copyright 2021 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 org.junit.Assert.assertThrows; + +import com.google.common.collect.ImmutableSet; +import com.google.devtools.build.lib.actions.BuildFailedException; +import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase; +import com.google.devtools.build.lib.events.EventKind; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.PathFragment; +import org.junit.Before; +import org.junit.Ignore; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Integration test for invalidation of actions that consume source directories. */ +@RunWith(JUnit4.class) +public final class SourceDirectoryIntegrationTest extends BuildIntegrationTestCase { + + private Path sourceDir; + + @Override + protected ImmutableSet additionalEventsToCollect() { + return ImmutableSet.of(EventKind.FINISH); + } + + @Before + public void setUpGenrule() throws Exception { + write( + "foo/BUILD", + """ + genrule( + name = "foo", + srcs = ["dir"], + outs = ["foo.out"], + cmd = "touch $@", + ) + """); + + sourceDir = getWorkspace().getRelative("foo/dir"); + sourceDir.createDirectoryAndParents(); + FileSystemUtils.writeIsoLatin1(sourceDir.getRelative("file1"), "content"); + FileSystemUtils.writeIsoLatin1(sourceDir.getRelative("file2"), "content"); + FileSystemUtils.writeIsoLatin1(sourceDir.getRelative("file3"), "other content"); + sourceDir.getRelative("symlink").createSymbolicLink(PathFragment.create("file3")); + sourceDir + .getRelative("dangling_symlink") + .createSymbolicLink(PathFragment.create("does_not_exist")); + + Path subDir = sourceDir.getRelative("subdir"); + subDir.createDirectory(); + FileSystemUtils.writeIsoLatin1(subDir.getRelative("file1"), "content"); + FileSystemUtils.writeIsoLatin1(subDir.getRelative("file2"), "content"); + FileSystemUtils.writeIsoLatin1(subDir.getRelative("file3"), "other content"); + subDir.getRelative("symlink").createSymbolicLink(PathFragment.create("file3")); + subDir + .getRelative("dangling_symlink") + .createSymbolicLink(PathFragment.create("does_not_exist")); + + subDir.getRelative("nested").createDirectory(); + subDir.getRelative("nested2").createDirectory(); + subDir.getRelative("nested_non_empty").createDirectory(); + FileSystemUtils.writeIsoLatin1(subDir.getRelative("nested_non_empty/file1"), "content"); + + buildTarget("//foo"); + assertContainsEvent(events.collector(), "Executing genrule //foo:foo"); + + events.collector().clear(); + } + + @Test + public void nothingModified_doesNotInvalidateAction() throws Exception { + assertNotInvalidatedByBuild(); + } + + @Test + public void touched_doesNotInvalidateAction() throws Exception { + sourceDir.setLastModifiedTime(Path.NOW_SENTINEL_TIME); + assertNotInvalidatedByBuild(); + } + + @Test + public void topLevelFileTouched_doesNotInvalidateAction() throws Exception { + sourceDir.getRelative("file1").setLastModifiedTime(Path.NOW_SENTINEL_TIME); + assertNotInvalidatedByBuild(); + } + + @Test + public void topLevelDirTouched_doesNotInvalidateAction() throws Exception { + sourceDir.getRelative("subdir").setLastModifiedTime(Path.NOW_SENTINEL_TIME); + assertNotInvalidatedByBuild(); + } + + @Test + public void nestedFileTouched_doesNotInvalidateAction() throws Exception { + sourceDir.getRelative("subdir/file1").setLastModifiedTime(Path.NOW_SENTINEL_TIME); + assertNotInvalidatedByBuild(); + } + + @Test + public void nestedDirTouched_doesNotInvalidateAction() throws Exception { + sourceDir.getRelative("subdir/nested").setLastModifiedTime(Path.NOW_SENTINEL_TIME); + assertNotInvalidatedByBuild(); + } + + @Test + public void topLevelFileDeleted_invalidatesAction() throws Exception { + sourceDir.getRelative("file1").delete(); + assertInvalidatedByBuild(); + } + + @Test + public void nestedFileDeleted_invalidatesAction() throws Exception { + sourceDir.getRelative("subdir/file1").delete(); + assertInvalidatedByBuild(); + } + + @Test + public void topLevelFileModified_invalidatesAction() throws Exception { + FileSystemUtils.writeIsoLatin1(sourceDir.getRelative("file1"), "modified content"); + assertInvalidatedByBuild(); + } + + @Test + public void nestedFileModified_invalidatesAction() throws Exception { + FileSystemUtils.writeIsoLatin1(sourceDir.getRelative("subdir/file1"), "modified content"); + assertInvalidatedByBuild(); + } + + @Test + public void topLevelFileAdded_invalidatesAction() throws Exception { + FileSystemUtils.writeIsoLatin1(sourceDir.getRelative("new_file"), "modified content"); + assertInvalidatedByBuild(); + } + + @Test + public void nestedFileAdded_invalidatesAction() throws Exception { + FileSystemUtils.writeIsoLatin1(sourceDir.getRelative("subdir/new_file"), "modified content"); + assertInvalidatedByBuild(); + } + + @Test + public void emptyDirDeleted_invalidatesAction() throws Exception { + sourceDir.getRelative("subdir/nested").delete(); + assertInvalidatedByBuild(); + } + + @Test + public void fileReplacedByIdenticalSymlink_doesNotInvalidateAction() throws Exception { + Path file = sourceDir.getRelative("file1"); + file.delete(); + file.createSymbolicLink(sourceDir.getRelative("file2")); + assertNotInvalidatedByBuild(); + } + + @Test + public void fileReplacedByDifferentSymlink_invalidatesAction() throws Exception { + Path file = sourceDir.getRelative("file1"); + file.delete(); + file.createSymbolicLink(sourceDir.getRelative("file3")); + assertInvalidatedByBuild(); + } + + @Test + @Ignore("TODO(#25834)") + public void emptyDirReplacedWithIdenticalSymlink_doesNotInvalidateAction() throws Exception { + Path dir = sourceDir.getRelative("subdir/nested2"); + dir.delete(); + dir.createSymbolicLink(PathFragment.create("nested")); + assertNotInvalidatedByBuild(); + } + + @Test + public void emptyDirReplacedWithDifferentSymlink_invalidatesAction() throws Exception { + Path dir = sourceDir.getRelative("subdir/nested2"); + dir.delete(); + dir.createSymbolicLink(PathFragment.create("nested_non_empty")); + assertInvalidatedByBuild(); + } + + @Test + public void infiniteSymlinkExpansion() throws Exception { + Path dir = sourceDir.getRelative("subdir/nested2"); + dir.delete(); + dir.createSymbolicLink(PathFragment.create("..")); + assertThrows(BuildFailedException.class, () -> buildTarget("//foo")); + assertContainsEvent("infinite symlink expansion detected"); + assertContainsEvent("foo/dir/subdir/nested2"); + } + + @Test + @Ignore("TODO(#25834)") + public void danglingSymlinkModified_invalidatesAction() throws Exception { + FileSystemUtils.ensureSymbolicLink( + sourceDir.getRelative("dangling_symlink"), PathFragment.create("still_does_not_exist")); + assertInvalidatedByBuild(); + } + + @Test + @Ignore("TODO(#25834)") + public void subPackageAdded_invalidatesAction() throws Exception { + FileSystemUtils.touchFile(sourceDir.getRelative("subdir/BUILD")); + assertInvalidatedByBuild(); + } + + private static final String GENRULE_EVENT = "Executing genrule //foo:foo"; + + private void assertInvalidatedByBuild() throws Exception { + buildTarget("//foo"); + assertContainsEvent(events.collector(), GENRULE_EVENT); + } + + private void assertNotInvalidatedByBuild() throws Exception { + buildTarget("//foo"); + assertDoesNotContainEvent(events.collector(), GENRULE_EVENT); + } +} From 1aaf2517effd8cd031c0abb482d6127697125702 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 15 Apr 2025 19:13:14 +0200 Subject: [PATCH 2/3] Prohibit subpackage traversals --- .../google/devtools/build/lib/skyframe/BUILD | 1 + .../SourceDirectoryIntegrationTest.java | 30 +++++++++++-------- 2 files changed, 18 insertions(+), 13 deletions(-) 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 58d734feacb8dc..501281fc73c2fc 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD @@ -1930,6 +1930,7 @@ java_test( deps = [ "//src/main/java/com/google/devtools/build/lib:runtime", "//src/main/java/com/google/devtools/build/lib/actions", + "//src/main/java/com/google/devtools/build/lib/bugreport", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java index f6b101be45d21c..20d6b0132a2863 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java @@ -17,6 +17,7 @@ import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.BuildFailedException; +import com.google.devtools.build.lib.bugreport.BugReport; import com.google.devtools.build.lib.buildtool.util.BuildIntegrationTestCase; import com.google.devtools.build.lib.events.EventKind; import com.google.devtools.build.lib.vfs.FileSystemUtils; @@ -193,16 +194,6 @@ public void emptyDirReplacedWithDifferentSymlink_invalidatesAction() throws Exce assertInvalidatedByBuild(); } - @Test - public void infiniteSymlinkExpansion() throws Exception { - Path dir = sourceDir.getRelative("subdir/nested2"); - dir.delete(); - dir.createSymbolicLink(PathFragment.create("..")); - assertThrows(BuildFailedException.class, () -> buildTarget("//foo")); - assertContainsEvent("infinite symlink expansion detected"); - assertContainsEvent("foo/dir/subdir/nested2"); - } - @Test @Ignore("TODO(#25834)") public void danglingSymlinkModified_invalidatesAction() throws Exception { @@ -212,10 +203,23 @@ public void danglingSymlinkModified_invalidatesAction() throws Exception { } @Test - @Ignore("TODO(#25834)") - public void subPackageAdded_invalidatesAction() throws Exception { + public void crossingPackageBoundary_fails() throws Exception { FileSystemUtils.touchFile(sourceDir.getRelative("subdir/BUILD")); - assertInvalidatedByBuild(); + // TODO(#25834): This should not crash Bazel. + assertThrows(IllegalStateException.class, () -> buildTarget("//foo")); + BugReport.getAndResetLastCrashingThrowableIfInTest(); + assertContainsEvent( + "Directory artifact foo/dir crosses package boundary into package rooted at foo/dir/subdir"); + } + + @Test + public void infiniteSymlinkExpansion_fails() throws Exception { + Path dir = sourceDir.getRelative("subdir/nested2"); + dir.delete(); + dir.createSymbolicLink(PathFragment.create("..")); + assertThrows(BuildFailedException.class, () -> buildTarget("//foo")); + assertContainsEvent("infinite symlink expansion detected"); + assertContainsEvent("foo/dir/subdir/nested2"); } private static final String GENRULE_EVENT = "Executing genrule //foo:foo"; From 3e2fb54a1cb0e89652caf15536501e9f3b04ab99 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 15 Apr 2025 19:49:57 +0200 Subject: [PATCH 3/3] Include unresolved symlink target into the source directory hash This makes it so that changes to the targets of unresolved symlinks invalidate actions that depend on the source directory containing them. --- .../RecursiveFilesystemTraversalFunction.java | 4 +--- .../RecursiveFilesystemTraversalValue.java | 14 +++++++++++--- .../RecursiveFilesystemTraversalFunctionTest.java | 5 ++--- .../skyframe/SourceDirectoryIntegrationTest.java | 11 ++++++++++- 4 files changed, 24 insertions(+), 10 deletions(-) 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 b248a14722bb07..2d341fa985e82c 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 @@ -187,9 +187,7 @@ public RecursiveFilesystemTraversalValue compute(SkyKey skyKey, Environment env) if (rootInfo.type.isSymlink()) { return RecursiveFilesystemTraversalValue.of( ResolvedFileFactory.danglingSymlink( - traversal.root().asRootedPath(), - rootInfo.unresolvedSymlinkTarget, - rootInfo.metadata)); + traversal.root().asRootedPath(), rootInfo.unresolvedSymlinkTarget)); } else { return RecursiveFilesystemTraversalValue.EMPTY; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java index 72a1aba0ac2c21..ce9fdf833308f0 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/RecursiveFilesystemTraversalValue.java @@ -14,12 +14,14 @@ package com.google.devtools.build.lib.skyframe; import static com.google.common.base.Preconditions.checkNotNull; +import static java.nio.charset.StandardCharsets.ISO_8859_1; import com.google.common.base.Objects; import com.google.devtools.build.lib.actions.HasDigest; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder; import com.google.devtools.build.lib.collect.nestedset.Order; +import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.SkyValue; @@ -574,9 +576,15 @@ public static ResolvedFile symlinkToDirectory( return new SymlinkToDirectory(targetPath, linkNamePath, linkValue, metadata); } - public static ResolvedFile danglingSymlink( - RootedPath linkNamePath, PathFragment linkValue, HasDigest metadata) { - return new DanglingSymlink(linkNamePath, linkValue, metadata); + public static ResolvedFile danglingSymlink(RootedPath linkNamePath, PathFragment linkValue) { + byte[] digest = + DigestHashFunction.SHA256 + .getHashFunction() + .hashString(linkValue.getPathString(), ISO_8859_1) + .asBytes(); + // Ensure that the digest does not collide with that of a regular file. + digest[0] ^= 1; + return new DanglingSymlink(linkNamePath, linkValue, new HasDigest.ByteStringDigest(digest)); } } 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 85b42897f4d7d4..3ba84d85ac1eda 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 @@ -830,8 +830,7 @@ public void testTraversalOfDanglingSymlink() throws Exception { PathFragment linkTarget = PathFragment.create("non_existent"); parentOf(link).asPath().createDirectory(); link.asPath().createSymbolicLink(linkTarget); - traverseAndAssertFiles( - fileLikeRoot(linkArtifact), danglingSymlink(link, linkTarget, EMPTY_METADATA)); + traverseAndAssertFiles(fileLikeRoot(linkArtifact), danglingSymlink(link, linkTarget)); } @Test @@ -845,7 +844,7 @@ public void testTraversalOfDanglingSymlinkInADirectory() throws Exception { traverseAndAssertFiles( fileLikeRoot(dirArtifact), regularFile(file, EMPTY_METADATA), - danglingSymlink(link, linkTarget, EMPTY_METADATA)); + danglingSymlink(link, linkTarget)); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java index 20d6b0132a2863..5ccecbc3d8834a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/SourceDirectoryIntegrationTest.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import static java.nio.charset.StandardCharsets.ISO_8859_1; import static org.junit.Assert.assertThrows; import com.google.common.collect.ImmutableSet; @@ -195,13 +196,21 @@ public void emptyDirReplacedWithDifferentSymlink_invalidatesAction() throws Exce } @Test - @Ignore("TODO(#25834)") public void danglingSymlinkModified_invalidatesAction() throws Exception { FileSystemUtils.ensureSymbolicLink( sourceDir.getRelative("dangling_symlink"), PathFragment.create("still_does_not_exist")); assertInvalidatedByBuild(); } + @Test + public void danglingSymlinkReplacedWithFile_invalidatesAction() throws Exception { + Path danglingSymlink = sourceDir.getRelative("dangling_symlink"); + String target = danglingSymlink.readSymbolicLink().getPathString(); + danglingSymlink.delete(); + FileSystemUtils.writeContent(danglingSymlink, ISO_8859_1, target); + assertInvalidatedByBuild(); + } + @Test public void crossingPackageBoundary_fails() throws Exception { FileSystemUtils.touchFile(sourceDir.getRelative("subdir/BUILD"));