Make runfiles incrementally correct with --nobuild_runfile_links

Fixes two separate but related issues:
* On all OSes, with `--nobuild_runfile_links`, 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, with `--nobuild_runfile_links`, 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.

Work towards bazelbuild#20676
Fixes bazelbuild#19333

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,15 @@ private void updateRunfilesTree(
         break;
     }
   }
+
+  private boolean isRunfilesDirectoryPopulated(Path runfilesDirPath) throws IOException {
+    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];
+    }
+    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/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()