Do not force unresolved symlinks to be absolute

The documentation of `ctx.actions.symlink(target_path = ...)` states
that the given path is used as the symlink target without any changes,
but in reality this path has so far always been converted into an
absolute path by prepending the exec root. This is changed by this
commit, which gets very close to the documented behavior by preserving
the target path as is except for the standard normalization applied by
`PathFragment`. Improving the situation even further would require
modifying or adding to Bazel's core file system API, which may be done
in a follow-up PR.

Since relative symlinks only resolve correctly at the location they were
originally created, they have to be handled specially when staged as
runfiles. This commit adds a new `declared_symlinks` parameter to the
rule context's `runfiles` method that takes in symlink artifacts
declared via `ctx.actions.declare_symlink`. These symlinks are staged at
their runfiles path directly, with no further processing of their target
and without any intermediate runfiles pointing back to the artifact's
location under the exec root. This has to main benefits:
* With local execution, symlinks are resolved within the runfiles tree,
  which is more hermetic than following the runfiles symlink back into
  the exec root and resolving the symlink artifact there.
* Actions can expect symlink artifacts to be stages as is without
  intermediate symlinks with local and sandboxed execution, both inside
  and outside the runfiles tree. This is important for packaging actions
  as well as rulesets sensitive to symlinks (e.g. rules_js).

As a side-effect of the switch to relative symlinks, this commit
resolves a non-hermeticity issue observed in
bazelbuild#10298 (comment)
Integration tests are added to verify that symlinks staged in the
sandbox are no longer resolved non-hermetically.

Fixes bazelbuild#14224

scripts/bootstrap/compile.sh

@@ -335,8 +335,9 @@ if [ "${PLATFORM}" = "windows" ]; then
   # We don't rely on runfiles trees on Windows
   cat <<'EOF' >${ARCHIVE_DIR}/build-runfiles${EXE_EXT}
 #!/bin/sh
-mkdir -p $2
-cp $1 $2/MANIFEST
+# Skip over --allow_relative.
+mkdir -p $3
+cp $2 $3/MANIFEST
 EOF
 else
   cat <<'EOF' >${ARCHIVE_DIR}/build-runfiles${EXE_EXT}
@@ -350,8 +351,9 @@ else
 # bootstrap version of Bazel, but we'd still need a shell wrapper around it, so
 # it's not clear whether that would be a win over a few lines of Lovecraftian
 # code)
-MANIFEST="$1"
-TREE="$2"
+# Skip over --allow_relative.
+MANIFEST="$2"
+TREE="$3"
 
 rm -fr "$TREE"
 mkdir -p "$TREE"

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/Runfiles.java

