Only inject symlink metadata

src/main/java/com/google/devtools/build/lib/actions/BUILD

@@ -54,6 +54,7 @@ java_library(
             "LocalHostResourceManagerDarwin.java",
             "LocalHostResourceFallback.java",
             "MiddlemanType.java",
+            "RemoteFileStatus.java",
             "ResourceManager.java",
             "ResourceSet.java",
             "ResourceSetOrBuilder.java",
@@ -263,6 +264,7 @@ java_library(
         "FileStateType.java",
         "FileStateValue.java",
         "FileValue.java",
+        "RemoteFileStatus.java",
         "cache/MetadataDigestUtils.java",
     ],
     deps = [

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

@@ -0,0 +1,28 @@
+// Copyright 2022 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.actions;
+
+import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
+import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
+
+/**
+ * A FileStatus that exists remotely and provides remote metadata.
+ *
+ * <p>Filesystem may return implementation of this interface if the files are stored remotely. When
+ * checking outputs of actions, Skyframe will inject the metadata returned by {@link
+ * #getRemoteMetadata()} if the output file has {@link RemoteFileStatus}.
+ */
+public interface RemoteFileStatus extends FileStatusWithDigest {
+  RemoteFileArtifactValue getRemoteMetadata();
+}

src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java

@@ -40,6 +40,7 @@
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.actions.RemoteFileStatus;
 import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.build.lib.vfs.inmemoryfs.FileInfo;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryContentInfo;
@@ -128,41 +129,7 @@ void flush() throws IOException {
       PathFragment path = execRoot.getRelative(entry.getKey());
       Artifact output = entry.getValue();
 
-      if (maybeInjectMetadataForSymlink(path, output)) {
-        continue;
-      }
-
-      if (output.isTreeArtifact()) {
-        if (remoteOutputTree.exists(path)) {
-          SpecialArtifact parent = (SpecialArtifact) output;
-          TreeArtifactValue.Builder tree = TreeArtifactValue.newBuilder(parent);
-
-          // TODO: Check directory content on the local fs to support mixed tree.
-          TreeArtifactValue.visitTree(
-              remoteOutputTree.getPath(path),
-              (parentRelativePath, type) -> {
-                if (type == Dirent.Type.DIRECTORY) {
-                  return;
-                }
-                RemoteFileInfo remoteFile =
-                    remoteOutputTree.getRemoteFileInfo(
-                        path.getRelative(parentRelativePath), /* followSymlinks= */ true);
-                if (remoteFile != null) {
-                  TreeFileArtifact child =
-                      TreeFileArtifact.createTreeOutput(parent, parentRelativePath);
-                  tree.putChild(child, createRemoteMetadata(remoteFile));
-                }
-              });
-
-          metadataInjector.injectTree(parent, tree.build());
-        }
-      } else {
-        RemoteFileInfo remoteFile =
-            remoteOutputTree.getRemoteFileInfo(path, /* followSymlinks= */ true);
-        if (remoteFile != null) {
-          metadataInjector.injectFile(output, createRemoteMetadata(remoteFile));
-        }
-      }
+      maybeInjectMetadataForSymlink(path, output);
     }
   }
 
@@ -183,26 +150,24 @@ void flush() throws IOException {
    *       the output should be materialized as a symlink to the original location, which avoids
    *       fetching multiple copies when multiple symlinks to the same artifact are created in the
    *       same build.
-   *
-   * @return Whether the metadata was injected.
    */
-  private boolean maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact output)
+  private void maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact output)
       throws IOException {
     if (output.isSymlink()) {
-      return false;
+      return;
     }
 
     Path outputTreePath = remoteOutputTree.getPath(linkPath);
 
     if (!outputTreePath.exists(Symlinks.NOFOLLOW)) {
-      return false;
+      return;
     }
 
     PathFragment targetPath;
     try {
       targetPath = outputTreePath.readSymbolicLink();
     } catch (NotASymlinkException e) {
-      return false;
+      return;
     }
 
     checkState(
@@ -212,7 +177,7 @@ private boolean maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact ou
     if (output.isTreeArtifact()) {
       TreeArtifactValue metadata = getRemoteTreeMetadata(targetPath);
       if (metadata == null) {
-        return false;
+        return;
       }
 
       SpecialArtifact parent = (SpecialArtifact) output;
@@ -233,7 +198,7 @@ private boolean maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact ou
     } else {
       RemoteFileArtifactValue metadata = getRemoteMetadata(targetPath);
       if (metadata == null) {
-        return false;
+        return;
       }
 
       RemoteFileArtifactValue injectedMetadata =
@@ -247,8 +212,6 @@ private boolean maybeInjectMetadataForSymlink(PathFragment linkPath, Artifact ou
 
       metadataInjector.injectFile(output, injectedMetadata);
     }
-
-    return true;
   }
 
   private RemoteFileArtifactValue createRemoteMetadata(RemoteFileInfo remoteFile) {
@@ -498,7 +461,12 @@ protected FileStatus stat(PathFragment path, boolean followSymlinks) throws IOEx
   }
 
   private static FileStatus statFromRemoteMetadata(RemoteFileArtifactValue m) {
-    return new FileStatus() {
+    return new RemoteFileStatus() {
+      @Override
+      public byte[] getDigest() {
+        return m.getDigest();
+      }
+
       @Override
       public boolean isFile() {
         return m.getType().isFile();
@@ -538,6 +506,11 @@ public long getLastChangeTime() {
       public long getNodeId() {
         throw new UnsupportedOperationException("Cannot get node id for " + m);
       }
+
+      @Override
+      public RemoteFileArtifactValue getRemoteMetadata() {
+        return m;
+      }
     };
   }
 

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

@@ -47,6 +47,7 @@
 import com.google.devtools.build.lib.vfs.FileStatusWithDigestAdapter;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.actions.RemoteFileStatus;
 import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.devtools.build.lib.vfs.XattrProvider;
@@ -656,13 +657,19 @@ private static FileArtifactValue fileArtifactValueFromStat(
       return FileArtifactValue.MISSING_FILE_MARKER;
     }
 
+    if (stat.isDirectory()) {
+        return FileArtifactValue.createForDirectoryWithMtime(stat.getLastModifiedTime());
+    }
+
+    if (stat instanceof RemoteFileStatus) {
+      return ((RemoteFileStatus) stat).getRemoteMetadata();
+    }
+
     FileStateValue fileStateValue =
         FileStateValue.createWithStatNoFollow(
             rootedPath, stat, digestWillBeInjected, xattrProvider, tsgm);
 
-    return stat.isDirectory()
-        ? FileArtifactValue.createForDirectoryWithMtime(stat.getLastModifiedTime())
-        : FileArtifactValue.createForNormalFile(
+    return FileArtifactValue.createForNormalFile(
             fileStateValue.getDigest(), fileStateValue.getContentsProxy(), stat.getSize());
   }
 

src/test/java/com/google/devtools/build/lib/remote/BuildWithoutTheBytesIntegrationTestBase.java

@@ -439,6 +439,36 @@ public void downloadToplevel_treeArtifacts() throws Exception {
     assertValidOutputFile("foo/file-3", "file-3\n");
   }
 
+  @Test
+  public void treeOutputsFromLocalFileSystem_works() throws Exception {
+    // Test that tree artifact generated locally can be consumed by other actions.
+    // See https://github.com/bazelbuild/bazel/issues/16789
+
+    // Disable remote execution so tree outputs are generated locally
+    addOptions("--modify_execution_info=OutputDir=+no-remote-exec");
+    setDownloadToplevel();
+    writeOutputDirRule();
+    write(
+        "BUILD",
+        "load(':output_dir.bzl', 'output_dir')",
+        "output_dir(",
+        "  name = 'foo',",
+        "  manifest = ':manifest',",
+        ")",
+        "genrule(",
+        "  name = 'foobar',",
+        "  srcs = [':foo'],",
+        "  outs = ['out/foobar.txt'],",
+        "  cmd = 'cat $(location :foo)/file-1 > $@ && echo bar >> $@',",
+        ")");
+    write("manifest", "file-1");
+
+    buildTarget("//:foobar");
+    waitDownloads();
+
+    assertValidOutputFile("out/foobar.txt", "file-1\nbar\n");
+  }
+
   @Test
   public void incrementalBuild_deleteOutputsInUnwritableParentDirectory() throws Exception {
     write(
@@ -695,6 +725,7 @@ protected void writeOutputDirRule() throws IOException {
         "def _output_dir_impl(ctx):",
         "  output_dir = ctx.actions.declare_directory(ctx.attr.name)",
         "  ctx.actions.run_shell(",
+        "    mnemonic = 'OutputDir',",
         "    inputs = [ctx.file.manifest],",
         "    outputs = [output_dir],",
         "    arguments = [ctx.file.manifest.path, output_dir.path],",