Create symlink trees through the native filesystem, not the action fi…

…lesystem. The comment added in SymlinkTreeStrategy explains why this is required. Fixes #24867. PiperOrigin-RevId: 715305548 Change-Id: I376d360a0d072c0d5912e14e3115a7fb3b5f2281
bazelbuild · Jan 14, 2025 · 4e89bc2 · 4e89bc2
1 parent cc9eb34
commit 4e89bc2
Show file tree

Hide file tree

Showing 7 changed files with 45 additions and 33 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
@@ -186,7 +186,10 @@ public class ExecutionTool {
     actionContextRegistryBuilder.register(
         SymlinkTreeActionContext.class,
         new SymlinkTreeStrategy(
-            env.getOutputService(), env.getBlazeWorkspace().getBinTools(), env.getWorkspaceName()));
+            env.getOutputService(),
+            env.getExecRoot(),
+            env.getBlazeWorkspace().getBinTools(),
+            env.getWorkspaceName()));
     // TODO(philwo) - the ExecutionTool should not add arbitrary dependencies on its own, instead
     // these dependencies should be added to the ActionContextConsumer of the module that actually
     // depends on them.

diff --git a/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java b/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java
@@ -121,7 +121,6 @@ private void updateRunfilesTree(
     if (!inputManifest.exists()) {
       return;
     }
-
     Path outputManifest =
         execRoot.getRelative(RunfilesSupport.outputManifestExecPath(tree.getExecPath()));
     try {
@@ -155,6 +154,7 @@ private void updateRunfilesTree(
         new SymlinkTreeHelper(
             execRoot,
             inputManifest,
+            outputManifest,
             runfilesDir,
             /* filesetTree= */ false,
             tree.getWorkspaceName());

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
@@ -56,27 +56,31 @@ public final class SymlinkTreeHelper {
 
   private final Path execRoot;
   private final Path inputManifest;
+  private final Path outputManifest;
   private final Path symlinkTreeRoot;
   private final boolean filesetTree;
   private final String workspaceName;
 
   /**
    * Creates SymlinkTreeHelper instance. Can be used independently of SymlinkTreeAction.
    *
-   * @param inputManifest exec path to the input runfiles manifest
-   * @param symlinkTreeRoot the root of the symlink tree to be created
-   * @param filesetTree true if this is fileset symlink tree, false if this is a runfiles symlink
+   * @param inputManifest path to the input runfiles manifest
+   * @param outputManifest path to the output runfiles manifest
+   * @param symlinkTreeRoot path to the root of the symlink tree
+   * @param filesetTree true if this is a fileset symlink tree, false if this is a runfiles symlink
    *     tree.
    * @param workspaceName the name of the workspace, used to create the workspace subdirectory
    */
   public SymlinkTreeHelper(
       Path execRoot,
       Path inputManifest,
+      Path outputManifest,
       Path symlinkTreeRoot,
       boolean filesetTree,
       String workspaceName) {
     this.execRoot = ensureNonSnapshotting(execRoot);
     this.inputManifest = ensureNonSnapshotting(inputManifest);
+    this.outputManifest = ensureNonSnapshotting(outputManifest);
     this.symlinkTreeRoot = ensureNonSnapshotting(symlinkTreeRoot);
     this.filesetTree = filesetTree;
     this.workspaceName = workspaceName;
@@ -92,10 +96,6 @@ private static Path ensureNonSnapshotting(Path path) {
     return path;
   }
 
-  private Path getOutputManifest() {
-    return symlinkTreeRoot.getChild("MANIFEST");
-  }
-
   interface TargetPathFunction<T> {
     /** Obtains a symlink target path from a T. */
     PathFragment get(T target) throws IOException;
@@ -190,7 +190,6 @@ 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.
-    Path outputManifest = getOutputManifest();
     try {
       symlinkTreeRoot.createDirectoryAndParents();
       outputManifest.delete();

diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategy.java
@@ -54,11 +54,14 @@ public final class SymlinkTreeStrategy implements SymlinkTreeActionContext {
       (artifact) -> artifact == null ? null : artifact.getPath().asFragment();
 
   private final OutputService outputService;
+  private final Path execRoot;
   private final BinTools binTools;
   private final String workspaceName;
 
-  public SymlinkTreeStrategy(OutputService outputService, BinTools binTools, String workspaceName) {
+  public SymlinkTreeStrategy(
+      OutputService outputService, Path execRoot, BinTools binTools, String workspaceName) {
     this.outputService = outputService;
+    this.execRoot = execRoot;
     this.binTools = binTools;
     this.workspaceName = workspaceName;
   }
@@ -92,10 +95,10 @@ public void createSymlinks(
           // 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();
+          createSymlinkTreeHelper(action).clearRunfilesDirectory();
         } else if (action.getRunfileSymlinksMode() == RunfileSymlinksMode.INTERNAL) {
           try {
-            SymlinkTreeHelper helper = createSymlinkTreeHelper(action, actionExecutionContext);
+            SymlinkTreeHelper helper = createSymlinkTreeHelper(action);
             if (action.isFilesetTree()) {
               helper.createFilesetSymlinksDirectly(getFilesetMap(action, actionExecutionContext));
             } else {
@@ -111,7 +114,7 @@ public void createSymlinks(
         } else {
           Map<String, String> resolvedEnv = new LinkedHashMap<>();
           action.getEnvironment().resolve(resolvedEnv, actionExecutionContext.getClientEnv());
-          createSymlinkTreeHelper(action, actionExecutionContext)
+          createSymlinkTreeHelper(action)
               .createSymlinksUsingCommand(
                   binTools, resolvedEnv, actionExecutionContext.getFileOutErr());
         }
@@ -121,7 +124,7 @@ public void createSymlinks(
     }
   }
 
-  private static ImmutableMap<PathFragment, PathFragment> getFilesetMap(
+  private ImmutableMap<PathFragment, PathFragment> getFilesetMap(
       SymlinkTreeAction action, ActionExecutionContext actionExecutionContext) {
     ImmutableList<FilesetOutputSymlink> filesetLinks;
     try {
@@ -135,9 +138,7 @@ private static ImmutableMap<PathFragment, PathFragment> getFilesetMap(
     }
 
     return SymlinkTreeHelper.processFilesetLinks(
-        filesetLinks,
-        action.getWorkspaceNameForFileset(),
-        actionExecutionContext.getExecRoot().asFragment());
+        filesetLinks, action.getWorkspaceNameForFileset(), execRoot.asFragment());
   }
 
   private static Map<PathFragment, Artifact> getRunfilesMap(SymlinkTreeAction action) {
@@ -160,12 +161,19 @@ private static void createOutput(
     }
   }
 
-  private SymlinkTreeHelper createSymlinkTreeHelper(
-      SymlinkTreeAction action, ActionExecutionContext actionExecutionContext) {
+  private SymlinkTreeHelper createSymlinkTreeHelper(SymlinkTreeAction action) {
+    // Do not indirect paths through the action filesystem, for two reasons:
+    // (1) we always want to create the symlinks on disk, even if the action filesystem creates them
+    //     in memory (at the time of writing, no action filesystem implementations do so, but this
+    //     may change in the future).
+    // (2) current action filesystem implementations are not a true overlay filesystem, so errors
+    //     might occur in an incremental build when the parent directory of a symlink exists on disk
+    //     but not in memory (see https://github.com/bazelbuild/bazel/issues/24867).
     return new SymlinkTreeHelper(
-        actionExecutionContext.getExecRoot(),
-        actionExecutionContext.getInputPath(action.getInputManifest()),
-        actionExecutionContext.getInputPath(action.getOutputManifest()).getParentDirectory(),
+        execRoot,
+        action.getInputManifest().getPath(),
+        action.getOutputManifest().getPath(),
+        action.getOutputManifest().getPath().getParentDirectory(),
         action.isFilesetTree(),
         workspaceName);
   }

diff --git a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java
@@ -63,15 +63,13 @@ public void setUp() throws Exception {
   public void checkCreatedSpawn() {
     Path execRoot = fs.getPath("/my/workspace");
     Path inputManifestPath = execRoot.getRelative("input_manifest");
+    Path outputManifestPath = execRoot.getRelative("output/MANIFEST");
+    Path symlinkTreeRoot = execRoot.getRelative("output");
     BinTools binTools =
         BinTools.forUnitTesting(execRoot, ImmutableList.of(SymlinkTreeHelper.BUILD_RUNFILES));
     Command command =
         new SymlinkTreeHelper(
-                execRoot,
-                inputManifestPath,
-                execRoot.getRelative("output/MANIFEST"),
-                false,
-                "__main__")
+                execRoot, inputManifestPath, outputManifestPath, symlinkTreeRoot, false, "__main__")
             .createCommand(binTools, ImmutableMap.of());
     assertThat(command.getEnvironment()).isEmpty();
     assertThat(command.getWorkingDirectory()).isEqualTo(execRoot.getPathFile());
@@ -80,7 +78,7 @@ public void checkCreatedSpawn() {
     assertThat(commandLine.get(0)).endsWith(SymlinkTreeHelper.BUILD_RUNFILES);
     assertThat(commandLine.get(1)).isEqualTo("--allow_relative");
     assertThat(commandLine.get(2)).isEqualTo("input_manifest");
-    assertThat(commandLine.get(3)).isEqualTo("output/MANIFEST");
+    assertThat(commandLine.get(3)).isEqualTo("output");
   }
 
   @Test
@@ -134,8 +132,10 @@ public void createSymlinksDirectly(
       @TestParameter TreeType treeType, @TestParameter boolean replace) throws Exception {
     Path treeRoot = execRoot.getRelative("foo.runfiles");
     Path inputManifestPath = execRoot.getRelative("foo.runfiles_manifest");
+    Path outputManifestPath = execRoot.getRelative("foo.runfiles/MANIFEST");
     SymlinkTreeHelper helper =
-        new SymlinkTreeHelper(execRoot, inputManifestPath, treeRoot, false, WORKSPACE_NAME);
+        new SymlinkTreeHelper(
+            execRoot, inputManifestPath, outputManifestPath, treeRoot, false, WORKSPACE_NAME);
 
     Artifact file = ActionsTestUtil.createArtifact(outputRoot, "file");
     Artifact symlink = ActionsTestUtil.createUnresolvedSymlinkArtifact(outputRoot, "symlink");

diff --git a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeStrategyTest.java
@@ -67,7 +67,7 @@ public void outputServiceInteraction() throws Exception {
     StoredEventHandler eventHandler = new StoredEventHandler();
 
     when(context.getContext(SymlinkTreeActionContext.class))
-        .thenReturn(new SymlinkTreeStrategy(outputService, null, "__main__"));
+        .thenReturn(new SymlinkTreeStrategy(outputService, getExecRoot(), null, "__main__"));
     when(context.getInputPath(any())).thenAnswer((i) -> ((Artifact) i.getArgument(0)).getPath());
     when(context.getPathResolver()).thenReturn(ArtifactPathResolver.IDENTITY);
     when(context.getEventHandler()).thenReturn(eventHandler);
@@ -131,7 +131,7 @@ public void inprocessSymlinkCreation() throws Exception {
 
     when(context.getExecRoot()).thenReturn(getExecRoot());
     when(context.getContext(SymlinkTreeActionContext.class))
-        .thenReturn(new SymlinkTreeStrategy(outputService, null, "__main__"));
+        .thenReturn(new SymlinkTreeStrategy(outputService, getExecRoot(), null, "__main__"));
     when(context.getInputPath(any())).thenAnswer((i) -> ((Artifact) i.getArgument(0)).getPath());
     when(context.getEventHandler()).thenReturn(eventHandler);
     when(outputService.canCreateSymlinkTree()).thenReturn(false);

diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/TestExecutorBuilder.java b/src/test/java/com/google/devtools/build/lib/exec/util/TestExecutorBuilder.java
@@ -71,7 +71,9 @@ public TestExecutorBuilder(FileSystem fileSystem, Path execRoot, BinTools binToo
     this.execRoot = execRoot;
     addContext(FileWriteActionContext.class, new FileWriteStrategy());
     addContext(TemplateExpansionContext.class, new LocalTemplateExpansionStrategy());
-    addContext(SymlinkTreeActionContext.class, new SymlinkTreeStrategy(null, binTools, "__main__"));
+    addContext(
+        SymlinkTreeActionContext.class,
+        new SymlinkTreeStrategy(null, execRoot, binTools, "__main__"));
     addContext(SpawnStrategyResolver.class, new SpawnStrategyResolver());
   }