diff --git a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java index d88614b7e953d6..31ca339a3a599e 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/actions/FileArtifactValue.java @@ -526,8 +526,17 @@ protected boolean couldBeModifiedByMetadata(FileArtifactValue o) { return true; } - RegularFileArtifactValue lastKnown = (RegularFileArtifactValue) o; - return size != lastKnown.size || !Objects.equals(proxy, lastKnown.proxy); + switch (o) { + case RegularFileArtifactValue lastKnown -> { + return size != lastKnown.size || !Objects.equals(proxy, lastKnown.proxy); + } + case RemoteFileArtifactValueWithMaterializationData lastKnown -> { + return size != lastKnown.getSize() || !Objects.equals(proxy, lastKnown.proxy); + } + default -> { + return true; + } + } } } @@ -536,36 +545,25 @@ public static class RemoteFileArtifactValue extends FileArtifactValue { private final byte[] digest; private final long size; private final int locationIndex; - @Nullable private final PathFragment materializationExecPath; - private RemoteFileArtifactValue( - byte[] digest, - long size, - int locationIndex, - @Nullable PathFragment materializationExecPath) { - this.digest = Preconditions.checkNotNull(digest); + private RemoteFileArtifactValue(byte[] digest, long size, int locationIndex) { + this.digest = checkNotNull(digest); this.size = size; this.locationIndex = locationIndex; - this.materializationExecPath = materializationExecPath; } - public static RemoteFileArtifactValue create( - byte[] digest, long size, int locationIndex, long expireAtEpochMilli) { - return create( - digest, size, locationIndex, expireAtEpochMilli, /* materializationExecPath= */ null); + public static RemoteFileArtifactValue create(byte[] digest, long size, int locationIndex) { + return new RemoteFileArtifactValue(digest, size, locationIndex); } - @VisibleForTesting - public static RemoteFileArtifactValue create( + public static RemoteFileArtifactValueWithMaterializationData createWithMaterializationData( byte[] digest, long size, int locationIndex, long expireAtEpochMilli, @Nullable PathFragment materializationExecPath) { - return expireAtEpochMilli < 0 - ? new RemoteFileArtifactValue(digest, size, locationIndex, materializationExecPath) - : new RemoteFileArtifactValueWithExpiration( - digest, size, locationIndex, materializationExecPath, expireAtEpochMilli); + return new RemoteFileArtifactValueWithMaterializationData( + digest, size, locationIndex, materializationExecPath, expireAtEpochMilli); } /** @@ -575,10 +573,10 @@ public static RemoteFileArtifactValue create( public static RemoteFileArtifactValue createFromExistingWithMaterializationPath( RemoteFileArtifactValue metadata, PathFragment materializationExecPath) { checkNotNull(materializationExecPath); - if (metadata.materializationExecPath != null) { + if (metadata.getMaterializationExecPath().isPresent()) { return metadata; } - return create( + return createWithMaterializationData( metadata.getDigest(), metadata.getSize(), metadata.getLocationIndex(), @@ -598,13 +596,12 @@ public boolean equals(Object o) { RemoteFileArtifactValue that = (RemoteFileArtifactValue) o; return Arrays.equals(digest, that.digest) && size == that.size - && locationIndex == that.locationIndex - && Objects.equals(materializationExecPath, that.materializationExecPath); + && locationIndex == that.locationIndex; } @Override - public final int hashCode() { - return Objects.hash(Arrays.hashCode(digest), size, locationIndex, materializationExecPath); + public int hashCode() { + return Objects.hash(Arrays.hashCode(digest), size, locationIndex); } @Override @@ -618,7 +615,7 @@ public final byte[] getDigest() { } @Override - public final FileContentsProxy getContentsProxy() { + public FileContentsProxy getContentsProxy() { throw new UnsupportedOperationException(); } @@ -638,11 +635,6 @@ public final int getLocationIndex() { return locationIndex; } - @Override - public final Optional getMaterializationExecPath() { - return Optional.ofNullable(materializationExecPath); - } - /** * Returns the time when the remote file expires in milliseconds since epoch. A negative value * means the remote is not known to expire. @@ -674,28 +666,33 @@ public final boolean isRemote() { } @Override - public final String toString() { + public String toString() { return MoreObjects.toStringHelper(this) .add("digest", bytesToString(digest)) .add("size", size) .add("locationIndex", locationIndex) - .add("materializationExecPath", materializationExecPath) - .add("expireAtEpochMilli", getExpireAtEpochMilli()) .toString(); } } - /** A remote artifact that expires at a particular time. */ - private static final class RemoteFileArtifactValueWithExpiration extends RemoteFileArtifactValue { + /** + * A remote artifact that contains additional data for materialization. This is used when the + * output mode allows Bazel to materialize remote output to local filesystem. + */ + public static final class RemoteFileArtifactValueWithMaterializationData + extends RemoteFileArtifactValue { + @Nullable private final PathFragment materializationExecPath; private long expireAtEpochMilli; + @Nullable private FileContentsProxy proxy; - private RemoteFileArtifactValueWithExpiration( + private RemoteFileArtifactValueWithMaterializationData( byte[] digest, long size, int locationIndex, PathFragment materializationExecPath, long expireAtEpochMilli) { - super(digest, size, locationIndex, materializationExecPath); + super(digest, size, locationIndex); + this.materializationExecPath = materializationExecPath; this.expireAtEpochMilli = expireAtEpochMilli; } @@ -706,14 +703,75 @@ public long getExpireAtEpochMilli() { @Override public void extendExpireAtEpochMilli(long expireAtEpochMilli) { + if (expireAtEpochMilli < 0) { + return; + } Preconditions.checkState(expireAtEpochMilli > this.expireAtEpochMilli); this.expireAtEpochMilli = expireAtEpochMilli; } + /** + * Returns a non-null {@link FileContentsProxy} if this remote metadata is backed by a local + * file, e.g. the file is materialized after action execution. + */ + @Override + public FileContentsProxy getContentsProxy() { + return proxy; + } + + /** + * Sets the {@link FileContentsProxy} if the output backed by this remote metadata is + * materialized later. + */ + public void setContentsProxy(FileContentsProxy proxy) { + this.proxy = proxy; + } + @Override public boolean isAlive(Instant now) { + if (expireAtEpochMilli < 0) { + return true; + } return now.toEpochMilli() < expireAtEpochMilli; } + + @Override + public Optional getMaterializationExecPath() { + return Optional.ofNullable(materializationExecPath); + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof RemoteFileArtifactValueWithMaterializationData that)) { + return false; + } + + return Arrays.equals(getDigest(), that.getDigest()) + && getSize() == that.getSize() + && getLocationIndex() == that.getLocationIndex() + && Objects.equals(materializationExecPath, that.materializationExecPath); + } + + @Override + public int hashCode() { + return Objects.hash( + Arrays.hashCode(getDigest()), getSize(), getLocationIndex(), materializationExecPath); + } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("digest", bytesToString(getDigest())) + .add("size", getSize()) + .add("locationIndex", getLocationIndex()) + .add("materializationExecPath", materializationExecPath) + .add("expireAtEpochMilli", expireAtEpochMilli) + .add("proxy", proxy) + .toString(); + } } /** A {@link FileArtifactValue} representing a symlink that is not to be resolved. */ diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java index d906498770e072..8ebd34aefc9485 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCache.java @@ -514,7 +514,11 @@ private static RemoteFileArtifactValue decodeRemoteMetadata( PathFragment.create(getStringForIndex(indexer, VarInt.getVarInt(source))); } - return RemoteFileArtifactValue.create( + if (expireAtEpochMilli < 0 && materializationExecPath == null) { + return RemoteFileArtifactValue.create(digest, size, locationIndex); + } + + return RemoteFileArtifactValue.createWithMaterializationData( digest, size, locationIndex, expireAtEpochMilli, materializationExecPath); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java index cd8c0c97b42c11..e9f542aa0dea8b 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractActionInputPrefetcher.java @@ -40,6 +40,9 @@ import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValueWithMaterializationData; +import com.google.devtools.build.lib.actions.FileContentsProxy; +import com.google.devtools.build.lib.actions.InputMetadataProvider; import com.google.devtools.build.lib.actions.cache.OutputMetadataStore; import com.google.devtools.build.lib.actions.cache.VirtualActionInput; import com.google.devtools.build.lib.events.Reporter; @@ -531,7 +534,8 @@ private Completable downloadFileNoCheckRx( directExecutor()) .doOnComplete( () -> { - finalizeDownload(tempPath, finalPath, dirsWithOutputPermissions); + finalizeDownload( + metadata, tempPath, finalPath, dirsWithOutputPermissions); alreadyDeleted.set(true); }) .doOnError( @@ -553,7 +557,8 @@ private Completable downloadFileNoCheckRx( })); } - private void finalizeDownload(Path tmpPath, Path finalPath, Set dirsWithOutputPermissions) + private void finalizeDownload( + FileArtifactValue metadata, Path tmpPath, Path finalPath, Set dirsWithOutputPermissions) throws IOException { Path parentDir = checkNotNull(finalPath.getParentDirectory()); @@ -585,6 +590,9 @@ private void finalizeDownload(Path tmpPath, Path finalPath, Set dirsWithOu // for artifacts produced by local actions. tmpPath.chmod(outputPermissions.getPermissionsMode()); FileSystemUtils.moveFile(tmpPath, finalPath); + if (metadata instanceof RemoteFileArtifactValueWithMaterializationData remote) { + remote.setContentsProxy(FileContentsProxy.create(finalPath.stat())); + } } private interface TaskWithTempPath { diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java index 3a111ed844784d..2eb88f1bc0eb81 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java @@ -306,7 +306,12 @@ void injectRemoteFile(PathFragment path, byte[] digest, long size, long expireAt return; } var metadata = - RemoteFileArtifactValue.create(digest, size, /* locationIndex= */ 1, expireAtEpochMilli); + RemoteFileArtifactValue.createWithMaterializationData( + digest, + size, + /* locationIndex= */ 1, + expireAtEpochMilli, + /* materializationExecPath= */ null); remoteOutputTree.injectFile(path, metadata); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java index 60a08ddf688d9d..cb34602ea69b65 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java @@ -13,6 +13,7 @@ // limitations under the License. package com.google.devtools.build.lib.skyframe; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static java.util.concurrent.TimeUnit.MINUTES; import com.google.common.base.Preconditions; @@ -31,6 +32,7 @@ import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValueWithMaterializationData; import com.google.devtools.build.lib.actions.FileStateType; import com.google.devtools.build.lib.actions.RemoteArtifactChecker; import com.google.devtools.build.lib.concurrent.ExecutorUtil; @@ -476,21 +478,39 @@ private boolean treeArtifactIsDirty(Artifact artifact, TreeArtifactValue value) // This could be improved by short-circuiting as soon as we see a child that is not present in // the TreeArtifactValue, but it doesn't seem to be a major source of overhead. - // visitTree() is called from multiple threads in parallel so this need to be a hash set - Set currentChildren = Sets.newConcurrentHashSet(); + // visitTree() is called from multiple threads in parallel so this need to be a concurrent set + Set currentLocalChildren = Sets.newConcurrentHashSet(); try { TreeArtifactValue.visitTree( path, (child, type, traversedSymlink) -> { if (type != Dirent.Type.DIRECTORY) { - currentChildren.add(child); + currentLocalChildren.add(child); } }); } catch (IOException e) { return true; } - return !(currentChildren.isEmpty() && value.isEntirelyRemote()) - && !currentChildren.equals(value.getChildPaths()); + + if (currentLocalChildren.isEmpty() && value.isEntirelyRemote()) { + return false; + } + + var lastKnownLocalChildren = + value.getChildValues().entrySet().stream() + .filter( + entry -> { + var metadata = entry.getValue(); + if (!(metadata + instanceof RemoteFileArtifactValueWithMaterializationData remote)) { + return true; + } + return remote.getContentsProxy() != null; + }) + .map(entry -> entry.getKey().getParentRelativePath()) + .collect(toImmutableSet()); + + return !currentLocalChildren.equals(lastKnownLocalChildren); } private boolean artifactIsDirtyWithDirectSystemCalls( diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java index 0ec5e14ad05270..49e60d7ef513bd 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java @@ -512,14 +512,14 @@ private RemoteFileArtifactValue createRemoteFileMetadata(String content) { private RemoteFileArtifactValue createRemoteFileMetadata( String content, @Nullable PathFragment materializationExecPath) { byte[] bytes = content.getBytes(UTF_8); - return RemoteFileArtifactValue.create( + return RemoteFileArtifactValue.createWithMaterializationData( digest(bytes), bytes.length, 1, /* expireAtEpochMilli= */ -1, materializationExecPath); } private RemoteFileArtifactValue createRemoteFileMetadata( String content, long expireAtEpochMilli, @Nullable PathFragment materializationExecPath) { byte[] bytes = content.getBytes(UTF_8); - return RemoteFileArtifactValue.create( + return RemoteFileArtifactValue.createWithMaterializationData( digest(bytes), bytes.length, 1, expireAtEpochMilli, materializationExecPath); } diff --git a/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java b/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java index e02257551e95c5..a25a464ce610f3 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/CompletionContextTest.java @@ -41,10 +41,7 @@ public class CompletionContextTest { private static final FileArtifactValue DUMMY_METADATA = RemoteFileArtifactValue.create( - /* digest= */ new byte[0], - /* size= */ 0, - /* locationIndex= */ 0, - /* expireAtEpochMilli= */ -1); + /* digest= */ new byte[0], /* size= */ 0, /* locationIndex= */ 0); private Path execRoot; private ArtifactRoot outputRoot; diff --git a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java index 905e25e2b74326..2738b2af83d237 100644 --- a/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java +++ b/src/test/java/com/google/devtools/build/lib/actions/cache/CompactPersistentActionCacheTest.java @@ -212,7 +212,7 @@ private RemoteFileArtifactValue createRemoteMetadata( .getHashFunction() .hashBytes(bytes) .asBytes(); - return RemoteFileArtifactValue.create( + return RemoteFileArtifactValue.createWithMaterializationData( digest, bytes.length, 1, expireAtEpochMilli, materializationExecPath); } diff --git a/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java index 78f260ecb13763..540e9d73cd6ecb 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ActionInputPrefetcherTestBase.java @@ -155,7 +155,7 @@ protected Artifact createRemoteArtifact( byte[] contentsBytes = contents.getBytes(UTF_8); HashCode hashCode = HASH_FUNCTION.getHashFunction().hashBytes(contentsBytes); RemoteFileArtifactValue f = - RemoteFileArtifactValue.create( + RemoteFileArtifactValue.createWithMaterializationData( hashCode.asBytes(), contentsBytes.length, /* locationIndex= */ 1, @@ -213,11 +213,12 @@ protected Pair> createRemoteTre byte[] contents = entry.getValue().getBytes(UTF_8); HashCode hashCode = HASH_FUNCTION.getHashFunction().hashBytes(contents); RemoteFileArtifactValue childValue = - RemoteFileArtifactValue.create( + RemoteFileArtifactValue.createWithMaterializationData( hashCode.asBytes(), contents.length, /* locationIndex= */ 1, - /* expireAtEpochMilli= */ -1); + /* expireAtEpochMilli= */ -1, + /* materializationExecPath= */ null); treeBuilder.putChild(child, childValue); metadata.put(child, childValue); cas.put(hashCode, contents); diff --git a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java index 165fb67b4c4976..15b52272adbcea 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java +++ b/src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java @@ -1366,8 +1366,7 @@ public void incrementalBuild_intermediateOutputDeleted_nothingIsReEvaluated() th } @Test - public void incrementalBuild_remoteFileMetadataIsReplacedWithLocalFileMetadata() - throws Exception { + public void incrementalBuild_fileOutputIsPrefetched_noRuns() throws Exception { // We need to download the intermediate output if (!hasAccessToRemoteOutputs()) { return; @@ -1400,13 +1399,58 @@ public void incrementalBuild_remoteFileMetadataIsReplacedWithLocalFileMetadata() getRuntimeWrapper().registerSubscriber(actionEventCollector); buildTarget("//:foobar"); - // Assert: remote file metadata is replaced with local file metadata + // Assert: remote file metadata has contents proxy and action node is not marked as dirty. assertValidOutputFile("out/foo.txt", "foo\n"); assertValidOutputFile("out/foobar.txt", "foo\nbar\n"); assertThat(actionEventCollector.getActionExecutedEvents()).isEmpty(); - // Two actions are invalidated but were able to hit the action cache - assertThat(actionEventCollector.getCachedActionEvents()).hasSize(2); - assertThat(getOnlyElement(getMetadata("//:foo").values()).isRemote()).isFalse(); + assertThat(actionEventCollector.getCachedActionEvents()).isEmpty(); + var metadata = getOnlyElement(getMetadata("//:foo").values()); + assertThat(metadata.isRemote()).isTrue(); + assertThat(metadata.getContentsProxy()).isNotNull(); + } + + @Test + public void incrementalBuild_treeOutputIsPrefetched_noRuns() throws Exception { + // We need to download the intermediate output + if (!hasAccessToRemoteOutputs()) { + return; + } + + // Arrange: Prepare workspace and run a clean build + writeOutputDirRule(); + write( + "BUILD", + "load(':output_dir.bzl', 'output_dir')", + "output_dir(", + " name = 'foo',", + " content_map = {'file-1': '1', 'file-2': '2', 'file-3': '3'},", + ")", + "genrule(", + " name = 'foobar',", + " srcs = [':foo'],", + " outs = ['out/foobar.txt'],", + " cmd = 'echo bar >> $@',", + " tags = ['no-remote'],", + ")"); + + buildTarget("//:foobar"); + assertValidOutputFile("foo/file-1", "1"); + assertValidOutputFile("foo/file-2", "2"); + assertValidOutputFile("foo/file-3", "3"); + assertValidOutputFile("out/foobar.txt", "bar\n"); + assertThat(getOnlyElement(getTreeMetadata("//:foo").values()).isEntirelyRemote()).isTrue(); + + // Act: Do an incremental build without any modifications + ActionEventCollector actionEventCollector = new ActionEventCollector(); + getRuntimeWrapper().registerSubscriber(actionEventCollector); + buildTarget("//:foobar"); + + // Assert: action node is not marked as dirty. + assertValidOutputFile("foo/file-1", "1"); + assertValidOutputFile("foo/file-2", "2"); + assertValidOutputFile("foo/file-3", "3"); + assertThat(actionEventCollector.getActionExecutedEvents()).isEmpty(); + assertThat(actionEventCollector.getCachedActionEvents()).isEmpty(); } protected ImmutableMap getMetadata(String target) throws Exception { @@ -1423,6 +1467,21 @@ protected ImmutableMap getMetadata(String target) t return result.buildOrThrow(); } + protected ImmutableMap getTreeMetadata(String target) + throws Exception { + var result = ImmutableMap.builder(); + var evaluator = getRuntimeWrapper().getSkyframeExecutor().getEvaluator(); + for (var artifact : getArtifacts(target)) { + var value = evaluator.getExistingValue(Artifact.key(artifact)); + if (value instanceof ActionExecutionValue actionExecutionValue) { + result.putAll(actionExecutionValue.getAllTreeArtifactValues()); + } else if (value instanceof TreeArtifactValue treeArtifactValue) { + result.put(artifact, treeArtifactValue); + } + } + return result.buildOrThrow(); + } + protected FileArtifactValue getMetadata(Artifact output) throws Exception { var evaluator = getRuntimeWrapper().getSkyframeExecutor().getEvaluator(); var value = evaluator.getExistingValue(Artifact.key(output)); diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java index 9f0c55ef5db591..0e875e42e7e935 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java @@ -564,8 +564,7 @@ private Artifact createRemoteArtifact( byte[] b = contents.getBytes(StandardCharsets.UTF_8); HashCode h = HashCode.fromString(DIGEST_UTIL.compute(b).getHash()); FileArtifactValue f = - RemoteFileArtifactValue.create( - h.asBytes(), b.length, /* locationIndex= */ 1, /* expireAtEpochMilli= */ -1); + RemoteFileArtifactValue.create(h.asBytes(), b.length, /* locationIndex= */ 1); inputs.putWithNoDepOwner(a, f); return a; } diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java index 4e6a3bf8d6ba08..0926ec62071187 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteActionFileSystemTest.java @@ -1325,8 +1325,12 @@ protected FileArtifactValue injectRemoteFile( int size = Utf8.encodedLength(content); ((RemoteActionFileSystem) actionFs) .injectRemoteFile(path, digest, size, /* expireAtEpochMilli= */ -1); - return RemoteFileArtifactValue.create( - digest, size, /* locationIndex= */ 1, /* expireAtEpochMilli= */ -1); + return RemoteFileArtifactValue.createWithMaterializationData( + digest, + size, + /* locationIndex= */ 1, + /* expireAtEpochMilli= */ -1, + /* materializationExecPath= */ null); } @Override @@ -1342,11 +1346,12 @@ private Artifact createRemoteArtifact( String pathFragment, String content, ActionInputMap inputs) { Artifact a = ActionsTestUtil.createArtifact(outputRoot, pathFragment); RemoteFileArtifactValue f = - RemoteFileArtifactValue.create( + RemoteFileArtifactValue.createWithMaterializationData( getDigest(content), Utf8.encodedLength(content), /* locationIndex= */ 1, - /* expireAtEpochMilli= */ -1); + /* expireAtEpochMilli= */ -1, + /* materializationExecPath= */ null); inputs.putWithNoDepOwner(a, f); return a; } @@ -1367,11 +1372,12 @@ private TreeArtifactValue createRemoteTreeArtifactValue( TreeFileArtifact child = TreeFileArtifact.createTreeOutput(a, entry.getKey()); String content = entry.getValue(); RemoteFileArtifactValue childMeta = - RemoteFileArtifactValue.create( + RemoteFileArtifactValue.createWithMaterializationData( getDigest(content), Utf8.encodedLength(content), /* locationIndex= */ 0, - /* expireAtEpochMilli= */ -1); + /* expireAtEpochMilli= */ -1, + /* materializationExecPath= */ null); builder.putChild(child, childMeta); } return builder.build(); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java index bbbe03ea4074a7..2a5a75f684e74a 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTest.java @@ -54,16 +54,10 @@ public final class ActionExecutionValueTest { private static final FileArtifactValue VALUE_1_REMOTE = RemoteFileArtifactValue.create( - /* digest= */ new byte[0], - /* size= */ 0, - /* locationIndex= */ 1, - /* expireAtEpochMilli= */ -1); + /* digest= */ new byte[0], /* size= */ 0, /* locationIndex= */ 1); private static final FileArtifactValue VALUE_2_REMOTE = RemoteFileArtifactValue.create( - /* digest= */ new byte[0], - /* size= */ 0, - /* locationIndex= */ 2, - /* expireAtEpochMilli= */ -1); + /* digest= */ new byte[0], /* size= */ 0, /* locationIndex= */ 2); private static final ActionLookupKey KEY = ActionsTestUtil.NULL_ARTIFACT_OWNER; private static final ActionLookupData ACTION_LOOKUP_DATA_1 = ActionLookupData.create(KEY, 1); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java index 9eddd2e1c19c9c..6ba264a5875ff8 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStoreTest.java @@ -290,9 +290,7 @@ public void resettingOutputs() throws Exception { assertThat(chmodCalls).containsExactly(outputPath, 0555); // Inject a remote file of size 42. - store.injectFile( - artifact, - RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 42, 0, /* expireAtEpochMilli= */ -1)); + store.injectFile(artifact, RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 42, 0)); assertThat(store.getOutputMetadata(artifact).getSize()).isEqualTo(42); // Reset this output, which will make the store stat the file again. @@ -313,9 +311,7 @@ public void injectRemoteArtifactMetadata() throws Exception { byte[] digest = new byte[] {1, 2, 3}; int size = 10; store.injectFile( - artifact, - RemoteFileArtifactValue.create( - digest, size, /* locationIndex= */ 1, /* expireAtEpochMilli= */ -1)); + artifact, RemoteFileArtifactValue.create(digest, size, /* locationIndex= */ 1)); FileArtifactValue v = store.getOutputMetadata(artifact); assertThat(v).isNotNull(); @@ -333,8 +329,7 @@ public void cannotInjectTreeArtifactChildIndividually() { ActionOutputMetadataStore store = createStore(/* outputs= */ ImmutableSet.of(treeArtifact)); store.prepareForActionExecution(); - RemoteFileArtifactValue childValue = - RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1, /* expireAtEpochMilli= */ -1); + RemoteFileArtifactValue childValue = RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1); assertThrows(IllegalArgumentException.class, () -> store.injectFile(child, childValue)); assertThat(store.getAllArtifactData()).isEmpty(); @@ -353,8 +348,7 @@ public void canInjectTemplateExpansionOutput() { ActionOutputMetadataStore store = createStore(/* outputs= */ ImmutableSet.of(treeArtifact)); store.prepareForActionExecution(); - RemoteFileArtifactValue value = - RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1, /* expireAtEpochMilli= */ -1); + RemoteFileArtifactValue value = RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1); store.injectFile(output, value); assertThat(store.getAllArtifactData()).containsExactly(output, value); @@ -373,12 +367,10 @@ public void injectRemoteTreeArtifactMetadata() throws Exception { TreeArtifactValue.newBuilder(treeArtifact) .putChild( TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), - RemoteFileArtifactValue.create( - new byte[] {1, 2, 3}, 5, 1, /* expireAtEpochMilli= */ -1)) + RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 5, 1)) .putChild( TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), - RemoteFileArtifactValue.create( - new byte[] {4, 5, 6}, 10, 1, /* expireAtEpochMilli= */ -1)) + RemoteFileArtifactValue.create(new byte[] {4, 5, 6}, 10, 1)) .build(); store.injectTree(treeArtifact, tree); @@ -452,7 +444,7 @@ private FileArtifactValue createFileMetadataForSymlinkTest( case LOCAL: return FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, /* proxy= */ null, 10); case REMOTE: - return RemoteFileArtifactValue.create( + return RemoteFileArtifactValue.createWithMaterializationData( new byte[] {1, 2, 3}, 10, 1, -1, materializationExecPath); } throw new AssertionError(); @@ -522,9 +514,9 @@ private TreeArtifactValue createTreeMetadataForSymlinkTest( FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, /* proxy= */ null, 20); RemoteFileArtifactValue remoteMetadata1 = - RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 10, 1, -1); + RemoteFileArtifactValue.create(new byte[] {1, 2, 3}, 10, 1); RemoteFileArtifactValue remoteMetadata2 = - RemoteFileArtifactValue.create(new byte[] {4, 5, 6}, 20, 1, -1); + RemoteFileArtifactValue.create(new byte[] {4, 5, 6}, 20, 1); switch (composition) { case EMPTY: diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java index c0a393dcd18679..fbeea1835c2658 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FileArtifactValueTest.java @@ -89,8 +89,18 @@ public void testEqualsAndHashCode() { FileArtifactValue.createForDirectoryWithMtime(2)) .addEqualityGroup( // expireAtEpochMilli doesn't contribute to the equality - RemoteFileArtifactValue.create(toBytes("00112233445566778899AABBCCDDEEFF"), 1, 1, 1), - RemoteFileArtifactValue.create(toBytes("00112233445566778899AABBCCDDEEFF"), 1, 1, 2)) + RemoteFileArtifactValue.createWithMaterializationData( + toBytes("00112233445566778899AABBCCDDEEFF"), + 1, + 1, + 1, + /* materializationExecPath= */ null), + RemoteFileArtifactValue.createWith( + toBytes("00112233445566778899AABBCCDDEEFF"), + 1, + 1, + 2, + /* materializationExecPath= */ null)) .addEqualityGroup(FileArtifactValue.OMITTED_FILE_MARKER) .addEqualityGroup(FileArtifactValue.MISSING_FILE_MARKER) .addEqualityGroup(FileArtifactValue.DEFAULT_MIDDLEMAN) @@ -152,7 +162,7 @@ public void testNoMtimeIfNonemptyFile() throws Exception { @Test public void testDirectory() throws Exception { - Path path = scratchDir("/dir", /*mtime=*/ 1L); + Path path = scratchDir("/dir", /* mtime= */ 1L); FileArtifactValue value = createForTesting(path); assertThat(value.getDigest()).isNull(); assertThat(value.getModifiedTime()).isEqualTo(1L); @@ -272,9 +282,9 @@ public void testUptodateUnresolvedSymlink() throws Exception { @Test public void addToFingerprint_equalByDigest() throws Exception { FileArtifactValue value1 = - FileArtifactValue.createForTesting(scratchFile("/dir/file1", /*mtime=*/ 1, "content")); + FileArtifactValue.createForTesting(scratchFile("/dir/file1", /* mtime= */ 1, "content")); FileArtifactValue value2 = - FileArtifactValue.createForTesting(scratchFile("/dir/file2", /*mtime=*/ 2, "content")); + FileArtifactValue.createForTesting(scratchFile("/dir/file2", /* mtime= */ 2, "content")); Fingerprint fingerprint1 = new Fingerprint(); Fingerprint fingerprint2 = new Fingerprint(); @@ -289,9 +299,9 @@ public void addToFingerprint_equalByDigest() throws Exception { @Test public void addToFingerprint_notEqualByDigest() throws Exception { FileArtifactValue value1 = - FileArtifactValue.createForTesting(scratchFile("/dir/file1", /*mtime=*/ 1, "content1")); + FileArtifactValue.createForTesting(scratchFile("/dir/file1", /* mtime= */ 1, "content1")); FileArtifactValue value2 = - FileArtifactValue.createForTesting(scratchFile("/dir/file2", /*mtime=*/ 1, "content2")); + FileArtifactValue.createForTesting(scratchFile("/dir/file2", /* mtime= */ 1, "content2")); Fingerprint fingerprint1 = new Fingerprint(); Fingerprint fingerprint2 = new Fingerprint(); @@ -306,9 +316,9 @@ public void addToFingerprint_notEqualByDigest() throws Exception { @Test public void addToFingerprint_equalByMtime() throws Exception { FileArtifactValue value1 = - FileArtifactValue.createForTesting(scratchDir("/dir1", /*mtime=*/ 1)); + FileArtifactValue.createForTesting(scratchDir("/dir1", /* mtime= */ 1)); FileArtifactValue value2 = - FileArtifactValue.createForTesting(scratchDir("/dir2", /*mtime=*/ 1)); + FileArtifactValue.createForTesting(scratchDir("/dir2", /* mtime= */ 1)); Fingerprint fingerprint1 = new Fingerprint(); Fingerprint fingerprint2 = new Fingerprint(); @@ -323,9 +333,9 @@ public void addToFingerprint_equalByMtime() throws Exception { @Test public void addToFingerprint_notEqualByMtime() throws Exception { FileArtifactValue value1 = - FileArtifactValue.createForTesting(scratchDir("/dir1", /*mtime=*/ 1)); + FileArtifactValue.createForTesting(scratchDir("/dir1", /* mtime= */ 1)); FileArtifactValue value2 = - FileArtifactValue.createForTesting(scratchDir("/dir2", /*mtime=*/ 2)); + FileArtifactValue.createForTesting(scratchDir("/dir2", /* mtime= */ 2)); Fingerprint fingerprint1 = new Fingerprint(); Fingerprint fingerprint2 = new Fingerprint(); @@ -339,9 +349,10 @@ public void addToFingerprint_notEqualByMtime() throws Exception { @Test public void addToFingerprint_fileWithDigestNotEqualToFileWithOnlyMtime() throws Exception { - FileArtifactValue value1 = FileArtifactValue.createForTesting(scratchDir("/dir", /*mtime=*/ 1)); + FileArtifactValue value1 = + FileArtifactValue.createForTesting(scratchDir("/dir", /* mtime= */ 1)); FileArtifactValue value2 = - FileArtifactValue.createForTesting(scratchFile("/dir/file", /*mtime=*/ 1, "contents")); + FileArtifactValue.createForTesting(scratchFile("/dir/file", /* mtime= */ 1, "contents")); Fingerprint fingerprint1 = new Fingerprint(); Fingerprint fingerprint2 = new Fingerprint(); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java index b7543949cd388c..006fde6835161c 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/FilesystemValueCheckerTest.java @@ -38,6 +38,8 @@ import com.google.devtools.build.lib.actions.ArtifactRoot.RootType; import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValueWithMaterializationData; +import com.google.devtools.build.lib.actions.FileContentsProxy; import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileValue; import com.google.devtools.build.lib.actions.RemoteArtifactChecker; @@ -1406,32 +1408,34 @@ private static Delta actionValueWithRemoteArtifact( ActionsTestUtil.createActionExecutionValue(ImmutableMap.of(output, value))); } - private RemoteFileArtifactValue createRemoteFileArtifactValue(String contents) { + private RemoteFileArtifactValueWithMaterializationData createRemoteFileArtifactValue( + String contents) { return createRemoteFileArtifactValue(contents, /* expireAtEpochMilli= */ -1); } - private RemoteFileArtifactValue createRemoteFileArtifactValue( + private RemoteFileArtifactValueWithMaterializationData createRemoteFileArtifactValue( String contents, long expireAtEpochMilli) { byte[] data = contents.getBytes(); DigestHashFunction hashFn = fs.getDigestFunction(); HashCode hash = hashFn.getHashFunction().hashBytes(data); - return RemoteFileArtifactValue.create(hash.asBytes(), data.length, -1, expireAtEpochMilli); + return RemoteFileArtifactValue.createWithMaterializationData( + hash.asBytes(), data.length, -1, expireAtEpochMilli, /* materializationExecPath= */ null); } @Test - public void testRemoteAndLocalArtifacts() throws Exception { - // Test that injected remote artifacts are trusted by the FileSystemValueChecker - // if it is configured to trust remote artifacts, and that local files always take precedence - // over remote files. + public void testRemoteAndLocalArtifacts(@TestParameter boolean setContentsProxy) + throws Exception { + // Test that injected remote artifacts are trusted by the FileSystemValueChecker if it is + // configured to trust remote artifacts, and that local files always take precedence over remote + // files if they are different. SkyKey actionKey1 = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); SkyKey actionKey2 = ActionLookupData.create(ACTION_LOOKUP_KEY, 1); Artifact out1 = createDerivedArtifact("foo"); Artifact out2 = createDerivedArtifact("bar"); Map metadataToInject = new HashMap<>(); - metadataToInject.put( - actionKey1, - actionValueWithRemoteArtifact(out1, createRemoteFileArtifactValue("foo-content"))); + var out1Metadata = createRemoteFileArtifactValue("foo-content"); + metadataToInject.put(actionKey1, actionValueWithRemoteArtifact(out1, out1Metadata)); metadataToInject.put( actionKey2, actionValueWithRemoteArtifact(out2, createRemoteFileArtifactValue("bar-content"))); @@ -1462,6 +1466,11 @@ public void testRemoteAndLocalArtifacts() throws Exception { (ignored, ignored2) -> {})) .isEmpty(); + if (setContentsProxy) { + FileSystemUtils.writeContentAsLatin1(out1.getPath(), "foo-content"); + out1Metadata.setContentsProxy(FileContentsProxy.create(out1.getPath().stat())); + } + // Create the "out1" artifact on the filesystem and test that it invalidates the generating // action's SkyKey. FileSystemUtils.writeContentAsLatin1(out1.getPath(), "new-foo-content"); @@ -1481,18 +1490,18 @@ public void testRemoteAndLocalArtifacts() throws Exception { } @Test - public void testRemoteAndLocalArtifacts_identicalContent() throws Exception { - // Test that even if injected remote artifacts and local files are NO_OVERRIDE, the generating - // actions are marked as dirty. + public void testRemoteAndLocalArtifacts_identicalContent(@TestParameter boolean setContentsProxy) + throws Exception { + // Test that if injected remote artifacts and local files are identical, the generating actions + // are not marked as dirty if it has contents proxy. SkyKey actionKey1 = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); SkyKey actionKey2 = ActionLookupData.create(ACTION_LOOKUP_KEY, 1); Artifact out1 = createDerivedArtifact("foo"); Artifact out2 = createDerivedArtifact("bar"); Map metadataToInject = new HashMap<>(); - metadataToInject.put( - actionKey1, - actionValueWithRemoteArtifact(out1, createRemoteFileArtifactValue("foo-content"))); + var out1Metadata = createRemoteFileArtifactValue("foo-content"); + metadataToInject.put(actionKey1, actionValueWithRemoteArtifact(out1, out1Metadata)); metadataToInject.put( actionKey2, actionValueWithRemoteArtifact(out2, createRemoteFileArtifactValue("bar-content"))); @@ -1523,22 +1532,29 @@ public void testRemoteAndLocalArtifacts_identicalContent() throws Exception { (ignored, ignored2) -> {})) .isEmpty(); - // Create NO_OVERRIDE "out1" artifact on the filesystem and test that it invalidates the - // generating action's SkyKey. + // Create identical "out1" artifact on the filesystem and test that it doesn't invalidate the + // generating action's SkyKey if contents proxy is set. FileSystemUtils.writeContentAsLatin1(out1.getPath(), "foo-content"); - assertThat( - new FilesystemValueChecker( - /* tsgm= */ null, - SyscallCache.NO_CACHE, - XattrProviderOverrider.NO_OVERRIDE, - FSVC_THREADS_FOR_TEST) - .getDirtyActionValues( - evaluator.getValues(), - /* batchStatter= */ null, - ModifiedFileSet.EVERYTHING_MODIFIED, - RemoteArtifactChecker.TRUST_ALL, - (ignored, ignored2) -> {})) - .containsExactly(actionKey1); + if (setContentsProxy) { + out1Metadata.setContentsProxy(FileContentsProxy.create(out1.getPath().stat())); + } + var dirtyActionKeys = + new FilesystemValueChecker( + /* tsgm= */ null, + SyscallCache.NO_CACHE, + XattrProviderOverrider.NO_OVERRIDE, + FSVC_THREADS_FOR_TEST) + .getDirtyActionValues( + evaluator.getValues(), + /* batchStatter= */ null, + ModifiedFileSet.EVERYTHING_MODIFIED, + RemoteArtifactChecker.TRUST_ALL, + (ignored, ignored2) -> {}); + if (setContentsProxy) { + assertThat(dirtyActionKeys).isEmpty(); + } else { + assertThat(dirtyActionKeys).containsExactly(actionKey1); + } } @Test @@ -1587,8 +1603,7 @@ public void testRemoteArtifactsExpired() throws Exception { @Test public void testRemoteAndLocalTreeArtifacts() throws Exception { - // Test that injected remote tree artifacts are trusted by the FileSystemValueChecker - // and that local files always takes preference over remote files. + // Test that change to local tree files invalidates generating action. SkyKey actionKey = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); SpecialArtifact treeArtifact = createTreeArtifact("dir"); @@ -1646,18 +1661,18 @@ public void testRemoteAndLocalTreeArtifacts() throws Exception { } @Test - public void testRemoteAndLocalTreeArtifacts_identicalContent() throws Exception { - // Test that even if injected remote tree artifacts and local files are NO_OVERRIDE, the - // generating actions are marked as dirty. + public void testRemoteAndLocalTreeArtifacts_partiallyDownloaded( + @TestParameter boolean setContentsProxy) throws Exception { + // Test that if injected remote tree artifacts and local files are identical, but the tree is + // partially downloaded, the generating action is not marked as dirty. SkyKey actionKey = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); SpecialArtifact treeArtifact = createTreeArtifact("dir"); treeArtifact.getPath().createDirectoryAndParents(); + var fooMetadata = createRemoteFileArtifactValue("foo-content"); TreeArtifactValue tree = TreeArtifactValue.newBuilder(treeArtifact) - .putChild( - TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), - createRemoteFileArtifactValue("foo-content")) + .putChild(TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), fooMetadata) .putChild( TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), createRemoteFileArtifactValue("bar-content")) @@ -1687,10 +1702,59 @@ public void testRemoteAndLocalTreeArtifacts_identicalContent() throws Exception (ignored, ignored2) -> {})) .isEmpty(); - // Create NO_OVERRIDE dir/foo on the local disk and test that it invalidates the associated sky - // key. + // Create identical dir/foo on the local disk and test that it doesn't invalidate the associated + // sky key. TreeFileArtifact fooArtifact = TreeFileArtifact.createTreeOutput(treeArtifact, "foo"); FileSystemUtils.writeContentAsLatin1(fooArtifact.getPath(), "foo-content"); + if (setContentsProxy) { + fooMetadata.setContentsProxy(FileContentsProxy.create(fooArtifact.getPath().stat())); + } + var dirtyActionKeys = + new FilesystemValueChecker( + /* tsgm= */ null, + SyscallCache.NO_CACHE, + XattrProviderOverrider.NO_OVERRIDE, + FSVC_THREADS_FOR_TEST) + .getDirtyActionValues( + evaluator.getValues(), + /* batchStatter= */ null, + ModifiedFileSet.EVERYTHING_MODIFIED, + RemoteArtifactChecker.TRUST_ALL, + (ignored, ignored2) -> {}); + if (setContentsProxy) { + assertThat(dirtyActionKeys).isEmpty(); + } else { + assertThat(dirtyActionKeys).containsExactly(actionKey); + } + } + + @Test + public void testRemoteAndLocalTreeArtifacts_identicalContent( + @TestParameter boolean setContentsProxy) throws Exception { + // Test that if injected remote tree artifacts and local files are identical, the generating + // actions are not marked as dirty if contents proxy is set. + SkyKey actionKey = ActionLookupData.create(ACTION_LOOKUP_KEY, 0); + + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + treeArtifact.getPath().createDirectoryAndParents(); + var fooMetadata = createRemoteFileArtifactValue("foo-content"); + var barMetadata = createRemoteFileArtifactValue("bar-content"); + TreeArtifactValue tree = + TreeArtifactValue.newBuilder(treeArtifact) + .putChild(TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), fooMetadata) + .putChild(TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), barMetadata) + .build(); + + differencer.inject(ImmutableMap.of(actionKey, actionValueWithTreeArtifact(treeArtifact, tree))); + + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setParallelism(1) + .setEventHandler(NullEventHandler.INSTANCE) + .build(); + assertThat(evaluator.evaluate(ImmutableList.of(actionKey), evaluationContext).hasError()) + .isFalse(); assertThat( new FilesystemValueChecker( /* tsgm= */ null, @@ -1703,7 +1767,35 @@ public void testRemoteAndLocalTreeArtifacts_identicalContent() throws Exception ModifiedFileSet.EVERYTHING_MODIFIED, RemoteArtifactChecker.TRUST_ALL, (ignored, ignored2) -> {})) - .containsExactly(actionKey); + .isEmpty(); + + // Create identical dir/foo and dir/bar on the local disk and test that it doesn't invalidate + // the associated sky key. + TreeFileArtifact fooArtifact = TreeFileArtifact.createTreeOutput(treeArtifact, "foo"); + FileSystemUtils.writeContentAsLatin1(fooArtifact.getPath(), "foo-content"); + TreeFileArtifact barArtifact = TreeFileArtifact.createTreeOutput(treeArtifact, "bar"); + FileSystemUtils.writeContentAsLatin1(barArtifact.getPath(), "bar-content"); + if (setContentsProxy) { + fooMetadata.setContentsProxy(FileContentsProxy.create(fooArtifact.getPath().stat())); + barMetadata.setContentsProxy(FileContentsProxy.create(barArtifact.getPath().stat())); + } + var dirtyActionKeys = + new FilesystemValueChecker( + /* tsgm= */ null, + SyscallCache.NO_CACHE, + XattrProviderOverrider.NO_OVERRIDE, + FSVC_THREADS_FOR_TEST) + .getDirtyActionValues( + evaluator.getValues(), + /* batchStatter= */ null, + ModifiedFileSet.EVERYTHING_MODIFIED, + RemoteArtifactChecker.TRUST_ALL, + (ignored, ignored2) -> {}); + if (setContentsProxy) { + assertThat(dirtyActionKeys).isEmpty(); + } else { + assertThat(dirtyActionKeys).containsExactly(actionKey); + } } @Test diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java index 12c72258c3898c..3ce5a043d63289 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java @@ -659,14 +659,12 @@ public void remoteDirectoryInjection() throws Exception { RemoteFileArtifactValue.create( Hashing.sha256().hashString("one", UTF_8).asBytes(), /* size= */ 3, - /* locationIndex= */ 1, - /* expireAtEpochMilli= */ -1); + /* locationIndex= */ 1); RemoteFileArtifactValue remoteFile2 = RemoteFileArtifactValue.create( Hashing.sha256().hashString("two", UTF_8).asBytes(), /* size= */ 3, - /* locationIndex= */ 2, - /* expireAtEpochMilli= */ -1); + /* locationIndex= */ 2); Action action = new SimpleTestAction(out) { diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java index bf3f25c5392ad4..0a4512d98cb638 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java @@ -860,8 +860,7 @@ private static SpecialArtifact createTreeArtifact(String execPath, ArtifactRoot } private static FileArtifactValue metadataWithId(int id) { - return RemoteFileArtifactValue.create( - new byte[] {(byte) id}, id, id, /* expireAtEpochMilli= */ -1); + return RemoteFileArtifactValue.create(new byte[] {(byte) id}, id, id); } private static FileArtifactValue metadataWithIdNoDigest(int id) {