Automated rollback of commit a98fc73915bc83a1807305b596cca2cb6f6500f2.
*** Reason for rollback ***
b/157485360
*** Original change description ***
Allow tree artifacts to be omitted.
PiperOrigin-RevId: 313246058
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 8e47a39..7116a0a 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
@@ -70,12 +70,8 @@
}
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 :
- treeArtifact.getChildValues().entrySet()) {
+ tree.getValue().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 03fed1a..9c7a570 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,10 +157,6 @@
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 475f63e..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
@@ -512,19 +512,9 @@
@Override
public void markOmitted(Artifact output) {
- 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(output, TreeArtifactValue.OMITTED_TREE_MARKER);
- }
- } else {
- Preconditions.checkState(newlyOmitted, "%s marked as omitted twice", output);
- store.putArtifactData(output, FileArtifactValue.OMITTED_FILE_MARKER);
- }
+ Preconditions.checkState(executionMode.get());
+ Preconditions.checkState(omittedOutputs.add(output), "%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 d9f50b4..e2c4621 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 TreeArtifactValue createTreeArtifactValueFromActionKey(
+ private static SkyValue 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 TreeArtifactValue.OMITTED_TREE_MARKER;
+ return FileArtifactValue.OMITTED_FILE_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 098c34b..649cf3f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -2631,7 +2631,6 @@
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 01920ca..7ac6c27 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,7 +26,6 @@
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;
@@ -44,12 +43,14 @@
* 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 {
- @SerializationConstant @AutoCodec.VisibleForSerialization
- static final TreeArtifactValue EMPTY =
+ private 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;
@@ -160,67 +161,57 @@
}
/**
- * 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 = createMarker("MISSING_TREE_ARTIFACT");
+ static final TreeArtifactValue MISSING_TREE_ARTIFACT =
+ new TreeArtifactValue(null, ImmutableSortedMap.of(), /* remote= */ false) {
+ @Override
+ FileArtifactValue getSelfData() {
+ 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
+ public ImmutableSet<TreeFileArtifact> getChildren() {
+ throw new UnsupportedOperationException();
+ }
- @Override
- public ImmutableSet<TreeFileArtifact> getChildren() {
- throw new UnsupportedOperationException(toString());
- }
+ @Override
+ ImmutableMap<TreeFileArtifact, FileArtifactValue> getChildValues() {
+ throw new UnsupportedOperationException();
+ }
- @Override
- ImmutableMap<TreeFileArtifact, FileArtifactValue> getChildValues() {
- throw new UnsupportedOperationException(toString());
- }
+ @Override
+ FileArtifactValue getMetadata() {
+ throw new UnsupportedOperationException();
+ }
- @Override
- FileArtifactValue getMetadata() {
- throw new UnsupportedOperationException(toString());
- }
+ @Override
+ ImmutableSet<PathFragment> getChildPaths() {
+ throw new UnsupportedOperationException();
+ }
- @Override
- ImmutableSet<PathFragment> getChildPaths() {
- throw new UnsupportedOperationException(toString());
- }
+ @Nullable
+ @Override
+ public byte[] getDigest() {
+ throw new UnsupportedOperationException();
+ }
- @Nullable
- @Override
- public byte[] getDigest() {
- throw new UnsupportedOperationException(toString());
- }
+ @Override
+ public int hashCode() {
+ return 24; // my favorite number
+ }
- @Override
- public int hashCode() {
- return System.identityHashCode(this);
- }
+ @Override
+ public boolean equals(Object other) {
+ return this == other;
+ }
- @Override
- public boolean equals(Object other) {
- return this == other;
- }
-
- @Override
- public String toString() {
- return toStringRepresentation;
- }
- };
- }
+ @Override
+ public String toString() {
+ return "MISSING_TREE_ARTIFACT";
+ }
+ };
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 4d9a7bc..db232b7 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,7 +18,6 @@
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;
@@ -440,67 +439,6 @@
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 5607782..8017fef 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(TreeArtifactValue.OMITTED_TREE_MARKER);
+ assertThat(value).isEqualTo(FileArtifactValue.OMITTED_FILE_MARKER);
}
@Test
