From 7c1d64af6c6ddd72b22f19f240f49bd057fa2f5a Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 13 Aug 2026 17:24:39 +0200 Subject: [PATCH 1/2] Detect a materialized runfiles directory via `_repo_mapping` Runfiles libraries currently can't tell a fully materialized runfiles directory apart from one that only contains the manifest and the workspace subdirectory, which is what `RunfileSymlinksMode.SKIP` leaves behind. They are told about this out of band via the `RUNFILES_MANIFEST_ONLY` environment variable, which Bazel derives from the value of `--enable_runfiles` alone and thus is wrong whenever the runfiles are materialized by the sandbox or by remote execution anyway. The `_repo_mapping` file is exactly the signal that is missing here: it is an ordinary runfiles entry and is thus created by all the mechanisms that also materialize the remaining runfiles, but not by `SymlinkTreeHelper#createMinimalRunfilesDirectory`. Since it has been part of every runfiles tree since Bazel 6.0, runfiles libraries can start relying on it without waiting for a new Bazel release. Use it in the test setup scripts and in the Windows launcher, which no longer needs the `symlink_runfiles_enabled` launch info key. `RUNFILES_MANIFEST_ONLY` is still set for runfiles libraries that don't check for `_repo_mapping` yet, but the test setup scripts now derive it from the runfiles directory rather than passing it through, which makes `TestRunnerAction` setting it redundant. Note that `tw.cc` has to clear it just like `test-setup.sh` does: `TestPolicy#computeTestEnvironment` resolves the environment common to all actions into every test's environment, and `BazelRuleClassProvider` adds it to that whenever `--enable_runfiles` is off. --- .../lib/analysis/test/TestRunnerAction.java | 5 -- .../bazel/rules/BazelRuleClassProvider.java | 13 ++-- src/test/py/bazel/runfiles_test.py | 43 +++++++++++ src/test/shell/bazel/runfiles_test.sh | 74 +++++++++++++++++++ src/tools/launcher/launcher.cc | 32 +++++--- src/tools/launcher/launcher.h | 6 +- tools/test/test-setup.sh | 15 +++- tools/test/windows/tw.cc | 49 +++++++----- 8 files changed, 188 insertions(+), 49 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java index d7b71c983c0970..5e98a9664b352b 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java @@ -785,11 +785,6 @@ public void setupEnvVariables(Map 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. diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java index 2639ed2e225f67..82e22e805138f5 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java +++ b/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"); } diff --git a/src/test/py/bazel/runfiles_test.py b/src/test/py/bazel/runfiles_test.py index 721f974d868f62..dadb61e3d1981e 100644 --- a/src/test/py/bazel/runfiles_test.py +++ b/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") diff --git a/src/test/shell/bazel/runfiles_test.sh b/src/test/shell/bazel/runfiles_test.sh index ed7bb26dfd24c9..71b4a7999d7b19 100755 --- a/src/test/shell/bazel/runfiles_test.sh +++ b/src/test/shell/bazel/runfiles_test.sh @@ -84,6 +84,80 @@ EOF [[ -f bazel-bin/bin.runfiles/MANIFEST ]] || fail "expected output manifest to exist" } +# Runfiles libraries rely on _repo_mapping being present in the runfiles directory if and only if +# it has been fully materialized. +function test_repo_mapping_signals_materialized_runfiles_directory() { + mkdir data && echo "hello" > data/hello + + touch bin.sh + chmod 755 bin.sh + + add_rules_shell "MODULE.bazel" + + cat > BUILD <<'EOF' +load("@rules_shell//shell:sh_binary.bzl", "sh_binary") + +sh_binary( + name = "bin", + srcs = ["bin.sh"], + data = glob(["data/*"]), +) +EOF + + bazel build --enable_runfiles //:bin || fail "Building //:bin failed" + + [[ -f bazel-bin/bin.runfiles/_repo_mapping ]] \ + || fail "expected _repo_mapping in a materialized runfiles directory" + grep -q '^_repo_mapping ' bazel-bin/bin.runfiles/MANIFEST \ + || fail "expected _repo_mapping in the manifest" + + bazel build --noenable_runfiles //:bin || fail "Building //:bin failed" + + [[ ! -f bazel-bin/bin.runfiles/_repo_mapping ]] \ + || fail "expected no _repo_mapping in an unmaterialized runfiles directory" + grep -q '^_repo_mapping ' bazel-bin/bin.runfiles/MANIFEST \ + || fail "expected _repo_mapping in the manifest" +} + +# The sandbox materializes the runfiles directory of a tool even with +# --noenable_runfiles, so _repo_mapping has to be present there. +function test_repo_mapping_in_sandbox() { + mkdir data && echo "hello" > data/hello + + cat > tool.sh <<'EOF' +#!/usr/bin/env bash +set -euo pipefail +[[ -f "$0.runfiles/_repo_mapping" ]] || { + echo "_repo_mapping missing from $0.runfiles" >&2 + exit 1 +} +touch "$1" +EOF + chmod 755 tool.sh + + add_rules_shell "MODULE.bazel" + + cat > BUILD <<'EOF' +load("@rules_shell//shell:sh_binary.bzl", "sh_binary") + +sh_binary( + name = "tool", + srcs = ["tool.sh"], + data = glob(["data/*"]), +) + +genrule( + name = "gen", + outs = ["gen.txt"], + tools = [":tool"], + cmd = "$(execpath :tool) $@", +) +EOF + + bazel build --spawn_strategy=sandboxed --noenable_runfiles //:gen >& $TEST_log \ + || fail "Building //:gen failed" +} + # Test that the local strategy creates a runfiles tree during test if no --nobuild_runfile_links # is specified. function test_nobuild_runfile_links() { diff --git a/src/tools/launcher/launcher.cc b/src/tools/launcher/launcher.cc index b1c989aee4b99e..032971ec3a16d6 100644 --- a/src/tools/launcher/launcher.cc +++ b/src/tools/launcher/launcher.cc @@ -63,6 +63,13 @@ static wstring GetRunfilesDir(const wchar_t* launcher_path) { return result; } +// Returns true if the runfiles directory has been fully materialized. The +// _repo_mapping file is an ordinary runfile and is thus present if and only if +// that is the case. +static bool IsRunfilesDirectoryPopulated(const wstring& runfiles_dir) { + return DoesFilePathExist((runfiles_dir + L"\\_repo_mapping").c_str()); +} + BinaryLauncherBase::BinaryLauncherBase( const LaunchDataParser::LaunchInfo& _launch_info, const std::wstring& launcher_path, int argc, wchar_t* argv[]) @@ -71,15 +78,13 @@ BinaryLauncherBase::BinaryLauncherBase( manifest_file(FindManifestFile(launcher_path.c_str())), runfiles_dir(GetRunfilesDir(launcher_path.c_str())), workspace_name(GetLaunchInfoByKey(WORKSPACE_NAME)), - symlink_runfiles_enabled(GetLaunchInfoByKey(SYMLINK_RUNFILES_ENABLED) == - L"1") { + runfiles_dir_populated(IsRunfilesDirectoryPopulated(runfiles_dir)) { for (int i = 0; i < argc; i++) { commandline_arguments.push_back(argv[i]); } - // Prefer to use the runfiles manifest, if it exists, but otherwise the - // runfiles directory will be used by default. On Windows, the manifest is - // used locally, and the runfiles directory is used remotely. - if (!manifest_file.empty()) { + // Prefer to resolve runfiles against the runfiles directory and only fall + // back to the manifest if the directory hasn't been fully materialized. + if (!runfiles_dir_populated && !manifest_file.empty()) { ParseManifestFile(&manifest_file_map, manifest_file); } } @@ -259,14 +264,17 @@ ExitCode BinaryLauncherBase::LaunchProcess(const wstring& executable, if (PrintLauncherCommandLine(executable, escaped_arguments)) { return 0; } - // Set RUNFILES_DIR if: - // 1. Symlink runfiles tree is enabled, or - // 2. We couldn't find manifest file (which probably means we are running - // remotely). - // Otherwise, set RUNFILES_MANIFEST_ONLY and RUNFILES_MANIFEST_FILE - if (symlink_runfiles_enabled || manifest_file.empty()) { + // Set RUNFILES_DIR if the runfiles directory has been fully materialized or + // if there is no manifest to fall back to. Otherwise, point the child at the + // manifest. + if (runfiles_dir_populated || manifest_file.empty()) { SetEnv(L"RUNFILES_DIR", runfiles_dir); + // Clear a value inherited from the environment, which may not reflect how + // the runfiles of this binary were materialized. + SetEnv(L"RUNFILES_MANIFEST_ONLY", L""); } else { + // TODO: Remove RUNFILES_MANIFEST_ONLY once all runfiles libraries determine + // whether the runfiles directory is usable by checking for _repo_mapping. SetEnv(L"RUNFILES_MANIFEST_ONLY", L"1"); SetEnv(L"RUNFILES_MANIFEST_FILE", manifest_file); } diff --git a/src/tools/launcher/launcher.h b/src/tools/launcher/launcher.h index 43fe8dfb5ece5c..0b51d6ab6b3a08 100644 --- a/src/tools/launcher/launcher.h +++ b/src/tools/launcher/launcher.h @@ -26,8 +26,6 @@ namespace launcher { typedef int32_t ExitCode; static constexpr const char* WORKSPACE_NAME = "workspace_name"; -static constexpr const char* SYMLINK_RUNFILES_ENABLED = - "symlink_runfiles_enabled"; // The maximum length of lpCommandLine is 32768 characters. // https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx @@ -117,8 +115,8 @@ class BinaryLauncherBase { // A map to store all entries of the manifest file. ManifestFileMap manifest_file_map; - // If symlink runfiles tree is enabled, this value is true. - const bool symlink_runfiles_enabled; + // Whether the runfiles directory has been fully materialized. + const bool runfiles_dir_populated; // If --print_launcher_command is presented in arguments, // then print the command line. diff --git a/tools/test/test-setup.sh b/tools/test/test-setup.sh index c135507051ac96..88d77e577a87a7 100755 --- a/tools/test/test-setup.sh +++ b/tools/test/test-setup.sh @@ -132,11 +132,18 @@ function rlocation() { fi } -# If RUNFILES_MANIFEST_ONLY is set to 1 and the manifest file does exist, -# then test programs should use manifest file to find runfiles. -if [[ "${RUNFILES_MANIFEST_ONLY:-}" == "1" && -e "${RUNFILES_MANIFEST_FILE:-}" ]]; then +# The _repo_mapping file is an ordinary runfile and is thus present in the runfiles directory if and +# only if that directory has been fully materialized. If it hasn't, test programs have to use the +# manifest to find their runfiles. Bazel adds RUNFILES_MANIFEST_ONLY to the environment common to +# all actions whenever --enable_runfiles is off, which doesn't account for the runfiles directory +# being materialized by the sandbox or by remote execution, so it has to be cleared if it is. +if [[ -e "${TEST_SRCDIR}/_repo_mapping" || ! -e "${RUNFILES_MANIFEST_FILE:-}" ]]; then + unset RUNFILES_MANIFEST_ONLY +else export RUNFILES_MANIFEST_FILE - export RUNFILES_MANIFEST_ONLY + # TODO: Remove this once all runfiles libraries determine whether the runfiles directory is + # usable by checking for _repo_mapping. + export RUNFILES_MANIFEST_ONLY=1 fi DIR="$TEST_SRCDIR" diff --git a/tools/test/windows/tw.cc b/tools/test/windows/tw.cc index 0b95aa0ee45cd7..43f4a9b1b0abfc 100644 --- a/tools/test/windows/tw.cc +++ b/tools/test/windows/tw.cc @@ -338,6 +338,15 @@ bool DirectoryExists(const Path& p) { ((attrs & FILE_ATTRIBUTE_DIRECTORY) != 0); } +// Returns true if the runfiles directory has been fully materialized. The +// _repo_mapping file is an ordinary runfile and is thus present if and only if +// that is the case. +bool IsRunfilesDirectoryPopulated(const Path& runfiles_dir) { + Path repo_mapping; + return repo_mapping.Set(runfiles_dir.Get() + L"\\_repo_mapping") && + IsReadableFile(repo_mapping); +} + // Gets an environment variable's value. // Returns: // - true, if the envvar is defined and successfully fetched, or it's empty or @@ -599,20 +608,27 @@ bool ExportRunfiles(const Path& cwd, const Path& test_srcdir, return false; } - std::wstring mf_only_str; - int mf_only_value = 0; - if (!GetIntEnv(L"RUNFILES_MANIFEST_ONLY", &mf_only_str, &mf_only_value)) { - return false; - } - if (mf_only_value == 1) { - // If RUNFILES_MANIFEST_ONLY is set to 1 then test programs should use the - // manifest file to find their runfiles. + // If the runfiles directory hasn't been fully materialized then test programs + // have to use the manifest file to find their runfiles. Bazel adds + // RUNFILES_MANIFEST_ONLY to the environment common to all actions whenever + // --enable_runfiles is off, which doesn't account for the runfiles directory + // being materialized by remote execution, so it has to be cleared if it is. + if (IsRunfilesDirectoryPopulated(test_srcdir)) { + if (!UnsetEnv(L"RUNFILES_MANIFEST_ONLY")) { + return false; + } + } else { Path runfiles_mf; if (!runfiles_mf.Set(test_srcdir.Get() + L"\\MANIFEST") || (IsReadableFile(runfiles_mf) && !SetPathEnv(env_prefix + L"RUNFILES_MANIFEST_FILE", runfiles_mf))) { return false; } + // TODO: Remove this once all runfiles libraries determine whether the + // runfiles directory is usable by checking for _repo_mapping. + if (!SetEnv(L"RUNFILES_MANIFEST_ONLY", L"1")) { + return false; + } } return true; @@ -1191,17 +1207,12 @@ bool FindTestBinary(const Path& argv0, const Path& cwd, std::wstring test_path, return false; } - std::wstring mf_only_str; - int mf_only_value = 0; - if (!GetIntEnv(L"RUNFILES_MANIFEST_ONLY", &mf_only_str, &mf_only_value)) { - return false; - } - - // If runfiles is enabled on Windows, we use the test binary in the runfiles - // tree, which is consistent with the behavior on Linux and macOS. - // Otherwise, we use Rlocation function to find the actual test binary - // location. - if (mf_only_value != 1 && IsReadableFile(test_bin_in_runfiles)) { + // If the runfiles directory has been fully materialized, we use the test + // binary in the runfiles tree, which is consistent with the behavior on + // Linux and macOS. Otherwise, we use the Rlocation function to find the + // actual test binary location. + if (IsRunfilesDirectoryPopulated(abs_test_srcdir) && + IsReadableFile(test_bin_in_runfiles)) { test_path = test_bin_in_runfiles.Get(); } else { std::string utf8_test_path; From 1743eaefb52bf799f289a4af6de11661485bc171 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Thu, 13 Aug 2026 17:24:45 +0200 Subject: [PATCH 2/2] Don't let a runfiles tree of copies go stale `RunfilesTreeUpdater` skipped recreating a runfiles tree if the output manifest in it matched the input manifest. The manifest only records the *set* of runfiles, so this 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 a runfile that is modified without the manifest changing leaves a stale copy behind. Since `linkManifest()` makes the output manifest a symbolic link on every file system that supports them, the check would never both apply and be sound, so drop it. To keep recreating the tree cheap where the check used to fire, `SymlinkTreeHelper` now keeps an existing copy that is still up to date instead of deleting and rewriting it, which also stops a single modified runfile from causing the entire tree to be copied again. Claude-Session: https://claude.ai/code/session_013o72rMrpgYKD9wYYQ2Nr98 --- .../build/lib/exec/RunfilesTreeUpdater.java | 60 +++----------- .../build/lib/exec/SymlinkTreeHelper.java | 38 ++++++++- .../build/lib/runtime/CommandEnvironment.java | 2 +- .../build/lib/exec/SymlinkTreeHelperTest.java | 81 +++++++++++++++++++ 4 files changed, 128 insertions(+), 53 deletions(-) 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 index 09c14b1709adfb..ed47021166af27 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java +++ b/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> 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); - } } 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 index 7dd64d99ff72c8..bef9753b3abaa9 100644 --- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java +++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java @@ -204,6 +204,34 @@ static ImmutableMap processFilesetLinks( return symlinks.buildOrThrow(); } + /** + * Returns whether the given path is an existing, up-to-date copy of the given symlink target. + * + *

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. + * + *

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 { private final Map symlinks = new HashMap<>(); private final Map> directories = new HashMap<>(); @@ -250,12 +278,18 @@ void syncTreeRecursively(Path at, TargetPathFunction 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 nextDir = directories.remove(basename); diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java index 8d1307ab83681d..7d1870425697b6 100644 --- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java +++ b/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; } 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 index 404aafb4e9ec06..cf8a43f3989e80 100644 --- a/src/test/java/com/google/devtools/build/lib/exec/SymlinkTreeHelperTest.java +++ b/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 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); + } + } }