Tighten parameter types for markOmitted and injectDigest.
ActionMetadataHandler ignores non-Artifacts anyway. Additionally add a test utility for a throwing MetadataHandler.
RELNOTES: None.
PiperOrigin-RevId: 312686047
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 e83f34a..a944cae 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
@@ -13,7 +13,6 @@
// limitations under the License.
package com.google.devtools.build.lib.actions.cache;
-import com.google.devtools.build.lib.actions.ActionInput;
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;
@@ -47,10 +46,10 @@
* <p>This is used as an optimization to not download "orphaned artifacts" (=artifacts that no
* action depends on) from a remote system.
*/
- void markOmitted(ActionInput output);
+ void markOmitted(Artifact output);
/**
* Injects provided digest into the metadata handler, simultaneously caching lstat() data as well.
*/
- void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] digest);
+ void injectDigest(Artifact output, FileStatus statNoFollow, byte[] 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 8acefb5..d3b4aad 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
@@ -436,58 +436,53 @@
}
@Override
- public void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] digest) {
+ public void injectDigest(Artifact output, FileStatus statNoFollow, byte[] digest) {
Preconditions.checkState(executionMode.get());
Preconditions.checkState(!output.isSymlink());
- // Assumption: any non-Artifact output is 'virtual' and should be ignored here.
- if (output instanceof Artifact) {
- final Artifact artifact = (Artifact) output;
- // We have to add the artifact to injectedFiles before calling constructFileArtifactValue
- // to avoid duplicate chmod calls.
- store.injectedFiles().add(artifact);
- FileArtifactValue fileMetadata;
- try {
- // This call may do an unnecessary call to Path#getFastDigest to see if the digest is
- // readily available. We cannot pass the digest in, though, because if it is not available
- // from the filesystem, this ArtifactFileMetadata will not compare equal to another one
- // created for the
- // same file, because the other one will be missing its digest.
- fileMetadata =
- constructFileArtifactValue(artifact, FileStatusWithDigestAdapter.adapt(statNoFollow));
- // Ensure the digest supplied matches the actual digest if it exists.
- byte[] fileDigest = fileMetadata.getDigest();
- if (fileDigest != null && !Arrays.equals(digest, fileDigest)) {
- BaseEncoding base16 = BaseEncoding.base16();
- String digestString = (digest != null) ? base16.encode(digest) : "null";
- String fileDigestString = base16.encode(fileDigest);
- throw new IllegalStateException(
- "Expected digest "
- + digestString
- + " for artifact "
- + artifact
- + ", but got "
- + fileDigestString
- + " ("
- + fileMetadata
- + ")");
- }
- } catch (IOException e) {
- // Do nothing - we just failed to inject metadata. Real error handling will be done later,
- // when somebody will try to access that file.
- return;
- }
- // If needed, insert additional data. Note that this can only be true if the file is empty or
- // the filesystem does not support fast digests. Since we usually only inject digests when
- // running with a filesystem that supports fast digests, this is fairly unlikely.
- try {
- maybeStoreAdditionalData(artifact, fileMetadata, digest);
- } catch (IOException e) {
+ // We have to add the artifact to injectedFiles before calling constructFileArtifactValue
+ // to avoid duplicate chmod calls.
+ store.injectedFiles().add(output);
+ FileArtifactValue fileMetadata;
+ try {
+ // This call may do an unnecessary call to Path#getFastDigest to see if the digest is readily
+ // available. We cannot pass the digest in, though, because if it is not available from the
+ // filesystem, this ArtifactFileMetadata will not compare equal to another one created for the
+ // same file, because the other one will be missing its digest.
+ fileMetadata =
+ constructFileArtifactValue(output, FileStatusWithDigestAdapter.adapt(statNoFollow));
+ // Ensure the digest supplied matches the actual digest if it exists.
+ byte[] fileDigest = fileMetadata.getDigest();
+ if (fileDigest != null && !Arrays.equals(digest, fileDigest)) {
+ BaseEncoding base16 = BaseEncoding.base16();
+ String digestString = (digest != null) ? base16.encode(digest) : "null";
+ String fileDigestString = base16.encode(fileDigest);
throw new IllegalStateException(
- "Filesystem should not have been accessed while injecting data for "
- + artifact.prettyPrint(),
- e);
+ "Expected digest "
+ + digestString
+ + " for artifact "
+ + output
+ + ", but got "
+ + fileDigestString
+ + " ("
+ + fileMetadata
+ + ")");
}
+ } catch (IOException e) {
+ // Do nothing - we just failed to inject metadata. Real error handling will be done later,
+ // when somebody will try to access that file.
+ return;
+ }
+ // If needed, insert additional data. Note that this can only be true if the file is empty or
+ // the filesystem does not support fast digests. Since we usually only inject digests when
+ // running with a filesystem that supports fast digests, this is fairly unlikely.
+ try {
+ maybeStoreAdditionalData(output, fileMetadata, digest);
+ } catch (IOException e) {
+ throw new IllegalStateException(
+ "Filesystem should not have been accessed while injecting data for "
+ + output.prettyPrint(),
+ e);
}
}
@@ -516,13 +511,10 @@
}
@Override
- public void markOmitted(ActionInput output) {
+ public void markOmitted(Artifact output) {
Preconditions.checkState(executionMode.get());
- if (output instanceof Artifact) {
- Artifact artifact = (Artifact) output;
- Preconditions.checkState(omittedOutputs.add(artifact), artifact);
- store.putArtifactData(artifact, FileArtifactValue.OMITTED_FILE_MARKER);
- }
+ Preconditions.checkState(omittedOutputs.add(output), "%s marked as omitted twice", output);
+ store.putArtifactData(output, FileArtifactValue.OMITTED_FILE_MARKER);
}
@Override
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 ae92ac8..29b5eb0 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
@@ -60,6 +60,7 @@
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;
@@ -930,10 +931,23 @@
}
/**
+ * A {@link MetadataHandler} for tests that throws {@link UnsupportedOperationException} for its
+ * operations.
+ */
+ public static final MetadataHandler THROWING_METADATA_HANDLER =
+ new FakeMetadataHandlerBase() {
+ @Override
+ public String toString() {
+ return "THROWING_METADATA_HANDLER";
+ }
+ };
+
+ /**
* A {@link MetadataHandler} all of whose operations throw an exception.
*
- * <p>This is to be used as a base class by other test programs that need to implement only a
- * few of the hooks required by the scenario under test.
+ * <p>This is to be used as a base class by other test programs that need to implement only a few
+ * of the hooks required by the scenario under test. Tests that need an instance but do not need
+ * any functionality can use {@link #THROWING_METADATA_HANDLER}.
*/
public static class FakeMetadataHandlerBase implements MetadataHandler {
@Override
@@ -957,7 +971,7 @@
}
@Override
- public void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] digest) {
+ public void injectDigest(Artifact output, FileStatus statNoFollow, byte[] digest) {
throw new UnsupportedOperationException();
}
@@ -973,7 +987,7 @@
}
@Override
- public void markOmitted(ActionInput output) {
+ public void markOmitted(Artifact output) {
throw new UnsupportedOperationException();
}
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
index 2ab08c1..b32abdb 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
@@ -38,16 +38,15 @@
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
-import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
-import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
import com.google.devtools.build.lib.actions.MetadataProvider;
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.SimpleSpawn;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.SpawnResult.Status;
import com.google.devtools.build.lib.actions.cache.MetadataInjector;
+import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
import com.google.devtools.build.lib.clock.JavaClock;
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
import com.google.devtools.build.lib.collect.nestedset.Order;
@@ -69,7 +68,6 @@
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.DigestHashFunction;
-import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
@@ -81,7 +79,6 @@
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
-import java.util.Map;
import java.util.SortedMap;
import org.junit.Before;
import org.junit.Test;
@@ -177,29 +174,7 @@
@Override
public MetadataInjector getMetadataInjector() {
- return new MetadataInjector() {
- @Override
- public void injectRemoteFile(Artifact output, RemoteFileArtifactValue metadata) {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void injectRemoteDirectory(
- Artifact.SpecialArtifact output,
- Map<TreeFileArtifact, RemoteFileArtifactValue> children) {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void markOmitted(ActionInput output) {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] digest) {
- throw new UnsupportedOperationException();
- }
- };
+ return ActionsTestUtil.THROWING_METADATA_HANDLER;
}
@Override
diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/BUILD b/src/test/java/com/google/devtools/build/lib/remote/util/BUILD
index 0ec7965..d68132c 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/util/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/remote/util/BUILD
@@ -24,6 +24,7 @@
"//src/main/java/com/google/devtools/build/lib/util/io",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
+ "//src/test/java/com/google/devtools/build/lib/actions/util",
"//third_party:guava",
"//third_party/protobuf:protobuf_java",
"@remoteapis//:build_bazel_remote_execution_v2_remote_execution_java_proto",
diff --git a/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java b/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java
index 0c505bc..907ec22 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/util/FakeSpawnExecutionContext.java
@@ -19,23 +19,20 @@
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
-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.RemoteFileArtifactValue;
import com.google.devtools.build.lib.actions.MetadataProvider;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.cache.MetadataInjector;
+import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
import com.google.devtools.build.lib.exec.SpawnInputExpander;
import com.google.devtools.build.lib.exec.SpawnRunner.ProgressStatus;
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
import com.google.devtools.build.lib.util.io.FileOutErr;
-import com.google.devtools.build.lib.vfs.FileStatus;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.io.IOException;
import java.time.Duration;
import java.util.Collection;
-import java.util.Map;
import java.util.SortedMap;
/** Execution context for tests */
@@ -130,29 +127,7 @@
@Override
public MetadataInjector getMetadataInjector() {
- return new MetadataInjector() {
- @Override
- public void injectRemoteFile(Artifact output, RemoteFileArtifactValue metadata) {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void injectRemoteDirectory(
- Artifact.SpecialArtifact output,
- Map<TreeFileArtifact, RemoteFileArtifactValue> children) {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void markOmitted(ActionInput output) {
- throw new UnsupportedOperationException();
- }
-
- @Override
- public void injectDigest(ActionInput output, FileStatus statNoFollow, byte[] digest) {
- throw new UnsupportedOperationException();
- }
- };
+ return ActionsTestUtil.THROWING_METADATA_HANDLER;
}
@Override
