Allow tree artifacts to be omitted, take 2.
PiperOrigin-RevId: 314357168
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
index b465a6e..1c63e3e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
@@ -73,8 +73,12 @@
}
for (Map.Entry<Artifact, TreeArtifactValue> tree : treeArtifactData.entrySet()) {
+ TreeArtifactValue treeArtifact = tree.getValue();
+ if (TreeArtifactValue.OMITTED_TREE_MARKER.equals(treeArtifact)) {
+ continue;
+ }
for (Map.Entry<TreeFileArtifact, FileArtifactValue> file :
- tree.getValue().getChildValues().entrySet()) {
+ treeArtifact.getChildValues().entrySet()) {
// We should only have RegularFileValue instances in here, but apparently tree artifacts
// sometimes store their own root directory in here. Sad.
// https://github.com/bazelbuild/bazel/issues/9058
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
index 9c7a570..03fed1a 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionInputMapHelper.java
@@ -157,6 +157,10 @@
Map<Artifact, Collection<Artifact>> expandedArtifacts,
ActionInputMapSink inputMap,
Artifact depOwner) {
+ if (TreeArtifactValue.OMITTED_TREE_MARKER.equals(value)) {
+ inputMap.put(treeArtifact, FileArtifactValue.OMITTED_FILE_MARKER, depOwner);
+ return;
+ }
ImmutableSet.Builder<Artifact> children = ImmutableSet.builder();
for (Map.Entry<Artifact.TreeFileArtifact, FileArtifactValue> child :
value.getChildValues().entrySet()) {
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 0d9de61..1a65a9c 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
@@ -424,9 +424,19 @@
@Override
public void markOmitted(Artifact output) {
- Preconditions.checkState(executionMode.get());
- Preconditions.checkState(omittedOutputs.add(output), "%s marked as omitted twice", output);
- store.putArtifactData(output, FileArtifactValue.OMITTED_FILE_MARKER);
+ Preconditions.checkState(
+ executionMode.get(), "Tried to mark %s omitted outside of execution", output);
+ boolean newlyOmitted = omittedOutputs.add(output);
+ if (output.isTreeArtifact()) {
+ // Tolerate marking a tree artifact as omitted multiple times so that callers don't have to
+ // deduplicate when a tree artifact has several omitted children.
+ if (newlyOmitted) {
+ store.putTreeArtifactData((SpecialArtifact) output, TreeArtifactValue.OMITTED_TREE_MARKER);
+ }
+ } else {
+ Preconditions.checkState(newlyOmitted, "%s marked as omitted twice", output);
+ store.putArtifactData(output, FileArtifactValue.OMITTED_FILE_MARKER);
+ }
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
index e2c4621..d9f50b4 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ArtifactFunction.java
@@ -161,7 +161,7 @@
}
}
- private static SkyValue createTreeArtifactValueFromActionKey(
+ private static TreeArtifactValue createTreeArtifactValueFromActionKey(
ArtifactDependencies artifactDependencies, Environment env) throws InterruptedException {
// Request the list of expanded actions from the ActionTemplate.
ActionTemplateExpansion actionTemplateExpansion =
@@ -232,7 +232,7 @@
children.isEmpty(),
"Action template expansion has some but not all outputs omitted, present outputs: %s",
children);
- return FileArtifactValue.OMITTED_FILE_MARKER;
+ return TreeArtifactValue.OMITTED_TREE_MARKER;
}
return TreeArtifactValue.create(children);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index 26b90ab..baf24a3 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -2634,6 +2634,7 @@
deps = [
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
+ "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
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 7ac6c27..01920ca 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
@@ -26,6 +26,7 @@
import com.google.devtools.build.lib.actions.HasDigest;
import com.google.devtools.build.lib.actions.cache.DigestUtils;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
+import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
import com.google.devtools.build.lib.vfs.Dirent;
import com.google.devtools.build.lib.vfs.Dirent.Type;
import com.google.devtools.build.lib.vfs.Path;
@@ -43,14 +44,12 @@
* Value for TreeArtifacts, which contains a digest and the {@link FileArtifactValue}s of its child
* {@link TreeFileArtifact}s.
*/
-@AutoCodec
public class TreeArtifactValue implements HasDigest, SkyValue {
- private static final TreeArtifactValue EMPTY =
+ @SerializationConstant @AutoCodec.VisibleForSerialization
+ static final TreeArtifactValue EMPTY =
new TreeArtifactValue(
- DigestUtils.fromMetadata(ImmutableMap.of()),
- ImmutableSortedMap.of(),
- /* remote= */ false);
+ DigestUtils.fromMetadata(ImmutableMap.of()), ImmutableSortedMap.of(), /*remote=*/ false);
private final byte[] digest;
private final ImmutableSortedMap<TreeFileArtifact, FileArtifactValue> childData;
@@ -161,57 +160,67 @@
}
/**
+ * Represents a tree artifact that was intentionally omitted, similar to {@link
+ * FileArtifactValue#OMITTED_FILE_MARKER}.
+ */
+ @SerializationConstant
+ public static final TreeArtifactValue OMITTED_TREE_MARKER = createMarker("OMITTED_TREE_MARKER");
+
+ /**
* A TreeArtifactValue that represents a missing TreeArtifact. This is occasionally useful because
* Java's concurrent collections disallow null members.
*/
- static final TreeArtifactValue MISSING_TREE_ARTIFACT =
- new TreeArtifactValue(null, ImmutableSortedMap.of(), /* remote= */ false) {
- @Override
- FileArtifactValue getSelfData() {
- throw new UnsupportedOperationException();
- }
+ static final TreeArtifactValue MISSING_TREE_ARTIFACT = createMarker("MISSING_TREE_ARTIFACT");
- @Override
- public ImmutableSet<TreeFileArtifact> getChildren() {
- throw new UnsupportedOperationException();
- }
+ private static TreeArtifactValue createMarker(String toStringRepresentation) {
+ return new TreeArtifactValue(null, ImmutableSortedMap.of(), /*remote=*/ false) {
+ @Override
+ FileArtifactValue getSelfData() {
+ throw new UnsupportedOperationException(toString());
+ }
- @Override
- ImmutableMap<TreeFileArtifact, FileArtifactValue> getChildValues() {
- throw new UnsupportedOperationException();
- }
+ @Override
+ public ImmutableSet<TreeFileArtifact> getChildren() {
+ throw new UnsupportedOperationException(toString());
+ }
- @Override
- FileArtifactValue getMetadata() {
- throw new UnsupportedOperationException();
- }
+ @Override
+ ImmutableMap<TreeFileArtifact, FileArtifactValue> getChildValues() {
+ throw new UnsupportedOperationException(toString());
+ }
- @Override
- ImmutableSet<PathFragment> getChildPaths() {
- throw new UnsupportedOperationException();
- }
+ @Override
+ FileArtifactValue getMetadata() {
+ throw new UnsupportedOperationException(toString());
+ }
- @Nullable
- @Override
- public byte[] getDigest() {
- throw new UnsupportedOperationException();
- }
+ @Override
+ ImmutableSet<PathFragment> getChildPaths() {
+ throw new UnsupportedOperationException(toString());
+ }
- @Override
- public int hashCode() {
- return 24; // my favorite number
- }
+ @Nullable
+ @Override
+ public byte[] getDigest() {
+ throw new UnsupportedOperationException(toString());
+ }
- @Override
- public boolean equals(Object other) {
- return this == other;
- }
+ @Override
+ public int hashCode() {
+ return System.identityHashCode(this);
+ }
- @Override
- public String toString() {
- return "MISSING_TREE_ARTIFACT";
- }
- };
+ @Override
+ public boolean equals(Object other) {
+ return this == other;
+ }
+
+ @Override
+ public String toString() {
+ return toStringRepresentation;
+ }
+ };
+ }
private static void explodeDirectory(
Path treeArtifactPath,
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 91b15b5..70e9eba 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
@@ -18,6 +18,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.ActionInputMap;
@@ -516,6 +517,67 @@
assertThat(handler.getMetadata(ActionInputHelper.fromPath("/does/not/exist"))).isNull();
}
+ @Test
+ public void omitRegularArtifact() {
+ OutputStore store = new MinimalOutputStore();
+ Artifact omitted =
+ ActionsTestUtil.createArtifactWithRootRelativePath(
+ outputRoot, PathFragment.create("omitted"));
+ Artifact consumed =
+ ActionsTestUtil.createArtifactWithRootRelativePath(
+ outputRoot, PathFragment.create("consumed"));
+ ActionMetadataHandler handler =
+ new ActionMetadataHandler(
+ new ActionInputMap(1),
+ /*expandedFilesets=*/ ImmutableMap.of(),
+ /*missingArtifactsAllowed=*/ false,
+ ImmutableSet.of(omitted, consumed),
+ /*tsgm=*/ null,
+ ArtifactPathResolver.IDENTITY,
+ store,
+ outputRoot.getRoot().asPath());
+
+ handler.discardOutputMetadata();
+ handler.markOmitted(omitted);
+
+ assertThat(handler.artifactOmitted(omitted)).isTrue();
+ assertThat(handler.artifactOmitted(consumed)).isFalse();
+ assertThat(store.getAllArtifactData())
+ .containsExactly(omitted, FileArtifactValue.OMITTED_FILE_MARKER);
+ assertThat(store.getAllTreeArtifactData()).isEmpty();
+ }
+
+ @Test
+ public void omitTreeArtifact() {
+ OutputStore store = new MinimalOutputStore();
+ SpecialArtifact omittedTree =
+ ActionsTestUtil.createTreeArtifactWithGeneratingAction(
+ outputRoot, PathFragment.create("omitted"));
+ SpecialArtifact consumedTree =
+ ActionsTestUtil.createTreeArtifactWithGeneratingAction(
+ outputRoot, PathFragment.create("consumed"));
+ ActionMetadataHandler handler =
+ new ActionMetadataHandler(
+ new ActionInputMap(1),
+ /*expandedFilesets=*/ ImmutableMap.of(),
+ /*missingArtifactsAllowed=*/ false,
+ ImmutableSet.of(omittedTree, consumedTree),
+ /*tsgm=*/ null,
+ ArtifactPathResolver.IDENTITY,
+ store,
+ outputRoot.getRoot().asPath());
+
+ handler.discardOutputMetadata();
+ handler.markOmitted(omittedTree);
+ handler.markOmitted(omittedTree); // Marking a tree artifact as omitted twice is tolerated.
+
+ assertThat(handler.artifactOmitted(omittedTree)).isTrue();
+ assertThat(handler.artifactOmitted(consumedTree)).isFalse();
+ assertThat(store.getAllTreeArtifactData())
+ .containsExactly(omittedTree, TreeArtifactValue.OMITTED_TREE_MARKER);
+ assertThat(store.getAllArtifactData()).isEmpty();
+ }
+
private ImmutableMap<Artifact, ImmutableList<FilesetOutputSymlink>> createFilesetOutputSymlinkMap(
HasDigest... digests) {
int index = 1;
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index 8017fef..5607782 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -278,7 +278,7 @@
omittedOutputs.add(treeFileArtifact2);
SkyValue value = evaluateArtifactValue(artifact2);
- assertThat(value).isEqualTo(FileArtifactValue.OMITTED_FILE_MARKER);
+ assertThat(value).isEqualTo(TreeArtifactValue.OMITTED_TREE_MARKER);
}
@Test
