Make runfiles incrementally correct with --nobuild_runfile_links

Fixes three separate but related issues with `--nobuild_runfile_links`:
* On all OSes, flipping `--enable_runfiles` from on to off did not result in runfiles being cleared due to a missing call to `SymlinkTreeHelper#clearRunfilesDirectory` that was only added to `SymlinkTreeStrategy` in f84329e.
* On Windows only, flipping `--enable_runfiles` from off to on did not result in runfiles being created since the logic in `RunfilesTreeUpdater` would see a copy of the manifest instead of a symlink. This is fixed by additionally checking whether the runfiles directory contains the first runfile on Windows. If runfiles were disabled before, it won't, otherwise it will.
* On all OSes, `--noenable_runfiles` was ignored by `bazel run`, with runfiles always being created. This is fixed by using `RunfilesTreeUpdater` instead of custom and incorrect logic.

With the fixed behavior, the runfiles tree for tests is now cleared right before the test spawn is executed, which makes the test action unable to create its working directory, the subdirectory of the runfiles directory corresponding to the workspace name. To fix this and also get rid of the inconsistency of having another action write into the runfiles tree, this logic is moved into the `SymlinkTreeHelper` and thus applied to all runfiles trees.

Work towards #20676
Fixes #19333

Closes #20695.

PiperOrigin-RevId: 595369235
Change-Id: I32efc0e6a1d29291470ce5eafcc69bbd9ab276b9

src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java

@@ -49,6 +49,9 @@ interface RunfilesTree {
 
     /** Returns whether the runfile symlinks should be materialized during the build. */
     boolean isBuildRunfileLinks();
+
+    /** Returns the name of the workspace that the build is occurring in. */
+    String getWorkspaceName();
   }
 
   /** Returns the runfiles trees to be materialized on the inputs of the action. */

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

@@ -343,8 +343,9 @@ public NestedSet<Artifact> getRunfilesArtifacts() {
   }
 
   /** Returns the name of the workspace that the build is occurring in. */
