Make runfiles incrementally correct with --nobuild_runfile_links

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

@@ -199,6 +199,7 @@ java_library(
         "//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",

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) {
@@ -142,7 +154,7 @@ private void updateRunfilesTree(
 
     switch (tree.getSymlinksMode()) {
       case SKIP:
-        helper.linkManifest();
+        helper.clearAndLinkManifest();
         break;
       case EXTERNAL:
         helper.createSymlinksUsingCommand(execRoot, binTools, env, outErr);
@@ -153,4 +165,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(" ")[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/SymlinkTreeHelper.java

@@ -121,8 +121,17 @@ public void createSymlinksDirectly(Path symlinkTreeRoot, Map<PathFragment, Artif
     }
   }
 
+  /**
+   * Ensures that the runfiles directory is empty except for the symlinked MANIFEST. This is the
+   * expected state with --noenable_runfiles.
+   */
+  public void clearAndLinkManifest() throws ExecException {
+    clearRunfilesDirectory();
+    linkManifest();
+  }
+
   /** Deletes the contents of the runfiles directory. */
-  public void clearRunfilesDirectory() throws ExecException {
+  private void clearRunfilesDirectory() throws ExecException {
     try (SilentCloseable c = Profiler.instance().profile("Clear symlink tree")) {
       symlinkTreeRoot.deleteTreesBelow();
     } catch (IOException e) {
@@ -131,7 +140,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 {

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

@@ -105,9 +105,7 @@ 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.
-          var helper = createSymlinkTreeHelper(action, actionExecutionContext);
-          helper.clearRunfilesDirectory();
-          helper.linkManifest();
+          createSymlinkTreeHelper(action, actionExecutionContext).clearAndLinkManifest();
         } else if (action.getRunfileSymlinksMode() == RunfileSymlinksMode.INTERNAL
             && !action.isFilesetTree()) {
           try {

src/main/java/com/google/devtools/build/lib/runtime/commands/BUILD

@@ -58,6 +58,7 @@ java_library(
         "//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/exec:execution_options",
+        "//src/main/java/com/google/devtools/build/lib/exec:runfiles_tree_updater",
         "//src/main/java/com/google/devtools/build/lib/exec:symlink_tree_helper",
         "//src/main/java/com/google/devtools/build/lib/exec:test_policy",
         "//src/main/java/com/google/devtools/build/lib/exec/local:options",

src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java

@@ -62,6 +62,7 @@
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.Reporter;
 import com.google.devtools.build.lib.exec.ExecutionOptions;
+import com.google.devtools.build.lib.exec.RunfilesTreeUpdater;
 import com.google.devtools.build.lib.exec.SymlinkTreeHelper;
 import com.google.devtools.build.lib.exec.TestPolicy;
 import com.google.devtools.build.lib.packages.InputFile;
@@ -158,8 +159,6 @@ public static class RunOptions extends OptionsBase {
       "'run' only works with tests with one shard ('--test_sharding_strategy=disabled' is okay) "
           + "and without --runs_per_test";
 
-  private static final FileType RUNFILES_MANIFEST = FileType.of(".runfiles_manifest");
-
   private static final ImmutableList<String> ENV_VARIABLES_TO_CLEAR =
       ImmutableList.of(
           // These variables are all used by runfiles libraries to locate the runfiles directory or
@@ -468,6 +467,7 @@ private static BuiltTargets getBuiltTargets(
     // target to run needs to be preserved, as it acts as the working directory.
     Path targetToRunRunfilesDir = null;
     RunfilesSupport targetToRunRunfilesSupport = null;
+    RunfilesTreeUpdater runfilesTreeUpdater = RunfilesTreeUpdater.forCommandEnvironment(env);
     for (ConfiguredTarget target : topLevelTargets) {
       FilesToRunProvider provider = target.getProvider(FilesToRunProvider.class);
       RunfilesSupport runfilesSupport = provider == null ? null : provider.getRunfilesSupport();
@@ -481,7 +481,8 @@ private static BuiltTargets getBuiltTargets(
                 env,
                 runfilesSupport,
                 env.getSkyframeExecutor()
-                    .getConfiguration(env.getReporter(), target.getConfigurationKey()));
+                    .getConfiguration(env.getReporter(), target.getConfigurationKey()),
+                runfilesTreeUpdater);
         if (target == targetToRun) {
           targetToRunRunfilesDir = runfilesDir;
           targetToRunRunfilesSupport = runfilesSupport;
@@ -935,9 +936,9 @@ private static BlazeCommandResult reportAndCreateFailureResult(
   private static Path ensureRunfilesBuilt(
       CommandEnvironment env,
       RunfilesSupport runfilesSupport,
-      BuildConfigurationValue configuration)
+      BuildConfigurationValue configuration,
+      RunfilesTreeUpdater runfilesTreeUpdater)
       throws RunfilesException, InterruptedException {
-    Artifact manifest = Preconditions.checkNotNull(runfilesSupport.getRunfilesManifest());
     PathFragment runfilesDir = runfilesSupport.getRunfilesDirectoryExecPath();
     Path workingDir = env.getExecRoot().getRelative(runfilesDir);
     // On Windows, runfiles tree is disabled.
@@ -962,22 +963,10 @@ private static Path ensureRunfilesBuilt(
           e);
     }
 
-    // When runfiles are not generated, getManifest() returns the
-    // .runfiles_manifest file, otherwise it returns the MANIFEST file. This is
-    // a handy way to check whether runfiles were built or not.
-    if (!RUNFILES_MANIFEST.matches(manifest.getFilename())) {
-      return workingDir;
-    }
-
-    SymlinkTreeHelper helper =
-        new SymlinkTreeHelper(manifest.getPath(), runfilesSupport.getRunfilesDirectory(), false);
     try {
-      helper.createSymlinksUsingCommand(
-          env.getExecRoot(),
-          env.getBlazeWorkspace().getBinTools(),
-          /* shellEnvironment= */ ImmutableMap.of(),
-          /* outErr= */ null);
-    } catch (EnvironmentalExecException e) {
+      runfilesTreeUpdater.updateRunfiles(
+          runfilesSupport, /* shellEnvironment= */ ImmutableMap.of(), /* outErr= */ null);
+    } catch (ExecException | IOException e) {
       throw new RunfilesException(
           "Failed to create runfiles symlinks: " + e.getMessage(),
           Code.RUNFILES_SYMLINKS_CREATION_FAILURE,

src/test/py/bazel/runfiles_test.py

@@ -339,6 +339,93 @@ def testRunfilesLibrariesFindRlocationpathExpansion(self):
     self.assertEqual(stdout[0], "Hello, Bazel!")
     self.assertEqual(stdout[1], "Hello, World!")
 
+  def setUpRunfilesDirectoryIncrementalityTest(self):
+    self.ScratchFile("MODULE.bazel")
+    self.ScratchFile("BUILD", [
+      "sh_test(",
+      "  name = 'test',",
+      "  srcs = ['test.sh'],",
+      "  data = ['data.txt'],",
+      ")",
+    ])
+    self.ScratchFile("data.txt")
+    self.ScratchFile("test.sh", ["[[ -f data.txt ]]"], executable=True)
+    self.ScratchFile(".bazelrc", [
+      "startup --nowindows_enable_symlinks",
+      "common --spawn_strategy=local",
+    ])
+
+  def testRunfilesDirectoryIncrementalityEnableRunfilesFlippedOn(self):
+    self.setUpRunfilesDirectoryIncrementalityTest()
+
+    exit_code, _, _ = self.RunBazel(["test", ":test", "--noenable_runfiles"], allow_failure=True)
+    self.assertEqual(exit_code, 3)
+    exit_code, _, _ = self.RunBazel(["test", ":test", "--enable_runfiles"])
+    self.assertEqual(exit_code, 0)
+
+  def testRunfilesDirectoryIncrementalityEnableRunfilesFlippedOff(self):
+    self.setUpRunfilesDirectoryIncrementalityTest()
+
+    exit_code, _, _ = self.RunBazel(["test", ":test", "--enable_runfiles"])
+    self.assertEqual(exit_code, 0)
+    exit_code, _, _ = self.RunBazel(["test", ":test", "--noenable_runfiles"], allow_failure=True)
+    self.assertEqual(exit_code, 3)
+
+  def testRunfilesDirectoryIncrementalityNoBuildRunfileLinksEnableRunfilesFlippedOn(self):
+    self.setUpRunfilesDirectoryIncrementalityTest()
+
+    exit_code, _, _ = self.RunBazel(
+      ["test", ":test", "--nobuild_runfile_links", "--noenable_runfiles"], allow_failure=True)
+    self.assertEqual(exit_code, 3)
+    exit_code, _, _ = self.RunBazel(
+      ["test", ":test", "--nobuild_runfile_links", "--enable_runfiles"])
+    self.assertEqual(exit_code, 0)
+
+  def testRunfilesDirectoryIncrementalityNoBuildRunfileLinksEnableRunfilesFlippedOff(self):
+    self.setUpRunfilesDirectoryIncrementalityTest()
+
+    exit_code, _, _ = self.RunBazel(
+      ["test", ":test", "--nobuild_runfile_links", "--enable_runfiles"])
+    self.assertEqual(exit_code, 0)
+    exit_code, _, _ = self.RunBazel(
+      ["test", ":test", "--nobuild_runfile_links", "--noenable_runfiles"], allow_failure=True)
+    self.assertEqual(exit_code, 3)
+
+  def testRunfilesDirectoryIncrementalityEnableRunfilesFlippedOnRun(self):
+    self.setUpRunfilesDirectoryIncrementalityTest()
+
+    exit_code, _, _ = self.RunBazel(["run", ":test", "--noenable_runfiles"], allow_failure=True)
+    self.assertNotEqual(exit_code, 0)
+    exit_code, _, _ = self.RunBazel(["run", ":test", "--enable_runfiles"])
+    self.assertEqual(exit_code, 0)
+
+  def testRunfilesDirectoryIncrementalityEnableRunfilesFlippedOffRun(self):
+    self.setUpRunfilesDirectoryIncrementalityTest()
+
+    exit_code, _, _ = self.RunBazel(["run", ":test", "--enable_runfiles"])
+    self.assertEqual(exit_code, 0)
+    exit_code, _, _ = self.RunBazel(["run", ":test", "--noenable_runfiles"], allow_failure=True)
+    self.assertNotEqual(exit_code, 0)
+
+  def testRunfilesDirectoryIncrementalityNoBuildRunfileLinksEnableRunfilesFlippedOnRun(self):
+    self.setUpRunfilesDirectoryIncrementalityTest()
+
+    exit_code, _, _ = self.RunBazel(
+      ["run", ":test", "--nobuild_runfile_links", "--noenable_runfiles"], allow_failure=True)
+    self.assertNotEqual(exit_code, 0)
+    exit_code, _, _ = self.RunBazel(
+      ["run", ":test", "--nobuild_runfile_links", "--enable_runfiles"])
+    self.assertEqual(exit_code, 0)
+
+  def testRunfilesDirectoryIncrementalityNoBuildRunfileLinksEnableRunfilesFlippedOffRun(self):
+    self.setUpRunfilesDirectoryIncrementalityTest()
+
+    exit_code, _, _ = self.RunBazel(
+      ["run", ":test", "--nobuild_runfile_links", "--enable_runfiles"])
+    self.assertEqual(exit_code, 0)
+    exit_code, _, _ = self.RunBazel(
+      ["run", ":test", "--nobuild_runfile_links", "--noenable_runfiles"], allow_failure=True)
+    self.assertNotEqual(exit_code, 0)
 
 if __name__ == "__main__":
   absltest.main()

src/test/shell/integration/python_stub_test.sh

@@ -142,8 +142,9 @@ EOF
 
   touch python_through_bash/inner.py
 
-  bazel run --nobuild_runfile_links //python_through_bash:outer \
-    &> $TEST_log || fail "bazel run failed"
+  # The inner Python script requires runfiles, so force them on Windows.
+  bazel run --nobuild_runfile_links --enable_runfiles \
+    //python_through_bash:outer &> $TEST_log || fail "bazel run failed"
   expect_log "I am Python"
 }