Do not inject tree artifact values using `MetadataConsumer`.
Action file system expects `TreeArtifactValue`s to be injected to the output
store directly. Restrict injection to files only.
PiperOrigin-RevId: 332343506
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 81a05fe..093b840 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
@@ -43,10 +43,7 @@
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
import javax.annotation.Nullable;
-import javax.annotation.concurrent.ThreadSafe;
/**
* Value for TreeArtifacts, which contains a digest and the {@link FileArtifactValue}s of its child
@@ -70,14 +67,22 @@
}
/** Builder for constructing multiple instances of {@link TreeArtifactValue} at once. */
- public interface MultiBuilder {
+ public static final class MultiBuilder {
+
+ private final Map<SpecialArtifact, Builder> map = new HashMap<>();
+
+ private MultiBuilder() {}
+
/**
* Puts a child tree file into this builder under its {@linkplain TreeFileArtifact#getParent
* parent}.
*
* @return {@code this} for convenience
*/
- MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata);
+ public MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata) {
+ map.computeIfAbsent(child.getParent(), Builder::new).putChild(child, metadata);
+ return this;
+ }
/**
* Sets the archived representation and its metadata for the {@linkplain
@@ -86,24 +91,25 @@
* <p>Setting an archived representation is only allowed once per {@linkplain SpecialArtifact
* tree artifact}.
*/
- MultiBuilder setArchivedRepresentation(
- ArchivedTreeArtifact archivedArtifact, FileArtifactValue metadata);
+ public MultiBuilder setArchivedRepresentation(
+ ArchivedTreeArtifact archivedArtifact, FileArtifactValue metadata) {
+ map.computeIfAbsent(archivedArtifact.getParent(), Builder::new)
+ .setArchivedRepresentation(ArchivedRepresentation.create(archivedArtifact, metadata));
+ return this;
+ }
/**
* For each unique parent seen by this builder, passes the aggregated metadata to {@link
* TreeArtifactInjector#injectTree}.
*/
- void injectTo(TreeArtifactInjector treeInjector);
+ public void injectTo(TreeArtifactInjector treeInjector) {
+ map.forEach((parent, builder) -> treeInjector.injectTree(parent, builder.build()));
+ }
}
/** Returns a new {@link MultiBuilder}. */
public static MultiBuilder newMultiBuilder() {
- return new BasicMultiBuilder();
- }
-
- /** Returns a new thread-safe {@link MultiBuilder}. */
- public static MultiBuilder newConcurrentMultiBuilder() {
- return new ConcurrentMultiBuilder();
+ return new MultiBuilder();
}
/**
@@ -138,6 +144,7 @@
private final byte[] digest;
private final ImmutableSortedMap<TreeFileArtifact, FileArtifactValue> childData;
+
/**
* Optional archived representation of the entire tree artifact which can be sent instead of all
* the items in the directory.
@@ -471,74 +478,4 @@
fingerprint.digestAndReset(), finalChildData, archivedRepresentation, entirelyRemote);
}
}
-
- private static final class BasicMultiBuilder implements MultiBuilder {
- private final Map<SpecialArtifact, Builder> map = new HashMap<>();
-
- @Override
- public MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata) {
- map.computeIfAbsent(child.getParent(), Builder::new).putChild(child, metadata);
- return this;
- }
-
- @Override
- public MultiBuilder setArchivedRepresentation(
- ArchivedTreeArtifact archivedArtifact, FileArtifactValue metadata) {
- map.computeIfAbsent(archivedArtifact.getParent(), Builder::new)
- .setArchivedRepresentation(ArchivedRepresentation.create(archivedArtifact, metadata));
- return this;
- }
-
- @Override
- public void injectTo(TreeArtifactInjector treeInjector) {
- map.forEach((parent, builder) -> treeInjector.injectTree(parent, builder.build()));
- }
- }
-
- @ThreadSafe
- private static final class ConcurrentMultiBuilder implements MultiBuilder {
- private final ConcurrentMap<SpecialArtifact, ConcurrentMap<TreeFileArtifact, FileArtifactValue>>
- children = new ConcurrentHashMap<>();
- private final ConcurrentMap<SpecialArtifact, ArchivedRepresentation> archivedRepresentations =
- new ConcurrentHashMap<>();
-
- @Override
- public MultiBuilder putChild(TreeFileArtifact child, FileArtifactValue metadata) {
- children
- .computeIfAbsent(child.getParent(), parent -> new ConcurrentHashMap<>())
- .put(child, metadata);
- return this;
- }
-
- @Override
- public MultiBuilder setArchivedRepresentation(
- ArchivedTreeArtifact archivedArtifact, FileArtifactValue metadata) {
- Object oldValue =
- archivedRepresentations.putIfAbsent(
- archivedArtifact.getParent(),
- ArchivedRepresentation.create(archivedArtifact, metadata));
- 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;
- }
-
- @Override
- public void injectTo(TreeArtifactInjector treeInjector) {
- children.forEach(
- (parent, children) -> {
- Builder builder = new Builder(parent);
- children.forEach(builder::putChild);
- ArchivedRepresentation archivedRepresentation = archivedRepresentations.get(parent);
- if (archivedRepresentation != null) {
- builder.setArchivedRepresentation(archivedRepresentation);
- }
- treeInjector.injectTree(parent, builder.build());
- });
- }
- }
}
