commit d83b12427a3553b762ec92d8a81d88d099bc5751
author ajurkowski <ajurkowski@google.com> Mon Aug 31 10:02:05 2020 -0700
committer Copybara-Service <copybara-worker@google.com> Mon Aug 31 10:03:20 2020 -0700
tree f7a646ad9ce31fadc0cb8c28b8078f1e27c33e70
parent 9def382ade829a1b043623808ff346b3e190a0cb

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

src/main/java/com/google/devtools/build/lib/skyframe/TreeArtifactValue.java

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;
     }