Fix concurrent `TreeArtifactValue` builder to always inject empty tree
artifacts with archived representation.
Make sure we always add a `TreeArtifactValue` if all we set for it is the
archived representation. Currently, that is only true for
`TreeArtifactValue.BasicMultiBuilder`, not
`TreeArtifactValue.ConcurrentMultiBuilder`. Fix the concurrent builder to
inject empty tree artifacts when the archived representation is specified.
Change the `TreeArtifactValue.Builder#setArchivedRepresentation` interface to
match `TreeArtifactValue.MultiBuilder`. Add missing test coverage for setting
archived representation when using the builders.
PiperOrigin-RevId: 329323323
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 6ab2c81..0cf4c7b 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
@@ -297,12 +297,11 @@
.ifPresent(
archivedRepresentation -> {
newTree.setArchivedRepresentation(
- ArchivedRepresentation.create(
- new ArchivedTreeArtifact(
- newParent,
- archivedRepresentation.archivedTreeFileArtifact().getRoot(),
- archivedRepresentation.archivedTreeFileArtifact().getExecPath()),
- archivedRepresentation.archivedFileValue()));
+ new ArchivedTreeArtifact(
+ newParent,
+ archivedRepresentation.archivedTreeFileArtifact().getRoot(),
+ archivedRepresentation.archivedTreeFileArtifact().getExecPath()),
+ archivedRepresentation.archivedFileValue());
});
return newTree.build();
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 c6c6aa6..56c9dbb 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
@@ -38,7 +38,6 @@
import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
import com.google.devtools.build.lib.actions.cache.DigestUtils;
import com.google.devtools.build.lib.actions.cache.MetadataHandler;
-import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation;
import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
import com.google.devtools.build.lib.vfs.Dirent;
import com.google.devtools.build.lib.vfs.FileStatus;
@@ -366,9 +365,7 @@
ArchivedTreeArtifact archivedTreeArtifact =
ArchivedTreeArtifact.create(parent, derivedPathPrefix);
tree.setArchivedRepresentation(
- ArchivedRepresentation.create(
- archivedTreeArtifact,
- constructFileArtifactValueFromFilesystem(archivedTreeArtifact)));
+ archivedTreeArtifact, constructFileArtifactValueFromFilesystem(archivedTreeArtifact));
}
return tree.build();
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 747df12..78d2bc9 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
@@ -21,7 +21,6 @@
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
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.ImmutableSortedMap;
@@ -419,16 +418,27 @@
return this;
}
- public Builder setArchivedRepresentation(ArchivedRepresentation archivedRepresentation) {
+ public Builder setArchivedRepresentation(
+ ArchivedTreeArtifact archivedTreeArtifact, FileArtifactValue metadata) {
+ return setArchivedRepresentation(
+ ArchivedRepresentation.create(archivedTreeArtifact, metadata));
+ }
+
+ private Builder setArchivedRepresentation(ArchivedRepresentation archivedRepresentation) {
checkState(
this.archivedRepresentation == null,
"Tried to add 2 archived representations for: %s",
- archivedRepresentation);
+ parent);
checkArgument(
- archivedRepresentation.archivedTreeFileArtifact().getParent().equals(parent),
+ parent.equals(archivedRepresentation.archivedTreeFileArtifact().getParent()),
"Cannot add archived representation: %s for a mismatching tree artifact: %s",
archivedRepresentation,
parent);
+ checkArgument(
+ !archivedRepresentation.archivedFileValue().equals(FileArtifactValue.OMITTED_FILE_MARKER),
+ "Cannot add archived representation: %s to %s because it has omitted metadata.",
+ archivedRepresentation,
+ parent);
this.archivedRepresentation = archivedRepresentation;
return this;
}
@@ -507,10 +517,13 @@
archivedRepresentations.putIfAbsent(
archivedArtifact.getParent(),
ArchivedRepresentation.create(archivedArtifact, metadata));
- Preconditions.checkArgument(
+ checkArgument(
oldValue == null,
"Tried to add 2 archived representations for %s",
archivedArtifact.getParent());
+ // We inject entries based on keys in the children map. Make sure a placeholder exists in case
+ // the tree artifact is otherwise empty.
+ children.computeIfAbsent(archivedArtifact.getParent(), ignored -> new ConcurrentHashMap<>());
return this;
}