@@ -172,6 +172,8 @@ public void repr(Printer printer) {
    */
   private final NestedSet<Artifact> artifacts;
 
+  private final NestedSet<Artifact> symlinkArtifacts;
+
   /**
    * A map of symlinks that should be present in the runfiles directory. In general, the symlink can
    * be determined from the artifact by using the output-dir-relative path, so this should only be
@@ -235,6 +237,7 @@ public enum ConflictPolicy {
   private Runfiles(
       PathFragment suffix,
       NestedSet<Artifact> artifacts,
+      NestedSet<Artifact> symlinkArtifacts,
       NestedSet<SymlinkEntry> symlinks,
       NestedSet<SymlinkEntry> rootSymlinks,
       NestedSet<Artifact> extraMiddlemen,
@@ -243,6 +246,7 @@ private Runfiles(
       boolean legacyExternalRunfiles) {
     this.suffix = suffix;
     this.artifacts = Preconditions.checkNotNull(artifacts);
+    this.symlinkArtifacts = Preconditions.checkNotNull(symlinkArtifacts);
     this.symlinks = Preconditions.checkNotNull(symlinks);
     this.rootSymlinks = Preconditions.checkNotNull(rootSymlinks);
     this.extraMiddlemen = Preconditions.checkNotNull(extraMiddlemen);
@@ -277,6 +281,15 @@ public NestedSet<Artifact> getArtifacts() {
     return artifacts;
   }
 
+  @Override
+  public Depset getSymlinkArtifactsForStarlark() {
+    return Depset.of(Artifact.TYPE, symlinkArtifacts);
+  }
+
+  public NestedSet<Artifact> getSymlinkArtifacts() {
+    return symlinkArtifacts;
+  }
+
   /** Returns the symlinks. */
   @Override
   public Depset /*<SymlinkEntry>*/ getSymlinksForStarlark() {
@@ -395,8 +408,21 @@ public Map<PathFragment, Artifact> getRunfilesInputs(
     Map<PathFragment, Artifact> manifest = getSymlinksAsMap(checker);
     // Add artifacts (committed to inclusion on construction of runfiles).
     for (Artifact artifact : artifacts.toList()) {
+      if (artifact.isSymlink()) {
+        eventHandler.handle(Event.of(EventKind.ERROR,
+            location,
+            String.format("symlink artifact unexpected in artifacts: %s", artifact)));
+      }
       checker.put(manifest, artifact.getRunfilesPath(), artifact);
     }
+    for (Artifact symlinkArtifact : symlinkArtifacts.toList()) {
+      if (!symlinkArtifact.isSymlink()) {
+        eventHandler.handle(Event.of(EventKind.ERROR, location,
+            String.format("non-symlink artifact unexpected in symlinkArtifacts: %s",
+                symlinkArtifact)));
+      }
+      checker.put(manifest, symlinkArtifact.getRunfilesPath(), symlinkArtifact);
+    }
 
     manifest = filterListForObscuringSymlinks(eventHandler, location, manifest);
 
@@ -556,6 +582,7 @@ public NestedSet<Artifact> getAllArtifacts() {
     NestedSetBuilder<Artifact> allArtifacts = NestedSetBuilder.stableOrder();
     allArtifacts
         .addTransitive(artifacts)
+        .addTransitive(symlinkArtifacts)
         .addAll(Iterables.transform(symlinks.toList(), SymlinkEntry::getArtifact))
         .addAll(Iterables.transform(rootSymlinks.toList(), SymlinkEntry::getArtifact));
     return allArtifacts.build();
@@ -566,6 +593,7 @@ public NestedSet<Artifact> getAllArtifacts() {
    */
   public boolean isEmpty() {
     return artifacts.isEmpty()
+        && symlinkArtifacts.isEmpty()
         && symlinks.isEmpty()
         && rootSymlinks.isEmpty()
         && extraMiddlemen.isEmpty();
@@ -677,6 +705,7 @@ public static final class Builder {
      * entries with later ones, so we want a post-order iteration.
      */
     private final NestedSetBuilder<Artifact> artifactsBuilder = NestedSetBuilder.compileOrder();
+    private final NestedSetBuilder<Artifact> symlinkArtifactsBuilder = NestedSetBuilder.compileOrder();
 
     private final NestedSetBuilder<SymlinkEntry> symlinksBuilder = NestedSetBuilder.stableOrder();
     private final NestedSetBuilder<SymlinkEntry> rootSymlinksBuilder =
@@ -736,6 +765,7 @@ public Runfiles build() {
       return new Runfiles(
           suffix,
           artifactsBuilder.build(),
+          symlinkArtifactsBuilder.build(),
           symlinksBuilder.build(),
           rootSymlinksBuilder.build(),
           extraMiddlemenBuilder.build(),
@@ -750,6 +780,8 @@ public Builder addArtifact(Artifact artifact) {
       Preconditions.checkNotNull(artifact);
       Preconditions.checkArgument(
           !artifact.isMiddlemanArtifact(), "unexpected middleman artifact: %s", artifact);
+      Preconditions.checkArgument(
+          !artifact.isSymlink(), "unexpected symlink artifact: %s", artifact);
       artifactsBuilder.add(artifact);
       return this;
     }
@@ -785,6 +817,26 @@ public Builder addTransitiveArtifactsWrappedInStableOrder(NestedSet<Artifact> ar
       return this;
     }
 
+    @CanIgnoreReturnValue
+    public Builder addSymlinkArtifact(Artifact symlinkArtifact) {
+      Preconditions.checkNotNull(symlinkArtifact);
+      Preconditions.checkArgument(
+          symlinkArtifact.isSymlink(), "expected symlink artifact, got: %s", symlinkArtifact);
+      symlinkArtifactsBuilder.add(symlinkArtifact);
+      return this;
+    }
+
+    /**
+     * Adds several symlink artifacts to the internal collection.
+     */
+    @CanIgnoreReturnValue
+    public Builder addSymlinkArtifacts(Iterable<Artifact> symlinkArtifacts) {
+      for (Artifact artifact : symlinkArtifacts) {
+        addSymlinkArtifact(artifact);
+      }
+      return this;
+    }
+
     /** Adds a symlink. */
     @CanIgnoreReturnValue
     public Builder addSymlink(PathFragment link, Artifact target) {
@@ -875,6 +927,7 @@ private Builder merge(Runfiles runfiles, boolean includeArtifacts) {
           suffix.equals(runfiles.suffix), "%s != %s", suffix, runfiles.suffix);
       if (includeArtifacts) {
         artifactsBuilder.addTransitive(runfiles.getArtifacts());
+        symlinkArtifactsBuilder.addTransitive(runfiles.getSymlinkArtifacts());
       }
       symlinksBuilder.addTransitive(runfiles.getSymlinks());
       rootSymlinksBuilder.addTransitive(runfiles.getRootSymlinks());
@@ -1143,11 +1196,18 @@ public void fingerprint(Fingerprint fp) {
       fp.addPath(rootSymlink.getValue().getExecPath());
     }
 
+    fp.addInt(artifacts.toList().size());
     for (Artifact artifact : artifacts.toList()) {
       fp.addPath(artifact.getRunfilesPath());
       fp.addPath(artifact.getExecPath());
     }
 
+    fp.addInt(symlinkArtifacts.toList().size());
+    for (Artifact symlinkArtifact : symlinkArtifacts.toList()) {
+      fp.addPath(symlinkArtifact.getRunfilesPath());
+      // Symlink targets are tracked as inputs of actions using this method.
+    }
+
     for (String name : getEmptyFilenames().toList()) {
       fp.addString(name);
     }
@@ -1180,6 +1240,11 @@ public String describeFingerprint() {
               "artifact: '%s' '%s'\n", artifact.getRunfilesPath(), artifact.getExecPath()));
     }
 
+    for (Artifact symlinkArtifact : artifacts.toList()) {
+      sb.append(
+          String.format("symlinkArtifact: '%s'\n", symlinkArtifact.getRunfilesPath()));
+    }
+
     for (String name : getEmptyFilenames().toList()) {
       sb.append(String.format("emptyFilename: '%s'\n", name));
     }

src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java

@@ -23,8 +23,6 @@
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.actions.AbstractFileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.DeterministicWriter;
-import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
-import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.util.Fingerprint;
@@ -129,7 +127,7 @@ public SourceManifestAction(
       Artifact primaryOutput,
       Runfiles runfiles,
       boolean remotableSourceManifestActions) {
-    super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), primaryOutput, false);
+    super(owner, runfiles.getSymlinkArtifacts(), primaryOutput, false);
     this.manifestWriter = manifestWriter;
     this.runfiles = runfiles;
     this.remotableSourceManifestActions = remotableSourceManifestActions;
@@ -227,7 +225,11 @@ public void writeEntry(Writer manifestWriter, PathFragment rootRelativePath, Art
         // This trailing whitespace is REQUIRED to process the single entry line correctly.
         manifestWriter.append(' ');
         if (symlink != null) {
-          manifestWriter.append(symlink.getPath().getPathString());
+          if (symlink.isSymlink()) {
+            manifestWriter.append(symlink.getPath().readSymbolicLink().getPathString());
+          } else {
+            manifestWriter.append(symlink.getPath().getPathString());
+          }
         }
         manifestWriter.append('\n');
       }

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/StarlarkRuleContext.java

@@ -912,6 +912,7 @@ public String expandLocation(String input, Sequence<?> targets, StarlarkThread t
   public Runfiles runfiles(
       Sequence<?> files,
       Object transitiveFiles,
+      Sequence<?> declaredSymlinks,
       Boolean collectData,
       Boolean collectDefault,
       Object symlinks,
@@ -929,7 +930,15 @@ public Runfiles runfiles(
       builder.addRunfiles(ruleContext, RunfilesProvider.DEFAULT_RUNFILES);
     }
     if (!files.isEmpty()) {
-      builder.addArtifacts(Sequence.cast(files, Artifact.class, "files"));
+      Sequence<Artifact> artifacts = Sequence.cast(files, Artifact.class, "files");
+      for (Artifact artifact : artifacts) {
+        if (artifact.isSymlink()) {
+          throw Starlark.errorf(
+              "declared symlink '%s' is invalid for files, add via declared_symlinks instead",
+              artifact);
+        }
+      }
+      builder.addArtifacts(artifacts);
     }
     if (transitiveFiles != Starlark.NONE) {
       NestedSet<Artifact> transitiveArtifacts =
@@ -943,6 +952,22 @@ public Runfiles runfiles(
       }
       builder.addTransitiveArtifacts(transitiveArtifacts);
     }
+    if (!declaredSymlinks.isEmpty()) {
+      if (!ruleContext.getConfiguration().allowUnresolvedSymlinks()) {
+        throw Starlark.errorf(
+            "declared_symlink is not allowed; "
+                + "use the --experimental_allow_unresolved_symlinks command line option");
+      }
+      Sequence<Artifact> artifacts = Sequence.cast(declaredSymlinks, Artifact.class, "declared_symlinks");
+      for (Artifact artifact : artifacts) {
+        if (!artifact.isSymlink()) {
+          throw Starlark.errorf(
+              "file '%s' is invalid for declared_symlinks, add via files instead",
+              artifact);
+        }
+      }
+      builder.addSymlinkArtifacts(artifacts);
+    }
     if (isDepset(symlinks)) {
       builder.addSymlinks(((Depset) symlinks).getSet(SymlinkEntry.class));
     } else if (isNonEmptyDict(symlinks)) {