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/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); + } + } } 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;