Make runfiles usage on Windows more flexible to support remote execution.
When trying to find a runfile on Windows:
1. First look for the runfiles MANIFEST and find runfile locations using this if it exists (current behavior).
2. If no MANIFEST file exists, look for runfiles in the runfiles directory (new behavior).
As part of this, remove setting RUNFILES_MANIFEST_ONLY for the benefit of test-setup.sh. Instead of telling it what to do, it decides what to do based on the observed state of the world. Launchers still set RUNFILES_MANIFEST_ONLY for the benefit of launched programs, since some may depend on this.
Fixes https://github.com/bazelbuild/bazel/issues/4962.
RELNOTES: Remote execution works for Windows binaries with launchers.
PiperOrigin-RevId: 194785440
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 eb754bd..16f15b5 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
@@ -508,11 +508,6 @@
}
env.put("XML_OUTPUT_FILE", getXmlOutputPath().getPathString());
- if (!isEnableRunfiles()) {
- // 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/test/py/bazel/test_base.py b/src/test/py/bazel/test_base.py
index 345e341..5ef0084 100644
--- a/src/test/py/bazel/test_base.py
+++ b/src/test/py/bazel/test_base.py
@@ -61,12 +61,25 @@
self._test_cwd = tempfile.mkdtemp(dir=self._tests_root)
os.chdir(self._test_cwd)
- def AssertExitCode(self, actual_exit_code, expected_exit_code, stderr_lines):
+ def AssertExitCode(self,
+ actual_exit_code,
+ expected_exit_code,
+ stderr_lines,
+ stdout_lines=None):
"""Assert that `actual_exit_code` == `expected_exit_code`."""
if actual_exit_code != expected_exit_code:
+ # If stdout was provided, include it in the output. This is mostly useful
+ # for tests.
+ stdout = '\n'.join([
+ '(start stdout)----------------------------------------',
+ ] + stdout_lines + [
+ '(end stdout)------------------------------------------',
+ ]) if stdout_lines is not None else ''
+
self.fail('\n'.join([
'Bazel exited with %d (expected %d), stderr:' % (actual_exit_code,
expected_exit_code),
+ stdout,
'(start stderr)----------------------------------------',
] + (stderr_lines or []) + [
'(end stderr)------------------------------------------',
diff --git a/src/test/py/bazel/windows_remote_test.py b/src/test/py/bazel/windows_remote_test.py
index 11b6d0d..0ec3fbd 100644
--- a/src/test/py/bazel/windows_remote_test.py
+++ b/src/test/py/bazel/windows_remote_test.py
@@ -21,7 +21,9 @@
class WindowsRemoteTest(test_base.TestBase):
- def _RunRemoteBazel(self, args, port, env_remove=None, env_add=None):
+ _worker_port = None
+
+ def _RunRemoteBazel(self, args, env_remove=None, env_add=None):
return self.RunBazel(
args + [
'--spawn_strategy=remote',
@@ -29,8 +31,8 @@
'--strategy=Closure=remote',
'--genrule_strategy=remote',
'--define=EXECUTOR=remote',
- '--remote_executor=localhost:' + str(port),
- '--remote_cache=localhost:' + str(port),
+ '--remote_executor=localhost:' + str(self._worker_port),
+ '--remote_cache=localhost:' + str(self._worker_port),
'--experimental_strict_action_env=true',
'--remote_timeout=3600',
'--auth_enabled=false',
@@ -39,10 +41,18 @@
env_remove=env_remove,
env_add=env_add)
+ def setUp(self):
+ test_base.TestBase.setUp(self)
+ self._worker_port = self.StartRemoteWorker()
+
+ def tearDown(self):
+ test_base.TestBase.tearDown(self)
+ self.StopRemoteWorker()
+
# Check that a binary built remotely is runnable locally. Among other things,
# this means the runfiles manifest, which is not present remotely, must exist
# locally.
- def testBinaryRunnableLocally(self):
+ def testBinaryRunsLocally(self):
self.ScratchFile('WORKSPACE')
self.ScratchFile('foo/BUILD', [
'sh_binary(',
@@ -53,7 +63,7 @@
])
self.ScratchFile(
'foo/foo.sh', [
- '#!/bin/bash',
+ '#!/bin/sh',
'echo hello shell',
], executable=True)
self.ScratchFile('bar/BUILD', ['exports_files(["bar.txt"])'])
@@ -63,25 +73,148 @@
self.AssertExitCode(exit_code, 0, stderr)
bazel_bin = stdout[0]
- port = self.StartRemoteWorker()
+ # Build.
+ exit_code, stdout, stderr = self._RunRemoteBazel(['build', '//foo:foo'])
+ self.AssertExitCode(exit_code, 0, stderr, stdout)
- try:
- # Build.
- exit_code, stdout, stderr = self._RunRemoteBazel(['build', '//foo:foo'],
- port)
- print('\n'.join(stdout))
- self.AssertExitCode(exit_code, 0, stderr)
+ # Run.
+ foo_bin = os.path.join(bazel_bin, 'foo', 'foo.exe')
+ self.assertTrue(os.path.exists(foo_bin))
+ exit_code, stdout, stderr = self.RunProgram([foo_bin])
+ self.AssertExitCode(exit_code, 0, stderr, stdout)
+ self.assertEqual(stdout, ['hello shell'])
- # Run.
- foo_bin = os.path.join(bazel_bin, 'foo', 'foo.exe')
- self.assertTrue(os.path.exists(foo_bin))
- exit_code, stdout, stderr = self.RunProgram([foo_bin])
- self.AssertExitCode(exit_code, 0, stderr)
- self.assertEqual(stdout, ['hello shell'])
- finally:
- # Always stop the worker so we obtain logs in case an assertion failed
- # above.
- self.StopRemoteWorker()
+ def testShTestRunsLocally(self):
+ self.ScratchFile('WORKSPACE')
+ self.ScratchFile('foo/BUILD', [
+ 'sh_test(',
+ ' name = "foo_test",',
+ ' srcs = ["foo_test.sh"],',
+ ' data = ["//bar:bar.txt"],',
+ ')',
+ ])
+ self.ScratchFile(
+ 'foo/foo_test.sh', ['#!/bin/sh', 'echo hello test'], executable=True)
+ self.ScratchFile('bar/BUILD', ['exports_files(["bar.txt"])'])
+ self.ScratchFile('bar/bar.txt', ['hello'])
+
+ exit_code, stdout, stderr = self.RunBazel(['info', 'bazel-bin'])
+ self.AssertExitCode(exit_code, 0, stderr)
+ bazel_bin = stdout[0]
+
+ # Build.
+ exit_code, stdout, stderr = self._RunRemoteBazel(
+ ['build', '--test_output=all', '//foo:foo_test'])
+ self.AssertExitCode(exit_code, 0, stderr, stdout)
+
+ # Test.
+ foo_test_bin = os.path.join(bazel_bin, 'foo', 'foo_test.exe')
+ self.assertTrue(os.path.exists(foo_test_bin))
+ exit_code, stdout, stderr = self.RunProgram([foo_test_bin])
+ self.AssertExitCode(exit_code, 0, stderr, stdout)
+
+ # Remotely, the runfiles manifest does not exist.
+ def testShTestRunsRemotely(self):
+ self.ScratchFile('WORKSPACE')
+ self.ScratchFile('foo/BUILD', [
+ 'sh_test(',
+ ' name = "foo_test",',
+ ' srcs = ["foo_test.sh"],',
+ ' data = ["//bar:bar.txt"],',
+ ')',
+ ])
+ self.ScratchFile(
+ 'foo/foo_test.sh', ['#!/bin/sh', 'echo hello test'], executable=True)
+ self.ScratchFile('bar/BUILD', ['exports_files(["bar.txt"])'])
+ self.ScratchFile('bar/bar.txt', ['hello'])
+
+ # Test.
+ exit_code, stdout, stderr = self._RunRemoteBazel(
+ ['test', '--test_output=all', '//foo:foo_test'])
+ self.AssertExitCode(exit_code, 0, stderr, stdout)
+
+ # The Java launcher uses Rlocation which has differing behavior for local and
+ # remote.
+ def testJavaTestRunsRemotely(self):
+ self.ScratchFile('WORKSPACE')
+ self.ScratchFile('foo/BUILD', [
+ 'java_test(',
+ ' name = "foo_test",',
+ ' srcs = ["TestFoo.java"],',
+ ' main_class = "TestFoo",',
+ ' use_testrunner = 0,',
+ ' data = ["//bar:bar.txt"],',
+ ')',
+ ])
+ self.ScratchFile(
+ 'foo/TestFoo.java', [
+ 'public class TestFoo {',
+ 'public static void main(String[] args) {',
+ 'System.out.println("hello java test");',
+ '}',
+ '}',
+ ],
+ executable=True)
+ self.ScratchFile('bar/BUILD', ['exports_files(["bar.txt"])'])
+ self.ScratchFile('bar/bar.txt', ['hello'])
+
+ # Test.
+ exit_code, stdout, stderr = self._RunRemoteBazel(
+ ['test', '--test_output=all', '//foo:foo_test'])
+ self.AssertExitCode(exit_code, 0, stderr, stdout)
+
+ # Genrules are notably different than tests because RUNFILES_DIR is not set
+ # for genrule tool launchers, so the runfiles directory is discovered based on
+ # the executable path.
+ def testGenruleWithToolRunsRemotely(self):
+ self.ScratchFile('WORKSPACE')
+ # TODO(jsharpe): Replace sh_binary with py_binary once
+ # https://github.com/bazelbuild/bazel/issues/5087 resolved.
+ self.ScratchFile('foo/BUILD', [
+ 'sh_binary(',
+ ' name = "data_tool",',
+ ' srcs = ["data_tool.sh"],',
+ ' data = ["//bar:bar.txt"],',
+ ')',
+ 'sh_binary(',
+ ' name = "tool",',
+ ' srcs = ["tool.sh"],',
+ ' data = [":data_tool"],',
+ ')',
+ 'genrule(',
+ ' name = "genrule",',
+ ' srcs = [],',
+ ' outs = ["out.txt"],',
+ ' cmd = "$(location :tool) > \\"$@\\"",',
+ ' tools = [":tool"],',
+ ')',
+ ])
+ self.ScratchFile(
+ 'foo/tool.sh', [
+ '#!/bin/sh',
+ 'echo hello tool',
+ # TODO(jsharpe): This is kind of an ugly way to call the data
+ # dependency, but the best I can find. Instead, use py_binary +
+ # Python runfiles library here once that's possible.
+ '$RUNFILES_DIR/__main__/foo/data_tool',
+ ],
+ executable=True)
+ self.ScratchFile(
+ 'foo/data_tool.sh', [
+ '#!/bin/sh',
+ 'echo hello data tool',
+ ],
+ executable=True)
+ self.ScratchFile('bar/BUILD', ['exports_files(["bar.txt"])'])
+ self.ScratchFile('bar/bar.txt', ['hello'])
+
+ # Build.
+ exit_code, stdout, stderr = self._RunRemoteBazel(
+ ['build', '//foo:genrule'])
+ self.AssertExitCode(exit_code, 0, stderr, stdout)
+
+ # TODO(jsharpe): Add a py_test example here. Blocked on
+ # https://github.com/bazelbuild/bazel/issues/5087
if __name__ == '__main__':
diff --git a/src/tools/launcher/launcher.cc b/src/tools/launcher/launcher.cc
index d26506e..5157338 100644
--- a/src/tools/launcher/launcher.cc
+++ b/src/tools/launcher/launcher.cc
@@ -33,15 +33,33 @@
using std::unordered_map;
using std::vector;
+static std::string GetRunfilesDir(const char* argv0) {
+ string runfiles_dir;
+ // If RUNFILES_DIR is already set (probably we are either in a test or in a
+ // data dependency) then use it.
+ if (GetEnv("RUNFILES_DIR", &runfiles_dir)) {
+ return runfiles_dir;
+ }
+ // Otherwise this is probably a top-level non-test binary (e.g. a genrule
+ // tool) and should look for its runfiles beside the executable.
+ return GetBinaryPathWithExtension(argv0) + ".runfiles";
+}
+
BinaryLauncherBase::BinaryLauncherBase(
const LaunchDataParser::LaunchInfo& _launch_info, int argc, char* argv[])
: launch_info(_launch_info),
manifest_file(FindManifestFile(argv[0])),
+ runfiles_dir(GetRunfilesDir(argv[0])),
workspace_name(GetLaunchInfoByKey(WORKSPACE_NAME)) {
for (int i = 0; i < argc; i++) {
- this->commandline_arguments.push_back(argv[i]);
+ commandline_arguments.push_back(argv[i]);
}
- ParseManifestFile(&this->manifest_file_map, this->manifest_file);
+ // 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 != "") {
+ ParseManifestFile(&manifest_file_map, manifest_file);
+ }
}
static bool FindManifestFileImpl(const char* argv0, string* result) {
@@ -80,7 +98,7 @@
string BinaryLauncherBase::FindManifestFile(const char* argv0) {
string manifest_file;
if (!FindManifestFileImpl(argv0, &manifest_file)) {
- die("Couldn't find runfiles manifest file.");
+ return "";
}
// The path will be set as the RUNFILES_MANIFEST_FILE envvar and used by the
// shell script, so let's convert backslashes to forward slashes.
@@ -117,6 +135,17 @@
string BinaryLauncherBase::Rlocation(const string& path,
bool need_workspace_name) const {
+ // If the manifest file map is empty, then we're using the runfiles directory
+ // instead.
+ if (manifest_file_map.empty()) {
+ string query_path = runfiles_dir;
+ if (need_workspace_name) {
+ query_path += "/" + this->workspace_name;
+ }
+ query_path += "/" + path;
+ return query_path;
+ }
+
string query_path = path;
if (need_workspace_name) {
query_path = this->workspace_name + "/" + path;
@@ -182,8 +211,12 @@
if (PrintLauncherCommandLine(executable, arguments)) {
return 0;
}
- SetEnv("RUNFILES_MANIFEST_ONLY", "1");
- SetEnv("RUNFILES_MANIFEST_FILE", manifest_file);
+ if (manifest_file != "") {
+ SetEnv("RUNFILES_MANIFEST_ONLY", "1");
+ SetEnv("RUNFILES_MANIFEST_FILE", manifest_file);
+ } else {
+ SetEnv("RUNFILES_DIR", runfiles_dir);
+ }
CmdLine cmdline;
CreateCommandLine(&cmdline, executable, arguments);
PROCESS_INFORMATION processInfo = {0};
diff --git a/src/tools/launcher/launcher.h b/src/tools/launcher/launcher.h
index 088bd5d..4dca6e0 100644
--- a/src/tools/launcher/launcher.h
+++ b/src/tools/launcher/launcher.h
@@ -82,9 +82,12 @@
// A map to store all the launch information.
const LaunchDataParser::LaunchInfo& launch_info;
- // Absolute path to the runfiles manifest file.
+ // Absolute path to the runfiles manifest file, if one exists.
const std::string manifest_file;
+ // Path to the runfiles directory, if one exists.
+ const std::string runfiles_dir;
+
// The commandline arguments recieved.
// The first argument is the path of this launcher itself.
std::vector<std::string> commandline_arguments;
@@ -111,7 +114,7 @@
void CreateCommandLine(CmdLine* result, const std::string& executable,
const std::vector<std::string>& arguments) const;
- // Find manifest file of the binary
+ // Find manifest file of the binary.
//
// Expect the manifest file to be at
// 1. <path>/<to>/<binary>/<target_name>.runfiles/MANIFEST
diff --git a/tools/test/test-setup.sh b/tools/test/test-setup.sh
index faac6bf..b403513 100755
--- a/tools/test/test-setup.sh
+++ b/tools/test/test-setup.sh
@@ -106,27 +106,27 @@
RUNFILES_MANIFEST_FILE="${TEST_SRCDIR}/MANIFEST"
-if [[ "${RUNFILES_MANIFEST_ONLY:-}" != "1" ]]; then
- function rlocation() {
- if is_absolute "$1" ; then
- echo "$1"
- else
- echo "$TEST_SRCDIR/$1"
- fi
- }
-else
- function rlocation() {
- if is_absolute "$1" ; then
- echo "$1"
- else
- echo "$(grep "^$1 " "${RUNFILES_MANIFEST_FILE}" | sed 's/[^ ]* //')"
- fi
- }
-fi
+function rlocation() {
+ if is_absolute "$1" ; then
+ # If the file path is already fully specified, simply return it.
+ echo "$1"
+ elif [[ -e "$TEST_SRCDIR/$1" ]]; then
+ # If the file exists in the $TEST_SRCDIR then just use it.
+ echo "$TEST_SRCDIR/$1"
+ elif [[ -e "$RUNFILES_MANIFEST_FILE" ]]; then
+ # If a runfiles manifest file exists then use it.
+ echo "$(grep "^$1 " "$RUNFILES_MANIFEST_FILE" | sed 's/[^ ]* //')"
+ fi
+}
export -f rlocation
export -f is_absolute
export RUNFILES_MANIFEST_FILE
+# If the runfiles manifest exist, then test programs should use it to find
+# runfiles.
+if [[ -e "$RUNFILES_MANIFEST_FILE" ]]; then
+ export RUNFILES_MANIFEST_ONLY=1
+fi
DIR="$TEST_SRCDIR"
if [ ! -z "$TEST_WORKSPACE" ]; then
