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

… by skyf… (#16879)

* Update RemoteActionFileSystem to apply permission changes to local files even if remote file exists.

Previously, it only applies permission changes to local files when the remote one doesn't exist. It is fine because the call sites only use these method when remote file are missing. However, this isn't true with future changes.

PiperOrigin-RevId: 490872822
Change-Id: I7a19d99cd828294cbafa7b5f3fdc368d64e556ec

* Fix permission operations for the case of only remote directory.

PiperOrigin-RevId: 491280334
Change-Id: I30afef9f069eca8aee4d983664f42b3961e95adf

* Only inject metadata for outputs that cannot be reconstructed by skyframe later

Currently, they are symlinks.

For other outputs, let skyframe read action file system to construct the metadata.

Before this change, we inject metadata of symlink outputs, tree outputs and file outputs inside `RemoteActionFileSystem#flush()` if these outputs are stored inside the in-memory fs. If the outputs are somehow generated in the local fs, skyframe will read the fs later to construct metadata for them.

However, for tree outputs, skyframe always create an empty directory before action execution in the in-memory fs. So inside the `flush`, we always inject metadata for it. It means local tree outputs are ignored. We could update the code to also read local file system when reading tree files, but then the problem is how to construct metadata from file status which is well done by skyframe.

So instead of injecting metadata by traversal the filesystem inside `flush`, we just let skyframe to do the job.

Fixes #16789.

Closes #16812.

PiperOrigin-RevId: 491622005
Change-Id: I10434e6856a1b2a207f39e07122a9b646edf518c

Co-authored-by: kshyanashree <109167932+kshyanashree@users.noreply.github.com>

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],",