Add a _runfiles_enabled marker file to runfiles trees

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

@@ -785,11 +785,6 @@ public void setupEnvVariables(Map<String, String> env) {
     }
     env.put("XML_OUTPUT_FILE", testXml.getExecPathString());
 
-    if (!configuration.runfilesEnabled()) {
-      // If runfiles are disabled, tell remote-runtest.sh/local-runtest.sh about that.
-      env.put("RUNFILES_MANIFEST_ONLY", "1");
-    }
-
     if (isCoverageMode()) {
       // Instruct remote-runtest.sh/local-runtest.sh not to cd into the runfiles directory.
       // TODO(ulfjack): Find a way to avoid setting this variable.

src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java

@@ -195,11 +195,14 @@ static PathFragment getShellExecutableForOs(OS os, ShellConfiguration.Options op
         }
 
         if (!BuildConfigurationValue.runfilesEnabled(options.get(CoreOptions.class))) {
-          // Setting this environment variable is for telling the binary running
-          // in a Bazel action when to use runfiles library or runfiles tree.
-          // The downside is that it will discard cache for all actions once
-          // --enable_runfiles changes, but this also prevents wrong caching result if a binary
-          // behaves differently with and without runfiles tree.
+          // The legacy way of telling a binary running in a Bazel action to look up its runfiles in
+          // the manifest rather than in the runfiles directory. This is flawed because it is purely
+          // based on analysis time information (--enable_runfiles) and thus doesn't account for the
+          // runfiles directory being materialized by the sandbox or by remote execution.
+          // Runfiles libraries should instead check for the presence of the _repo_mapping file,
+          // which is an ordinary runfile and thus only present in a fully materialized runfiles
+          // directory.
+          // TODO: Remove this once all runfiles libraries check for _repo_mapping.
           env.put("RUNFILES_MANIFEST_ONLY", "1");
         }
 

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

@@ -13,24 +13,13 @@
 // limitations under the License.
 package com.google.devtools.build.lib.exec;
 
-import static java.nio.charset.StandardCharsets.ISO_8859_1;
-
-import com.google.common.base.Splitter;
 import com.google.common.base.Throwables;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.RunfilesTree;
 import com.google.devtools.build.lib.analysis.RunfilesSupport;
-import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode;
-import com.google.devtools.build.lib.util.OS;
-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;
 import java.util.concurrent.ConcurrentHashMap;
@@ -45,7 +34,6 @@
 @ThreadSafe
 public class RunfilesTreeUpdater {
   private final Path execRoot;
-  private final XattrProvider xattrProvider;
 
   /**
    * Deduplicates multiple attempts to update the same runfiles tree.
@@ -58,9 +46,8 @@ public class RunfilesTreeUpdater {
   private final ConcurrentHashMap<PathFragment, CompletableFuture<Void>> updatedTrees =
       new ConcurrentHashMap<>();
 
-  public RunfilesTreeUpdater(Path execRoot, XattrProvider xattrProvider) {
+  public RunfilesTreeUpdater(Path execRoot) {
     this.execRoot = execRoot;
-    this.xattrProvider = xattrProvider;
   }
 
   /** Creates or updates input runfiles trees for a spawn. */
@@ -111,28 +98,15 @@ private void updateRunfilesTree(RunfilesTree tree) throws IOException, ExecExcep
     }
     Path outputManifest =
         execRoot.getRelative(RunfilesSupport.outputManifestExecPath(tree.getExecPath()));
-    try {
-      // Avoid rebuilding the runfiles directory if the manifest in it matches the input manifest,
-      // 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.
-      // 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() == RunfileSymlinksMode.CREATE
-          && !outputManifest.isSymbolicLink()
-          && Arrays.equals(
-              DigestUtils.getDigestWithManualFallback(outputManifest, xattrProvider),
-              DigestUtils.getDigestWithManualFallback(inputManifest, xattrProvider))
-          && (OS.getCurrent() != OS.WINDOWS
-              || isRunfilesDirectoryPopulated(runfilesDir, outputManifest))) {
-        return;
-      }
-    } catch (IOException e) {
-      // Ignore it - we will just try to create runfiles directory.
-    }
+    // Note that the runfiles directory is not checked for being up to date here: the only cheap
+    // signal available for that is the output manifest matching the input manifest, which merely
+    // states that the *set* of runfiles is unchanged. That implies that the tree is up to date only
+    // if it consists of symbolic links, whose targets are the authoritative files. It does not on a
+    // file system that materializes symlinks as copies (Windows without --windows_enable_symlinks),
+    // where the contents of the tree can be stale even though the manifest is unchanged - and since
+    // linkManifest() makes the output manifest a symbolic link on every other file system, that is
+    // the only situation in which such a check would ever apply. Recreating the tree is cheap if it
+    // is already up to date: SymlinkTreeHelper only mutates entries that don't match.
 
     if (!runfilesDir.exists()) {
       runfilesDir.createDirectoryAndParents();
@@ -149,18 +123,4 @@ private void updateRunfilesTree(RunfilesTree tree) throws IOException, ExecExcep
       case SKIP -> helper.createMinimalRunfilesDirectory();
     }
   }
