From bfdfa6ebfd21b388f1c91f512291c848e1a92a96 Mon Sep 17 00:00:00 2001 From: mai93 Date: Mon, 14 Dec 2020 06:07:48 -0800 Subject: [PATCH] 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: #10598 Closes #12650. PiperOrigin-RevId: 347370712 --- src/test/py/bazel/runfiles_test.py | 79 +++++++++++++++++- .../testdata/runfiles_test/bar/BUILD.mock | 5 ++ .../runfiles_test/bar/bar-run-under.sh | 17 ++++ src/tools/launcher/launcher.cc | 33 ++++---- tools/cpp/runfiles/runfiles_src.cc | 80 +++++++++++++++---- 5 files changed, 180 insertions(+), 34 deletions(-) create mode 100644 src/test/py/bazel/testdata/runfiles_test/bar/bar-run-under.sh diff --git a/src/test/py/bazel/runfiles_test.py b/src/test/py/bazel/runfiles_test.py index 30196366aea9b9..7682ae4bbd5965 100644 --- a/src/test/py/bazel/runfiles_test.py +++ b/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() diff --git a/src/test/py/bazel/testdata/runfiles_test/bar/BUILD.mock b/src/test/py/bazel/testdata/runfiles_test/bar/BUILD.mock index de3a5cf9d086bf..21ce2e0bc9b877 100644 --- a/src/test/py/bazel/testdata/runfiles_test/bar/BUILD.mock +++ b/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"], +) diff --git a/src/test/py/bazel/testdata/runfiles_test/bar/bar-run-under.sh b/src/test/py/bazel/testdata/runfiles_test/bar/bar-run-under.sh new file mode 100644 index 00000000000000..57fbf389e37095 --- /dev/null +++ b/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!" +"$@" diff --git a/src/tools/launcher/launcher.cc b/src/tools/launcher/launcher.cc index 36c420af44aede..f4763a716a9278 100644 --- a/src/tools/launcher/launcher.cc +++ b/src/tools/launcher/launcher.cc @@ -73,14 +73,27 @@ 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())) { @@ -88,20 +101,6 @@ static bool FindManifestFileImpl(const wchar_t* argv0, wstring* result) { } } - // 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; } diff --git a/tools/cpp/runfiles/runfiles_src.cc b/tools/cpp/runfiles/runfiles_src.cc index 212293c864defe..101017ca14168a 100644 --- a/tools/cpp/runfiles/runfiles_src.cc +++ b/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 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 is_runfiles_manifest, + std::function is_runfiles_directory, + std::string* out_manifest, std::string* out_directory); + bool ParseManifest(const string& path, map* 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 is_runfiles_manifest, + function 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; }