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;
}
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java
index c2117e5..c515cd2 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ActionExecutionValueTransformSharedTreeArtifactsTest.java
@@ -228,8 +228,7 @@
ArchivedTreeArtifact.create(treeArtifact, DERIVED_PATH_PREFIX);
createFile(archivedArtifact.getPath());
builder.setArchivedRepresentation(
- ArchivedRepresentation.create(
- archivedArtifact, FileArtifactValue.createForTesting(archivedArtifact)));
+ archivedArtifact, FileArtifactValue.createForTesting(archivedArtifact));
}
return builder.build();
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java
index 5a21c1e..09ab04f 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactValueTest.java
@@ -14,12 +14,14 @@
package com.google.devtools.build.lib.skyframe;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth8.assertThat;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.fail;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType;
import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
@@ -27,6 +29,7 @@
import com.google.devtools.build.lib.actions.FileArtifactValue;
import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
+import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation;
import com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.vfs.Dirent;
@@ -50,11 +53,13 @@
@RunWith(JUnit4.class)
public final class TreeArtifactValueTest {
+ private static final PathFragment BIN_PATH = PathFragment.create("bin");
+
private final Scratch scratch = new Scratch();
- private final ArtifactRoot root = ArtifactRoot.asDerivedRoot(scratch.resolve("root"), "bin");
+ private final ArtifactRoot root = ArtifactRoot.asDerivedRoot(scratch.resolve("root"), BIN_PATH);
@Test
- public void builderCreatesCorrectValue() {
+ public void createsCorrectValue() {
SpecialArtifact parent = createTreeArtifact("bin/tree");
TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(parent, "child1");
TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(parent, "child2");
@@ -76,6 +81,34 @@
}
@Test
+ public void createsCorrectValueWithArchivedRepresentation() {
+ SpecialArtifact parent = createTreeArtifact("bin/tree");
+ TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(parent, "child1");
+ TreeFileArtifact child2 = TreeFileArtifact.createTreeOutput(parent, "child2");
+ ArchivedTreeArtifact archivedTreeArtifact = createArchivedTreeArtifact(parent);
+ FileArtifactValue child1Metadata = metadataWithId(1);
+ FileArtifactValue child2Metadata = metadataWithId(2);
+ FileArtifactValue archivedArtifactMetadata = metadataWithId(3);
+
+ TreeArtifactValue tree =
+ TreeArtifactValue.newBuilder(parent)
+ .putChild(child1, child1Metadata)
+ .putChild(child2, child2Metadata)
+ .setArchivedRepresentation(archivedTreeArtifact, archivedArtifactMetadata)
+ .build();
+
+ assertThat(tree.getChildren()).containsExactly(child1, child2);
+ assertThat(tree.getChildValues())
+ .containsExactly(child1, child1Metadata, child2, child2Metadata);
+ assertThat(tree.getChildPaths())
+ .containsExactly(child1.getParentRelativePath(), child2.getParentRelativePath());
+ assertThat(tree.getDigest()).isNotNull();
+ assertThat(tree.getMetadata().getDigest()).isEqualTo(tree.getDigest());
+ assertThat(tree.getArchivedRepresentation())
+ .hasValue(ArchivedRepresentation.create(archivedTreeArtifact, archivedArtifactMetadata));
+ }
+
+ @Test
public void empty() {
TreeArtifactValue emptyTree = TreeArtifactValue.empty();
@@ -87,7 +120,7 @@
}
@Test
- public void canonicalEmptyInstance() {
+ public void createsCanonicalEmptyInstance() {
SpecialArtifact parent = createTreeArtifact("bin/tree");
TreeArtifactValue emptyTreeFromBuilder = TreeArtifactValue.newBuilder(parent).build();
@@ -96,6 +129,26 @@
}
@Test
+ public void createsCorrectEmptyValueWithArchivedRepresentation() {
+ SpecialArtifact parent = createTreeArtifact("bin/tree");
+ ArchivedTreeArtifact archivedTreeArtifact = createArchivedTreeArtifact(parent);
+ FileArtifactValue archivedArtifactMetadata = metadataWithId(1);
+
+ TreeArtifactValue tree =
+ TreeArtifactValue.newBuilder(parent)
+ .setArchivedRepresentation(archivedTreeArtifact, archivedArtifactMetadata)
+ .build();
+
+ assertThat(tree.getChildren()).isEmpty();
+ assertThat(tree.getChildValues()).isEmpty();
+ assertThat(tree.getChildPaths()).isEmpty();
+ assertThat(tree.getDigest()).isNotNull();
+ assertThat(tree.getMetadata().getDigest()).isEqualTo(tree.getDigest());
+ assertThat(tree.getArchivedRepresentation())
+ .hasValue(ArchivedRepresentation.create(archivedTreeArtifact, archivedArtifactMetadata));
+ }
+
+ @Test
public void cannotCreateBuilderForNonTreeArtifact() {
SpecialArtifact notTreeArtifact =
new SpecialArtifact(
@@ -122,6 +175,19 @@
}
@Test
+ public void cannotAddArchivedRepresentationWithWrongParent() {
+ SpecialArtifact parent = createTreeArtifact("bin/tree");
+ ArchivedTreeArtifact archivedDifferentTreeArtifact =
+ createArchivedTreeArtifact(createTreeArtifact("bin/other_tree"));
+ TreeArtifactValue.Builder builderForParent = TreeArtifactValue.newBuilder(parent);
+ FileArtifactValue metadata = metadataWithId(1);
+
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> builderForParent.setArchivedRepresentation(archivedDifferentTreeArtifact, metadata));
+ }
+
+ @Test
public void cannotAddOmittedChildToBuilder() {
SpecialArtifact parent = createTreeArtifact("bin/tree");
TreeFileArtifact child = TreeFileArtifact.createTreeOutput(parent, "child");
@@ -134,6 +200,19 @@
}
@Test
+ public void cannotAddOmittedArchivedRepresentation() {
+ SpecialArtifact parent = createTreeArtifact("bin/tree");
+ ArchivedTreeArtifact archivedTreeArtifact = createArchivedTreeArtifact(parent);
+ TreeArtifactValue.Builder builder = TreeArtifactValue.newBuilder(parent);
+
+ assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ builder.setArchivedRepresentation(
+ archivedTreeArtifact, FileArtifactValue.OMITTED_FILE_MARKER));
+ }
+
+ @Test
public void orderIndependence() {
SpecialArtifact parent = createTreeArtifact("bin/tree");
TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(parent, "child1");
@@ -173,6 +252,27 @@
}
@Test
+ public void nullDigestsForArchivedRepresentation_equal() {
+ SpecialArtifact parent = createTreeArtifact("bin/tree");
+ ArchivedTreeArtifact archivedTreeArtifact = createArchivedTreeArtifact(parent);
+ FileArtifactValue metadataNoDigest = metadataWithIdNoDigest(1);
+
+ TreeArtifactValue tree1 =
+ TreeArtifactValue.newBuilder(parent)
+ .setArchivedRepresentation(archivedTreeArtifact, metadataNoDigest)
+ .build();
+ TreeArtifactValue tree2 =
+ TreeArtifactValue.newBuilder(parent)
+ .setArchivedRepresentation(archivedTreeArtifact, metadataNoDigest)
+ .build();
+
+ assertThat(metadataNoDigest.getDigest()).isNull();
+ assertThat(tree1.getDigest()).isNotNull();
+ assertThat(tree2.getDigest()).isNotNull();
+ assertThat(tree1).isEqualTo(tree2);
+ }
+
+ @Test
public void nullDigests_notEqual() {
SpecialArtifact parent = createTreeArtifact("bin/tree");
TreeFileArtifact child = TreeFileArtifact.createTreeOutput(parent, "child");
@@ -192,6 +292,29 @@
}
@Test
+ public void nullDigestsForArchivedRepresentation_notEqual() {
+ SpecialArtifact parent = createTreeArtifact("bin/tree");
+ ArchivedTreeArtifact archivedTreeArtifact = createArchivedTreeArtifact(parent);
+ FileArtifactValue metadataNoDigest1 = metadataWithIdNoDigest(1);
+ FileArtifactValue metadataNoDigest2 = metadataWithIdNoDigest(2);
+
+ TreeArtifactValue tree1 =
+ TreeArtifactValue.newBuilder(parent)
+ .setArchivedRepresentation(archivedTreeArtifact, metadataNoDigest1)
+ .build();
+ TreeArtifactValue tree2 =
+ TreeArtifactValue.newBuilder(parent)
+ .setArchivedRepresentation(archivedTreeArtifact, metadataNoDigest2)
+ .build();
+
+ assertThat(metadataNoDigest1.getDigest()).isNull();
+ assertThat(metadataNoDigest2.getDigest()).isNull();
+ assertThat(tree1.getDigest()).isNotNull();
+ assertThat(tree2.getDigest()).isNotNull();
+ assertThat(tree1.getDigest()).isNotEqualTo(tree2.getDigest());
+ }
+
+ @Test
public void visitTree_visitsEachChild() throws Exception {
Path treeDir = scratch.dir("tree");
scratch.file("tree/file1");
@@ -324,7 +447,7 @@
public static final class MultiBuilderTest {
private static final ArtifactRoot ROOT =
- ArtifactRoot.asDerivedRoot(new InMemoryFileSystem().getPath("/root"), "bin");
+ ArtifactRoot.asDerivedRoot(new InMemoryFileSystem().getPath("/root"), BIN_PATH);
private enum MultiBuilderType {
BASIC {
@@ -352,7 +475,7 @@
}
@Test
- public void empty() {
+ public void emptyBuilder_injectsNothing() {
TreeArtifactValue.MultiBuilder treeArtifacts = multiBuilderType.newMultiBuilder();
treeArtifacts.injectTo(results::put);
@@ -361,7 +484,7 @@
}
@Test
- public void singleTreeArtifact() {
+ public void injectsSingleTreeArtifact() {
TreeArtifactValue.MultiBuilder treeArtifacts = multiBuilderType.newMultiBuilder();
SpecialArtifact parent = createTreeArtifact("bin/tree");
TreeFileArtifact child1 = TreeFileArtifact.createTreeOutput(parent, "child1");
@@ -382,7 +505,7 @@
}
@Test
- public void multipleTreeArtifacts() {
+ public void injectsMultipleTreeArtifacts() {
TreeArtifactValue.MultiBuilder treeArtifacts = multiBuilderType.newMultiBuilder();
SpecialArtifact parent1 = createTreeArtifact("bin/tree1");
TreeFileArtifact parent1Child1 = TreeFileArtifact.createTreeOutput(parent1, "child1");
@@ -409,11 +532,86 @@
.build());
}
+ @Test
+ public void injectsTreeArtifactWithArchivedRepresentation() {
+ TreeArtifactValue.MultiBuilder builder = multiBuilderType.newMultiBuilder();
+ SpecialArtifact parent = createTreeArtifact("bin/tree");
+ TreeFileArtifact child = TreeFileArtifact.createTreeOutput(parent, "child");
+ FileArtifactValue childMetadata = metadataWithId(1);
+ ArchivedTreeArtifact archivedTreeArtifact = createArchivedTreeArtifact(parent);
+ FileArtifactValue archivedTreeArtifactMetadata = metadataWithId(2);
+
+ builder
+ .putChild(child, childMetadata)
+ .setArchivedRepresentation(archivedTreeArtifact, archivedTreeArtifactMetadata)
+ .injectTo(results::put);
+
+ assertThat(results)
+ .containsExactly(
+ parent,
+ TreeArtifactValue.newBuilder(parent)
+ .putChild(child, childMetadata)
+ .setArchivedRepresentation(archivedTreeArtifact, archivedTreeArtifactMetadata)
+ .build());
+ }
+
+ @Test
+ public void injectsEmptyTreeArtifactWithArchivedRepresentation() {
+ TreeArtifactValue.MultiBuilder builder = multiBuilderType.newMultiBuilder();
+ SpecialArtifact parent = createTreeArtifact("bin/tree");
+ ArchivedTreeArtifact archivedTreeArtifact = createArchivedTreeArtifact(parent);
+ FileArtifactValue metadata = metadataWithId(1);
+
+ builder.setArchivedRepresentation(archivedTreeArtifact, metadata).injectTo(results::put);
+
+ assertThat(results)
+ .containsExactly(
+ parent,
+ TreeArtifactValue.newBuilder(parent)
+ .setArchivedRepresentation(archivedTreeArtifact, metadata)
+ .build());
+ }
+
+ @Test
+ public void injectsTreeArtifactsWithAndWithoutArchivedRepresentation() {
+ TreeArtifactValue.MultiBuilder builder = multiBuilderType.newMultiBuilder();
+ SpecialArtifact parent1 = createTreeArtifact("bin/tree1");
+ ArchivedTreeArtifact archivedArtifact1 = createArchivedTreeArtifact(parent1);
+ FileArtifactValue archivedArtifact1Metadata = metadataWithId(1);
+ TreeFileArtifact parent1Child = TreeFileArtifact.createTreeOutput(parent1, "child");
+ FileArtifactValue parent1ChildMetadata = metadataWithId(2);
+ SpecialArtifact parent2 = createTreeArtifact("bin/tree2");
+ TreeFileArtifact parent2Child = TreeFileArtifact.createTreeOutput(parent2, "child");
+ FileArtifactValue parent2ChildMetadata = metadataWithId(3);
+
+ builder
+ .setArchivedRepresentation(archivedArtifact1, archivedArtifact1Metadata)
+ .putChild(parent1Child, parent1ChildMetadata)
+ .putChild(parent2Child, parent2ChildMetadata)
+ .injectTo(results::put);
+
+ assertThat(results)
+ .containsExactly(
+ parent1,
+ TreeArtifactValue.newBuilder(parent1)
+ .putChild(parent1Child, parent1ChildMetadata)
+ .setArchivedRepresentation(archivedArtifact1, metadataWithId(1))
+ .build(),
+ parent2,
+ TreeArtifactValue.newBuilder(parent2)
+ .putChild(parent2Child, parent2ChildMetadata)
+ .build());
+ }
+
private static SpecialArtifact createTreeArtifact(String execPath) {
return TreeArtifactValueTest.createTreeArtifact(execPath, ROOT);
}
}
+ private static ArchivedTreeArtifact createArchivedTreeArtifact(SpecialArtifact specialArtifact) {
+ return ArchivedTreeArtifact.create(specialArtifact, BIN_PATH);
+ }
+
private SpecialArtifact createTreeArtifact(String execPath) {
return createTreeArtifact(execPath, root);
}
