Create and stage symlink artifacts with unmodified target path

Unresolved symlink artifacts created with `ctx.actions.symlink(target_path = ...)` are now created without making the target path absolute by prepending the exec root, which diverged from the documentation and intended goal and also gave rise to hermeticity issues as such symlinks would regularly resolve outside the sandbox. Furthermore, in order to bring local execution in line with other execution types, the runfiles manifest entry (and thus the runfiles directory contents) for symlink artifacts are now their target paths rather than their exec paths, which along the way resolves another soure of non-hermetic resolution outside the runfiles directory. This requires making symlink artifacts (but not artifacts representing regular files) inputs of `SourceManifestAction`, which previously had no inputs. The change flattens a nested set in `getInputs`, but benchmarks confirm that this does not result in a performance regression. Alternatives considered either result in a substantially worse Starlark API or wouldn't work for symlinks created by spawns. Work towards #10298 Closes #16272. PiperOrigin-RevId: 474784371 Change-Id: I15d318c30542c1da54d86d9b1ae769fe2a0ec970
bazelbuild · Sep 16, 2022 · d834905 · d834905
1 parent 720dc5f
commit d834905
Show file tree

Hide file tree

Showing 13 changed files with 574 additions and 29 deletions.
diff --git a/scripts/bootstrap/compile.sh b/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"

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD
@@ -248,6 +248,7 @@ java_library(
         "starlark/StarlarkRuleConfiguredTargetUtil.java",
         "starlark/StarlarkRuleContext.java",
         "starlark/StarlarkRuleTransitionProvider.java",
+        "starlark/UnresolvedSymlinkAction.java",
         "test/AnalysisTestActionBuilder.java",
         "test/BaselineCoverageAction.java",
         "test/CoverageCommon.java",

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java b/src/main/java/com/google/devtools/build/lib/analysis/SourceManifestAction.java
@@ -13,16 +13,21 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis;
 
+import static com.google.common.collect.ImmutableList.toImmutableList;
 import static java.nio.charset.StandardCharsets.ISO_8859_1;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.actions.AbstractAction;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.ActionOwner;
 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.analysis.starlark.UnresolvedSymlinkAction;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 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;
@@ -99,6 +104,8 @@ void writeEntry(
 
   private final boolean remotableSourceManifestActions;
 
+  private NestedSet<Artifact> symlinkArtifacts = null;
+
   /**
    * Creates a new AbstractSourceManifestAction instance using latin1 encoding to write the manifest
    * file and with a specified root path for manifest entries.
@@ -129,12 +136,47 @@ public SourceManifestAction(
       Artifact primaryOutput,
       Runfiles runfiles,
       boolean remotableSourceManifestActions) {
+    // The real set of inputs is computed in #getInputs().
     super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), primaryOutput, false);
     this.manifestWriter = manifestWriter;
     this.runfiles = runfiles;
     this.remotableSourceManifestActions = remotableSourceManifestActions;
   }
 
+  /**
+   * The manifest entry for a symlink artifact should contain the target of the symlink rather than
+   * its exec path. Reading the symlink target requires that the symlink artifact is declared as an
+   * input of this action. Since declaring all runfiles as inputs of the manifest action would
+   * unnecessarily delay its execution, this action exceptionally overrides {@link
+   * AbstractAction#getInputs()} and filters out the non-symlink runfiles by flattening the nested
+   * set of runfiles. Benchmarks confirmed that this does not regress performance.
+   *
+   * <p>Alternatives considered:
+   *
+   * <ul>
+   *   <li>Having users separate normal artifacts from symlink artifacts during analysis: Makes it
+   *       impossible to pass symlink artifacts to rules that aren't aware of them and requires the
+   *       use of custom providers to pass symlinks to stage as inputs to actions.
+   *   <li>Reaching into {@link ActionExecutionContext} to look up the generating action of symlink
+   *       artifacts and retrieving the target from {@link UnresolvedSymlinkAction}: This would not
+   *       work for symlinks whose target is determined in the execution phase.
+   *   <li>Input discovery: Complex and error-prone in general and conceptually not necessary here -
+   *       we already know what the inputs will be during analysis, we just want to delay the
+   *       required computations.
+   * </ul>
+   */
+  @Override
+  public synchronized NestedSet<Artifact> getInputs() {
+    if (symlinkArtifacts == null) {
+      ImmutableList<Artifact> symlinks =
+          runfiles.getArtifacts().toList().stream()
+              .filter(Artifact::isSymlink)
+              .collect(toImmutableList());
+      symlinkArtifacts = NestedSetBuilder.wrap(Order.STABLE_ORDER, symlinks);
+    }
+    return symlinkArtifacts;
+  }
+
   @VisibleForTesting
   public void writeOutputFile(OutputStream out, EventHandler eventHandler) throws IOException {
     writeFile(out, runfiles.getRunfilesInputs(eventHandler, getOwner().getLocation()));
@@ -227,7 +269,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');
       }

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkAction.java b/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

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkActionFactory.java
@@ -274,7 +274,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()) {
@@ -328,11 +328,8 @@ public void symlink(
       }
 
       action =
-          SymlinkAction.createUnresolved(
-              ruleContext.getActionOwner(),
-              outputArtifact,
-              PathFragment.create((String) targetPath),
-              progressMessage);
+          UnresolvedSymlinkAction.create(
+              ruleContext.getActionOwner(), outputArtifact, (String) targetPath, progressMessage);
     }
     registerAction(action);
   }

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/UnresolvedSymlinkAction.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/UnresolvedSymlinkAction.java
@@ -0,0 +1,113 @@
+// 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.base.Preconditions;
+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.
+ *
+ * <p>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 PathFragment target;
+  private final String progressMessage;
+
+  private UnresolvedSymlinkAction(
+      ActionOwner owner, Artifact primaryOutput, String target, String progressMessage) {
+    super(owner, NestedSetBuilder.emptySet(Order.STABLE_ORDER), ImmutableSet.of(primaryOutput));
+    // TODO: PathFragment#create normalizes the symlink target, which may change how it resolves
+    //  when combined with directory symlinks. Ideally, Bazel's file system abstraction would
+    //  offer a way to create symlinks without any preprocessing of the target.
+    this.target = PathFragment.create(target);
+    this.progressMessage = progressMessage;
+  }
+
+  public static UnresolvedSymlinkAction create(
+      ActionOwner owner, Artifact primaryOutput, String target, String progressMessage) {
+    Preconditions.checkArgument(primaryOutput.isSymlink());
+    return new UnresolvedSymlinkAction(owner, primaryOutput, target, progressMessage);
+  }
+
+  @Override
+  public ActionResult execute(ActionExecutionContext actionExecutionContext)
+      throws ActionExecutionException {
+
+    Path outputPath = actionExecutionContext.getInputPath(getPrimaryOutput());
+    try {
+      outputPath.createSymbolicLink(target);
+    } catch (IOException e) {
+      String message =
+          String.format(
+              "failed to create symbolic link '%s' with target '%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.addPath(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());
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
@@ -180,8 +180,8 @@ Command createCommand(Path execRoot, BinTools binTools, Map<String, String> shel
     Preconditions.checkNotNull(shellEnvironment);
     List<String> args = Lists.newArrayList();
     args.add(binTools.getEmbeddedPath(BUILD_RUNFILES).asFragment().getPathString());
+    args.add("--allow_relative");
     if (filesetTree) {
-      args.add("--allow_relative");
       args.add("--use_metadata");
     }
     args.add(inputManifest.relativeTo(execRoot).getPathString());

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BUILD b/src/test/java/com/google/devtools/build/lib/analysis/BUILD
@@ -325,6 +325,7 @@ java_test(
         "//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/analysis:analysis_cluster",
+        "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/SourceManifestActionTest.java
@@ -19,11 +19,14 @@
 
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
+import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.actions.ArtifactRoot.RootType;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.analysis.SourceManifestAction.ManifestType;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -53,6 +56,8 @@ public final class SourceManifestActionTest extends BuildViewTestCase {
   private Artifact pythonSourceFile;
   private Path buildFilePath;
   private Artifact buildFile;
+  private Artifact relativeSymlink;
+  private Artifact absoluteSymlink;
 
   private Path manifestOutputPath;
   private Artifact manifestOutputFile;
@@ -75,8 +80,26 @@ public final void createFiles() throws Exception  {
     fakeManifest.put(pythonSourcePath.relativeTo(rootDirectory), pythonSourceFile);
     ArtifactRoot outputDir =
         ArtifactRoot.asDerivedRoot(rootDirectory, RootType.Output, "blaze-output");
+    outputDir.getRoot().asPath().createDirectoryAndParents();
     manifestOutputPath = rootDirectory.getRelative("blaze-output/trivial.runfiles_manifest");
     manifestOutputFile = ActionsTestUtil.createArtifact(outputDir, manifestOutputPath);
+
+    Path relativeSymlinkPath = outputDir.getRoot().asPath().getChild("relative_symlink");
+    relativeSymlinkPath.createSymbolicLink(PathFragment.create("../some/relative/path"));
+    relativeSymlink =
+        SpecialArtifact.create(
+            outputDir,
+            outputDir.getExecPath().getChild("relative_symlink"),
+            ActionsTestUtil.NULL_ARTIFACT_OWNER,
+            SpecialArtifactType.UNRESOLVED_SYMLINK);
+    Path absoluteSymlinkPath = outputDir.getRoot().asPath().getChild("absolute_symlink");
+    absoluteSymlinkPath.createSymbolicLink(PathFragment.create("/absolute/path"));
+    absoluteSymlink =
+        SpecialArtifact.create(
+            outputDir,
+            outputDir.getExecPath().getChild("absolute_symlink"),
+            ActionsTestUtil.NULL_ARTIFACT_OWNER,
+            SpecialArtifactType.UNRESOLVED_SYMLINK);
   }
 
   private SourceManifestAction createSymlinkAction() {
@@ -299,6 +322,35 @@ public void testEmptyFilesAffectKey() throws Exception {
     assertThat(computeKey(action2)).isNotEqualTo(computeKey(action1));
   }
 
+  @Test
+  public void testUnresolvedSymlink() throws Exception {
+    Artifact manifest = getBinArtifactWithNoOwner("manifest1");
+
+    SourceManifestAction action =
+        new SourceManifestAction(
+            ManifestType.SOURCE_SYMLINKS,
+            NULL_ACTION_OWNER,
+            manifest,
+            new Runfiles.Builder("TESTING", false)
+                .addArtifact(absoluteSymlink)
+                .addArtifact(buildFile)
+                .addArtifact(relativeSymlink)
+                .build());
+
+    NestedSet<Artifact> inputs = action.getInputs();
+    assertThat(inputs.toList()).containsExactly(absoluteSymlink, relativeSymlink);
+
+    // Verify that the return value of getInputs is cached.
+    assertThat(inputs).isEqualTo(action.getInputs());
+    assertThat(inputs.toList()).isEqualTo(action.getInputs().toList());
+
+    assertThat(action.getFileContentsAsString(reporter))
+        .isEqualTo(
+            "TESTING/BUILD /workspace/trivial/BUILD\n"
+                + "TESTING/absolute_symlink /absolute/path\n"
+                + "TESTING/relative_symlink ../some/relative/path\n");
+  }
+
   private String computeKey(SourceManifestAction action) {
     Fingerprint fp = new Fingerprint();
     action.computeKey(actionKeyContext, /*artifactExpander=*/ null, fp);