[6.0.0] Only inject metadata for outputs that cannot be reconstructed by skyf…

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

@@ -57,6 +57,7 @@ java_library(
             "ResourceManager.java",
             "ResourceSet.java",
             "ResourceSetOrBuilder.java",
+            "RemoteFileStatus.java",
             "PackageRootResolver.java",
             "PackageRoots.java",
             "ThreadStateReceiver.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

@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
+import com.google.devtools.build.lib.actions.RemoteFileStatus;
 import com.google.devtools.build.lib.actions.cache.MetadataInjector;
 import com.google.devtools.build.lib.clock.Clock;
 import com.google.devtools.build.lib.skyframe.TreeArtifactValue;
@@ -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) {
@@ -313,54 +276,85 @@ protected byte[] getDigest(PathFragment path) throws IOException {
   }
 
   // -------------------- File Permissions --------------------
+  // Remote files are always readable, writable and executable since we can't control their
+  // permissions.
+
+  private boolean existsInMemory(PathFragment path) {
+    return remoteOutputTree.getPath(path).isDirectory() || getRemoteMetadata(path) != null;
+  }
 
   @Override
   protected boolean isReadable(PathFragment path) throws IOException {
-    FileArtifactValue m = getRemoteMetadata(path);
-    return m != null || super.isReadable(path);
+    return existsInMemory(path) || super.isReadable(path);
   }
 
   @Override
   protected boolean isWritable(PathFragment path) throws IOException {
-    FileArtifactValue m = getRemoteMetadata(path);
-    return m != null || super.isWritable(path);
+    if (existsInMemory(path)) {
+      // If path exists locally, also check whether it's writable. We need this check for the case
+      // where the action need to delete their local outputs but the parent directory is not
+      // writable.
+      try {
+        return super.isWritable(path);
+      } catch (FileNotFoundException e) {
+        // Intentionally ignored
+        return true;
+      }
+    }
+
+    return super.isWritable(path);
   }
 
   @Override
   protected boolean isExecutable(PathFragment path) throws IOException {
-    FileArtifactValue m = getRemoteMetadata(path);
-    return m != null || super.isExecutable(path);
+    return existsInMemory(path) || super.isExecutable(path);
   }
 
   @Override
   protected void setReadable(PathFragment path, boolean readable) throws IOException {
-    RemoteFileArtifactValue m = getRemoteMetadata(path);
-    if (m == null) {
+    try {
       super.setReadable(path, readable);
+    } catch (FileNotFoundException e) {
+      // in case of missing in-memory path, re-throw the error.
+      if (!existsInMemory(path)) {
+        throw e;
+      }
     }
   }
 
   @Override
   public void setWritable(PathFragment path, boolean writable) throws IOException {
-    RemoteFileArtifactValue m = getRemoteMetadata(path);
-    if (m == null) {
+    try {
       super.setWritable(path, writable);
+    } catch (FileNotFoundException e) {
+      // in case of missing in-memory path, re-throw the error.
+      if (!existsInMemory(path)) {
+        throw e;
+      }
     }
   }
 
   @Override
   protected void setExecutable(PathFragment path, boolean executable) throws IOException {
-    RemoteFileArtifactValue m = getRemoteMetadata(path);
-    if (m == null) {
+    try {
       super.setExecutable(path, executable);
+    } catch (FileNotFoundException e) {
+      // in case of missing in-memory path, re-throw the error.
+      if (!existsInMemory(path)) {
+        throw e;
+      }
     }
   }
 
   @Override
   protected void chmod(PathFragment path, int mode) throws IOException {
-    RemoteFileArtifactValue m = getRemoteMetadata(path);
-    if (m == null) {
+    try {
       super.chmod(path, mode);
+    } catch (FileNotFoundException e) {
+      // in case of missing in-memory path, re-throw the error.
+      if (!existsInMemory(path)) {
+        throw e;
+      }
     }
   }
 
@@ -467,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();
@@ -507,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

@@ -38,6 +38,7 @@
 import com.google.devtools.build.lib.actions.FilesetManifest;
 import com.google.devtools.build.lib.actions.FilesetManifest.RelativeSymlinkBehaviorWithoutError;
 import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
+import com.google.devtools.build.lib.actions.RemoteFileStatus;
 import com.google.devtools.build.lib.actions.cache.MetadataHandler;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
 import com.google.devtools.build.lib.vfs.DigestUtils;
@@ -656,14 +657,20 @@ 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(
-            fileStateValue.getDigest(), fileStateValue.getContentsProxy(), stat.getSize());
+    return FileArtifactValue.createForNormalFile(
+        fileStateValue.getDigest(), fileStateValue.getContentsProxy(), stat.getSize());
   }
 
   private static void setPathReadOnlyAndExecutableIfFile(Path path) throws IOException {

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

@@ -378,6 +378,42 @@ public void downloadToplevel_treeArtifacts() throws Exception {
     assertValidOutputFile("foo/file-3", "file-3\n");
   }
 
+  @Test
+  public void treeOutputsFromLocalFileSystem_works() throws Exception {
+    // Disable on Windows since it fails for unknown reasons.
+    // TODO(chiwang): Enable it on windows.
+    if (OS.getCurrent() == OS.WINDOWS) {
+      return;
+    }
+
+    // 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(
@@ -513,6 +549,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],",