Add test coverage for tree artifacts containing directory symlinks.

We have been bitten twice by this use case. See bazelbuild/bazel#9054 and bazelbuild/bazel#11813. Fixes #9058. RELNOTES: None. PiperOrigin-RevId: 322590818
luca-digrazia · Sep 4, 2022 · e83d4f8 · e83d4f8
1 parent cfa6c00
commit e83d4f8
Show file tree

Hide file tree

Showing 2 changed files with 10 additions and 43 deletions.
diff --git a/...uild.bazel/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java b/...uild.bazel/src/main/java/com/google/devtools/build/lib/skyframe/ActionExecutionValue.java
@@ -21,7 +21,6 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.Artifact.ArchivedTreeArtifact;
 import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
 import com.google.devtools.build.lib.actions.Artifact.OwnerlessArtifactWrapper;
 import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
@@ -31,10 +30,8 @@
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
-import com.google.devtools.build.lib.skyframe.TreeArtifactValue.ArchivedRepresentation;
 import com.google.devtools.build.skyframe.SkyValue;
 import java.util.Map;
-import java.util.NoSuchElementException;
 import java.util.function.BiFunction;
 import javax.annotation.Nullable;
 
@@ -142,17 +139,6 @@ public FileArtifactValue getExistingFileArtifactValue(Artifact artifact) {
     if (artifact.isChildOfDeclaredDirectory()) {
       TreeArtifactValue tree = treeArtifactData.get(artifact.getParent());
       result = tree == null ? null : tree.getChildValues().get(artifact);
-    } else if (artifact instanceof ArchivedTreeArtifact) {
-      TreeArtifactValue tree = treeArtifactData.get(artifact.getParent());
-      ArchivedRepresentation archivedRepresentation =
-          tree.getArchivedRepresentation()
-              .orElseThrow(
-                  () -> new NoSuchElementException("Missing archived representation in: " + tree));
-      Preconditions.checkArgument(
-          archivedRepresentation.archivedTreeFileArtifact().equals(artifact),
-          "Multiple archived tree artifacts for: %s",
-          artifact.getParent());
-      result = archivedRepresentation.archivedFileValue();
     } else {
       result = artifactData.get(artifact);
     }
@@ -285,26 +271,16 @@ private static TreeArtifactValue transformSharedTree(
     }
 
     SpecialArtifact newParent = (SpecialArtifact) newArtifact;
-    TreeArtifactValue.Builder newTree = TreeArtifactValue.newBuilder(newParent);
 
+    Map<TreeFileArtifact, FileArtifactValue> newChildren =
+        Maps.newHashMapWithExpectedSize(tree.getChildValues().size());
     for (Map.Entry<TreeFileArtifact, FileArtifactValue> child : tree.getChildValues().entrySet()) {
-      newTree.putChild(
+      newChildren.put(
           TreeFileArtifact.createTreeOutput(newParent, child.getKey().getParentRelativePath()),
           child.getValue());
     }
 
-    tree.getArchivedRepresentation()
-        .ifPresent(
-            archivedRepresentation -> {
-              newTree.setArchivedRepresentation(
-                  new ArchivedTreeArtifact(
-                      newParent,
-                      archivedRepresentation.archivedTreeFileArtifact().getRoot(),
-                      archivedRepresentation.archivedTreeFileArtifact().getExecPath()),
-                  archivedRepresentation.archivedFileValue());
-            });
-
-    return newTree.build();
+    return TreeArtifactValue.create(newChildren);
   }
 
   ActionExecutionValue transformForSharedAction(ImmutableSet<Artifact> outputs) {

diff --git a/...ild.bazel/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java b/...ild.bazel/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactBuildTest.java
@@ -21,6 +21,7 @@
 
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.hash.Hashing;
 import com.google.devtools.build.lib.actions.Action;
@@ -29,7 +30,7 @@
 import com.google.devtools.build.lib.actions.ActionExecutionException;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.ActionLookupData;
-import com.google.devtools.build.lib.actions.ActionLookupKey;
+import com.google.devtools.build.lib.actions.ActionLookupValue.ActionLookupKey;
 import com.google.devtools.build.lib.actions.ActionResult;
 import com.google.devtools.build.lib.actions.Actions;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -61,7 +62,6 @@
 import com.google.devtools.build.lib.testutil.TestUtils;
 import com.google.devtools.build.lib.util.CrashFailureDetails;
 import com.google.devtools.build.lib.util.DetailedExitCode;
-import com.google.devtools.build.lib.vfs.DigestHashFunction;
 import com.google.devtools.build.lib.vfs.FileStatus;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -581,16 +581,12 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
             FileStatus stat = child1.getPath().stat(Symlinks.NOFOLLOW);
             FileArtifactValue metadata1 =
                 md.constructMetadataForDigest(
-                    child1,
-                    stat,
-                    DigestHashFunction.SHA256.getHashFunction().hashString("one", UTF_8).asBytes());
+                    child1, stat, Hashing.sha256().hashString("one", UTF_8).asBytes());
 
             stat = child2.getPath().stat(Symlinks.NOFOLLOW);
             FileArtifactValue metadata2 =
                 md.constructMetadataForDigest(
-                    child2,
-                    stat,
-                    DigestHashFunction.SHA256.getHashFunction().hashString("two", UTF_8).asBytes());
+                    child2, stat, Hashing.sha256().hashString("two", UTF_8).asBytes());
 
             // The metadata will not be equal to reading from the filesystem since the filesystem
             // won't have the digest. However, we should be able to detect that nothing could have
@@ -631,12 +627,7 @@ void run(ActionExecutionContext actionExecutionContext) throws IOException {
 
             actionExecutionContext
                 .getMetadataHandler()
-                .injectTree(
-                    out,
-                    TreeArtifactValue.newBuilder(out)
-                        .putChild(child1, remoteFile1)
-                        .putChild(child2, remoteFile2)
-                        .build());
+                .injectDirectory(out, ImmutableMap.of(child1, remoteFile1, child2, remoteFile2));
           }
         };
 
@@ -1040,7 +1031,7 @@ private static final class DummyActionTemplateExpansionFunction implements SkyFu
     }
 
     @Override
-    public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
+    public SkyValue compute(SkyKey skyKey, Environment env) {
       try {
         return new ActionTemplateExpansionValue(
             Actions.assignOwnersAndFilterSharedActionsAndThrowActionConflict(