From 486d153d1981c3f47129f675de20189667667fa7 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Mon, 7 Feb 2022 05:49:47 -0800 Subject: [PATCH] Find runfiles in directories that are themselves runfiles When a target has a runfile that is contained in a directory that is itself one of its runfiles, the runfile will be shadowed by the `SymlinkEntry` for the directory. While this still allows to resolve the file in the runfiles symlink tree, a manifest-based lookup will fail. Since the manifest is used to create the symlink tree for which it is important that no path is a prefix of another, this can only be fixed in the runfiles libraries. This PR extends the lookup logic of the Bash, C++, Java, and Python runfiles libraries to also find runfiles contained within directories that are themselves runfiles. It does so by searching the manifest not only for the exact provided rlocation path, but also for all path prefixes. If a prefix is looked up successfully, the corresponding suffix is resolved relative to the looked up path. Fixes #14336. Closes #14335. PiperOrigin-RevId: 426894517 --- tools/bash/runfiles/runfiles.bash | 47 +++++++++++++--- tools/bash/runfiles/runfiles_test.bash | 14 ++++- tools/cpp/runfiles/runfiles_src.cc | 22 ++++++-- tools/cpp/runfiles/runfiles_test.cc | 4 ++ tools/java/runfiles/Runfiles.java | 16 +++++- tools/java/runfiles/testing/RunfilesTest.java | 7 ++- tools/python/runfiles/runfiles.py | 17 +++++- tools/python/runfiles/runfiles_test.py | 54 +++++++++++-------- 8 files changed, 145 insertions(+), 36 deletions(-) diff --git a/tools/bash/runfiles/runfiles.bash b/tools/bash/runfiles/runfiles.bash index d3aa0c3d238d14..d0aedd41deca89 100644 --- a/tools/bash/runfiles/runfiles.bash +++ b/tools/bash/runfiles/runfiles.bash @@ -122,16 +122,51 @@ function rlocation() { echo >&2 "INFO[runfiles.bash]: rlocation($1): looking in RUNFILES_MANIFEST_FILE ($RUNFILES_MANIFEST_FILE)" fi local -r result=$(grep -m1 "^$1 " "${RUNFILES_MANIFEST_FILE}" | cut -d ' ' -f 2-) - if [[ -e "${result:-/dev/null}" ]]; then - if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then - echo >&2 "INFO[runfiles.bash]: rlocation($1): found in manifest as ($result)" - fi - echo "$result" - else + if [[ -z "$result" ]]; then + # If path references a runfile that lies under a directory that itself + # is a runfile, then only the directory is listed in the manifest. Look + # up all prefixes of path in the manifest and append the relative path + # from the prefix if there is a match. + local prefix="$1" + local prefix_result= + local new_prefix= + while true; do + new_prefix="${prefix%/*}" + [[ "$new_prefix" == "$prefix" ]] && break + prefix="$new_prefix" + prefix_result=$(grep -m1 "^$prefix " "${RUNFILES_MANIFEST_FILE}" | cut -d ' ' -f 2-) + [[ -z "$prefix_result" ]] && continue + local -r candidate="${prefix_result}${1#"${prefix}"}" + if [[ -e "$candidate" ]]; then + if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then + echo >&2 "INFO[runfiles.bash]: rlocation($1): found in manifest as ($candidate) via prefix ($prefix)" + fi + echo "$candidate" + return 0 + fi + # At this point, the manifest lookup of prefix has been successful, + # but the file at the relative path given by the suffix does not + # exist. We do not continue the lookup with a shorter prefix for two + # reasons: + # 1. Manifests generated by Bazel never contain a path that is a + # prefix of another path. + # 2. Runfiles libraries for other languages do not check for file + # existence and would have returned the non-existent path. It seems + # better to return no path rather than a potentially different, + # non-empty path. + break + done if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then echo >&2 "INFO[runfiles.bash]: rlocation($1): not found in manifest" fi echo "" + else + if [[ -e "$result" ]]; then + if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then + echo >&2 "INFO[runfiles.bash]: rlocation($1): found in manifest as ($result)" + fi + echo "$result" + fi fi else if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then diff --git a/tools/bash/runfiles/runfiles_test.bash b/tools/bash/runfiles/runfiles_test.bash index e2b514910a6599..e532824770fa3c 100755 --- a/tools/bash/runfiles/runfiles_test.bash +++ b/tools/bash/runfiles/runfiles_test.bash @@ -139,10 +139,14 @@ function test_init_manifest_based_runfiles() { a/b $tmpdir/c/d e/f $tmpdir/g h y $tmpdir/y +c/dir $tmpdir/dir EOF mkdir "${tmpdir}/c" mkdir "${tmpdir}/y" + mkdir -p "${tmpdir}/dir/deeply/nested" touch "${tmpdir}/c/d" "${tmpdir}/g h" + touch "${tmpdir}/dir/file" + touch "${tmpdir}/dir/deeply/nested/file" export RUNFILES_DIR= export RUNFILES_MANIFEST_FILE=$tmpdir/foo.runfiles_manifest @@ -153,10 +157,18 @@ EOF [[ "$(rlocation a/b)" == "$tmpdir/c/d" ]] || fail [[ "$(rlocation e/f)" == "$tmpdir/g h" ]] || fail [[ "$(rlocation y)" == "$tmpdir/y" ]] || fail - rm -r "$tmpdir/c/d" "$tmpdir/g h" "$tmpdir/y" + [[ "$(rlocation c)" == "" ]] || fail + [[ "$(rlocation c/di)" == "" ]] || fail + [[ "$(rlocation c/dir)" == "$tmpdir/dir" ]] || fail + [[ "$(rlocation c/dir/file)" == "$tmpdir/dir/file" ]] || fail + [[ "$(rlocation c/dir/deeply/nested/file)" == "$tmpdir/dir/deeply/nested/file" ]] || fail + rm -r "$tmpdir/c/d" "$tmpdir/g h" "$tmpdir/y" "$tmpdir/dir" [[ -z "$(rlocation a/b)" ]] || fail [[ -z "$(rlocation e/f)" ]] || fail [[ -z "$(rlocation y)" ]] || fail + [[ -z "$(rlocation c/dir)" ]] || fail + [[ -z "$(rlocation c/dir/file)" ]] || fail + [[ -z "$(rlocation c/dir/deeply/nested/file)" ]] || fail } function test_manifest_based_envvars() { diff --git a/tools/cpp/runfiles/runfiles_src.cc b/tools/cpp/runfiles/runfiles_src.cc index 75d606d7c080ad..2d3078065b5f78 100644 --- a/tools/cpp/runfiles/runfiles_src.cc +++ b/tools/cpp/runfiles/runfiles_src.cc @@ -25,6 +25,7 @@ #include #endif // _WIN32 +#include #include #include #include @@ -176,9 +177,24 @@ string Runfiles::Rlocation(const string& path) const { if (IsAbsolute(path)) { return path; } - const auto value = runfiles_map_.find(path); - if (value != runfiles_map_.end()) { - return value->second; + const auto exact_match = runfiles_map_.find(path); + if (exact_match != runfiles_map_.end()) { + return exact_match->second; + } + if (!runfiles_map_.empty()) { + // If path references a runfile that lies under a directory that itself is a + // runfile, then only the directory is listed in the manifest. Look up all + // prefixes of path in the manifest and append the relative path from the + // prefix to the looked up path. + std::size_t prefix_end = path.size(); + while ((prefix_end = path.find_last_of('/', prefix_end - 1)) != + string::npos) { + const string prefix = path.substr(0, prefix_end); + const auto prefix_match = runfiles_map_.find(prefix); + if (prefix_match != runfiles_map_.end()) { + return prefix_match->second + "/" + path.substr(prefix_end + 1); + } + } } if (!directory_.empty()) { return directory_ + "/" + path; diff --git a/tools/cpp/runfiles/runfiles_test.cc b/tools/cpp/runfiles/runfiles_test.cc index 67a9048d9df5a7..cbe2952c1904cd 100644 --- a/tools/cpp/runfiles/runfiles_test.cc +++ b/tools/cpp/runfiles/runfiles_test.cc @@ -252,6 +252,8 @@ TEST_F(RunfilesTest, ManifestBasedRunfilesRlocationAndEnvVars) { EXPECT_EQ(r->Rlocation("/Foo"), "/Foo"); EXPECT_EQ(r->Rlocation("c:/Foo"), "c:/Foo"); EXPECT_EQ(r->Rlocation("c:\\Foo"), "c:\\Foo"); + EXPECT_EQ(r->Rlocation("a/b/file"), "c/d/file"); + EXPECT_EQ(r->Rlocation("a/b/deeply/nested/file"), "c/d/deeply/nested/file"); } TEST_F(RunfilesTest, DirectoryBasedRunfilesRlocationAndEnvVars) { @@ -316,6 +318,8 @@ TEST_F(RunfilesTest, ManifestAndDirectoryBasedRunfilesRlocationAndEnvVars) { EXPECT_EQ(r->Rlocation("/Foo"), "/Foo"); EXPECT_EQ(r->Rlocation("c:/Foo"), "c:/Foo"); EXPECT_EQ(r->Rlocation("c:\\Foo"), "c:\\Foo"); + EXPECT_EQ(r->Rlocation("a/b/file"), "c/d/file"); + EXPECT_EQ(r->Rlocation("a/b/deeply/nested/file"), "c/d/deeply/nested/file"); AssertEnvvars(*r, mf->Path(), dir); } diff --git a/tools/java/runfiles/Runfiles.java b/tools/java/runfiles/Runfiles.java index 0dd90cce437858..9a3aa26c4517a9 100644 --- a/tools/java/runfiles/Runfiles.java +++ b/tools/java/runfiles/Runfiles.java @@ -227,7 +227,21 @@ private static String findRunfilesDir(String manifest) { @Override public String rlocationChecked(String path) { - return runfiles.get(path); + String exactMatch = runfiles.get(path); + if (exactMatch != null) { + return exactMatch; + } + // If path references a runfile that lies under a directory that itself is a runfile, then + // only the directory is listed in the manifest. Look up all prefixes of path in the manifest + // and append the relative path from the prefix if there is a match. + int prefixEnd = path.length(); + while ((prefixEnd = path.lastIndexOf('/', prefixEnd - 1)) != -1) { + String prefixMatch = runfiles.get(path.substring(0, prefixEnd)); + if (prefixMatch != null) { + return prefixMatch + '/' + path.substring(prefixEnd + 1); + } + } + return null; } @Override diff --git a/tools/java/runfiles/testing/RunfilesTest.java b/tools/java/runfiles/testing/RunfilesTest.java index 92f303b2c5e8b6..e98ca554a8a27d 100644 --- a/tools/java/runfiles/testing/RunfilesTest.java +++ b/tools/java/runfiles/testing/RunfilesTest.java @@ -249,10 +249,15 @@ public void testManifestBasedRlocation() throws Exception { new MockFile( ImmutableList.of( "Foo/runfile1 C:/Actual Path\\runfile1", - "Foo/Bar/runfile2 D:\\the path\\run file 2.txt"))) { + "Foo/Bar/runfile2 D:\\the path\\run file 2.txt", + "Foo/Bar/Dir E:\\Actual Path\\Directory"))) { Runfiles r = Runfiles.createManifestBasedForTesting(mf.path.toString()); assertThat(r.rlocation("Foo/runfile1")).isEqualTo("C:/Actual Path\\runfile1"); assertThat(r.rlocation("Foo/Bar/runfile2")).isEqualTo("D:\\the path\\run file 2.txt"); + assertThat(r.rlocation("Foo/Bar/Dir")).isEqualTo("E:\\Actual Path\\Directory"); + assertThat(r.rlocation("Foo/Bar/Dir/File")).isEqualTo("E:\\Actual Path\\Directory/File"); + assertThat(r.rlocation("Foo/Bar/Dir/Deeply/Nested/File")) + .isEqualTo("E:\\Actual Path\\Directory/Deeply/Nested/File"); assertThat(r.rlocation("unknown")).isNull(); } } diff --git a/tools/python/runfiles/runfiles.py b/tools/python/runfiles/runfiles.py index 3a597cbb36a92b..03cff1c27ee386 100644 --- a/tools/python/runfiles/runfiles.py +++ b/tools/python/runfiles/runfiles.py @@ -174,7 +174,22 @@ def __init__(self, path): self._runfiles = _ManifestBased._LoadRunfiles(path) def RlocationChecked(self, path): - return self._runfiles.get(path) + """Returns the runtime path of a runfile.""" + exact_match = self._runfiles.get(path) + if exact_match: + return exact_match + # If path references a runfile that lies under a directory that itself is a + # runfile, then only the directory is listed in the manifest. Look up all + # prefixes of path in the manifest and append the relative path from the + # prefix to the looked up path. + prefix_end = len(path) + while True: + prefix_end = path.rfind("/", 0, prefix_end - 1) + if prefix_end == -1: + return None + prefix_match = self._runfiles.get(path[0:prefix_end]) + if prefix_match: + return prefix_match + "/" + path[prefix_end + 1:] @staticmethod def _LoadRunfiles(path): diff --git a/tools/python/runfiles/runfiles_test.py b/tools/python/runfiles/runfiles_test.py index 1212bbaad83340..70168cbb6107e5 100644 --- a/tools/python/runfiles/runfiles_test.py +++ b/tools/python/runfiles/runfiles_test.py @@ -28,26 +28,26 @@ def testRlocationArgumentValidation(self): self.assertRaises(ValueError, lambda: r.Rlocation(None)) self.assertRaises(ValueError, lambda: r.Rlocation("")) self.assertRaises(TypeError, lambda: r.Rlocation(1)) - self.assertRaisesRegexp(ValueError, "is not normalized", - lambda: r.Rlocation("../foo")) - self.assertRaisesRegexp(ValueError, "is not normalized", - lambda: r.Rlocation("foo/..")) - self.assertRaisesRegexp(ValueError, "is not normalized", - lambda: r.Rlocation("foo/../bar")) - self.assertRaisesRegexp(ValueError, "is not normalized", - lambda: r.Rlocation("./foo")) - self.assertRaisesRegexp(ValueError, "is not normalized", - lambda: r.Rlocation("foo/.")) - self.assertRaisesRegexp(ValueError, "is not normalized", - lambda: r.Rlocation("foo/./bar")) - self.assertRaisesRegexp(ValueError, "is not normalized", - lambda: r.Rlocation("//foobar")) - self.assertRaisesRegexp(ValueError, "is not normalized", - lambda: r.Rlocation("foo//")) - self.assertRaisesRegexp(ValueError, "is not normalized", - lambda: r.Rlocation("foo//bar")) - self.assertRaisesRegexp(ValueError, "is absolute without a drive letter", - lambda: r.Rlocation("\\foo")) + self.assertRaisesRegex(ValueError, "is not normalized", + lambda: r.Rlocation("../foo")) + self.assertRaisesRegex(ValueError, "is not normalized", + lambda: r.Rlocation("foo/..")) + self.assertRaisesRegex(ValueError, "is not normalized", + lambda: r.Rlocation("foo/../bar")) + self.assertRaisesRegex(ValueError, "is not normalized", + lambda: r.Rlocation("./foo")) + self.assertRaisesRegex(ValueError, "is not normalized", + lambda: r.Rlocation("foo/.")) + self.assertRaisesRegex(ValueError, "is not normalized", + lambda: r.Rlocation("foo/./bar")) + self.assertRaisesRegex(ValueError, "is not normalized", + lambda: r.Rlocation("//foobar")) + self.assertRaisesRegex(ValueError, "is not normalized", + lambda: r.Rlocation("foo//")) + self.assertRaisesRegex(ValueError, "is not normalized", + lambda: r.Rlocation("foo//bar")) + self.assertRaisesRegex(ValueError, "is absolute without a drive letter", + lambda: r.Rlocation("\\foo")) def testCreatesManifestBasedRunfiles(self): with _MockFile(contents=["a/b c/d"]) as mf: @@ -122,7 +122,7 @@ def testFailsToCreateManifestBasedBecauseManifestDoesNotExist(self): def _Run(): runfiles.Create({"RUNFILES_MANIFEST_FILE": "non-existing path"}) - self.assertRaisesRegexp(IOError, "non-existing path", _Run) + self.assertRaisesRegex(IOError, "non-existing path", _Run) def testFailsToCreateAnyRunfilesBecauseEnvvarsAreNotDefined(self): with _MockFile(contents=["a b"]) as mf: @@ -140,14 +140,22 @@ def testFailsToCreateAnyRunfilesBecauseEnvvarsAreNotDefined(self): def testManifestBasedRlocation(self): with _MockFile(contents=[ - "Foo/runfile1", "Foo/runfile2 C:/Actual Path\\runfile2", - "Foo/Bar/runfile3 D:\\the path\\run file 3.txt" + "Foo/runfile1", + "Foo/runfile2 C:/Actual Path\\runfile2", + "Foo/Bar/runfile3 D:\\the path\\run file 3.txt", + "Foo/Bar/Dir E:\\Actual Path\\Directory", ]) as mf: r = runfiles.CreateManifestBased(mf.Path()) self.assertEqual(r.Rlocation("Foo/runfile1"), "Foo/runfile1") self.assertEqual(r.Rlocation("Foo/runfile2"), "C:/Actual Path\\runfile2") self.assertEqual( r.Rlocation("Foo/Bar/runfile3"), "D:\\the path\\run file 3.txt") + self.assertEqual( + r.Rlocation("Foo/Bar/Dir/runfile4"), + "E:\\Actual Path\\Directory/runfile4") + self.assertEqual( + r.Rlocation("Foo/Bar/Dir/Deeply/Nested/runfile4"), + "E:\\Actual Path\\Directory/Deeply/Nested/runfile4") self.assertIsNone(r.Rlocation("unknown")) if RunfilesTest.IsWindows(): self.assertEqual(r.Rlocation("c:/foo"), "c:/foo")