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 591227b8252c20..db0e3db9c81c1b 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 @@ -51,6 +51,8 @@ *
  • a "middleman marker" object, which has a null digest, 0 size, and mtime of 0. *
  • The "self data" of a TreeArtifact, where we would expect to see a digest representing the * artifact's contents, and a size of 0. + *
  • a file that's only stored by a remote caching/execution system, in which case we would + * expect to see a digest and size. * */ @Immutable @@ -138,6 +140,13 @@ public final boolean isMarkerValue() { */ public abstract boolean wasModifiedSinceDigest(Path path) throws IOException; + /** + * Returns {@code true} if the file only exists remotely. + */ + public boolean isRemote() { + return false; + } + @Override public boolean equals(Object o) { if (this == o) { @@ -524,7 +533,12 @@ public int getLocationIndex() { @Override public boolean wasModifiedSinceDigest(Path path) { - throw new UnsupportedOperationException(); + return false; + } + + @Override + public boolean isRemote() { + return true; } @Override 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 9efc2b67a78305..85efa8213bf85b 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 @@ -26,6 +26,7 @@ import com.google.common.util.concurrent.ThreadFactoryBuilder; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.ArtifactFileMetadata; +import com.google.devtools.build.lib.actions.FileArtifactValue; import com.google.devtools.build.lib.concurrent.ExecutorUtil; import com.google.devtools.build.lib.concurrent.Sharder; import com.google.devtools.build.lib.concurrent.ThrowableRecordingRunnableWrapper; @@ -80,6 +81,7 @@ public class FilesystemValueChecker { private static final Predicate ACTION_FILTER = SkyFunctionName.functionIs(SkyFunctions.ACTION_EXECUTION); + @Nullable private final TimestampGranularityMonitor tsgm; @Nullable private final Range lastExecutionTimeRange; @@ -398,6 +400,9 @@ private boolean treeArtifactIsDirty(Artifact artifact, TreeArtifactValue value) try { Set currentDirectoryValue = TreeArtifactValue.explodeDirectory(artifact.getPath()); + if (currentDirectoryValue.isEmpty() && value.isRemote()) { + return false; + } Set valuePaths = value.getChildPaths(); return !currentDirectoryValue.equals(valuePaths); } catch (IOException e) { @@ -417,6 +422,14 @@ private boolean actionValueIsDirtyWithDirectSystemCalls(ActionExecutionValue act try { ArtifactFileMetadata fileMetadata = ActionMetadataHandler.fileMetadataFromArtifact(file, null, tsgm); + FileArtifactValue fileValue = actionValue.getArtifactValue(file); + boolean lastSeenRemotely = fileValue != null && fileValue.isRemote(); + if (!fileMetadata.exists() && lastSeenRemotely) { + // The output file does not exist in the output tree, but the last time we created it + // it was stored remotely so there is no need to invalidate it. + continue; + } + if (!fileMetadata.equals(lastKnownData)) { updateIntraBuildModifiedCounter( fileMetadata.exists() ? file.getPath().getLastModifiedTime(Symlinks.FOLLOW) : -1, diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java index dd72ee2aabfb5a..a17ae6dc7a1cbe 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java @@ -15,6 +15,7 @@ import com.google.common.base.Function; import com.google.common.base.MoreObjects; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; @@ -56,27 +57,40 @@ public PathFragment apply(Artifact artifact) { private final byte[] digest; private final Map childData; private BigInteger valueFingerprint; + private final boolean isRemote; @AutoCodec.VisibleForSerialization - TreeArtifactValue(byte[] digest, Map childData) { + TreeArtifactValue(byte[] digest, Map childData, boolean isRemote) { this.digest = digest; this.childData = ImmutableMap.copyOf(childData); + this.isRemote = isRemote; } /** * Returns a TreeArtifactValue out of the given Artifact-relative path fragments * and their corresponding FileArtifactValues. + * + *

    All {@code childFileValues} must return the same value for + * {@link FileArtifactValue#isRemote()}. */ static TreeArtifactValue create(Map childFileValues) { Map digestBuilder = Maps.newHashMapWithExpectedSize(childFileValues.size()); + Boolean isRemote = null; for (Map.Entry e : childFileValues.entrySet()) { - digestBuilder.put(e.getKey().getParentRelativePath().getPathString(), e.getValue()); + FileArtifactValue v = e.getValue(); + if (isRemote == null) { + isRemote = v.isRemote(); + } + Preconditions.checkArgument(v.isRemote() == isRemote, "files in a tree artifact must either" + + " be all remote or all local."); + digestBuilder.put(e.getKey().getParentRelativePath().getPathString(), v); } return new TreeArtifactValue( DigestUtils.fromMetadata(digestBuilder).getDigestBytesUnsafe(), - ImmutableMap.copyOf(childFileValues)); + ImmutableMap.copyOf(childFileValues), + isRemote != null ? isRemote : false); } FileArtifactValue getSelfData() { @@ -104,6 +118,13 @@ Map getChildValues() { return childData; } + /** + * Returns {@code true} if the @link TreeFileArtifact}s are only stored remotely. + */ + public boolean isRemote() { + return isRemote; + } + @Override public BigInteger getValueFingerprint() { if (valueFingerprint == null) { @@ -150,7 +171,7 @@ public String toString() { * Java's concurrent collections disallow null members. */ static final TreeArtifactValue MISSING_TREE_ARTIFACT = - new TreeArtifactValue(null, ImmutableMap.of()) { + new TreeArtifactValue(null, ImmutableMap.of(), false) { @Override FileArtifactValue getSelfData() { throw new UnsupportedOperationException(); 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 62691af58e13ab..4cf04992899128 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 @@ -21,8 +21,10 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.hash.HashCode; import com.google.common.util.concurrent.Runnables; import com.google.devtools.build.lib.actions.Action; +import com.google.devtools.build.lib.actions.ActionInputHelper; import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey; import com.google.devtools.build.lib.actions.Artifact; import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact; @@ -32,8 +34,10 @@ import com.google.devtools.build.lib.actions.ArtifactOwner; import com.google.devtools.build.lib.actions.ArtifactRoot; import com.google.devtools.build.lib.actions.FileArtifactValue; +import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue; import com.google.devtools.build.lib.actions.FileStateValue; import com.google.devtools.build.lib.actions.FileValue; +import com.google.devtools.build.lib.actions.cache.DigestUtils; import com.google.devtools.build.lib.actions.util.TestAction; import com.google.devtools.build.lib.analysis.BlazeDirectories; import com.google.devtools.build.lib.analysis.ServerDirectories; @@ -50,6 +54,7 @@ import com.google.devtools.build.lib.util.io.OutErr; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.BatchStat; +import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileStatusWithDigest; import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter; @@ -74,9 +79,11 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import java.io.IOException; +import java.nio.charset.Charset; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -787,6 +794,113 @@ private ActionExecutionValue actionValueWithTreeArtifacts(List /*actionDependsOnBuildId=*/ false); } + private ActionExecutionValue actionValueWithRemoteTreeArtifact(SpecialArtifact output, Map children) { + ImmutableMap.Builder childFileValues = ImmutableMap.builder(); + for (Map.Entry child : children.entrySet()) { + childFileValues.put(ActionInputHelper.treeFileArtifact(output, child.getKey()), child.getValue()); + } + TreeArtifactValue treeArtifactValue = TreeArtifactValue.create(childFileValues.build()); + return ActionExecutionValue.create(Collections.emptyMap(), + Collections.singletonMap(output, treeArtifactValue), ImmutableMap.of(), null, null, false); + } + + private ActionExecutionValue actionValueWithRemoteArtifact(Artifact output, RemoteFileArtifactValue value) { + return ActionExecutionValue.create( + Collections.singletonMap(output, ArtifactFileMetadata.PLACEHOLDER), ImmutableMap.of(), + Collections.singletonMap(output, value), null, null, false); + } + + private RemoteFileArtifactValue createRemoteFileArtifactValue(String contents) { + byte[] data = contents.getBytes(); + DigestHashFunction hashFn = fs.getDigestFunction(); + HashCode hash = hashFn.getHashFunction().hashBytes(data); + return new RemoteFileArtifactValue(hash.asBytes(), data.length, -1); + } + + @Test + public void testRemoteAndLocalArtifacts() throws Exception { + // Test that injected remote artifacts are trusted by the FileSystemValueChecker + // and that local files always takes preference over remote files. + ActionLookupKey actionLookupKey = + new ActionLookupKey() { + @Override + public SkyFunctionName functionName() { + return SkyFunctionName.FOR_TESTING; + } + }; + SkyKey actionKey1 = ActionExecutionValue.key(actionLookupKey, 0); + SkyKey actionKey2 = ActionExecutionValue.key(actionLookupKey, 1); + + Artifact out1 = createDerivedArtifact("foo"); + Artifact out2 = createDerivedArtifact("bar"); + Map metadataToInject = new HashMap<>(); + metadataToInject.put(actionKey1, actionValueWithRemoteArtifact(out1, createRemoteFileArtifactValue("foo-content"))); + metadataToInject.put(actionKey2, actionValueWithRemoteArtifact(out2, createRemoteFileArtifactValue("bar-content"))); + differencer.inject(metadataToInject); + + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setNumThreads(1) + .setEventHander(NullEventHandler.INSTANCE) + .build(); + assertThat(driver.evaluate(ImmutableList.of(actionKey1, actionKey2), evaluationContext).hasError()).isFalse(); + assertThat(new FilesystemValueChecker(null, null).getDirtyActionValues(evaluator.getValues(), + null, ModifiedFileSet.EVERYTHING_MODIFIED)).isEmpty(); + + // Create the "out1" artifact on the filesystem and test that it invalidates the generating + // action's SkyKey. + FileSystemUtils.writeContentAsLatin1(out1.getPath(), "new-foo-content"); + assertThat( + new FilesystemValueChecker(null, null) + .getDirtyActionValues( + evaluator.getValues(), null, ModifiedFileSet.EVERYTHING_MODIFIED)) + .containsExactly(actionKey1); + } + + @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. + ActionLookupKey actionLookupKey = + new ActionLookupKey() { + @Override + public SkyFunctionName functionName() { + return SkyFunctionName.FOR_TESTING; + } + }; + SkyKey actionKey = ActionExecutionValue.key(actionLookupKey, 0); + + SpecialArtifact treeArtifact = createTreeArtifact("dir"); + treeArtifact.getPath().createDirectoryAndParents(); + Map treeArtifactMetadata = new HashMap<>(); + treeArtifactMetadata.put(PathFragment.create("foo"), createRemoteFileArtifactValue("foo-content")); + treeArtifactMetadata.put(PathFragment.create("bar"), createRemoteFileArtifactValue("bar-content")); + + Map metadataToInject = new HashMap<>(); + metadataToInject.put(actionKey, actionValueWithRemoteTreeArtifact(treeArtifact, treeArtifactMetadata)); + differencer.inject(metadataToInject); + + EvaluationContext evaluationContext = + EvaluationContext.newBuilder() + .setKeepGoing(false) + .setNumThreads(1) + .setEventHander(NullEventHandler.INSTANCE) + .build(); + assertThat(driver.evaluate(ImmutableList.of(actionKey), evaluationContext).hasError()).isFalse(); + assertThat(new FilesystemValueChecker(null, null).getDirtyActionValues(evaluator.getValues(), + null, ModifiedFileSet.EVERYTHING_MODIFIED)).isEmpty(); + + // Create dir/foo on the local disk and test that it invalidates the associated sky key. + TreeFileArtifact fooArtifact = treeFileArtifact(treeArtifact, "foo"); + FileSystemUtils.writeContentAsLatin1(fooArtifact.getPath(), "new-foo-content"); + assertThat( + new FilesystemValueChecker(null, null) + .getDirtyActionValues( + evaluator.getValues(), null, ModifiedFileSet.EVERYTHING_MODIFIED)) + .containsExactly(actionKey); + } + @Test public void testPropagatesRuntimeExceptions() throws Exception { Collection values =