Update find runfiles manifest file logic

This pull request updates the `FindManifestFileImpl` function to look for the runfiles manifest file of the binary first. If it is not found, it can use the manifest file specified in the environment variables `RUNFILES_MANIFEST_FILE` if it exists. The same logic is applied in `runfiles_src.cc` to find runfiles manifest file / directory of cc binaries.

This is done to avoid using the manifest file in `RUNFILES_MANIFEST_FILE` environment variable if the binary already has its own runfiles manifest file. While, at the same time, allow using the value in the environment variables for binaries run as data-dependency that have no runfiles manifest/directory and should use the main binary's.

Fixes: bazelbuild#10598

Closes bazelbuild#12650.

PiperOrigin-RevId: 347370712

Author: mai93

src/test/py/bazel/runfiles_test.py

@@ -14,7 +14,10 @@
 # limitations under the License.
 
 import os
+import shutil
+import stat
 import unittest
+
 import six
 from src.test.py.bazel import test_base
 
@@ -228,7 +231,7 @@ def testRunfilesLibrariesFindRunfilesWithRunfilesManifestEnvvar(self):
         # runfiles tree, Bazel actually creates empty __init__.py files (again
         # on every platform). However to keep these manifest entries correct,
         # they need to have a space character.
-        # We could probably strip thses lines completely, but this test doesn't
+        # We could probably strip these lines completely, but this test doesn't
         # aim to exercise what would happen in that case.
         mock_manifest_data = [
             mock_manifest_line
@@ -239,6 +242,18 @@ def testRunfilesLibrariesFindRunfilesWithRunfilesManifestEnvvar(self):
       substitute_manifest = self.ScratchFile(
           "mock-%s.runfiles/MANIFEST" % lang[0], mock_manifest_data)
 
+      # remove the original manifest file and directory so the launcher picks
+      # the one in the environment variable RUNFILES_MANIFEST_FILE
+      manifest_dir = bin_path + ".runfiles"
+      manifest_dir_file = os.path.join(manifest_dir, "MANIFEST")
+      os.chmod(manifest_dir_file, stat.S_IRWXU)
+      shutil.rmtree(manifest_dir)
+      self.assertFalse(os.path.exists(manifest_dir))
+
+      os.chmod(manifest_path, stat.S_IRWXU)
+      os.remove(manifest_path)
+      self.assertFalse(os.path.exists(manifest_path))
+
       exit_code, stdout, stderr = self.RunProgram(
           [bin_path],
           env_remove=set(["RUNFILES_DIR"]),
@@ -313,6 +328,68 @@ def testLegacyExternalRunfilesOption(self):
                                    "host/bin/bin.runfiles_manifest")
     self.AssertFileContentNotContains(manifest_path, "__main__/external/A")
 
+  def testRunUnderWithRunfiles(self):
+    for s, t, exe in [
+        ("WORKSPACE.mock", "WORKSPACE", False),
+        ("bar/BUILD.mock", "bar/BUILD", False),
+        ("bar/bar.py", "bar/bar.py", True),
+        ("bar/bar-py-data.txt", "bar/bar-py-data.txt", False),
+        ("bar/Bar.java", "bar/Bar.java", False),
+        ("bar/bar-java-data.txt", "bar/bar-java-data.txt", False),
+        ("bar/bar.sh", "bar/bar.sh", True),
+        ("bar/bar-sh-data.txt", "bar/bar-sh-data.txt", False),
+        ("bar/bar-run-under.sh", "bar/bar-run-under.sh", True),
+        ("bar/bar.cc", "bar/bar.cc", False),
+        ("bar/bar-cc-data.txt", "bar/bar-cc-data.txt", False),
+    ]:
+      self.CopyFile(
+          self.Rlocation("io_bazel/src/test/py/bazel/testdata/runfiles_test/" +
+                         s), t, exe)
+
+    exit_code, stdout, stderr = self.RunBazel(["info", "bazel-bin"])
+    self.AssertExitCode(exit_code, 0, stderr)
+    bazel_bin = stdout[0]
+
+    # build bar-sh-run-under target to run targets under it
+    exit_code, _, stderr = self.RunBazel(
+        ["build", "--verbose_failures", "//bar:bar-sh-run-under"])
+    self.AssertExitCode(exit_code, 0, stderr)
+
+    if test_base.TestBase.IsWindows():
+      run_under_bin_path = os.path.join(bazel_bin, "bar/bar-sh-run-under.exe")
+    else:
+      run_under_bin_path = os.path.join(bazel_bin, "bar/bar-sh-run-under")
+
+    self.assertTrue(os.path.exists(run_under_bin_path))
+
+    for lang in [("py", "Python", "bar.py"), ("java", "Java", "Bar.java"),
+                 ("sh", "Bash", "bar.sh"), ("cc", "C++", "bar.cc")]:
+      exit_code, stdout, stderr = self.RunBazel([
+          "run", "--verbose_failures", "--run_under=//bar:bar-sh-run-under",
+          "//bar:bar-" + lang[0]
+      ])
+      self.AssertExitCode(exit_code, 0, stderr)
+
+      if test_base.TestBase.IsWindows():
+        bin_path = os.path.join(bazel_bin, "bar/bar-%s.exe" % lang[0])
+      else:
+        bin_path = os.path.join(bazel_bin, "bar/bar-" + lang[0])
+
+      self.assertTrue(os.path.exists(bin_path))
+
+      if len(stdout) < 3:
+        self.fail("stdout(%s): %s" % (lang[0], stdout))
+
+      self.assertEqual(stdout[0], "Hello Bash Bar Run under!")
+      self.assertEqual(stdout[1], "Hello %s Bar!" % lang[1])
+      six.assertRegex(self, stdout[2], "^rloc=.*/bar/bar-%s-data.txt" % lang[0])
+
+      with open(stdout[2].split("=", 1)[1], "r") as f:
+        lines = [l.strip() for l in f.readlines()]
+      if len(lines) != 1:
+        self.fail("lines(%s): %s" % (lang[0], lines))
+      self.assertEqual(lines[0], "data for " + lang[2])
+
 
 if __name__ == "__main__":
   unittest.main()

src/test/py/bazel/testdata/runfiles_test/bar/BUILD.mock

@@ -29,3 +29,8 @@ cc_binary(
     data = ["bar-cc-data.txt"],
     deps = ["@bazel_tools//tools/cpp/runfiles"],
 )
+
+sh_binary(
+    name = "bar-sh-run-under",
+    srcs = ["bar-run-under.sh"],
+)

src/test/py/bazel/testdata/runfiles_test/bar/bar-run-under.sh

@@ -0,0 +1,17 @@
+#!/bin/bash
+# Copyright 2018 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+echo "Hello Bash Bar Run under!"
+"$@"

src/tools/launcher/launcher.cc

@@ -73,35 +73,34 @@ BinaryLauncherBase::BinaryLauncherBase(
 }
 
 static bool FindManifestFileImpl(const wchar_t* argv0, wstring* result) {
-  // If this binary X runs as the data-dependency of some other binary Y, then
-  // X has no runfiles manifest/directory and should use Y's.
+  // Look for the runfiles manifest of the binary in a runfiles directory next
+  // to the binary, then look for it (the manifest) next to the binary.
+  wstring directory = GetBinaryPathWithExtension(argv0) + L".runfiles";
+  *result = directory + L"/MANIFEST";
+  if (DoesFilePathExist(result->c_str())) {
+    return true;
+  }
+
+  *result = directory + L"_manifest";
+  if (DoesFilePathExist(result->c_str())) {
+    return true;
+  }
+
+  // If the manifest is not found then this binary (X) runs as the
+  // data-dependency of some other binary (Y) and X has no runfiles
+  // manifest/directory so it should use Y's.
   if (GetEnv(L"RUNFILES_MANIFEST_FILE", result) &&
       DoesFilePathExist(result->c_str())) {
     return true;
   }
 
-  wstring directory;
   if (GetEnv(L"RUNFILES_DIR", &directory)) {
     *result = directory + L"/MANIFEST";
     if (DoesFilePathExist(result->c_str())) {
       return true;
     }
   }
 
-  // If this binary X runs by itself (not as a data-dependency of another
-  // binary), then look for the manifest in a runfiles directory next to the
-  // main binary, then look for it (the manifest) next to the main binary.
-  directory = GetBinaryPathWithExtension(argv0) + L".runfiles";
-  *result = directory + L"/MANIFEST";
-  if (DoesFilePathExist(result->c_str())) {
-    return true;
-  }
-
-  *result = directory + L"_manifest";
-  if (DoesFilePathExist(result->c_str())) {
-    return true;
-  }
-
   return false;
 }
 

tools/cpp/runfiles/runfiles_src.cc

@@ -93,6 +93,22 @@ bool IsDirectory(const string& path) {
 #endif
 }
 
+// Computes the path of the runfiles manifest and the runfiles directory.
+// By searching first next to the binary location `argv0` and then falling
+// back on the values passed in `runfiles_manifest_file` and `runfiles_dir`
+//
+// If the method finds both a valid manifest and valid directory according to
+// `is_runfiles_manifest` and `is_runfiles_directory`, then the method sets
+// the corresponding values to `out_manifest` and `out_directory` and returns
+// true.
+//
+// If the method only finds a valid manifest or a valid directory, but not
+// both, then it sets the corresponding output variable (`out_manifest` or
+// `out_directory`) to the value while clearing the other output variable. The
+// method still returns true in this case.
+//
+// If the method cannot find either a valid manifest or valid directory, it
+// clears both output variables and returns false.
 bool PathsFrom(const std::string& argv0, std::string runfiles_manifest_file,
                std::string runfiles_dir, std::string* out_manifest,
                std::string* out_directory);
@@ -103,6 +119,12 @@ bool PathsFrom(const std::string& argv0, std::string runfiles_manifest_file,
                std::function<bool(const std::string&)> is_runfiles_directory,
                std::string* out_manifest, std::string* out_directory);
 
+bool PathsFrom(const std::string& runfiles_manifest_file,
+               const std::string& runfiles_dir,
+               std::function<bool(const std::string&)> is_runfiles_manifest,
+               std::function<bool(const std::string&)> is_runfiles_directory,
+               std::string* out_manifest, std::string* out_directory);
+
 bool ParseManifest(const string& path, map<string, string>* result,
                    string* error);
 
@@ -265,45 +287,71 @@ bool PathsFrom(const string& argv0, string mf, string dir,
   out_manifest->clear();
   out_directory->clear();
 
-  bool mfValid = is_runfiles_manifest(mf);
-  bool dirValid = is_runfiles_directory(dir);
+  string existing_mf = mf;
+  string existing_dir = dir;
 
-  if (!argv0.empty() && !mfValid && !dirValid) {
+  // if argv0 is not empty, try to use it to find the runfiles manifest
+  // file/directory paths
+  if (!argv0.empty()) {
     mf = argv0 + ".runfiles/MANIFEST";
     dir = argv0 + ".runfiles";
-    mfValid = is_runfiles_manifest(mf);
-    dirValid = is_runfiles_directory(dir);
-    if (!mfValid) {
+    if (!is_runfiles_manifest(mf)) {
       mf = argv0 + ".runfiles_manifest";
-      mfValid = is_runfiles_manifest(mf);
     }
+    PathsFrom(mf, dir, is_runfiles_manifest, is_runfiles_directory,
+              out_manifest, out_directory);
+  }
+
+  // if the runfiles manifest file/directory paths are not found, use existing
+  // mf and dir to find the paths
+  if (out_manifest->empty() && out_directory->empty()) {
+    return PathsFrom(existing_mf, existing_dir, is_runfiles_manifest,
+                     is_runfiles_directory, out_manifest, out_directory);
   }
 
-  if (!mfValid && !dirValid) {
+  return true;
+}
+
+bool PathsFrom(const std::string& runfiles_manifest,
+               const std::string& runfiles_directory,
+               function<bool(const std::string&)> is_runfiles_manifest,
+               function<bool(const std::string&)> is_runfiles_directory,
+               std::string* out_manifest, std::string* out_directory) {
+  out_manifest->clear();
+  out_directory->clear();
+
+
+  std::string mf = runfiles_manifest;
+  std::string dir = runfiles_directory;
+
+  bool mf_valid = is_runfiles_manifest(mf);
+  bool dir_valid = is_runfiles_directory(dir);
+
+  if (!mf_valid && !dir_valid) {
     return false;
   }
 
-  if (!mfValid) {
+  if (!mf_valid) {
     mf = dir + "/MANIFEST";
-    mfValid = is_runfiles_manifest(mf);
-    if (!mfValid) {
+    mf_valid = is_runfiles_manifest(mf);
+    if (!mf_valid) {
       mf = dir + "_manifest";
-      mfValid = is_runfiles_manifest(mf);
+      mf_valid = is_runfiles_manifest(mf);
     }
   }
 
-  if (!dirValid &&
+  if (!dir_valid &&
       (ends_with(mf, ".runfiles_manifest") || ends_with(mf, "/MANIFEST"))) {
     static const size_t kSubstrLen = 9;  // "_manifest" or "/MANIFEST"
     dir = mf.substr(0, mf.size() - kSubstrLen);
-    dirValid = is_runfiles_directory(dir);
+    dir_valid = is_runfiles_directory(dir);
   }
 
-  if (mfValid) {
+  if (mf_valid) {
     *out_manifest = mf;
   }
 
-  if (dirValid) {
+  if (dir_valid) {
     *out_directory = dir;
   }