[7.0.1] Fix two issues with --incompatible_sandbox_hermetic_tmp that …

…manifested themselves when the output base was under /tmp (#20718)

1. Virtual action inputs didn't work. This was a fairly simple omission
in the path transformation logic.
2. Artifacts which are resolved symlinks (i.e. declared using
`declare_file`) did not work. This is fixed by keeping track of the
realpath of the symlink in `FileArtifactValue` / `TreeArtifactValue`.

Fixes #20515 and #20518.

RELNOTES: None.
Commit
fb6658c

PiperOrigin-RevId: 595145191
Change-Id: I833025928374c78bc719d8e3be1efb2b2950b9f1

Co-authored-by: Googler <[email protected]>

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

@@ -169,11 +169,19 @@ protected boolean couldBeModifiedByMetadata(FileArtifactValue lastKnown) {
   /**
    * Optional materialization path.
    *
-   * <p>If present, this artifact is a copy of another artifact. It is still tracked as a
-   * non-symlink by Bazel, but materialized in the local filesystem as a symlink to the original
-   * artifact, whose contents live at this location. This is used by {@link
-   * com.google.devtools.build.lib.remote.AbstractActionInputPrefetcher} to implement zero-cost
-   * copies of remotely stored artifacts.
+   * <p>If present, this artifact is a copy of another artifact whose contents live at this path.
+   * This can happen when it is declared as a file and not as an unresolved symlink but the action
+   * that creates it materializes it in the filesystem as a symlink to another output artifact. This
+   * information is useful in two situations:
+   *
+   * <ol>
+   *   <li>When the symlink target is a remotely stored artifact, we can avoid downloading it
+   *       multiple times when building without the bytes (see AbstractActionInputPrefetcher).
+   *   <li>When the symlink target is inaccessible from the sandboxed environment an action runs
+   *       under, we can rewrite it accordingly (see SandboxHelpers).
+   * </ol>
+   *
+   * @see com.google.devtools.build.lib.skyframe.TreeArtifactValue#getMaterializationExecPath().
    */
   public Optional<PathFragment> getMaterializationExecPath() {
     return Optional.empty();
@@ -214,6 +222,12 @@ public static FileArtifactValue createForSourceArtifact(
         xattrProvider);
   }
 
+  public static FileArtifactValue createForResolvedSymlink(
+      PathFragment realPath, FileArtifactValue metadata, @Nullable byte[] digest) {
+    return new ResolvedSymlinkFileArtifactValue(
+        realPath, digest, metadata.getContentsProxy(), metadata.getSize());
+  }
+
   public static FileArtifactValue createFromInjectedDigest(
       FileArtifactValue metadata, @Nullable byte[] digest) {
     return createForNormalFile(digest, metadata.getContentsProxy(), metadata.getSize());
@@ -439,7 +453,25 @@ public String toString() {
     }
   }
 
-  private static final class RegularFileArtifactValue extends FileArtifactValue {
+  private static final class ResolvedSymlinkFileArtifactValue extends RegularFileArtifactValue {
+    private final PathFragment realPath;
+
+    private ResolvedSymlinkFileArtifactValue(
+        PathFragment realPath,
+        @Nullable byte[] digest,
+        @Nullable FileContentsProxy proxy,
+        long size) {
+      super(digest, proxy, size);
+      this.realPath = realPath;
+    }
+
+    @Override
+    public Optional<PathFragment> getMaterializationExecPath() {
+      return Optional.of(realPath);
+    }
+  }
+
+  private static class RegularFileArtifactValue extends FileArtifactValue {
     private final byte[] digest;
     @Nullable private final FileContentsProxy proxy;
     private final long size;
@@ -462,7 +494,8 @@ public boolean equals(Object o) {
       RegularFileArtifactValue that = (RegularFileArtifactValue) o;
       return Arrays.equals(digest, that.digest)
           && Objects.equals(proxy, that.proxy)
-          && size == that.size;
+          && size == that.size
+          && Objects.equals(getMaterializationExecPath(), that.getMaterializationExecPath());
     }
 
     @Override

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

@@ -17,6 +17,7 @@ java_library(
     deps = [
         "//src/main/java/com/google/devtools/build/lib/actions",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
+        "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
         "//src/main/java/com/google/devtools/build/lib/analysis:test/test_configuration",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/lib/vfs",

src/main/java/com/google/devtools/build/lib/sandbox/DarwinSandboxedSpawnRunner.java

@@ -228,6 +228,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
     SandboxInputs inputs =
         helpers.processInputFiles(
             context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
+            context.getInputMetadataProvider(),
             execRoot,
             execRoot,
             packageRoots,

src/main/java/com/google/devtools/build/lib/sandbox/DockerSandboxedSpawnRunner.java

@@ -226,6 +226,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
     SandboxInputs inputs =
         helpers.processInputFiles(
             context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
+            context.getInputMetadataProvider(),
             execRoot,
             execRoot,
             packageRoots,

src/main/java/com/google/devtools/build/lib/sandbox/LinuxSandboxedSpawnRunner.java

@@ -280,6 +280,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
     SandboxInputs inputs =
         helpers.processInputFiles(
             context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
+            context.getInputMetadataProvider(),
             execRoot,
             withinSandboxExecRoot,
             packageRoots,

src/main/java/com/google/devtools/build/lib/sandbox/ProcessWrapperSandboxedSpawnRunner.java

@@ -100,6 +100,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
     SandboxInputs inputs =
         helpers.processInputFiles(
             context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
+            context.getInputMetadataProvider(),
             execRoot,
             execRoot,
             packageRoots,

src/main/java/com/google/devtools/build/lib/sandbox/SandboxHelpers.java

@@ -20,6 +20,7 @@
 import static com.google.devtools.build.lib.vfs.Dirent.Type.SYMLINK;
 
 import com.google.auto.value.AutoValue;
+import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -29,6 +30,8 @@
 import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.actions.ActionInput;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.FileArtifactValue;
+import com.google.devtools.build.lib.actions.InputMetadataProvider;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.UserExecException;
 import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
@@ -444,6 +447,29 @@ private static RootedPath processFilesetSymlink(
             symlink.getPathString()));
   }
 
+  private static RootedPath processResolvedSymlink(
+      Root absoluteRoot,
+      PathFragment execRootRelativeSymlinkTarget,
+      Root execRootWithinSandbox,
+      PathFragment execRootFragment,
+      ImmutableList<Root> packageRoots,
+      Function<Root, Root> sourceRooWithinSandbox) {
+    PathFragment symlinkTarget = execRootFragment.getRelative(execRootRelativeSymlinkTarget);
+    for (Root packageRoot : packageRoots) {
+      if (packageRoot.contains(symlinkTarget)) {
+        return RootedPath.toRootedPath(
+            sourceRooWithinSandbox.apply(packageRoot), packageRoot.relativize(symlinkTarget));
+      }
+    }
+
+    if (symlinkTarget.startsWith(execRootFragment)) {
+      return RootedPath.toRootedPath(
+          execRootWithinSandbox, symlinkTarget.relativeTo(execRootFragment));
+    }
+
+    return RootedPath.toRootedPath(absoluteRoot, symlinkTarget);
+  }
+
   /**
    * Returns the inputs of a Spawn as a map of PathFragments relative to an execRoot to paths in the
    * host filesystem where the input files can be found.
@@ -458,6 +484,7 @@ private static RootedPath processFilesetSymlink(
    */
   public SandboxInputs processInputFiles(
       Map<PathFragment, ActionInput> inputMap,
+      InputMetadataProvider inputMetadataProvider,
       Path execRootPath,
       Path withinSandboxExecRootPath,
       ImmutableList<Root> packageRoots,
@@ -468,12 +495,24 @@ public SandboxInputs processInputFiles(
         withinSandboxExecRootPath.equals(execRootPath)
             ? withinSandboxExecRoot
             : Root.fromPath(execRootPath);
+    Root absoluteRoot = Root.absoluteRoot(execRootPath.getFileSystem());
 
     Map<PathFragment, RootedPath> inputFiles = new TreeMap<>();
     Map<PathFragment, PathFragment> inputSymlinks = new TreeMap<>();
     Map<VirtualActionInput, byte[]> virtualInputs = new HashMap<>();
     Map<Root, Root> sourceRootToSandboxSourceRoot = new TreeMap<>();
 
+    Function<Root, Root> sourceRootWithinSandbox =
+        r -> {
+          if (!sourceRootToSandboxSourceRoot.containsKey(r)) {
+            int next = sourceRootToSandboxSourceRoot.size();
+            sourceRootToSandboxSourceRoot.put(
+                r, Root.fromPath(sandboxSourceRoots.getRelative(Integer.toString(next))));
+          }
+
+          return sourceRootToSandboxSourceRoot.get(r);
+        };
+
     for (Map.Entry<PathFragment, ActionInput> e : inputMap.entrySet()) {
       if (Thread.interrupted()) {
         throw new InterruptedException();
@@ -503,23 +542,63 @@ public SandboxInputs processInputFiles(
 
         if (actionInput instanceof EmptyActionInput) {
           inputPath = null;
+        } else if (actionInput instanceof VirtualActionInput) {
+          inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, actionInput.getExecPath());
         } else if (actionInput instanceof Artifact) {
           Artifact inputArtifact = (Artifact) actionInput;
-          if (inputArtifact.isSourceArtifact() && sandboxSourceRoots != null) {
-            Root sourceRoot = inputArtifact.getRoot().getRoot();
-            if (!sourceRootToSandboxSourceRoot.containsKey(sourceRoot)) {
-              int next = sourceRootToSandboxSourceRoot.size();
-              sourceRootToSandboxSourceRoot.put(
-                  sourceRoot,
-                  Root.fromPath(sandboxSourceRoots.getRelative(Integer.toString(next))));
-            }
-
-            inputPath =
-                RootedPath.toRootedPath(
-                    sourceRootToSandboxSourceRoot.get(sourceRoot),
-                    inputArtifact.getRootRelativePath());
-          } else {
+          if (sandboxSourceRoots == null) {
             inputPath = RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath());
+          } else {
+            if (inputArtifact.isSourceArtifact()) {
+              Root sourceRoot = inputArtifact.getRoot().getRoot();
+              inputPath =
+                  RootedPath.toRootedPath(
+                      sourceRootWithinSandbox.apply(sourceRoot),
+                      inputArtifact.getRootRelativePath());
+            } else {
+              PathFragment materializationExecPath = null;
+              if (inputArtifact.isChildOfDeclaredDirectory()) {
+                FileArtifactValue parentMetadata =
+                    inputMetadataProvider.getInputMetadata(inputArtifact.getParent());
+                if (parentMetadata.getMaterializationExecPath().isPresent()) {
+                  materializationExecPath =
+                      parentMetadata
+                          .getMaterializationExecPath()
+                          .get()
+                          .getRelative(inputArtifact.getParentRelativePath());
+                }
+              } else if (!inputArtifact.isTreeArtifact()) {
+                // Normally, one would not see tree artifacts here because they have already been
+                // expanded by the time the code gets here. However, there is one very special case:
+                // when an action has an archived tree artifact on its output and is executed on the
+                // local branch of the dynamic execution strategy, the tree artifact is zipped up
+                // in a little extra spawn, which has direct tree artifact on its inputs. Sadly,
+                // it's not easy to fix this because there isn't an easy way to inject this new
+                // tree artifact into the artifact expander being used.
+                //
+                // The best would be to not rely on spawn strategies for executing that little
+                // command: it's entirely under the control of Bazel so we can guarantee that it
+                // does not cause mischief.
+                FileArtifactValue metadata = inputMetadataProvider.getInputMetadata(actionInput);
+                if (metadata.getMaterializationExecPath().isPresent()) {
+                  materializationExecPath = metadata.getMaterializationExecPath().get();
+                }
+              }
+
+              if (materializationExecPath != null) {
+                inputPath =
+                    processResolvedSymlink(
+                        absoluteRoot,
+                        materializationExecPath,
+                        withinSandboxExecRoot,
+                        execRootPath.asFragment(),
+                        packageRoots,
+                        sourceRootWithinSandbox);
+              } else {
+                inputPath =
+                    RootedPath.toRootedPath(withinSandboxExecRoot, inputArtifact.getExecPath());
+              }
+            }
           }
         } else {
           PathFragment execPath = actionInput.getExecPath();
@@ -544,6 +623,7 @@ public SandboxInputs processInputFiles(
     return new SandboxInputs(inputFiles, virtualInputs, inputSymlinks, sandboxRootToSourceRoot);
   }
 
+
   /** The file and directory outputs of a sandboxed spawn. */
   @AutoValue
   public abstract static class SandboxOutputs {

src/main/java/com/google/devtools/build/lib/sandbox/WindowsSandboxedSpawnRunner.java

@@ -76,6 +76,7 @@ protected SandboxedSpawn prepareSpawn(Spawn spawn, SpawnExecutionContext context
     SandboxInputs readablePaths =
         helpers.processInputFiles(
             context.getInputMapping(PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
+            context.getInputMetadataProvider(),
             execRoot,
             execRoot,
             packageRoots,

src/main/java/com/google/devtools/build/lib/skyframe/ActionOutputMetadataStore.java

@@ -264,7 +264,15 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa
     Path treeDir = artifactPathResolver.toPath(parent);
     boolean chmod = executionMode.get();
 
-    FileStatus stat = treeDir.statIfFound(Symlinks.FOLLOW);
+    FileStatus lstat = treeDir.statIfFound(Symlinks.NOFOLLOW);
+    FileStatus stat;
+    if (lstat == null) {
+      stat = null;
+    } else if (!lstat.isSymbolicLink()) {
+      stat = lstat;
+    } else {
+      stat = treeDir.statIfFound(Symlinks.FOLLOW);
+    }
 
     // Make sure the tree artifact root exists and is a regular directory. Note that this is how the
     // action is initialized, so this should hold unless the action itself has deleted the root.
@@ -332,12 +340,18 @@ private TreeArtifactValue constructTreeArtifactValueFromFilesystem(SpecialArtifa
     }
 
     // Same rationale as for constructFileArtifactValue.
-    if (anyRemote.get() && treeDir.isSymbolicLink() && stat instanceof FileStatusWithMetadata) {
-      FileArtifactValue metadata = ((FileStatusWithMetadata) stat).getMetadata();
-      tree.setMaterializationExecPath(
-          metadata
-              .getMaterializationExecPath()
-              .orElse(treeDir.resolveSymbolicLinks().asFragment().relativeTo(execRoot)));
+    if (lstat.isSymbolicLink()) {
+      PathFragment materializationExecPath = null;
+      if (stat instanceof FileStatusWithMetadata) {
+        materializationExecPath =
+            ((FileStatusWithMetadata) stat).getMetadata().getMaterializationExecPath().orElse(null);
+      }
+
+      if (materializationExecPath == null) {
+        materializationExecPath = treeDir.resolveSymbolicLinks().asFragment().relativeTo(execRoot);
+      }
+
+      tree.setMaterializationExecPath(materializationExecPath);
     }
 
     return tree.build();
@@ -509,7 +523,13 @@ private FileArtifactValue constructFileArtifactValue(
       return value;
     }
 
-    if (type.isFile() && fileDigest != null) {
+    boolean isResolvedSymlink =
+        statAndValue.statNoFollow() != null
+            && statAndValue.statNoFollow().isSymbolicLink()
+            && statAndValue.realPath() != null
+            && !value.isRemote();
+
+    if (type.isFile() && !isResolvedSymlink && fileDigest != null) {
       // The digest is in the file value and that is all that is needed for this file's metadata.
       return value;
     }
@@ -525,22 +545,30 @@ private FileArtifactValue constructFileArtifactValue(
           artifactPathResolver.toPath(artifact).getLastModifiedTime());
     }
 
-    if (injectedDigest == null && type.isFile()) {
+    byte[] actualDigest = fileDigest != null ? fileDigest : injectedDigest;
+
+    if (actualDigest == null && type.isFile()) {
       // We don't have an injected digest and there is no digest in the file value (which attempts a
       // fast digest). Manually compute the digest instead.
-      Path path = statAndValue.pathNoFollow();
-      if (statAndValue.statNoFollow() != null
-          && statAndValue.statNoFollow().isSymbolicLink()
-          && statAndValue.realPath() != null) {
-        // If the file is a symlink, we compute the digest using the target path so that it's
-        // possible to hit the digest cache - we probably already computed the digest for the
-        // target during previous action execution.
-        path = statAndValue.realPath();
-      }
 
-      injectedDigest = DigestUtils.manuallyComputeDigest(path, value.getSize());
+      // If the file is a symlink, we compute the digest using the target path so that it's
+      // possible to hit the digest cache - we probably already computed the digest for the
+      // target during previous action execution.
+      Path pathToDigest = isResolvedSymlink ? statAndValue.realPath() : statAndValue.pathNoFollow();
+      actualDigest = DigestUtils.manuallyComputeDigest(pathToDigest, value.getSize());
     }
-    return FileArtifactValue.createFromInjectedDigest(value, injectedDigest);
+
+    if (!isResolvedSymlink) {
+      return FileArtifactValue.createFromInjectedDigest(value, actualDigest);
+    }
+
+    PathFragment realPathAsFragment = statAndValue.realPath().asFragment();
+    PathFragment execRootRelativeRealPath =
+        realPathAsFragment.startsWith(execRoot)
+            ? realPathAsFragment.relativeTo(execRoot)
+            : realPathAsFragment;
+    return FileArtifactValue.createForResolvedSymlink(
+        execRootRelativeRealPath, value, actualDigest);
   }
 
   /**

src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java

@@ -188,6 +188,7 @@ public SpawnResult exec(Spawn spawn, SpawnExecutionContext context)
             helpers.processInputFiles(
                 context.getInputMapping(
                     PathFragment.EMPTY_FRAGMENT, /* willAccessRepeatedly= */ true),
+                context.getInputMetadataProvider(),
                 execRoot,
                 execRoot,
                 packageRoots,