diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java index 7bd727db1c78ae..e3f6c739eafee9 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkState; import static java.util.concurrent.TimeUnit.MINUTES; +import com.google.auto.value.AutoValue; import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; @@ -512,7 +513,7 @@ private FileArtifactValue constructFileArtifactValue( throws IOException { checkState(!artifact.isTreeArtifact(), "%s is a tree artifact", artifact); - FileArtifactValue value = + var statAndValue = fileArtifactValueFromArtifact( artifact, artifactPathResolver, @@ -522,6 +523,7 @@ private FileArtifactValue constructFileArtifactValue( // Prevent constant metadata artifacts from notifying the timestamp granularity monitor // and potentially delaying the build for no reason. artifact.isConstantMetadata() ? null : tsgm); + var value = statAndValue.fileArtifactValue(); // Ensure that we don't have both an injected digest and a digest from the filesystem. byte[] fileDigest = value.getDigest(); @@ -568,8 +570,17 @@ private FileArtifactValue constructFileArtifactValue( if (injectedDigest == null && type.isFile()) { // We don't have an injected digest and there is no digest in the file value (which attempts a // fast digest). Manually compute the digest instead. - injectedDigest = - DigestUtils.manuallyComputeDigest(artifactPathResolver.toPath(artifact), value.getSize()); + Path path = statAndValue.pathNoFollow(); + if (statAndValue.statNoFollow() != null + && statAndValue.statNoFollow().isSymbolicLink() + && statAndValue.realPath() != null) { + // If the file is a symlink, we compute the digest using the target path so that it's + // possible to hit the digest cache - we probably already computed the digest for the + // target during previous action execution. + path = statAndValue.realPath(); + } + + injectedDigest = DigestUtils.manuallyComputeDigest(path, value.getSize()); } return FileArtifactValue.createFromInjectedDigest(value, injectedDigest); } @@ -594,10 +605,32 @@ static FileArtifactValue fileArtifactValueFromArtifact( statNoFollow, /*digestWillBeInjected=*/ false, xattrProvider, - tsgm); + tsgm).fileArtifactValue(); + } + + @AutoValue + abstract static class FileArtifactStatAndValue { + public static FileArtifactStatAndValue create( + Path pathNoFollow, + @Nullable Path realPath, + @Nullable FileStatusWithDigest statNoFollow, + FileArtifactValue fileArtifactValue) { + return new AutoValue_ActionMetadataHandler_FileArtifactStatAndValue( + pathNoFollow, realPath, statNoFollow, fileArtifactValue); + } + + public abstract Path pathNoFollow(); + + @Nullable + public abstract Path realPath(); + + @Nullable + public abstract FileStatusWithDigest statNoFollow(); + + public abstract FileArtifactValue fileArtifactValue(); } - private static FileArtifactValue fileArtifactValueFromArtifact( + private static FileArtifactStatAndValue fileArtifactValueFromArtifact( Artifact artifact, ArtifactPathResolver artifactPathResolver, @Nullable FileStatusWithDigest statNoFollow, @@ -611,7 +644,8 @@ private static FileArtifactValue fileArtifactValueFromArtifact( // If we expect a symlink, we can readlink it directly and handle errors appropriately - there // is no need for the stat below. if (artifact.isSymlink()) { - return FileArtifactValue.createForUnresolvedSymlink(pathNoFollow); + var fileArtifactValue = FileArtifactValue.createForUnresolvedSymlink(pathNoFollow); + return FileArtifactStatAndValue.create(pathNoFollow, /* realPath= */ null, statNoFollow, fileArtifactValue); } RootedPath rootedPathNoFollow = @@ -628,8 +662,9 @@ private static FileArtifactValue fileArtifactValueFromArtifact( } if (statNoFollow == null || !statNoFollow.isSymbolicLink()) { - return fileArtifactValueFromStat( + var fileArtifactValue = fileArtifactValueFromStat( rootedPathNoFollow, statNoFollow, digestWillBeInjected, xattrProvider, tsgm); + return FileArtifactStatAndValue.create(pathNoFollow, /* realPath= */ null, statNoFollow, fileArtifactValue); } // We use FileStatus#isSymbolicLink over Path#isSymbolicLink to avoid the unnecessary stat @@ -649,8 +684,9 @@ private static FileArtifactValue fileArtifactValueFromArtifact( // and is a source file (since changes to those are checked separately). FileStatus realStat = realRootedPath.asPath().statIfFound(Symlinks.NOFOLLOW); FileStatusWithDigest realStatWithDigest = FileStatusWithDigestAdapter.maybeAdapt(realStat); - return fileArtifactValueFromStat( + var fileArtifactValue = fileArtifactValueFromStat( realRootedPath, realStatWithDigest, digestWillBeInjected, xattrProvider, tsgm); + return FileArtifactStatAndValue.create(pathNoFollow, realPath, statNoFollow, fileArtifactValue); } private static FileArtifactValue fileArtifactValueFromStat( diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 3956783094310f..42a558cc6a59b4 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -521,6 +521,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/util/io", "//src/main/java/com/google/devtools/build/lib/vfs", "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment", + "//third_party:auto_value", "//third_party:flogger", "//third_party:guava", "//third_party:jsr305", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java index f70a7f951e9a78..ebc04c3ba3149c 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandlerTest.java @@ -41,6 +41,7 @@ import com.google.devtools.build.lib.testutil.Scratch; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.DigestHashFunction; +import com.google.devtools.build.lib.vfs.DigestUtils; import com.google.devtools.build.lib.vfs.OutputPermissions; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -756,4 +757,32 @@ public void fileArtifactValueFromArtifactCompatibleWithGetMetadata_notChanged() assertThat(fileArtifactValueFromArtifactResult.couldBeModifiedSince(getMetadataResult)) .isFalse(); } + + @Test + public void fileArtifactValueForSymlink_readFromCache() throws Exception { + DigestUtils.configureCache(1); + Artifact target = + ActionsTestUtil.createArtifactWithRootRelativePath( + outputRoot, PathFragment.create("bin/target")); + scratch.file(target.getPath().getPathString(), "contents"); + Artifact symlink = + ActionsTestUtil.createArtifactWithRootRelativePath( + outputRoot, PathFragment.create("bin/symlink")); + scratch + .getFileSystem() + .getPath(symlink.getPath().getPathString()) + .createSymbolicLink(scratch.getFileSystem().getPath(target.getPath().getPathString())); + ActionMetadataHandler handler = + createHandler( + new ActionInputMap(0), + /* forInputDiscovery= */ false, + /* outputs= */ ImmutableSet.of(target, symlink)); + var targetMetadata = handler.getMetadata(target); + assertThat(DigestUtils.getCacheStats().hitCount()).isEqualTo(0); + + var symlinkMetadata = handler.getMetadata(symlink); + + assertThat(symlinkMetadata).isEqualTo(targetMetadata); + assertThat(DigestUtils.getCacheStats().hitCount()).isEqualTo(1); + } }