Automated rollback of commit dcf96007e537c8abc497b2fd3c74862bbcc64deb.
*** Reason for rollback ***
The issue was fixed in the depot.
*** Original change description ***
Automated rollback of commit 0a0c96289068507a72cc6e315f27cf129aeef9b2.
*** Reason for rollback ***
This CL makes targets in the nightly crash.
*** Original change description ***
Teach the FilesystemValueChecker about remotely stored outputs
The FilesystemValueChecker is used by Bazel to detect modified outputs before
running a command. This change teaches it about remote output files that don't
exist in the output base, but are only stored physically on a remote storage
system with SkyFrame only tracking file met...
***
ROLLBACK_OF=238268954
RELNOTES: NONE
PiperOrigin-RevId: 238436477
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 591227b..7beaae8 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 @@
* <li>a "middleman marker" object, which has a null digest, 0 size, and mtime of 0.
* <li>The "self data" of a TreeArtifact, where we would expect to see a digest representing the
* artifact's contents, and a size of 0.
+ * <li>a file that's only stored by a remote caching/execution system, in which case we would
+ * expect to see a digest and size.
* </ul>
*/
@Immutable
@@ -138,6 +140,11 @@
*/
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 +531,12 @@
@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 9efc2b6..fad7df5 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,7 +81,7 @@
private static final Predicate<SkyKey> ACTION_FILTER =
SkyFunctionName.functionIs(SkyFunctions.ACTION_EXECUTION);
- private final TimestampGranularityMonitor tsgm;
+ @Nullable private final TimestampGranularityMonitor tsgm;
@Nullable
private final Range<Long> lastExecutionTimeRange;
private AtomicInteger modifiedOutputFilesCounter = new AtomicInteger(0);
@@ -398,8 +399,8 @@
try {
Set<PathFragment> currentDirectoryValue =
TreeArtifactValue.explodeDirectory(artifact.getPath());
- Set<PathFragment> valuePaths = value.getChildPaths();
- return !currentDirectoryValue.equals(valuePaths);
+ return !(currentDirectoryValue.isEmpty() && value.isRemote())
+ && !currentDirectoryValue.equals(value.getChildPaths());
} catch (IOException e) {
return true;
}
@@ -417,7 +418,10 @@
try {
ArtifactFileMetadata fileMetadata =
ActionMetadataHandler.fileMetadataFromArtifact(file, null, tsgm);
- if (!fileMetadata.equals(lastKnownData)) {
+ FileArtifactValue fileValue = actionValue.getArtifactValue(file);
+ boolean lastSeenRemotely = (fileValue != null) && fileValue.isRemote();
+ boolean trustRemoteValue = !fileMetadata.exists() && lastSeenRemotely;
+ if (!trustRemoteValue && !fileMetadata.equals(lastKnownData)) {
updateIntraBuildModifiedCounter(
fileMetadata.exists() ? file.getPath().getLastModifiedTime(Symlinks.FOLLOW) : -1,
lastKnownData.isSymlink(),
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 dd72ee2..b46ac77 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;
@@ -53,30 +54,55 @@
}
};
+ private static final TreeArtifactValue EMPTY =
+ new TreeArtifactValue(
+ DigestUtils.fromMetadata(ImmutableMap.of()).getDigestBytesUnsafe(),
+ ImmutableMap.of(),
+ /* remote= */ false);
+
private final byte[] digest;
private final Map<TreeFileArtifact, FileArtifactValue> childData;
private BigInteger valueFingerprint;
+ private final boolean remote;
@AutoCodec.VisibleForSerialization
- TreeArtifactValue(byte[] digest, Map<TreeFileArtifact, FileArtifactValue> childData) {
+ TreeArtifactValue(
+ byte[] digest, Map<TreeFileArtifact, FileArtifactValue> childData, boolean remote) {
this.digest = digest;
this.childData = ImmutableMap.copyOf(childData);
+ this.remote = remote;
}
/**
- * Returns a TreeArtifactValue out of the given Artifact-relative path fragments
- * and their corresponding FileArtifactValues.
+ * Returns a TreeArtifactValue out of the given Artifact-relative path fragments and their
+ * corresponding FileArtifactValues.
+ *
+ * <p>All {@code childFileValues} must return the same value for {@link
+ * FileArtifactValue#isRemote()}.
*/
static TreeArtifactValue create(Map<TreeFileArtifact, FileArtifactValue> childFileValues) {
+ if (childFileValues.isEmpty()) {
+ return EMPTY;
+ }
Map<String, FileArtifactValue> digestBuilder =
Maps.newHashMapWithExpectedSize(childFileValues.size());
+ Boolean remote = null;
for (Map.Entry<TreeFileArtifact, FileArtifactValue> e : childFileValues.entrySet()) {
- digestBuilder.put(e.getKey().getParentRelativePath().getPathString(), e.getValue());
+ FileArtifactValue value = e.getValue();
+ if (remote == null) {
+ remote = value.isRemote();
+ }
+ Preconditions.checkArgument(
+ value.isRemote() == remote,
+ "files in a tree artifact must either be all remote or all local. '%v', remote=%b",
+ value,
+ value.isRemote());
+ digestBuilder.put(e.getKey().getParentRelativePath().getPathString(), value);
}
-
return new TreeArtifactValue(
DigestUtils.fromMetadata(digestBuilder).getDigestBytesUnsafe(),
- ImmutableMap.copyOf(childFileValues));
+ ImmutableMap.copyOf(childFileValues),
+ Preconditions.checkNotNull(remote));
}
FileArtifactValue getSelfData() {
@@ -104,6 +130,11 @@
return childData;
}
+ /** Returns true if the {@link TreeFileArtifact}s are only stored remotely. */
+ public boolean isRemote() {
+ return remote;
+ }
+
@Override
public BigInteger getValueFingerprint() {
if (valueFingerprint == null) {
@@ -150,7 +181,7 @@
* Java's concurrent collections disallow null members.
*/
static final TreeArtifactValue MISSING_TREE_ARTIFACT =
- new TreeArtifactValue(null, ImmutableMap.<TreeFileArtifact, FileArtifactValue>of()) {
+ new TreeArtifactValue(null, ImmutableMap.<TreeFileArtifact, FileArtifactValue>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 62691af..b80697b 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,6 +34,7 @@
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.util.TestAction;
@@ -50,6 +53,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;
@@ -77,6 +81,7 @@
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 +792,150 @@
/*actionDependsOnBuildId=*/ false);
}
+ private ActionExecutionValue actionValueWithRemoteTreeArtifact(
+ SpecialArtifact output, Map<PathFragment, RemoteFileArtifactValue> children) {
+ ImmutableMap.Builder<TreeFileArtifact, FileArtifactValue> childFileValues =
+ ImmutableMap.builder();
+ for (Map.Entry<PathFragment, RemoteFileArtifactValue> 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(),
+ /* outputSymlinks= */ null,
+ /* discoveredModules= */ null,
+ /* actionDependsOnBuildId= */ false);
+ }
+
+ private ActionExecutionValue actionValueWithRemoteArtifact(
+ Artifact output, RemoteFileArtifactValue value) {
+ return ActionExecutionValue.create(
+ Collections.singletonMap(output, ArtifactFileMetadata.PLACEHOLDER),
+ ImmutableMap.of(),
+ Collections.singletonMap(output, value),
+ /* outputSymlinks= */ null,
+ /* discoveredModules= */ null,
+ /* actionDependsOnBuildId= */ 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<SkyKey, SkyValue> 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(/* tsgm= */ null, /* lastExecutionTimeRange= */ null)
+ .getDirtyActionValues(
+ evaluator.getValues(),
+ /* batchStatter= */ 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(/* tsgm= */ null, /* lastExecutionTimeRange= */ null)
+ .getDirtyActionValues(
+ evaluator.getValues(),
+ /* batchStatter= */ 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<PathFragment, RemoteFileArtifactValue> treeArtifactMetadata = new HashMap<>();
+ treeArtifactMetadata.put(
+ PathFragment.create("foo"), createRemoteFileArtifactValue("foo-content"));
+ treeArtifactMetadata.put(
+ PathFragment.create("bar"), createRemoteFileArtifactValue("bar-content"));
+
+ Map<SkyKey, SkyValue> 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(/* tsgm= */ null, /* lastExecutionTimeRange= */ null)
+ .getDirtyActionValues(
+ evaluator.getValues(),
+ /* batchStatter= */ 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(/* tsgm= */ null, /* lastExecutionTimeRange= */ null)
+ .getDirtyActionValues(
+ evaluator.getValues(),
+ /* batchStatter= */ null,
+ ModifiedFileSet.EVERYTHING_MODIFIED))
+ .containsExactly(actionKey);
+ }
+
@Test
public void testPropagatesRuntimeExceptions() throws Exception {
Collection<SkyKey> values =
