[9.0.0] Centralize more logic in SymlinkTreeHelper. (bazelbuild#27677)

Also make sure that the output manifest is always created last, as it
attests to the symlink tree having been updated.

PiperOrigin-RevId: 831838700
Change-Id: Iddd95f170dd27f3a4254fbfccf5ef649c424d469

Author: tjgq

src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java

@@ -145,11 +145,12 @@ private void updateRunfilesTree(RunfilesTree tree) throws IOException, ExecExcep
     SymlinkTreeHelper helper =
         new SymlinkTreeHelper(inputManifest, outputManifest, runfilesDir, tree.getWorkspaceName());
 
-    if (tree.getSymlinksMode() == RunfileSymlinksMode.CREATE) {
-      helper.createRunfilesSymlinks(tree.getMapping());
-      outputManifest.createSymbolicLink(inputManifest);
-    } else {
-      helper.clearRunfilesDirectory();
+    switch (tree.getSymlinksMode()) {
+      case CREATE -> {
+        helper.createRunfilesSymlinks(tree.getMapping());
+        helper.linkManifest();
+      }
+      case SKIP -> helper.createMinimalRunfilesDirectory();
     }
   }
 

src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java

@@ -79,12 +79,13 @@ interface TargetPathFunction<T> {
   }
 
   /** Creates a symlink tree for a fileset by making VFS calls. */
-  public void createFilesetSymlinks(Map<PathFragment, PathFragment> symlinkMap) throws IOException {
+  public void createFilesetSymlinks(Map<PathFragment, PathFragment> symlinkMap)
+      throws ExecException {
     createSymlinks(symlinkMap, (path) -> path);
   }
 
   /** Creates a symlink tree for a runfiles by making VFS calls. */
-  public void createRunfilesSymlinks(Map<PathFragment, Artifact> symlinks) throws IOException {
+  public void createRunfilesSymlinks(Map<PathFragment, Artifact> symlinks) throws ExecException {
     createSymlinks(
         symlinks,
         (artifact) ->
@@ -96,7 +97,7 @@ public void createRunfilesSymlinks(Map<PathFragment, Artifact> symlinks) throws
 
   /** Creates a symlink tree. */
   private <T> void createSymlinks(
-      Map<PathFragment, T> symlinkMap, TargetPathFunction<T> targetPathFn) throws IOException {
+      Map<PathFragment, T> symlinkMap, TargetPathFunction<T> targetPathFn) throws ExecException {
     // Our strategy is to minimize mutating file system operations as much as possible. Ideally, if
     // there is an existing symlink tree with the expected contents, we don't make any changes. Our
     // algorithm goes as follows:
@@ -139,25 +140,23 @@ private <T> void createSymlinks(
       }
       root.syncTreeRecursively(symlinkTreeRoot, targetPathFn);
       createWorkspaceSubdirectory();
+    } catch (IOException e) {
+      throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_CREATION_IO_EXCEPTION);
     }
   }
 
   /**
-   * Ensures that the runfiles directory is empty except for the symlinked MANIFEST and the
-   * workspace subdirectory. This is the expected state with --noenable_runfiles.
+   * Creates a minimal runfiles directory containing only the output manifest and the workspace
+   * subdirectory. This is the expected state with --noenable_runfiles or --nobuild_runfile_links.
    */
-  public void clearRunfilesDirectory() throws ExecException {
-    deleteRunfilesDirectory();
+  public void createMinimalRunfilesDirectory() throws ExecException {
+    clearRunfilesDirectory();
     linkManifest();
-    try {
-      createWorkspaceSubdirectory();
-    } catch (IOException e) {
-      throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_CREATION_IO_EXCEPTION);
-    }
+    createWorkspaceSubdirectory();
   }
 
   /** Deletes the contents of the runfiles directory. */
-  private void deleteRunfilesDirectory() throws ExecException {
+  private void clearRunfilesDirectory() throws ExecException {
     try (SilentCloseable c = Profiler.instance().profile("Clear symlink tree")) {
       symlinkTreeRoot.deleteTreesBelow();
     } catch (IOException e) {
@@ -166,23 +165,26 @@ private void deleteRunfilesDirectory() throws ExecException {
   }
 
   /** Links the output manifest to the input manifest. */
-  private void linkManifest() throws ExecException {
-    // Pretend we created the runfiles tree by symlinking the output manifest to the input manifest.
+  public void linkManifest() throws ExecException {
+    // TODO(b/443703909): This should probably be a copy, not a symlink.
     try {
-      symlinkTreeRoot.createDirectoryAndParents();
       outputManifest.delete();
       outputManifest.createSymbolicLink(inputManifest);
     } catch (IOException e) {
       throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_MANIFEST_LINK_IO_EXCEPTION);
     }
   }
 
-  private void createWorkspaceSubdirectory() throws IOException {
+  public void createWorkspaceSubdirectory() throws ExecException {
     // Always create the subdirectory corresponding to the workspace (i.e., the main repository).
     // This is required by tests as their working directory, even with --noenable_runfiles. But if
     // the test action creates the directory and then proceeds to execute the test spawn, this logic
     // would remove it. For the sake of consistency, always create the directory instead.
-    symlinkTreeRoot.getRelative(workspaceName).createDirectory();
+    try {
+      symlinkTreeRoot.getRelative(workspaceName).createDirectory();
+    } catch (IOException e) {
+      throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_CREATION_IO_EXCEPTION);
+    }
   }
 
   /**

src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java

@@ -21,7 +21,6 @@
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.ActionExecutionException;
 import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.actions.EnvironmentalExecException;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.FilesetOutputSymlink;
 import com.google.devtools.build.lib.actions.RunningActionEvent;
@@ -30,13 +29,8 @@
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode;
 import com.google.devtools.build.lib.profiler.AutoProfiler;
 import com.google.devtools.build.lib.profiler.GoogleAutoProfilerUtils;
-import com.google.devtools.build.lib.server.FailureDetails.Execution;
-import com.google.devtools.build.lib.server.FailureDetails.Execution.Code;
-import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
 import com.google.devtools.build.lib.vfs.OutputService;
-import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
-import java.io.IOException;
 import java.time.Duration;
 import java.util.Map;
 
@@ -66,11 +60,13 @@ public void createSymlinks(
     actionExecutionContext.getEventHandler().post(new RunningActionEvent(action, "local"));
     try (AutoProfiler p =
         GoogleAutoProfilerUtils.logged("running " + action.prettyPrint(), MIN_LOGGING)) {
+      SymlinkTreeHelper helper = createSymlinkTreeHelper(action, actionExecutionContext);
       // TODO(tjgq): Respect RunfileSymlinksMode.SKIP even in the presence of an OutputService.
       try {
+        // Note that the output manifest must always be created last, as its presence ascertains
+        // that the runfiles tree has been updated (only the output manifest is an action output,
+        // so Skyframe cannot invalidate the symlink tree).
         if (outputService.canCreateSymlinkTree()) {
-          Path inputManifest = actionExecutionContext.getInputPath(action.getInputManifest());
-
           Map<PathFragment, PathFragment> symlinks;
           if (action.isFilesetTree()) {
             symlinks = getFilesetMap(action, actionExecutionContext);
@@ -79,31 +75,22 @@ public void createSymlinks(
             // created textually.
             symlinks = Maps.transformValues(getRunfilesMap(action), TO_PATH);
           }
-
           outputService.createSymlinkTree(
               symlinks, action.getOutputManifest().getExecPath().getParentDirectory());
-
-          createOutput(action, actionExecutionContext, inputManifest);
+          helper.linkManifest();
         } else if (action.getRunfileSymlinksMode() == RunfileSymlinksMode.SKIP) {
-          // Delete symlinks possibly left over by a previous invocation with a different mode.
-          // This is required because only the output manifest is considered an action output, so
-          // Skyframe does not clear the directory for us.
-          createSymlinkTreeHelper(action, actionExecutionContext).clearRunfilesDirectory();
+          // Clear the runfiles directory, then create just the output manifest and the workspace
+          // subdirectory. This is required because only the output manifest is considered an action
+          // output, so if the previous invocation created a symlink tree, Skyframe will not clear
+          // it for us.
+          helper.createMinimalRunfilesDirectory();
         } else {
-          try {
-            SymlinkTreeHelper helper = createSymlinkTreeHelper(action, actionExecutionContext);
-            if (action.isFilesetTree()) {
-              helper.createFilesetSymlinks(getFilesetMap(action, actionExecutionContext));
-            } else {
-              helper.createRunfilesSymlinks(getRunfilesMap(action));
-            }
-          } catch (IOException e) {
-            throw ActionExecutionException.fromExecException(
-                new EnvironmentalExecException(e, Code.SYMLINK_TREE_CREATION_IO_EXCEPTION), action);
+          if (action.isFilesetTree()) {
+            helper.createFilesetSymlinks(getFilesetMap(action, actionExecutionContext));
+          } else {
+            helper.createRunfilesSymlinks(getRunfilesMap(action));
           }
-
-          Path inputManifest = actionExecutionContext.getInputPath(action.getInputManifest());
-          createOutput(action, actionExecutionContext, inputManifest);
+          helper.linkManifest();
         }
       } catch (ExecException e) {
         throw ActionExecutionException.fromExecException(e, action);
@@ -127,20 +114,6 @@ private static Map<PathFragment, Artifact> getRunfilesMap(SymlinkTreeAction acti
     return action.getRunfiles().getRunfilesInputs(action.getRepoMappingManifest());
   }
 
-  private static void createOutput(
-      SymlinkTreeAction action, ActionExecutionContext actionExecutionContext, Path inputManifest)
-      throws EnvironmentalExecException {
-    Path outputManifest = actionExecutionContext.getInputPath(action.getOutputManifest());
-    // Link output manifest on success. We avoid a file copy as these manifests may be
-    // large. Note that this step has to come last because the OutputService may delete any
-    // pre-existing symlink tree before creating a new one.
-    try {
-      outputManifest.createSymbolicLink(inputManifest);
-    } catch (IOException e) {
-      throw createLinkFailureException(outputManifest, e);
-    }
-  }
-
   private SymlinkTreeHelper createSymlinkTreeHelper(
       SymlinkTreeAction action, ActionExecutionContext actionExecutionContext) {
     return new SymlinkTreeHelper(
@@ -149,15 +122,4 @@ private SymlinkTreeHelper createSymlinkTreeHelper(
         actionExecutionContext.getInputPath(action.getOutputManifest()).getParentDirectory(),
         workspaceName);
   }
-
-  private static EnvironmentalExecException createLinkFailureException(
-      Path outputManifest, IOException e) {
-    return new EnvironmentalExecException(
-        e,
-        FailureDetail.newBuilder()
-            .setMessage("Failed to link output manifest '" + outputManifest.getPathString() + "'")
-            .setExecution(
-                Execution.newBuilder().setCode(Code.SYMLINK_TREE_MANIFEST_LINK_IO_EXCEPTION))
-            .build());
-  }
 }