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

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