Do not assume files are remote for MetadataInjector methods.
PiperOrigin-RevId: 314837801
diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java
index 6e06891..fb37eb8 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataHandler.java
@@ -18,6 +18,8 @@
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.MetadataProvider;
+import com.google.devtools.build.lib.vfs.FileStatus;
+import java.io.IOException;
/**
* Retrieves {@link FileArtifactValue} of {@link Artifact}s, and inserts virtual metadata as well.
@@ -34,6 +36,17 @@
/** Sets digest for virtual artifacts (e.g. middlemen). {@code digest} must not be null. */
void setDigestForVirtualArtifact(Artifact artifact, byte[] digest);
+ /**
+ * Constructs a {@link FileArtifactValue} for the given output whose digest is known.
+ *
+ * <p>This call does not inject the returned metadata. It should be injected with a followup call
+ * to {@link #injectFile} or {@link #injectDirectory} as appropriate.
+ *
+ * <p>chmod will not be called on the output.
+ */
+ FileArtifactValue constructMetadataForDigest(
+ Artifact output, FileStatus statNoFollow, byte[] digest) throws IOException;
+
/** Retrieves the artifacts inside the TreeArtifact, without injecting its digest. */
ImmutableSet<TreeFileArtifact> getExpandedOutputs(Artifact artifact);
@@ -46,8 +59,7 @@
/**
* Discards all known output artifact metadata, presumably because outputs will be modified. May
- * only be called before any metadata is injected using {@link #injectDigest} or {@link
- * #markOmitted};
+ * only be called before any metadata is injected.
*
* <p>Must be called at most once on any specific instance.
*/
diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java
index a944cae..ae9cec4 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/cache/MetadataInjector.java
@@ -16,29 +16,31 @@
import com.google.devtools.build.lib.actions.Artifact;
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.RemoteFileArtifactValue;
-import com.google.devtools.build.lib.vfs.FileStatus;
+import com.google.devtools.build.lib.actions.FileArtifactValue;
import java.util.Map;
/** Supports metadata injection of action outputs into skyframe. */
public interface MetadataInjector {
/**
- * Injects metadata of a file that is stored remotely.
+ * Injects the metadata of a file.
+ *
+ * <p>This can be used to save filesystem operations when the metadata is already known.
*
* @param output a regular output file
* @param metadata the remote file metadata
*/
- void injectRemoteFile(Artifact output, RemoteFileArtifactValue metadata);
+ void injectFile(Artifact output, FileArtifactValue metadata);
/**
- * Injects the metadata of a tree artifact whose contents are stored remotely.
+ * Injects the metadata of a tree artifact.
+ *
+ * <p>This can be used to save filesystem operations when the metadata is already known.
*
* @param output an output directory {@linkplain Artifact#isTreeArtifact tree artifact}
* @param children the metadata of the files stored in the directory
*/
- void injectRemoteDirectory(
- SpecialArtifact output, Map<TreeFileArtifact, RemoteFileArtifactValue> children);
+ void injectDirectory(SpecialArtifact output, Map<TreeFileArtifact, FileArtifactValue> children);
/**
* Marks an {@link Artifact} as intentionally omitted.
@@ -47,9 +49,4 @@
* action depends on) from a remote system.
*/
void markOmitted(Artifact output);
-
- /**
- * Injects provided digest into the metadata handler, simultaneously caching lstat() data as well.
- */
- void injectDigest(Artifact output, FileStatus statNoFollow, byte[] digest);
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java
index a7a0bdb..0c881f4 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteCache.java
@@ -44,6 +44,7 @@
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.EnvironmentalExecException;
import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
import com.google.devtools.build.lib.actions.UserExecException;
import com.google.devtools.build.lib.actions.cache.MetadataInjector;
@@ -614,7 +615,7 @@
+ "--experimental_remote_download_outputs=minimal");
}
SpecialArtifact parent = (SpecialArtifact) output;
- ImmutableMap.Builder<TreeFileArtifact, RemoteFileArtifactValue> childMetadata =
+ ImmutableMap.Builder<TreeFileArtifact, FileArtifactValue> childMetadata =
ImmutableMap.builderWithExpectedSize(directory.files.size());
for (FileMetadata file : directory.files()) {
TreeFileArtifact child =
@@ -627,7 +628,7 @@
actionId);
childMetadata.put(child, value);
}
- metadataInjector.injectRemoteDirectory(parent, childMetadata.build());
+ metadataInjector.injectDirectory(parent, childMetadata.build());
} else {
FileMetadata outputMetadata = metadata.file(execRoot.getRelative(output.getExecPathString()));
if (outputMetadata == null) {
@@ -635,7 +636,7 @@
// SkyFrame will make sure to fail.
return;
}
- metadataInjector.injectRemoteFile(
+ metadataInjector.injectFile(
output,
new RemoteFileArtifactValue(
DigestUtil.toBinaryDigest(outputMetadata.digest()),
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 1a65a9c..ed3404f 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
@@ -29,7 +29,6 @@
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.FileArtifactValue;
-import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
import com.google.devtools.build.lib.actions.FileStateType;
import com.google.devtools.build.lib.actions.FileStateValue;
import com.google.devtools.build.lib.actions.FilesetManifest;
@@ -375,7 +374,8 @@
}
@Override
- public void injectDigest(Artifact output, FileStatus statNoFollow, byte[] digest) {
+ public FileArtifactValue constructMetadataForDigest(
+ Artifact output, FileStatus statNoFollow, byte[] digest) throws IOException {
Preconditions.checkState(executionMode.get());
Preconditions.checkState(!output.isSymlink());
Preconditions.checkNotNull(digest);
@@ -383,37 +383,26 @@
// We have to add the artifact to injectedFiles before calling constructFileArtifactValue to
// avoid duplicate chmod calls.
store.injectedFiles().add(output);
- try {
- // This call may make an extra call to Path#getFastDigest to see if the digest is readily
- // available, even though we have the injected digest. This is necessary though, because
- // otherwise this FileArtifactValue will not compare equal to another one created for the same
- // file without an injected digest, because the other one will be missing its digest.
- FileArtifactValue fileMetadata =
- constructFileArtifactValue(
- output, FileStatusWithDigestAdapter.adapt(statNoFollow), digest);
- store.putArtifactData(output, fileMetadata);
- } catch (IOException e) {
- // Do nothing - we just failed to inject metadata. Real error handling will be done later,
- // when somebody tries to access that file.
- }
+
+ // TODO(jhorvitz): This unnecessarily calls getFastDigest even though we know the digest.
+ return constructFileArtifactValue(
+ output, FileStatusWithDigestAdapter.adapt(statNoFollow), digest);
}
@Override
- public void injectRemoteFile(Artifact output, RemoteFileArtifactValue metadata) {
+ public void injectFile(Artifact output, FileArtifactValue metadata) {
Preconditions.checkArgument(
isKnownOutput(output), "%s is not a declared output of this action", output);
Preconditions.checkArgument(
- !output.isTreeArtifact(),
- "injectRemoteFile must not be called on TreeArtifacts: %s",
- output);
+ !output.isTreeArtifact(), "injectFile must not be called on TreeArtifacts: %s", output);
Preconditions.checkState(
executionMode.get(), "Tried to inject %s outside of execution", output);
store.injectOutputData(output, metadata);
}
@Override
- public void injectRemoteDirectory(
- SpecialArtifact output, Map<TreeFileArtifact, RemoteFileArtifactValue> children) {
+ public void injectDirectory(
+ SpecialArtifact output, Map<TreeFileArtifact, FileArtifactValue> children) {
Preconditions.checkArgument(
isKnownOutput(output), "%s is not a declared output of this action", output);
Preconditions.checkArgument(output.isTreeArtifact(), "output must be a tree artifact");
diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
index 29b5eb0..9e12a2a 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
@@ -53,14 +53,12 @@
import com.google.devtools.build.lib.actions.BuildConfigurationEvent;
import com.google.devtools.build.lib.actions.Executor;
import com.google.devtools.build.lib.actions.FileArtifactValue;
-import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
import com.google.devtools.build.lib.actions.MutableActionGraph;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
import com.google.devtools.build.lib.actions.PackageRootResolver;
import com.google.devtools.build.lib.actions.cache.MetadataHandler;
import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics.MissDetail;
import com.google.devtools.build.lib.actions.cache.Protos.ActionCacheStatistics.MissReason;
-import com.google.devtools.build.lib.actions.util.ActionsTestUtil.FakeMetadataHandlerBase;
import com.google.devtools.build.lib.analysis.actions.CustomCommandLine;
import com.google.devtools.build.lib.analysis.actions.SpawnActionTemplate;
import com.google.devtools.build.lib.analysis.actions.SpawnActionTemplate.OutputPathMapper;
@@ -971,18 +969,19 @@
}
@Override
- public void injectDigest(Artifact output, FileStatus statNoFollow, byte[] digest) {
+ public FileArtifactValue constructMetadataForDigest(
+ Artifact output, FileStatus statNoFollow, byte[] digest) {
throw new UnsupportedOperationException();
}
@Override
- public void injectRemoteFile(Artifact output, RemoteFileArtifactValue metadata) {
+ public void injectFile(Artifact output, FileArtifactValue metadata) {
throw new UnsupportedOperationException();
}
@Override
- public void injectRemoteDirectory(
- SpecialArtifact treeArtifact, Map<TreeFileArtifact, RemoteFileArtifactValue> children) {
+ public void injectDirectory(
+ SpecialArtifact treeArtifact, Map<TreeFileArtifact, FileArtifactValue> children) {
throw new UnsupportedOperationException();
}
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java
index e95fab7..4808159 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteCacheTests.java
@@ -50,6 +50,7 @@
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.ExecException;
+import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
import com.google.devtools.build.lib.actions.cache.MetadataInjector;
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
@@ -929,8 +930,8 @@
// assert
assertThat(inMemoryOutput).isNull();
- verify(injector).injectRemoteFile(eq(a1), remoteFileMatchingDigest(d1));
- verify(injector).injectRemoteFile(eq(a2), remoteFileMatchingDigest(d2));
+ verify(injector).injectFile(eq(a1), remoteFileMatchingDigest(d1));
+ verify(injector).injectFile(eq(a2), remoteFileMatchingDigest(d2));
Path outputBase = artifactRoot.getRoot().asPath();
assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty();
@@ -992,13 +993,13 @@
// assert
assertThat(inMemoryOutput).isNull();
- Map<TreeFileArtifact, RemoteFileArtifactValue> m =
+ Map<TreeFileArtifact, FileArtifactValue> m =
ImmutableMap.of(
TreeFileArtifact.createTreeOutput(dir, "file1"),
new RemoteFileArtifactValue(toBinaryDigest(d1), d1.getSizeBytes(), 1, "action-id"),
TreeFileArtifact.createTreeOutput(dir, "a/file2"),
new RemoteFileArtifactValue(toBinaryDigest(d2), d2.getSizeBytes(), 1, "action-id"));
- verify(injector).injectRemoteDirectory(eq(dir), eq(m));
+ verify(injector).injectDirectory(eq(dir), eq(m));
Path outputBase = artifactRoot.getRoot().asPath();
assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty();
@@ -1144,8 +1145,8 @@
assertThat(inMemoryOutput.getContents()).isEqualTo(expectedContents);
assertThat(inMemoryOutput.getOutput()).isEqualTo(a1);
// The in memory file also needs to be injected as an output
- verify(injector).injectRemoteFile(eq(a1), remoteFileMatchingDigest(d1));
- verify(injector).injectRemoteFile(eq(a2), remoteFileMatchingDigest(d2));
+ verify(injector).injectFile(eq(a1), remoteFileMatchingDigest(d1));
+ verify(injector).injectFile(eq(a2), remoteFileMatchingDigest(d2));
Path outputBase = artifactRoot.getRoot().asPath();
assertThat(outputBase.readdir(Symlinks.NOFOLLOW)).isEmpty();
@@ -1181,7 +1182,7 @@
// assert
assertThat(inMemoryOutput).isNull();
// The in memory file metadata also should not have been injected.
- verify(injector, never()).injectRemoteFile(eq(a1), remoteFileMatchingDigest(d1));
+ verify(injector, never()).injectFile(eq(a1), remoteFileMatchingDigest(d1));
}
@Test
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 e77a62c..ca3d511 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
@@ -364,7 +364,7 @@
assertThat(handler.getMetadata(artifact).getSize()).isEqualTo(10);
// Inject a remote file of size 42.
- handler.injectRemoteFile(
+ handler.injectFile(
artifact, new RemoteFileArtifactValue(new byte[] {1, 2, 3}, 42, 0, "ultimate-answer"));
assertThat(handler.getMetadata(artifact).getSize()).isEqualTo(42);
@@ -391,7 +391,7 @@
byte[] digest = new byte[] {1, 2, 3};
int size = 10;
- handler.injectRemoteFile(
+ handler.injectFile(
artifact, new RemoteFileArtifactValue(digest, size, /*locationIndex=*/ 1, "action-id"));
FileArtifactValue v = handler.getMetadata(artifact);
@@ -428,8 +428,8 @@
RemoteFileArtifactValue child1Value = new RemoteFileArtifactValue(new byte[] {1, 2, 3}, 5, 1);
RemoteFileArtifactValue child2Value = new RemoteFileArtifactValue(new byte[] {4, 5, 6}, 10, 1);
- handler.injectRemoteFile(child1, child1Value);
- handler.injectRemoteFile(child2, child2Value);
+ handler.injectFile(child1, child1Value);
+ handler.injectFile(child2, child2Value);
FileArtifactValue treeMetadata = handler.getMetadata(treeArtifact);
FileArtifactValue child1Metadata = handler.getMetadata(child1);
@@ -464,12 +464,12 @@
new RemoteFileArtifactValue(new byte[] {1, 2, 3}, 5, 1, "foo");
RemoteFileArtifactValue barValue =
new RemoteFileArtifactValue(new byte[] {4, 5, 6}, 10, 1, "bar");
- Map<TreeFileArtifact, RemoteFileArtifactValue> children =
+ Map<TreeFileArtifact, FileArtifactValue> children =
ImmutableMap.of(
TreeFileArtifact.createTreeOutput(treeArtifact, "foo"), fooValue,
TreeFileArtifact.createTreeOutput(treeArtifact, "bar"), barValue);
- handler.injectRemoteDirectory(treeArtifact, children);
+ handler.injectDirectory(treeArtifact, children);
FileArtifactValue value = handler.getMetadata(treeArtifact);
assertThat(value).isNotNull();
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 2552a36..4a11445 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
@@ -39,6 +39,7 @@
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.ArtifactRoot;
import com.google.devtools.build.lib.actions.BuildFailedException;
+import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
import com.google.devtools.build.lib.actions.MetadataProvider;
import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
@@ -557,15 +558,8 @@
assertThat(errors.get(1).getMessage()).contains("not all outputs were created or valid");
}
- // This is more a smoke test than anything, because it turns out that:
- // 1) there is no easy way to turn fast digests on/off for these test cases, and
- // 2) injectDigest() doesn't really complain if you inject bad digests or digests
- // for nonexistent files. Instead some weird error shows up down the line.
- // In fact, there are no tests for injectDigest anywhere in the codebase.
- // So all we're really testing here is that injectDigest() doesn't throw a weird exception.
- // TODO(bazel-team): write real tests for injectDigest, here and elsewhere.
@Test
- public void digestInjection() throws Exception {
+ public void constructMetadataForDigest() throws Exception {
SpecialArtifact out = createTreeArtifact("output");
Action action =
new SimpleTestAction(out) {
@@ -578,10 +572,26 @@
MetadataHandler md = actionExecutionContext.getMetadataHandler();
FileStatus stat = child1.getPath().stat(Symlinks.NOFOLLOW);
- md.injectDigest(child1, stat, Hashing.sha256().hashString("one", UTF_8).asBytes());
+ FileArtifactValue metadata1 =
+ md.constructMetadataForDigest(
+ child1, stat, Hashing.sha256().hashString("one", UTF_8).asBytes());
stat = child2.getPath().stat(Symlinks.NOFOLLOW);
- md.injectDigest(child2, stat, Hashing.sha256().hashString("two", UTF_8).asBytes());
+ FileArtifactValue metadata2 =
+ md.constructMetadataForDigest(
+ child2, stat, Hashing.sha256().hashString("two", UTF_8).asBytes());
+
+ // The metadata will not be equal to reading from the filesystem since the filesystem
+ // won't have the digest. However, we should be able to detect that nothing could have
+ // been modified.
+ assertThat(
+ metadata1.couldBeModifiedSince(
+ FileArtifactValue.createForTesting(child1.getPath())))
+ .isFalse();
+ assertThat(
+ metadata2.couldBeModifiedSince(
+ FileArtifactValue.createForTesting(child2.getPath())))
+ .isFalse();
}
};
@@ -610,8 +620,7 @@
actionExecutionContext
.getMetadataHandler()
- .injectRemoteDirectory(
- out, ImmutableMap.of(child1, remoteFile1, child2, remoteFile2));
+ .injectDirectory(out, ImmutableMap.of(child1, remoteFile1, child2, remoteFile2));
}
};