-
-  private static boolean isRunfilesDirectoryPopulated(Path runfilesDir, Path outputManifest) {
-    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 = Splitter.on(' ').splitToList(reader.readLine()).get(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 runfilesDir.getRelative(relativeRunfilePath).exists(Symlinks.NOFOLLOW);
-  }
 }

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

@@ -204,6 +204,34 @@ static ImmutableMap<PathFragment, PathFragment> processFilesetLinks(
     return symlinks.buildOrThrow();
   }
 
+  /**
+   * Returns whether the given path is an existing, up-to-date copy of the given symlink target.
+   *
+   * <p>File systems that don't support symbolic links natively materialize them as copies of their
+   * target, so the contents of the tree they produce can go stale when a target is modified. Since
+   * a copy is created after its target was last modified, it is considered up to date as long as
+   * the target hasn't been modified since and its size is unchanged. This is the same approximation
+   * that {@link com.google.devtools.build.lib.actions.FileContentsProxy} makes for local files.
+   *
+   * <p>Always returns false on a file system that creates real symbolic links, where an entry that
+   * isn't a symlink is never up to date.
+   */
+  private static boolean isUpToDateCopy(Path link, PathFragment target) throws IOException {
+    if (link.getFileSystem().supportsSymbolicLinksNatively(link.asFragment())) {
+      return false;
+    }
+    // A relative target is resolved relative to the directory containing the link, matching how the
+    // file system resolves it when creating the copy.
+    Path targetPath = link.getParentDirectory().getRelative(target);
+    FileStatus linkStat = link.statIfFound(Symlinks.NOFOLLOW);
+    FileStatus targetStat = targetPath.statIfFound(Symlinks.FOLLOW);
+    if (linkStat == null || targetStat == null || !linkStat.isFile() || !targetStat.isFile()) {
+      return false;
+    }
+    return linkStat.getSize() == targetStat.getSize()
+        && linkStat.getLastModifiedTime() >= targetStat.getLastModifiedTime();
+  }
+
   private static final class Directory<T> {
     private final Map<String, T> symlinks = new HashMap<>();
     private final Map<String, Directory<T>> directories = new HashMap<>();
@@ -250,12 +278,18 @@ void syncTreeRecursively(Path at, TargetPathFunction<T> targetPathFn) throws IOE
             // TODO(tjgq): Ponder whether this is still necessary to preserve the intentional
             // non-hermeticity of symlink trees under source edits.
           } else {
+            PathFragment target = targetPathFn.get(value);
             // ensureSymbolicLink will replace a symlink that doesn't have the correct target, but
-            // everything else needs to be deleted first.
+            // everything else needs to be deleted first. An existing copy of the target is kept if
+            // it is still up to date, which avoids recreating the entire tree on every build on
+            // file systems that materialize symlinks as copies.
             if (dirent.getType() != Dirent.Type.SYMLINK) {
+              if (isUpToDateCopy(next, target)) {
+                continue;
+              }
               next.deleteTree();
             }
-            FileSystemUtils.ensureSymbolicLink(next, targetPathFn.get(value));
+            FileSystemUtils.ensureSymbolicLink(next, target);
           }
         } else if (directories.containsKey(basename)) {
           Directory<T> nextDir = directories.remove(basename);

src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java

@@ -757,7 +757,7 @@ public Path getActionTempsDirectory() {
   public RunfilesTreeUpdater getRunfilesTreeUpdater() {
     synchronized (runfilesTreeUpdaterLock) {
       if (runfilesTreeUpdater == null) {
-        runfilesTreeUpdater = new RunfilesTreeUpdater(getExecRoot(), getXattrProvider());
+        runfilesTreeUpdater = new RunfilesTreeUpdater(getExecRoot());
       }
       return runfilesTreeUpdater;
     }

src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java

@@ -19,6 +19,7 @@
 import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.actions.ArtifactRoot.RootType;
@@ -30,9 +31,11 @@
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
+import com.google.devtools.build.lib.vfs.SymlinkTargetType;
 import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem;
 import com.google.testing.junit.testparameterinjector.TestParameter;
 import com.google.testing.junit.testparameterinjector.TestParameterInjector;
+import java.io.IOException;
 import java.util.HashMap;
 import java.util.Map;
 import org.junit.Before;
@@ -139,4 +142,82 @@ public void createSymlinks(@TestParameter TreeType treeType, @TestParameter bool
     assertThat(treeSymlink.readSymbolicLink()).isEqualTo(PathFragment.create("/path/to/target"));
     assertThat(treeMissing.exists()).isFalse();
   }
+
+  /**
+   * A file system that materializes symbolic links to files as copies of their target, mimicking
+   * Windows without {@code --windows_enable_symlinks}.
+   */
+  private static final class CopyingFileSystem extends InMemoryFileSystem {
+    CopyingFileSystem() {
+      super(DigestHashFunction.SHA256);
+    }
+
+    @Override
+    public boolean supportsSymbolicLinksNatively(PathFragment path) {
+      return false;
+    }
+
+    @Override
+    public void createSymbolicLink(
+        PathFragment linkPath, PathFragment targetFragment, SymlinkTargetType type)
+        throws IOException {
+      PathFragment resolvedTarget =
+          targetFragment.isAbsolute()
+              ? targetFragment
+              : linkPath.getParentDirectory().getRelative(targetFragment);
+      Path target = getPath(resolvedTarget);
+      if (!target.isFile()) {
+        throw new IOException("not a file: " + resolvedTarget);
+      }
+      FileSystemUtils.copyFile(target, getPath(linkPath));
+    }
+  }
+
+  @Test
+  public void createSymlinks_copyingFileSystem(@TestParameter boolean targetModified)
+      throws Exception {
+    FileSystem copyingFs = new CopyingFileSystem();
+    Path copyingExecRoot = copyingFs.getPath("/execroot");
+    ArtifactRoot copyingOutputRoot =
+        ArtifactRoot.asDerivedRoot(copyingExecRoot, RootType.OUTPUT, "out");
+    copyingOutputRoot.getRoot().asPath().createDirectoryAndParents();
+
+    Path treeRoot = copyingExecRoot.getRelative("foo.runfiles");
+    SymlinkTreeHelper helper =
+        new SymlinkTreeHelper(
+            copyingExecRoot.getRelative("foo.runfiles_manifest"),
+            treeRoot.getRelative("MANIFEST"),
+            treeRoot,
+            WORKSPACE_NAME);
+
+    Artifact file = ActionsTestUtil.createArtifact(copyingOutputRoot, "file");
+    FileSystemUtils.writeContent(file.getPath(), UTF_8, "content");
+    file.getPath().setLastModifiedTime(1000);
+
+    Map<PathFragment, Artifact> symlinkMap =
+        ImmutableMap.of(PathFragment.create(WORKSPACE_NAME + "/file"), file);
+    helper.createRunfilesSymlinks(symlinkMap);
+
+    Path treeFile = treeRoot.getRelative(WORKSPACE_NAME + "/file");
+    assertThat(treeFile.isSymbolicLink()).isFalse();
+    assertThat(FileSystemUtils.readContent(treeFile, UTF_8)).isEqualTo("content");
+    treeFile.setLastModifiedTime(2000);
+    long nodeIdBefore = treeFile.stat().getNodeId();
+
+    if (targetModified) {
+      FileSystemUtils.writeContent(file.getPath(), UTF_8, "new content");
+      file.getPath().setLastModifiedTime(3000);
+    }
+
+    helper.createRunfilesSymlinks(symlinkMap);
+
+    if (targetModified) {
+      // The stale copy must be replaced with an up-to-date one.
+      assertThat(FileSystemUtils.readContent(treeFile, UTF_8)).isEqualTo("new content");
+    } else {
+      // The up-to-date copy must be left alone instead of being recreated.
+      assertThat(FileSystemUtils.readContent(treeFile, UTF_8)).isEqualTo("content");
+      assertThat(treeFile.stat().getNodeId()).isEqualTo(nodeIdBefore);
+    }
+  }
 }

src/test/py/bazel/runfiles_test.py

@@ -429,6 +429,49 @@ def testRunfilesDirectoryIncrementalityNoBuildRunfileLinksEnableRunfilesFlippedO
     )
     self.assertNotEqual(exit_code, 0)
 
+  def testRepoMappingSignalsMaterializedRunfilesDirectory(self):
+    self.ScratchFile("MODULE.bazel")
+    self.AddBazelDep("rules_shell")
+    self.ScratchFile(
+        "BUILD",
+        [
+            'load("@rules_shell//shell:sh_test.bzl", "sh_test")',
+            "sh_test(",
+            "  name = 'test',",
+            "  srcs = ['test.sh'],",
+            "  data = ['data.txt'],",
+            ")",
+        ],
+    )
+    self.ScratchFile("data.txt")
+    # _repo_mapping is present exactly if the runfiles directory has been
+    # materialized, in which case the test setup must not ask runfiles libraries
+    # to use the manifest.
+    self.ScratchFile(
+        "test.sh",
+        [
+            'if [[ -f "${TEST_SRCDIR}/_repo_mapping" ]]; then',
+            '  [[ -z "${RUNFILES_MANIFEST_ONLY:-}" ]] || exit 1',
+            '  [[ -f "${TEST_SRCDIR}/_main/data.txt" ]] || exit 1',
+            "else",
+            '  [[ "${RUNFILES_MANIFEST_ONLY:-}" == "1" ]] || exit 1',
+            '  [[ -f "${RUNFILES_MANIFEST_FILE}" ]] || exit 1',
+            "fi",
+        ],
+        executable=True,
+    )
+
+    _, stdout, _ = self.RunBazel(["info", "bazel-bin"])
+    repo_mapping = os.path.join(stdout[0], "test.runfiles", "_repo_mapping")
+
+    self.RunBazel(["test", ":test", "--enable_runfiles", "--test_output=errors"])
+    self.assertTrue(os.path.exists(repo_mapping))
+
+    self.RunBazel(
+        ["test", ":test", "--noenable_runfiles", "--test_output=errors"]
+    )
+    self.assertFalse(os.path.exists(repo_mapping))
+
   def testTestsRunWithNoBuildRunfileLinksAndNoEnableRunfiles(self):
     self.ScratchFile("MODULE.bazel")
     self.AddBazelDep("rules_shell")