Inject metadata when creating a filesystem symlink for a non-symlink …

…artifact.

A `ctx.actions.symlink` whose output is a declare_file or declare_directory
(as opposed to a declare_symlink) has "copy" semantics, i.e., the output
artifact is indistinguishable from the referent except for its name; the
symlink is just a filesystem-level optimization to avoid an actual copy,
and is transparently resolved when collecting the action output metadata.

When the symlink points to a directory that was built remotely and without the
bytes, we currently must download the directory contents before executing the
symlink action, so that the output metadata can be constructed from the local
filesystem. This change short-circuits the filesystem traversal by injecting
the input metadata into the output, thus avoiding the download.

(An alternative would have been to teach all of the RemoteActionFileSystem
methods to resolve symlinks by patching together the local and remote metadata,
but that results in an awful lot of complexity.)

Fixes #15678.

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

@@ -84,6 +84,21 @@ TrieArtifact getOrPutSubFolder(String name) {
       return trieArtifact;
     }
 
+    @Nullable
+    TreeArtifactValue findTreeArtifactNode(PathFragment execPath) {
+      TrieArtifact current = this;
+      for (String segment : execPath.segments()) {
+        current = current.subFolders.get(segment);
+        if (current == null) {
+          break;
+        }
+      }
+      if (current != null && current.treeArtifactValue != null) {
+        return current.treeArtifactValue;
+      }
+      return null;
+    }
+
     @Nullable
     TreeArtifactValue findTreeArtifactNodeAtPrefix(PathFragment execPath) {
       TrieArtifact current = this;
@@ -233,6 +248,11 @@ public FileArtifactValue getMetadata(PathFragment execPath) {
     return getMetadataFromTreeArtifacts(execPath);
   }
 
+  @Nullable
+  public TreeArtifactValue getTreeMetadata(PathFragment execPath) {
+    return treeArtifactsRoot.findTreeArtifactNode(execPath);
+  }
+
   @Nullable
   private FileArtifactValue getMetadataFromTreeArtifacts(PathFragment execPath) {
     TreeArtifactValue tree = treeArtifactsRoot.findTreeArtifactNodeAtPrefix(execPath);

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

@@ -16,7 +16,11 @@
 package com.google.devtools.build.lib.remote;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
+import static com.google.common.collect.Streams.stream;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.ActionInputMap;
 import com.google.devtools.build.lib.actions.Artifact;
@@ -43,7 +47,7 @@
 
 /**
  * This is a basic implementation and incomplete implementation of an action file system that's been
- * tuned to what native (non-spawn) actions in Bazel currently use.
+ * tuned to what internal (non-spawn) actions in Bazel currently use.
  *
  * <p>The implementation mostly delegates to the local file system except for the case where an
  * action input is a remotely stored action output. Most notably {@link
@@ -52,12 +56,13 @@
  * <p>This implementation only supports creating local action outputs.
  */
 public class RemoteActionFileSystem extends DelegateFileSystem {
-
   private final PathFragment execRoot;
   private final PathFragment outputBase;
   private final ActionInputMap inputArtifactData;
+  private final ImmutableMap<PathFragment, Artifact> outputArtifactByExecPath;
   private final RemoteActionInputFetcher inputFetcher;
-  private final Map<PathFragment, RemoteFileArtifactValue> injectedMetadata = new HashMap<>();
+  private final HashMap<PathFragment, RemoteFileArtifactValue> injectedMetadata = new HashMap<>();
+  private final HashMap<PathFragment, TreeArtifactValue> injectedTreeMetadata = new HashMap<>();
 
   @Nullable private MetadataInjector metadataInjector = null;
 
@@ -66,11 +71,15 @@ public class RemoteActionFileSystem extends DelegateFileSystem {
       PathFragment execRootFragment,
       String relativeOutputPath,
       ActionInputMap inputArtifactData,
+      Iterable<Artifact> outputArtifacts,
       RemoteActionInputFetcher inputFetcher) {
     super(localDelegate);
     this.execRoot = checkNotNull(execRootFragment, "execRootFragment");
     this.outputBase = execRoot.getRelative(checkNotNull(relativeOutputPath, "relativeOutputPath"));
     this.inputArtifactData = checkNotNull(inputArtifactData, "inputArtifactData");
+    this.outputArtifactByExecPath =
+        stream(checkNotNull(outputArtifacts, "outputArtifacts"))
+            .collect(toImmutableMap(Artifact::getExecPath, a -> a));
     this.inputFetcher = checkNotNull(inputFetcher, "inputFetcher");
   }
 
@@ -86,12 +95,13 @@ public void updateContext(MetadataInjector metadataInjector) {
   void injectTree(SpecialArtifact tree, TreeArtifactValue metadata) {
     checkNotNull(metadataInjector, "metadataInject is null");
 
+    injectedTreeMetadata.put(tree.getExecPath(), metadata);
+
     for (Map.Entry<TreeFileArtifact, FileArtifactValue> entry :
         metadata.getChildValues().entrySet()) {
       FileArtifactValue childMetadata = entry.getValue();
       if (childMetadata instanceof RemoteFileArtifactValue) {
-        TreeFileArtifact child = entry.getKey();
-        injectedMetadata.put(child.getExecPath(), (RemoteFileArtifactValue) childMetadata);
+        injectedMetadata.put(entry.getKey().getExecPath(), (RemoteFileArtifactValue) childMetadata);
       }
     }
 
@@ -101,7 +111,9 @@ void injectTree(SpecialArtifact tree, TreeArtifactValue metadata) {
   void injectFile(ActionInput file, RemoteFileArtifactValue metadata) {
     checkNotNull(metadataInjector, "metadataInject is null");
 
-    injectedMetadata.put(file.getExecPath(), metadata);
+    if (metadata instanceof RemoteFileArtifactValue) {
+      injectedMetadata.put(file.getExecPath(), metadata);
+    }
 
     if (file instanceof Artifact) {
       metadataInjector.injectFile((Artifact) file, metadata);
@@ -227,11 +239,44 @@ protected PathFragment readSymbolicLink(PathFragment path) throws IOException {
   @Override
   protected void createSymbolicLink(PathFragment linkPath, PathFragment targetFragment)
       throws IOException {
-    /*
-     * TODO(buchgr): Optimize the case where we are creating a symlink to a remote output. This does
-     * add a non-trivial amount of complications though (as symlinks tend to do).
-     */
-    downloadFileIfRemote(execRoot.getRelative(targetFragment));
+    FileStatus stat = statNullable(linkPath, /* followSymlinks= */ false);
+    if (stat != null) {
+      throw new IOException(linkPath + " (File exists)");
+    }
+
+    // If the output is a regular file or tree artifact, it has "copy" semantics, i.e., it will be
+    // transparently resolved into the artifact it points to; the symlink is just a filesystem-level
+    // optimization to avoid making an actual copy. We can inject its metadata and avoid the need to
+    // compute it from the filesystem later, which would be slower and require additional filesystem
+    // operations to be implemented.
+    Artifact output = outputArtifactByExecPath.get(linkPath.relativeTo(execRoot));
+    if (output != null) {
+      if (output.isTreeArtifact()) {
+        TreeArtifactValue metadata = getRemoteInputTreeMetadata(targetFragment);
+        if (metadata != null) {
+          // The archived representation is a Google-internal feature disabled on open source Bazel.
+          checkState(
+              metadata.getArchivedRepresentation().isEmpty(),
+              "symlink to archived tree artifact is not supported");
+          TreeArtifactValue.Builder treeBuilder =
+              TreeArtifactValue.newBuilder((SpecialArtifact) output);
+          for (Map.Entry<TreeFileArtifact, FileArtifactValue> entry :
+              metadata.getChildValues().entrySet()) {
+            treeBuilder.putChild(
+                TreeFileArtifact.createTreeOutput(
+                    (SpecialArtifact) output, entry.getKey().getParentRelativePath()),
+                entry.getValue());
+          }
+          injectTree((SpecialArtifact) output, treeBuilder.build());
+        }
+      } else if (!output.isSymlink()) {
+        RemoteFileArtifactValue metadata = getRemoteInputMetadata(targetFragment);
+        if (metadata != null) {
+          injectFile(output, metadata);
+        }
+      }
+    }
+
     super.createSymbolicLink(linkPath, targetFragment);
   }
 
@@ -370,10 +415,22 @@ private RemoteFileArtifactValue getRemoteInputMetadata(PathFragment path) {
     if (m != null && m.isRemote()) {
       return (RemoteFileArtifactValue) m;
     }
-
     return injectedMetadata.get(execPath);
   }
 
+  @Nullable
+  private TreeArtifactValue getRemoteInputTreeMetadata(PathFragment path) {
+    if (!path.startsWith(outputBase)) {
+      return null;
+    }
+    PathFragment execPath = path.relativeTo(execRoot);
+    TreeArtifactValue m = inputArtifactData.getTreeMetadata(execPath);
+    if (m != null && m.isEntirelyRemote()) {
+      return m;
+    }
+    return injectedTreeMetadata.get(execPath);
+  }
+
   private void downloadFileIfRemote(PathFragment path) throws IOException {
     FileArtifactValue m = getRemoteInputMetadata(path);
     if (m != null) {

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

@@ -70,6 +70,7 @@ public FileSystem createActionFileSystem(
         execRootFragment,
         relativeOutputPath,
         inputArtifactData,
+        outputArtifacts,
         actionInputFetcher);
   }
 
@@ -142,7 +143,12 @@ public ArtifactPathResolver createPathResolverForArtifactValues(
       Map<Artifact, ImmutableList<FilesetOutputSymlink>> filesets) {
     FileSystem remoteFileSystem =
         new RemoteActionFileSystem(
-            fileSystem, execRoot, relativeOutputPath, actionInputMap, actionInputFetcher);
+            fileSystem,
+            execRoot,
+            relativeOutputPath,
+            actionInputMap,
+            ImmutableList.of(),
+            actionInputFetcher);
     return ArtifactPathResolver.createPathResolver(remoteFileSystem, fileSystem.getPath(execRoot));
   }
 }

src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java

@@ -134,6 +134,10 @@ public void putTreeArtifact_addsTreeArtifactAndAllChildren() {
     assertContainsEqualMetadata(tree, treeValue.getMetadata());
     assertContainsSameInstance(child1, child1Metadata);
     assertContainsSameInstance(child2, child2Metadata);
+
+    assertThat(map.getMetadata(tree)).isEqualTo(treeValue.getMetadata());
+    assertThat(map.getTreeMetadata(tree.getExecPath())).isEqualTo(treeValue);
+    assertThat(map.getTreeMetadata(child1.getExecPath())).isNull();
   }
 
   @Test

src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java

@@ -267,6 +267,20 @@ public static SpecialArtifact createTreeArtifactWithGeneratingAction(
     return treeArtifact;
   }
 
+  public static SpecialArtifact createTreeArtifactWithGeneratingAction(
+      ArtifactRoot root, String path) {
+    return createTreeArtifactWithGeneratingAction(
+        root, root.getExecPath().getRelative(PathFragment.create(path)));
+  }
+
+  public static SpecialArtifact createUnresolvedSymlinkArtifact(ArtifactRoot root, String path) {
+    return SpecialArtifact.create(
+        root,
+        root.getExecPath().getRelative(PathFragment.create(path)),
+        NULL_ARTIFACT_OWNER,
+        SpecialArtifactType.UNRESOLVED_SYMLINK);
+  }
+
   public static void assertNoArtifactEndingWith(RuleConfiguredTarget target, String path) {
     Pattern endPattern = Pattern.compile(path + "$");
     for (ActionAnalysisMetadata action : target.getActions()) {

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

@@ -25,6 +25,7 @@
 import build.bazel.remote.execution.v2.Digest;
 import com.google.bytestream.ByteStreamProto.WriteRequest;
 import com.google.bytestream.ByteStreamProto.WriteResponse;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.eventbus.EventBus;
@@ -361,6 +362,7 @@ public void remoteFileShouldNotBeUploaded_actionFs() throws Exception {
             execRoot.asFragment(),
             outputRoot.getRoot().asPath().relativeTo(execRoot).getPathString(),
             outputs,
+            ImmutableList.of(artifact),
             actionInputFetcher);
     Path remotePath = remoteFs.getPath(artifact.getPath().getPathString());
     assertThat(remotePath.getFileSystem()).isEqualTo(remoteFs);