Only inject symlink metadata

bazelbuild · Nov 25, 2022 · 10235b1 · 10235b1
1 parent cc712ee
commit 10235b1
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 39 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/BUILD b/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 = [

diff --git a/src/main/java/com/google/devtools/build/lib/actions/RemoteFileStatus.java b/src/main/java/com/google/devtools/build/lib/actions/RemoteFileStatus.java
@@ -0,0 +1,8 @@
+package com.google.devtools.build.lib.actions;
+
+import com.google.devtools.build.lib.actions.FileArtifactValue.RemoteFileArtifactValue;
+import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
+
+public interface RemoteFileStatus extends FileStatusWithDigest {
+  RemoteFileArtifactValue getRemoteMetadata();
+}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteActionFileSystem.java b/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);
     }
   }
 
@@ -498,7 +465,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 +510,11 @@ public long getLastChangeTime() {
       public long getNodeId() {
         throw new UnsupportedOperationException("Cannot get node id for " + m);
       }
+
+      @Override
+      public RemoteFileArtifactValue getRemoteMetadata() {
+        return m;
+      }
     };
   }
 

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionMetadataHandler.java b/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());
   }
 

diff --git a/src/test/shell/bazel/remote/build_without_the_bytes_test.sh b/src/test/shell/bazel/remote/build_without_the_bytes_test.sh
@@ -1341,4 +1341,46 @@ EOF
   expect_log "bin-message"
 }
 
+function test_tree_artifact_from_local_file_system() {
+  # Test that tree artifact generated locally can be consumed by other actions.
+  # See https://github.com/bazelbuild/bazel/issues/16789
+
+  mkdir -p a
+  cat > a/my_rule.bzl <<'EOF'
+def _impl(ctx):
+  out = ctx.actions.declare_directory(ctx.label.name)
+  ctx.actions.run_shell(
+    outputs = [out],
+    command = "echo '1' > {out}/my_file".format(out = out.path),
+  )
+  return DefaultInfo(
+    files = depset([out]),
+    runfiles = ctx.runfiles(files = [out])
+  )
+my_rule = rule(
+  implementation = _impl,
+  attrs = {},
+)
+EOF
+  cat > a/out_dir_test.sh <<'EOF'
+cat $1/my_file
+EOF
+  chmod a+x a/out_dir_test.sh
+  cat > a/BUILD <<'EOF'
+load(":my_rule.bzl", "my_rule")
+my_rule(name = "out_dir")
+sh_test(
+  name = "out_dir_test",
+  srcs = ["out_dir_test.sh"],
+  args = ["$(rootpath :out_dir)"],
+  data = [":out_dir"],
+)
+EOF
+
+  bazel test \
+    --remote_cache=grpc://localhost:${worker_port} \
+    --remote_download_minimal \
+    //a:out_dir_test >& $TEST_log || fail "Failed to test //a:out_dir_test"
+}
+
 run_suite "Build without the Bytes tests"