-  public PathFragment getWorkspaceName() {
-    return runfiles.getSuffix();
+  @Override
+  public String getWorkspaceName() {
+    return runfiles.getSuffix().getPathString();
   }
 
   /**

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

@@ -166,6 +166,11 @@ public boolean isBuildRunfileLinks() {
     return buildRunfileLinks;
   }
 
+  @Override
+  public String getWorkspaceName() {
+    return runfiles.getSuffix().getPathString();
+  }
+
   /** Softly caches the result of {@link Runfiles#getRunfilesInputs}. */
   private static final class RunfilesCacher implements Supplier<Map<PathFragment, Artifact>> {
 

src/main/java/com/google/devtools/build/lib/analysis/test/TestStrategy.java

@@ -116,15 +116,11 @@ public synchronized void cancelOthers() {
    * Ensures that all directories used to run test are in the correct state and their content will
    * not result in stale files.
    */
-  protected void prepareFileSystem(
-      TestRunnerAction testAction, Path execRoot, Path tmpDir, Path workingDirectory)
+  protected void prepareFileSystem(TestRunnerAction testAction, Path execRoot, Path tmpDir)
       throws IOException {
     if (tmpDir != null) {
       recreateDirectory(tmpDir);
     }
-    if (workingDirectory != null) {
-      workingDirectory.createDirectoryAndParents();
-    }
 
     ResolvedPaths resolvedPaths = testAction.resolve(execRoot);
     if (testAction.isCoverageMode()) {
@@ -142,7 +138,7 @@ protected void prepareFileSystem(
    * not result in stale files. Only use this if no local tmp and working directory are required.
    */
   protected void prepareFileSystem(TestRunnerAction testAction, Path execRoot) throws IOException {
-    prepareFileSystem(testAction, execRoot, null, null);
+    prepareFileSystem(testAction, execRoot, null);
   }
 
   /** Removes directory if it exists and recreates it. */

src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java

@@ -181,7 +181,8 @@ public class ExecutionTool {
     }
     actionContextRegistryBuilder.register(
         SymlinkTreeActionContext.class,
-        new SymlinkTreeStrategy(env.getOutputService(), env.getBlazeWorkspace().getBinTools()));
+        new SymlinkTreeStrategy(
+            env.getOutputService(), 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.

src/main/java/com/google/devtools/build/lib/exec/BUILD

@@ -195,10 +195,10 @@ java_library(
         ":symlink_tree_helper",
         "//src/main/java/com/google/devtools/build/lib:runtime",
         "//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/actions:runfiles_supplier",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/analysis:config/build_configuration",
+        "//src/main/java/com/google/devtools/build/lib/util:os",
         "//src/main/java/com/google/devtools/build/lib/util/io:out-err",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
@@ -373,21 +373,18 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/actions:action_input_helper",
         "//src/main/java/com/google/devtools/build/lib/actions:artifacts",
         "//src/main/java/com/google/devtools/build/lib/actions:execution_requirements",
-        "//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
         "//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream",
         "//src/main/java/com/google/devtools/build/lib/buildeventstream/proto:build_event_stream_java_proto",
         "//src/main/java/com/google/devtools/build/lib/collect/nestedset",
         "//src/main/java/com/google/devtools/build/lib/events",
-        "//src/main/java/com/google/devtools/build/lib/skyframe:tree_artifact_value",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/util/io",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
         "//src/main/protobuf:failure_details_java_proto",
         "//src/main/protobuf:test_status_java_proto",
         "//third_party:guava",
-        "//third_party:jsr305",
         "//third_party/protobuf:protobuf_java",
         "//third_party/protobuf:protobuf_java_util",
     ],

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

@@ -13,6 +13,8 @@
 // limitations under the License.
 package com.google.devtools.build.lib.exec;
 
+import static com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode.SKIP;
+import static java.nio.charset.StandardCharsets.ISO_8859_1;
 
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableMap;
@@ -21,12 +23,16 @@
 import com.google.devtools.build.lib.actions.RunfilesSupplier.RunfilesTree;
 import com.google.devtools.build.lib.analysis.RunfilesSupport;
 import com.google.devtools.build.lib.runtime.CommandEnvironment;
+import com.google.devtools.build.lib.util.OS;
 import com.google.devtools.build.lib.util.io.OutErr;
 import com.google.devtools.build.lib.vfs.DigestUtils;
 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.devtools.build.lib.vfs.XattrProvider;
+import java.io.BufferedReader;
 import java.io.IOException;
+import java.io.InputStreamReader;
 import java.util.Arrays;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.CompletionException;
@@ -122,11 +128,17 @@ private void updateRunfilesTree(
       // implying the symlinks exist and are already up to date. If the output manifest is a
       // symbolic link, it is likely a symbolic link to the input manifest, so we cannot trust it as
       // an up-to-date check.
-      if (!outputManifest.isSymbolicLink()
+      // On Windows, where symlinks may be silently replaced by copies, a previous run in SKIP mode
+      // could have resulted in an output manifest that is an identical copy of the input manifest,
+      // which we must not treat as up to date, but we also don't want to unnecessarily rebuild the
+      // runfiles directory all the time. Instead, check for the presence of the first runfile in
+      // the manifest. If it is present, we can be certain that the previous mode wasn't SKIP.
+      if (tree.getSymlinksMode() != SKIP
+          && !outputManifest.isSymbolicLink()
           && Arrays.equals(
               DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(outputManifest, xattrProvider),
-              DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(
-                  inputManifest, xattrProvider))) {
+              DigestUtils.getDigestWithManualFallbackWhenSizeUnknown(inputManifest, xattrProvider))
+          && (OS.getCurrent() != OS.WINDOWS || isRunfilesDirectoryPopulated(runfilesDirPath))) {
         return;
       }
     } catch (IOException e) {
@@ -138,11 +150,12 @@ private void updateRunfilesTree(
     }
 
     SymlinkTreeHelper helper =
-        new SymlinkTreeHelper(inputManifest, runfilesDirPath, /* filesetTree= */ false);
+        new SymlinkTreeHelper(
+            inputManifest, runfilesDirPath, /* filesetTree= */ false, tree.getWorkspaceName());
 
     switch (tree.getSymlinksMode()) {
       case SKIP:
-        helper.linkManifest();
+        helper.clearRunfilesDirectory();
         break;
       case EXTERNAL:
         helper.createSymlinksUsingCommand(execRoot, binTools, env, outErr);
@@ -153,4 +166,19 @@ private void updateRunfilesTree(
         break;
     }
   }
+
+  private boolean isRunfilesDirectoryPopulated(Path runfilesDirPath) {
+    Path outputManifest = RunfilesSupport.outputManifestPath(runfilesDirPath);
+    String relativeRunfilePath;
+    try (BufferedReader reader =
+        new BufferedReader(new InputStreamReader(outputManifest.getInputStream(), ISO_8859_1))) {
+      // If it is created at all, the manifest always contains at least one line.
+      relativeRunfilePath = reader.readLine().split(" ", -1)[0];
+    } catch (IOException e) {
+      // Instead of failing outright, just assume the runfiles directory is not populated.
+      return false;
+    }
+    // The runfile could be a dangling symlink.
+    return runfilesDirPath.getRelative(relativeRunfilePath).exists(Symlinks.NOFOLLOW);
+  }
 }

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

@@ -136,11 +136,8 @@ public TestRunnerSpawn createTestRunnerSpawn(
             localResourcesSupplier);
     Path execRoot = actionExecutionContext.getExecRoot();
     ArtifactPathResolver pathResolver = actionExecutionContext.getPathResolver();
-    Path runfilesDir = pathResolver.convertPath(action.getExecutionSettings().getRunfilesDir());
     Path tmpDir = pathResolver.convertPath(tmpDirRoot.getChild(TestStrategy.getTmpDirName(action)));
-    Path workingDirectory = runfilesDir.getRelative(action.getRunfilesPrefix());
-    return new StandaloneTestRunnerSpawn(
-        action, actionExecutionContext, spawn, tmpDir, workingDirectory, execRoot);
+    return new StandaloneTestRunnerSpawn(action, actionExecutionContext, spawn, tmpDir, execRoot);
   }
 
   private static ImmutableList<Pair<String, Path>> renameOutputs(
@@ -556,21 +553,18 @@ private final class StandaloneTestRunnerSpawn implements TestRunnerSpawn {
     private final ActionExecutionContext actionExecutionContext;
     private final Spawn spawn;
     private final Path tmpDir;
-    private final Path workingDirectory;
     private final Path execRoot;
 
     StandaloneTestRunnerSpawn(
         TestRunnerAction testAction,
         ActionExecutionContext actionExecutionContext,
         Spawn spawn,
         Path tmpDir,
-        Path workingDirectory,
         Path execRoot) {
       this.testAction = testAction;
       this.actionExecutionContext = actionExecutionContext;
       this.spawn = spawn;
       this.tmpDir = tmpDir;
-      this.workingDirectory = workingDirectory;
       this.execRoot = execRoot;
     }
 
@@ -581,7 +575,7 @@ public ActionExecutionContext getActionExecutionContext() {
 
     @Override
     public TestAttemptResult execute() throws InterruptedException, IOException, ExecException {
-      prepareFileSystem(testAction, execRoot, tmpDir, workingDirectory);
+      prepareFileSystem(testAction, execRoot, tmpDir);
       return beginTestAttempt(testAction, spawn, actionExecutionContext, execRoot);
     }
 

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

@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.exec;
 
-
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
@@ -57,6 +56,7 @@ public final class SymlinkTreeHelper {
   private final Path inputManifest;
   private final Path symlinkTreeRoot;
   private final boolean filesetTree;
+  private final String workspaceName;
 
   /**
    * Creates SymlinkTreeHelper instance. Can be used independently of SymlinkTreeAction.
@@ -65,11 +65,14 @@ public final class SymlinkTreeHelper {
    * @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
    *     tree.
+   * @param workspaceName the name of the workspace, used to create the workspace subdirectory
    */
-  public SymlinkTreeHelper(Path inputManifest, Path symlinkTreeRoot, boolean filesetTree) {
+  public SymlinkTreeHelper(
+      Path inputManifest, Path symlinkTreeRoot, boolean filesetTree, String workspaceName) {
     this.inputManifest = inputManifest;
     this.symlinkTreeRoot = symlinkTreeRoot;
     this.filesetTree = filesetTree;
+    this.workspaceName = workspaceName;
   }
 
   private Path getOutputManifest() {
@@ -118,11 +121,26 @@ public void createSymlinksDirectly(Path symlinkTreeRoot, Map<PathFragment, Artif
         parentDir.addSymlink(entry.getKey().getBaseName(), entry.getValue());
       }
       root.syncTreeRecursively(symlinkTreeRoot);
+      createWorkspaceSubdirectory();
     }
   }
 
-  /** Deletes the contents of the runfiles directory. */
+  /**
+   * Ensures that the runfiles directory is empty except for the symlinked MANIFEST and the
+   * workspace subdirectory. This is the expected state with --noenable_runfiles.
+   */
   public void clearRunfilesDirectory() throws ExecException {
+    deleteRunfilesDirectory();
+    linkManifest();
+    try {
+      createWorkspaceSubdirectory();
+    } catch (IOException e) {
+      throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_CREATION_IO_EXCEPTION);
+    }
+  }
+
+  /** Deletes the contents of the runfiles directory. */
+  private void deleteRunfilesDirectory() throws ExecException {
     try (SilentCloseable c = Profiler.instance().profile("Clear symlink tree")) {
       symlinkTreeRoot.deleteTreesBelow();
     } catch (IOException e) {
@@ -131,7 +149,7 @@ public void clearRunfilesDirectory() throws ExecException {
   }
 
   /** Links the output manifest to the input manifest. */
-  public void linkManifest() throws ExecException {
+  private void linkManifest() throws ExecException {
     // Pretend we created the runfiles tree by symlinking the output manifest to the input manifest.
     Path outputManifest = getOutputManifest();
     try {
@@ -143,6 +161,14 @@ public void linkManifest() throws ExecException {
     }
   }
 
+  private void createWorkspaceSubdirectory() throws IOException {
+    // 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();
+  }
+
   /**
    * Creates a symlink tree using a CommandBuilder. This means that the symlink tree will always be
    * present on the developer's workstation. Useful when running commands locally.
@@ -170,6 +196,11 @@ public void createSymlinksUsingCommand(
                     Execution.newBuilder().setCode(Code.SYMLINK_TREE_CREATION_COMMAND_EXCEPTION))
                 .build());
       }
+      try {
+        createWorkspaceSubdirectory();
+      } catch (IOException e) {
+        throw new EnvironmentalExecException(e, Code.SYMLINK_TREE_CREATION_IO_EXCEPTION);
+      }
     }
   }
 

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

@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.exec;
 
-
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
@@ -56,10 +55,12 @@ public final class SymlinkTreeStrategy implements SymlinkTreeActionContext {
 
   private final OutputService outputService;
   private final BinTools binTools;
+  private final String workspaceName;
 
-  public SymlinkTreeStrategy(OutputService outputService, BinTools binTools) {
+  public SymlinkTreeStrategy(OutputService outputService, BinTools binTools, String workspaceName) {
     this.outputService = outputService;
     this.binTools = binTools;
+    this.workspaceName = workspaceName;
   }
 
   @Override
@@ -97,17 +98,14 @@ public void createSymlinks(
           }
 
           outputService.createSymlinkTree(
-              symlinks,
-              action.getOutputManifest().getExecPath().getParentDirectory());
+              symlinks, action.getOutputManifest().getExecPath().getParentDirectory());
 
           createOutput(action, actionExecutionContext, inputManifest);
         } 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.
-          var helper = createSymlinkTreeHelper(action, actionExecutionContext);
-          helper.clearRunfilesDirectory();
-          helper.linkManifest();
+          createSymlinkTreeHelper(action, actionExecutionContext).clearRunfilesDirectory();
         } else if (action.getRunfileSymlinksMode() == RunfileSymlinksMode.INTERNAL
             && !action.isFilesetTree()) {
           try {
@@ -163,12 +161,13 @@ private static void createOutput(
     }
   }
 
-  private static SymlinkTreeHelper createSymlinkTreeHelper(
+  private SymlinkTreeHelper createSymlinkTreeHelper(
       SymlinkTreeAction action, ActionExecutionContext actionExecutionContext) {
     return new SymlinkTreeHelper(
         actionExecutionContext.getInputPath(action.getInputManifest()),
         actionExecutionContext.getInputPath(action.getOutputManifest()).getParentDirectory(),
-        action.isFilesetTree());
+        action.isFilesetTree(),
+        workspaceName);
   }
 
   private static EnvironmentalExecException createLinkFailureException(