Upload dangling symlinks found in the outputs of local actions.

Since we don't know whether a dangling symlink is supposed to point to a file or a directory, we always report it as a symlink to a file. The distinction only matters in V2 of the protocol, with V2.1 reporting them uniformly, so it doesn't seem to be worth the effort to match up the input and output types.

Dangling symlinks to absolute paths are unconditionally uploaded. A followup PR will gate their upload on the presence of the respective server capability (SymlinkAbsolutePathStrategy.ALLOW).

The change is behind a new --incompatible_remote_dangling_symlinks flag, which will be introduced in the flipped state. See #16353.

Progress towards #16289.

Closes #16352.

PiperOrigin-RevId: 477735370
Change-Id: I6210d152529e4603a49377a9e72e8e116ced6b85

Author: tjgq

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

@@ -115,6 +115,7 @@
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.Symlinks;
 import com.google.protobuf.ByteString;
 import com.google.protobuf.ExtensionRegistry;
 import com.google.protobuf.Message;
@@ -1257,8 +1258,10 @@ private Single<UploadManifest> buildUploadManifestAsync(
           ImmutableList.Builder<Path> outputFiles = ImmutableList.builder();
           // Check that all mandatory outputs are created.
           for (ActionInput outputFile : action.getSpawn().getOutputFiles()) {
+            Symlinks followSymlinks = outputFile.isSymlink() ? Symlinks.NOFOLLOW : Symlinks.FOLLOW;
             Path localPath = execRoot.getRelative(outputFile.getExecPath());
-            if (action.getSpawn().isMandatoryOutput(outputFile) && !localPath.exists()) {
+            if (action.getSpawn().isMandatoryOutput(outputFile)
+                && !localPath.exists(followSymlinks)) {
               throw new IOException(
                   "Expected output " + prettyPrint(outputFile) + " was not created locally.");
             }

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

@@ -76,6 +76,7 @@ public class UploadManifest {
   private final RemotePathResolver remotePathResolver;
   private final ActionResult.Builder result;
   private final boolean followSymlinks;
+  private final boolean allowDanglingSymlinks;
   private final Map<Digest, Path> digestToFile = new HashMap<>();
   private final Map<Digest, ByteString> digestToBlobs = new HashMap<>();
   @Nullable private ActionKey actionKey;
@@ -103,7 +104,8 @@ public static UploadManifest create(
             digestUtil,
             remotePathResolver,
             result,
-            /* followSymlinks= */ !remoteOptions.incompatibleRemoteSymlinks);
+            /* followSymlinks= */ !remoteOptions.incompatibleRemoteSymlinks,
+            /* allowDanglingSymlinks= */ remoteOptions.incompatibleRemoteDanglingSymlinks);
     manifest.addFiles(outputFiles);
     manifest.setStdoutStderr(outErr);
     manifest.addAction(actionKey, action, command);
@@ -140,11 +142,13 @@ public UploadManifest(
       DigestUtil digestUtil,
       RemotePathResolver remotePathResolver,
       ActionResult.Builder result,
-      boolean followSymlinks) {
+      boolean followSymlinks,
+      boolean allowDanglingSymlinks) {
     this.digestUtil = digestUtil;
     this.remotePathResolver = remotePathResolver;
     this.result = result;
     this.followSymlinks = followSymlinks;
+    this.allowDanglingSymlinks = allowDanglingSymlinks;
   }
 
   private void setStdoutStderr(FileOutErr outErr) throws IOException {
@@ -186,25 +190,32 @@ void addFiles(Collection<Path> files) throws ExecException, IOException {
         // Need to resolve the symbolic link to know what to add, file or directory.
         FileStatus statFollow = file.statIfFound(Symlinks.FOLLOW);
         if (statFollow == null) {
-          throw new IOException(
-              String.format("Action output %s is a dangling symbolic link to %s ", file, target));
-        }
-        if (statFollow.isSpecialFile()) {
-          illegalOutput(file);
-        }
-        Preconditions.checkState(
-            statFollow.isFile() || statFollow.isDirectory(), "Unknown stat type for %s", file);
-        if (!followSymlinks && !target.isAbsolute()) {
-          if (statFollow.isFile()) {
+          if (allowDanglingSymlinks) {
+            // Report symlink to a file since we don't know any better.
+            // TODO(tjgq): Check for the SymlinkAbsolutePathStrategy.ALLOW server capability before
+            // uploading an absolute symlink.
             addFileSymbolicLink(file, target);
           } else {
-            addDirectorySymbolicLink(file, target);
+            throw new IOException(
+                String.format("Action output %s is a dangling symbolic link to %s ", file, target));
           }
+        } else if (statFollow.isSpecialFile()) {
+          illegalOutput(file);
         } else {
-          if (statFollow.isFile()) {
-            addFile(digestUtil.compute(file), file);
+          Preconditions.checkState(
+              statFollow.isFile() || statFollow.isDirectory(), "Unknown stat type for %s", file);
+          if (!followSymlinks && !target.isAbsolute()) {
+            if (statFollow.isFile()) {
+              addFileSymbolicLink(file, target);
+            } else {
+              addDirectorySymbolicLink(file, target);
+            }
           } else {
-            addDirectory(file);
+            if (statFollow.isFile()) {
+              addFile(digestUtil.compute(file), file);
+            } else {
+              addDirectory(file);
+            }
           }
         }
       } else {

src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java

@@ -412,6 +412,18 @@ public RemoteBuildEventUploadModeConverter() {
               + " represented as files or directories. See #6631 for details.")
   public boolean incompatibleRemoteSymlinks;
 
+  @Option(
+      name = "incompatible_remote_dangling_symlinks",
+      defaultValue = "true",
+      category = "remote",
+      documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY,
+      effectTags = {OptionEffectTag.EXECUTION},
+      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+      help =
+          "If set to true and --incompatible_remote_symlinks is also true, symlinks in action"
+              + " outputs are allowed to dangle.")
+  public boolean incompatibleRemoteDanglingSymlinks;
+
   @Option(
       name = "experimental_remote_cache_compression",
       defaultValue = "false",

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

@@ -267,6 +267,12 @@ public static SpecialArtifact createTreeArtifactWithGeneratingAction(
     return treeArtifact;
   }
 
+  public static SpecialArtifact createUnresolvedSymlinkArtifactWithExecPath(
+      ArtifactRoot root, PathFragment execPath) {
+    return SpecialArtifact.create(
+        root, execPath, 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/RemoteExecutionServiceTest.java

@@ -1367,6 +1367,35 @@ public void uploadOutputs_uploadNestedDirectory_works() throws Exception {
         .isEmpty();
   }
 
+  @Test
+  public void uploadOutputs_uploadRelativeDanglingSymlink_works() throws Exception {
+    // arrange
+    Path linkPath = execRoot.getRelative("outputs/link");
+    linkPath.createSymbolicLink(PathFragment.create("some/path"));
+    Artifact outputSymlink =
+        ActionsTestUtil.createUnresolvedSymlinkArtifactWithExecPath(
+            artifactRoot, linkPath.relativeTo(execRoot));
+    RemoteExecutionService service = newRemoteExecutionService();
+    Spawn spawn = newSpawn(ImmutableMap.of(), ImmutableSet.of(outputSymlink));
+    FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+    RemoteAction action = service.buildRemoteAction(spawn, context);
+    SpawnResult spawnResult =
+        new SpawnResult.Builder()
+            .setExitCode(0)
+            .setStatus(SpawnResult.Status.SUCCESS)
+            .setRunnerName("test")
+            .build();
+
+    // act
+    UploadManifest manifest = service.buildUploadManifest(action, spawnResult);
+    service.uploadOutputs(action, spawnResult);
+
+    // assert
+    ActionResult.Builder expectedResult = ActionResult.newBuilder();
+    expectedResult.addOutputFileSymlinksBuilder().setPath("outputs/link").setTarget("some/path");
+    assertThat(manifest.getActionResult()).isEqualTo(expectedResult.build());
+  }
+
   @Test
   public void uploadOutputs_emptyOutputs_doNotPerformUpload() throws Exception {
     // Test that uploading an empty output does not try to perform an upload.