Do not force unresolved symlinks to be absolute

Fixes bazelbuild#14224

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

@@ -249,6 +249,7 @@ java_library(
         "starlark/StarlarkRuleContext.java",
         "starlark/StarlarkRuleTransitionProvider.java",
         "starlark/StarlarkTransition.java",
+        "starlark/UnresolvedSymlinkAction.java",
         "test/AnalysisTestActionBuilder.java",
         "test/BaselineCoverageAction.java",
         "test/CoverageCommon.java",

src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java

@@ -36,7 +36,10 @@
 import java.io.IOException;
 import javax.annotation.Nullable;
 
-/** Action to create a symbolic link. */
+/**
+ * Action to create a symlink to a known-to-exist target with alias semantics similar to a true copy
+ * of the input (if any).
+ */
 public final class SymlinkAction extends AbstractAction {
   private static final String GUID = "7f4fab4d-d0a7-4f0f-8649-1d0337a21fee";
 
@@ -152,13 +155,6 @@ public static SymlinkAction toFileset(
         owner, execPath, primaryInput, primaryOutput, progressMessage, TargetType.FILESET);
   }
 
-  public static SymlinkAction createUnresolved(
-      ActionOwner owner, Artifact primaryOutput, PathFragment targetPath, String progressMessage) {
-    Preconditions.checkArgument(primaryOutput.isSymlink());
-    return new SymlinkAction(
-        owner, targetPath, null, primaryOutput, progressMessage, TargetType.OTHER);
-  }
-
   /**
    * Creates a new SymlinkAction instance, where an input artifact is not present. This is useful
    * when dealing with special cases where input paths that are outside the exec root directory

src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java

@@ -273,7 +273,7 @@ public void symlink(
             ? (String) progressMessageUnchecked
             : "Creating symlink " + outputArtifact.getExecPathString();
 
-    SymlinkAction action;
+    Action action;
     if (targetFile != Starlark.NONE) {
       Artifact inputArtifact = (Artifact) targetFile;
       if (outputArtifact.isSymlink()) {
@@ -324,10 +324,10 @@ public void symlink(
       }
 
       action =
-          SymlinkAction.createUnresolved(
+          new UnresolvedSymlinkAction(
               ruleContext.getActionOwner(),
               outputArtifact,
-              PathFragment.create((String) targetPath),
+              (String) targetPath,
               progressMessage);
     }
     registerAction(action);

src/main/java/com/google/devtools/build/lib/analysis/starlark/UnresolvedSymlinkAction.java

@@ -0,0 +1,109 @@
+// 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.analysis.starlark;
+
+import com.google.common.collect.ImmutableSet;
+import com.google.devtools.build.lib.actions.AbstractAction;
+import com.google.devtools.build.lib.actions.ActionExecutionContext;
+import com.google.devtools.build.lib.actions.ActionExecutionException;
+import com.google.devtools.build.lib.actions.ActionKeyContext;
+import com.google.devtools.build.lib.actions.ActionOwner;
+import com.google.devtools.build.lib.actions.ActionResult;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
+import com.google.devtools.build.lib.analysis.actions.SymlinkAction;
+import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.collect.nestedset.Order;
+import com.google.devtools.build.lib.server.FailureDetails;
+import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
+import com.google.devtools.build.lib.server.FailureDetails.SymlinkAction.Code;
+import com.google.devtools.build.lib.util.DetailedExitCode;
+import com.google.devtools.build.lib.util.Fingerprint;
+import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import java.io.IOException;
+import javax.annotation.Nullable;
+
+/**
+ * Action to create a possibly unresolved symbolic link to a raw path.
+ *
+ * To create a symlink to a known-to-exist target with alias semantics similar to a true copy of the
+ * input, use {@link SymlinkAction} instead.
+ */
+public final class UnresolvedSymlinkAction extends AbstractAction {
+  private static final String GUID = "0f302651-602c-404b-881c-58913193cfe7";
+
+  private final String target;
+  private final String progressMessage;
+
+  public UnresolvedSymlinkAction(
+      ActionOwner owner,
+      Artifact primaryOutput,
+      String target,
+      String progressMessage) {
+    super(
+        owner,
+        NestedSetBuilder.emptySet(Order.STABLE_ORDER),
+        ImmutableSet.of(primaryOutput));
+    this.target = target;
+    this.progressMessage = progressMessage;
+  }
+
+  @Override
+  public ActionResult execute(ActionExecutionContext actionExecutionContext)
+      throws ActionExecutionException {
+
+    Path outputPath = actionExecutionContext.getInputPath(getPrimaryOutput());
+    try {
+      outputPath.createSymbolicLink(PathFragment.create(target));
+    } catch (IOException e) {
+      String message =
+          String.format(
+              "failed to create symbolic link '%s' to '%s' due to I/O error: %s",
+              getPrimaryOutput().getExecPathString(), target, e.getMessage());
+      DetailedExitCode code = createDetailedExitCode(message, Code.LINK_CREATION_IO_EXCEPTION);
+      throw new ActionExecutionException(message, e, this, false, code);
+    }
+
+    return ActionResult.EMPTY;
+  }
+
+  @Override
+  protected void computeKey(
+      ActionKeyContext actionKeyContext,
+      @Nullable ArtifactExpander artifactExpander,
+      Fingerprint fp) {
+    fp.addString(GUID);
+    fp.addString(target);
+  }
+
+  @Override
+  public String getMnemonic() {
+    return "UnresolvedSymlink";
+  }
+
+  @Override
+  protected String getRawProgressMessage() {
+    return progressMessage;
+  }
+
+  private static DetailedExitCode createDetailedExitCode(String message, Code detailedCode) {
+    return DetailedExitCode.of(
+        FailureDetail.newBuilder()
+            .setMessage(message)
+            .setSymlinkAction(FailureDetails.SymlinkAction.newBuilder().setCode(detailedCode))
+            .build());
+  }
+}

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

@@ -21,6 +21,7 @@
 
 import com.google.auto.value.AutoValue;
 import com.google.common.base.Preconditions;
+import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
@@ -548,8 +549,7 @@ public SandboxInputs processInputFiles(Map<PathFragment, ActionInput> inputMap,
             virtualInputsWithDelayedMaterialization.add((VirtualActionInput) actionInput);
           }
         } else if (actionInput.isSymlink()) {
-          Path inputPath = execRoot.getRelative(actionInput.getExecPath());
-          inputSymlinks.put(pathFragment, inputPath.readSymbolicLink());
+          stageSymlink(inputSymlinks, execRoot, actionInput, pathFragment);
         } else {
           Path inputPath = execRoot.getRelative(actionInput.getExecPath());
           inputFiles.put(pathFragment, inputPath);
@@ -563,8 +563,7 @@ public SandboxInputs processInputFiles(Map<PathFragment, ActionInput> inputMap,
         }
 
         if (actionInput.isSymlink()) {
-          Path inputPath = execRoot.getRelative(actionInput.getExecPath());
-          inputSymlinks.put(pathFragment, inputPath.readSymbolicLink());
+          stageSymlink(inputSymlinks, execRoot, actionInput, pathFragment);
         } else {
           Path inputPath =
               actionInput instanceof EmptyActionInput
@@ -581,6 +580,26 @@ public SandboxInputs processInputFiles(Map<PathFragment, ActionInput> inputMap,
         inputSymlinks);
   }
 
+  private void stageSymlink(Map<PathFragment, PathFragment> inputSymlinks, Path execRoot,
+      ActionInput symlink, PathFragment location) throws IOException {
+    // Symlinks may be relative and thus only resolve correctly from their original location under
+    // the exec root. Thus, always stage symlinks at their original location.
+    Path inputPath = execRoot.getRelative(symlink.getExecPath());
+    inputSymlinks.put(symlink.getExecPath(), inputPath.readSymbolicLink());
+    if (location.equals(symlink.getExecPath())) {
+      return;
+    }
+    // If the symlink has to be staged at a different location (e.g., in a runfiles tree),
+    // additionally emit a relative symlink pointing to the actual symlink. This closely mimics how
+    // the symlink would be staged for local execution.
+    // Note: It may seem simpler to use a symlink to the absolute path to the original symlink under
+    // the unsandboxed exec root, but that may result in the symlink resolving non-hermetically to
+    // undeclared input files.
+    int locationDepth = location.getParentDirectory().segmentCount();
+    PathFragment backToExecRoot = PathFragment.create(Strings.repeat("../", locationDepth));
+    inputSymlinks.put(location, backToExecRoot.getRelative(symlink.getExecPath()));
+  }
+
   /** The file and directory outputs of a sandboxed spawn. */
   @AutoValue
   public abstract static class SandboxOutputs {

src/test/shell/bazel/bazel_symlink_test.sh

@@ -411,11 +411,11 @@ EOF
   cat > a/BUILD <<'EOF'
 load(":a.bzl", "a")
 
-a(name="a", link_target="/somewhere/in/my/heart")
+a(name="a", link_target="../somewhere/in/my/heart")
 EOF
 
   bazel build --experimental_allow_unresolved_symlinks //a:a || fail "build failed"
-  assert_contains "input link is /somewhere/in/my/heart" bazel-bin/a/a.file
+  assert_contains "input link is ../somewhere/in/my/heart" bazel-bin/a/a.file
 }
 
 function test_symlink_file_to_file_created_from_symlink_action() {
@@ -731,4 +731,126 @@ EOF
   bazel --windows_enable_symlinks build --experimental_allow_unresolved_symlinks //a:exec || fail "build failed"
 }
 
+function test_unresolved_symlink_hermetic_in_sandbox() {
+  if "$is_windows"; then
+    # TODO(#10298): Support unresolved symlinks on Windows.
+    return 0
+  fi
+
+  mkdir -p pkg
+  cat > pkg/def.bzl <<'EOF'
+def _r(ctx):
+  symlink = ctx.actions.declare_symlink(ctx.label.name + "_s")
+  ctx.actions.symlink(output=symlink, target_path=ctx.file.file.basename)
+
+  output = ctx.actions.declare_file(ctx.label.name)
+  ctx.actions.run_shell(
+    command = "{ cat %s || true; } >  %s" % (symlink.path, output.path),
+    inputs = [symlink] + ([ctx.file.file] if ctx.attr.stage_target else []),
+    outputs = [output],
+  )
+  return DefaultInfo(files=depset([output]))
+
+r = rule(
+    implementation=_r,
+    attrs = {
+        "file": attr.label(allow_single_file=True),
+        "stage_target": attr.bool(),
+    }
+)
+EOF
+  cat > pkg/BUILD <<'EOF'
+load(":def.bzl", "r")
+
+genrule(name = "a", outs = ["file"], cmd = "echo hello >$@")
+r(name="b", file="file")
+r(name="c", file="file", stage_target=True)
+EOF
+
+  bazel build //pkg:b --experimental_allow_unresolved_symlinks --spawn_strategy=local
+  [ -s bazel-bin/pkg/b ] && fail "symlink should not resolve"
+
+  bazel clean
+  bazel build //pkg:a --spawn_strategy=sandboxed
+  bazel build //pkg:b --experimental_allow_unresolved_symlinks --spawn_strategy=local
+  [ -s bazel-bin/pkg/b ] || fail "symlink should resolve"
+
+  bazel clean
+  bazel build //pkg:c --experimental_allow_unresolved_symlinks --spawn_strategy=local
+  [ -s bazel-bin/pkg/c ] || fail "symlink should resolve"
+
+  bazel clean
+  bazel build //pkg:b --experimental_allow_unresolved_symlinks --spawn_strategy=sandboxed
+  [ -s bazel-bin/pkg/b ] && fail "sandboxed build is not hermetic"
+
+  bazel clean
+  bazel build //pkg:a --spawn_strategy=sandboxed
+  bazel build //pkg:b --experimental_allow_unresolved_symlinks --spawn_strategy=sandboxed
+  [ -s bazel-bin/pkg/b ] && fail "sandboxed build is not hermetic"
+
+  bazel clean
+  bazel build //pkg:c --experimental_allow_unresolved_symlinks --spawn_strategy=sandboxed
+  [ -s bazel-bin/pkg/c ] || fail "symlink should resolve"
+}
+
+
+function test_unresolved_symlink_in_runfiles() {
+  if "$is_windows"; then
+    # TODO(#10298): Support unresolved symlinks on Windows.
+    return 0
+  fi
+
+  mkdir -p pkg
+  cat > pkg/def.bzl <<'EOF'
+def _r(ctx):
+  symlink = ctx.actions.declare_symlink(ctx.label.name + "_s")
+  target = ctx.file.file.basename
+  ctx.actions.symlink(output=symlink, target_path=target)
+
+  script = ctx.actions.declare_file(ctx.label.name)
+  content = """
+#!/usr/bin/env bash
+cd $0.runfiles/{workspace_name}
+[ -L {symlink} ] || {{ echo "runfile is not a symlink"; exit 1; }}
+[ $(cd $(dirname {symlink}) && readlink $(readlink $(basename {symlink}))) == "{target}" ] || {{ echo "runfile symlink does not point to a symlink pointing to the expected target"; exit 1; }}
+[ -s {symlink} ] || {{ echo "runfile not resolved"; exit 1; }}
+""".format(
+    symlink = symlink.short_path,
+    target = target,
+    workspace_name = ctx.workspace_name,
+  )
+  ctx.actions.write(script, content, is_executable=True)
+  return DefaultInfo(executable=script, runfiles=ctx.runfiles(files = [symlink]))
+
+r = rule(implementation=_r, attrs={"file":attr.label(allow_single_file=True)}, executable = True)
+EOF
+  cat > pkg/BUILD <<'EOF'
+load(":def.bzl", "r")
+
+genrule(name="a", outs=["file"], cmd="echo hello >$@")
+r(name="tool", file="file")
+genrule(
+    name = "use_tool",
+    outs = ["out"],
+    cmd = "$(location :tool) && touch $@",
+    tools = [":tool", "file"],
+)
+genrule(
+    name = "use_tool_non_hermetically",
+    outs = ["out_non_hermetic"],
+    cmd = "$(location :tool) && touch $@",
+    tools = [":tool"],
+)
+EOF
+
+  bazel build --experimental_allow_unresolved_symlinks --spawn_strategy=local //pkg:use_tool || fail "local build failed"
+  # Keep the implicitly built //pkg:a around to make the symlink resolve.
+  bazel build --experimental_allow_unresolved_symlinks --spawn_strategy=local //pkg:use_tool_non_hermetically || fail "local build failed"
+
+  bazel clean
+  bazel build --experimental_allow_unresolved_symlinks --spawn_strategy=sandboxed //pkg:use_tool || fail "sandboxed build failed"
+  # Keep the implicitly built //pkg:a around to make the symlink resolve outside the sandbox.
+  bazel build --experimental_allow_unresolved_symlinks --spawn_strategy=sandboxed //pkg:use_tool_non_hermetically && fail "sandboxed build is not hermetic" || true
+}
+
 run_suite "Tests for symlink artifacts"