[7.0.0] "Cannot get node id for DirectoryArtifactValue" on tree artif…

…act containing symlink to directory + remote cache + BES (bazelbuild#20424)

When a tree artifact contains a symlink to a directory, the
TreeArtifactValue contains a TreeFileArtifact mapping to a
DirectoryArtifactValue (see bazelbuild#20418).

Because the file type to be uploaded to the BES is determined solely by
the Artifact type, this causes the uploader to attempt to upload a
directory as if it were a file. This fails silently when building with
the bytes; when building without the bytes, it crashes when attempting
to digest the (in-memory) directory, which has no associated inode (see
bazelbuild#20415).

This PR fixes the file type computation to take into account both the
artifact and its metadata, preferring the latter when available.

Fixes bazelbuild#20415.

Closes bazelbuild#20419.

Commit
bazelbuild@bb5ff63

PiperOrigin-RevId: 587654070
Change-Id: Ib62bbaaed62b00c12bcabdc2bc9bee57aee0bcef

Co-authored-by: Tiago Quelhas <tjgq@google.com>

src/main/java/com/google/devtools/build/lib/actions/ActionExecutedEvent.java

@@ -173,7 +173,7 @@ public Collection<LocalFile> referencedLocalFiles() {
       localFiles.add(
           new LocalFile(
               primaryOutput,
-              LocalFileType.forArtifact(outputArtifact),
+              LocalFileType.forArtifact(outputArtifact, primaryOutputMetadata),
               outputArtifact,
               primaryOutputMetadata));
     }

src/main/java/com/google/devtools/build/lib/analysis/TargetCompleteEvent.java

@@ -406,12 +406,13 @@ public ImmutableList<LocalFile> referencedLocalFiles() {
             new ArtifactReceiver() {
               @Override
               public void accept(Artifact artifact) {
+                FileArtifactValue metadata = completionContext.getFileArtifactValue(artifact);
                 builder.add(
                     new LocalFile(
                         completionContext.pathResolver().toPath(artifact),
-                        LocalFileType.forArtifact(artifact),
+                        LocalFileType.forArtifact(artifact, metadata),
                         artifact,
-                        completionContext.getFileArtifactValue(artifact)));
+                        metadata));
               }
 
               @Override

src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEvent.java

@@ -66,7 +66,22 @@ public boolean isOutput() {
             || this == OUTPUT_SYMLINK;
       }
 
-      public static LocalFileType forArtifact(Artifact artifact) {
+      /**
+       * Returns the {@link LocalFileType} implied by a {@link FileArtifactValue}, or the associated
+       * {@link Artifact} if metadata is not available.
+       */
+      public static LocalFileType forArtifact(
+          Artifact artifact, @Nullable FileArtifactValue metadata) {
+        if (metadata != null) {
+          switch (metadata.getType()) {
+            case DIRECTORY:
+              return LocalFileType.OUTPUT_DIRECTORY;
+            case SYMLINK:
+              return LocalFileType.OUTPUT_SYMLINK;
+            default:
+              return LocalFileType.OUTPUT_FILE;
+          }
+        }
         if (artifact.isDirectory()) {
           return LocalFileType.OUTPUT_DIRECTORY;
         } else if (artifact.isSymlink()) {

src/main/java/com/google/devtools/build/lib/runtime/NamedArtifactGroup.java

@@ -75,14 +75,14 @@ public Collection<LocalFile> referencedLocalFiles() {
     for (Object elem : set.getLeaves()) {
       ExpandedArtifact expandedArtifact = (ExpandedArtifact) elem;
       if (expandedArtifact.relPath == null) {
-        FileArtifactValue fileMetadata =
+        FileArtifactValue metadata =
             completionContext.getFileArtifactValue(expandedArtifact.artifact);
         artifacts.add(
             new LocalFile(
                 completionContext.pathResolver().toPath(expandedArtifact.artifact),
-                getOutputType(fileMetadata),
-                fileMetadata == null ? null : expandedArtifact.artifact,
-                fileMetadata));
+                LocalFileType.forArtifact(expandedArtifact.artifact, metadata),
+                metadata == null ? null : expandedArtifact.artifact,
+                metadata));
       } else {
         // TODO(b/199940216): Can fileset metadata be properly handled here?
         artifacts.add(
@@ -96,20 +96,6 @@ public Collection<LocalFile> referencedLocalFiles() {
     return artifacts.build();
   }
 
-  private static LocalFileType getOutputType(@Nullable FileArtifactValue fileMetadata) {
-    if (fileMetadata == null) {
-      return LocalFileType.OUTPUT;
-    }
-    switch (fileMetadata.getType()) {
-      case DIRECTORY:
-        return LocalFileType.OUTPUT_DIRECTORY;
-      case SYMLINK:
-        return LocalFileType.OUTPUT_SYMLINK;
-      default:
-        return LocalFileType.OUTPUT_FILE;
-    }
-  }
-
   @Override
   public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventContext converters) {
     PathConverter pathConverter = converters.pathConverter();

src/test/java/com/google/devtools/build/lib/analysis/TargetCompleteEventTest.java

@@ -73,10 +73,9 @@ public void testReferencedSourceFile() throws Exception {
             artifactsToBuild.getAllArtifactsByOutputGroup(),
             /* announceTargetSummary= */ false);
 
-    assertThat(event.referencedLocalFiles()).hasSize(1);
-    LocalFile localFile = event.referencedLocalFiles().get(0);
-    assertThat(localFile.path).isEqualTo(artifact.getPath());
-    assertThat(localFile.type).isEqualTo(LocalFileType.OUTPUT_FILE);
+    assertThat(event.referencedLocalFiles())
+        .containsExactly(
+            new LocalFile(artifact.getPath(), LocalFileType.OUTPUT_FILE, artifact, metadata));
   }
 
   @Test
@@ -97,11 +96,9 @@ public void testReferencedSourceDirectory() throws Exception {
             artifactsToBuild.getAllArtifactsByOutputGroup(),
             /* announceTargetSummary= */ false);
 
-    assertThat(event.referencedLocalFiles()).hasSize(1);
-    LocalFile localFile = event.referencedLocalFiles().get(0);
-    assertThat(localFile.path).isEqualTo(artifact.getPath());
-    // TODO(tjgq): This should be reported as a directory.
-    assertThat(localFile.type).isEqualTo(LocalFileType.OUTPUT_FILE);
+    assertThat(event.referencedLocalFiles())
+        .containsExactly(
+            new LocalFile(artifact.getPath(), LocalFileType.OUTPUT_DIRECTORY, artifact, metadata));
   }
 
   @Test
@@ -110,10 +107,7 @@ public void testReferencedTreeArtifact() throws Exception {
         "defs.bzl",
         "def _impl(ctx):",
         "  d = ctx.actions.declare_directory(ctx.label.name)",
-        "  ctx.actions.run_shell(",
-        "    outputs = [d],",
-        "    command = 'touch %s/file.txt' % d.path,",
-        "  )",
+        "  ctx.actions.run_shell(outputs = [d], command = 'does not matter')",
         "  return DefaultInfo(files = depset([d]))",
         "dir = rule(_impl)");
     scratch.file(
@@ -123,16 +117,23 @@ public void testReferencedTreeArtifact() throws Exception {
         "filegroup(name = 'files', srcs = ['dir'])");
     ConfiguredTargetAndData ctAndData = getCtAndData("//:files");
     ArtifactsToBuild artifactsToBuild = getArtifactsToBuild(ctAndData);
-    SpecialArtifact artifact =
+    SpecialArtifact tree =
         (SpecialArtifact) Iterables.getOnlyElement(artifactsToBuild.getAllArtifacts().toList());
+    TreeFileArtifact fileChild =
+        TreeFileArtifact.createTreeOutput(tree, PathFragment.create("dir/file.txt"));
+    FileArtifactValue fileMetadata =
+        FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, null, 10);
+    // A TreeFileArtifact can be a directory, when materialized by a symlink.
+    // See https://github.com/bazelbuild/bazel/issues/20418.
+    TreeFileArtifact dirChild = TreeFileArtifact.createTreeOutput(tree, PathFragment.create("sym"));
+    FileArtifactValue dirMetadata = FileArtifactValue.createForDirectoryWithMtime(123456789);
     TreeArtifactValue metadata =
-        TreeArtifactValue.newBuilder(artifact)
-            .putChild(
-                TreeFileArtifact.createTreeOutput(artifact, PathFragment.create("file")),
-                FileArtifactValue.createForNormalFile(new byte[] {1, 2, 3}, null, 10))
+        TreeArtifactValue.newBuilder(tree)
+            .putChild(fileChild, fileMetadata)
+            .putChild(dirChild, dirMetadata)
             .build();
     CompletionContext completionContext =
-        getCompletionContext(ImmutableMap.of(), ImmutableMap.of(artifact, metadata));
+        getCompletionContext(ImmutableMap.of(), ImmutableMap.of(tree, metadata));
 
     TargetCompleteEvent event =
         TargetCompleteEvent.successfulBuild(
@@ -141,11 +142,11 @@ public void testReferencedTreeArtifact() throws Exception {
             artifactsToBuild.getAllArtifactsByOutputGroup(),
             /* announceTargetSummary= */ false);
 
-    assertThat(event.referencedLocalFiles()).hasSize(1);
-    LocalFile localFile = event.referencedLocalFiles().get(0);
-    assertThat(localFile.path)
-        .isEqualTo(Iterables.getOnlyElement(metadata.getChildren()).getPath());
-    assertThat(localFile.type).isEqualTo(LocalFileType.OUTPUT_FILE);
+    assertThat(event.referencedLocalFiles())
+        .containsExactly(
+            new LocalFile(fileChild.getPath(), LocalFileType.OUTPUT_FILE, fileChild, fileMetadata),
+            new LocalFile(
+                dirChild.getPath(), LocalFileType.OUTPUT_DIRECTORY, dirChild, dirMetadata));
   }
 
   @Test
@@ -154,10 +155,7 @@ public void testReferencedUnresolvedSymlink() throws Exception {
         "defs.bzl",
         "def _impl(ctx):",
         "  s = ctx.actions.declare_symlink(ctx.label.name)",
-        "  ctx.actions.symlink(",
-        "    output = s,",
-        "    target_path = '/some/path',",
-        "  )",
+        "  ctx.actions.symlink(output = s, target_path = 'does not matter')",
         "  return DefaultInfo(files = depset([s]))",
         "sym = rule(_impl)");
     scratch.file(
@@ -181,10 +179,9 @@ public void testReferencedUnresolvedSymlink() throws Exception {
             artifactsToBuild.getAllArtifactsByOutputGroup(),
             /* announceTargetSummary= */ false);
 
-    assertThat(event.referencedLocalFiles()).hasSize(1);
-    LocalFile localFile = event.referencedLocalFiles().get(0);
-    assertThat(localFile.path).isEqualTo(artifact.getPath());
-    assertThat(localFile.type).isEqualTo(LocalFileType.OUTPUT_SYMLINK);
+    assertThat(event.referencedLocalFiles())
+        .containsExactly(
+            new LocalFile(artifact.getPath(), LocalFileType.OUTPUT_SYMLINK, artifact, metadata));
   }
 
   /** Regression test for b/165671166. */