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

6 files changed, 231 insertions, 54 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 eb754bdbf4..16f15b5f9a 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 @@ public class TestRunnerAction extends AbstractAction implements NotifyOnActionCa
     }
     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 345e3414ed..5ef00845f7 100644
--- a/src/test/py/bazel/test_base.py
+++ b/src/test/py/bazel/test_base.py
@@ -61,12 +61,25 @@ class TestBase(unittest.TestCase):
     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 11b6d0d824..0ec3fbdf06 100644
--- a/src/test/py/bazel/windows_remote_test.py
+++ b/src/test/py/bazel/windows_remote_test.py
@@ -21,7 +21,9 @@ from src.test.py.bazel import test_base
 
 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 @@ class WindowsRemoteTest(test_base.TestBase):
             '--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 @@ class WindowsRemoteTest(test_base.TestBase):
         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 @@ class WindowsRemoteTest(test_base.TestBase):
     ])
     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 @@ class WindowsRemoteTest(test_base.TestBase):
     self.AssertExitCode(exit_code, 0, stderr)
     bazel_bin = stdout[0]
 
-    port = self.StartRemoteWorker()
-
-    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)
-      self.assertEqual(stdout, ['hello shell'])
-    finally:
-      # Always stop the worker so we obtain logs in case an assertion failed
-      # above.
-      self.StopRemoteWorker()
+    # Build.
+    exit_code, stdout, stderr = self._RunRemoteBazel(['build', '//foo:foo'])
+    self.AssertExitCode(exit_code, 0, stderr, stdout)
+
+    # 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'])
+
+  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 d26506e1b1..515733814f 100644
--- a/src/tools/launcher/launcher.cc
+++ b/src/tools/launcher/launcher.cc
@@ -33,15 +33,33 @@ using std::string;
 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]);
+  }
+  // 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);
   }
-  ParseManifestFile(&this->manifest_file_map, this->manifest_file);
 }
 
 static bool FindManifestFileImpl(const char* argv0, string* result) {
@@ -80,7 +98,7 @@ static bool FindManifestFileImpl(const char* argv0, string* result) {
 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 @@ void BinaryLauncherBase::ParseManifestFile(ManifestFileMap* manifest_file_map,
 
 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 @@ ExitCode BinaryLauncherBase::LaunchProcess(const string& executable,
   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 088bd5d740..4dca6e000d 100644
--- a/src/tools/launcher/launcher.h
+++ b/src/tools/launcher/launcher.h
@@ -82,9 +82,12 @@ class BinaryLauncherBase {
   // 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 @@ class BinaryLauncherBase {
   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 faac6bf4aa..b403513f10 100755
--- a/tools/test/test-setup.sh
+++ b/tools/test/test-setup.sh
@@ -106,27 +106,27 @@ GUNIT_OUTPUT="xml:${XML_OUTPUT_FILE}"
 
 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