Skip to content

Commit b560c25

Browse files
committed
fix(pkg_install): resolve files against own repository
When a `pkg_install` rule in an external repository references files from another repository, the installer would fail at runtime by attempting to resolve all files relative to its repository rather than each file's own repository. Example failure (`extrepo`: external repository installing file from main repository): ``` bazel run @extrepo//:install -- --destdir=/tmp/test [...] FileNotFoundError: [Errno 2] No such file or directory: '.../runfiles/+_repo_rules+extrepo/some-path/extrepo/file-in-main' ``` The file should be resolved against the main repository (`_main`) but is incorrectly looked up from the installer's repository runfiles (`+_repo_rules+extrepo`). The solution proposed here consists in extending the manifest to track each file's own repository and use this information during installation to resolve files against the correct repository. It adds a `repository` field to manifest entries using: - `Label.repo_name` ([canonical](https://bazel.build/rules/lib/builtins/Label#repo_name)) when available for the source file, - otherwise `ctx.workspace_name` when no [owner](https://bazel.build/rules/lib/builtins/File#owner) or `Label.repo_name` is [empty](https://rules-python.readthedocs.io/en/latest/api/py/runfiles/runfiles.runfiles.html#runfiles.runfiles.Runfiles.CurrentRepository). Testing: - `//tests/install:install_test` includes new `CrossRepoInstallTest`, - real-world validation: `bazel run @extrepo//:install` successfully, - verified both on Ubuntu Linux and Windows as well. tl;dr: this change builds on #984 which introduced the `locate()` helper and the `runfiles` infrastructure. That PR fixed Windows compatibility by enabling runfiles-less operation; this change extends it to support files from external repositories.
1 parent a9358d5 commit b560c25

12 files changed

Lines changed: 98 additions & 44 deletions

pkg/private/install.py.tpl

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,14 +36,13 @@ from python.runfiles import runfiles
3636
# https://docs.bazel.build/versions/4.1.0/skylark/rules.html#tools-with-runfiles
3737
# https://rules-python.readthedocs.io/en/latest/api/py/runfiles/runfiles.runfiles.html
3838
RUNFILES = runfiles.Create()
39-
REPOSITORY = RUNFILES.CurrentRepository() or "{WORKSPACE_NAME}" # the empty string denotes the "_main" repository
4039

41-
def locate(short_path):
42-
"""Resolve a path relative to the current repository and return its "runfile" location.
40+
def locate(short_path, repository):
41+
"""Resolve a path relative to the given repository and return its "runfile" location.
4342
4443
Uses `posixpath` because runfile lookups always use forward slashes, even on Windows.
4544
"""
46-
return RUNFILES.Rlocation(posixpath.normpath(posixpath.join(REPOSITORY, short_path)))
45+
return RUNFILES.Rlocation(posixpath.normpath(posixpath.join(repository, short_path)))
4746

4847

4948
# This is named "NativeInstaller" because it makes use of "native" python
@@ -189,7 +188,7 @@ class NativeInstaller(object):
189188
# Swap out the source with the actual "runfile" location, except for
190189
# symbolic links as their targets denote installation paths
191190
if entry.type != manifest.ENTRY_IS_LINK and entry.src is not None:
192-
entry.src = locate(entry.src)
191+
entry.src = locate(entry.src, entry.repository)
193192
# Prepend the destdir path to all installation paths, if one is
194193
# specified.
195194
if self.destdir is not None:
@@ -289,7 +288,7 @@ def main(args):
289288
wipe_destdir=args.wipe_destdir,
290289
)
291290

292-
installer.include_manifest_path(locate("{MANIFEST_INCLUSION}"))
291+
installer.include_manifest_path(locate("{MANIFEST_INCLUSION}", "{WORKSPACE_NAME}"))
293292
installer.do_the_thing()
294293

295294

pkg/private/manifest.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,9 @@ class ManifestEntry(object):
3737
uid: int
3838
gid: int
3939
origin: str = None
40+
repository: str = None
4041

41-
def __init__(self, type, dest, src, mode, user, group, uid = None, gid = None, origin = None):
42+
def __init__(self, type, dest, src, mode, user, group, uid = None, gid = None, origin = None, repository = None):
4243
self.type = type
4344
self.dest = dest
4445
self.src = src
@@ -48,6 +49,7 @@ def __init__(self, type, dest, src, mode, user, group, uid = None, gid = None, o
4849
self.uid = uid
4950
self.gid = gid
5051
self.origin = origin
52+
self.repository = repository
5153

5254
def __repr__(self):
5355
return "ManifestEntry<{}>".format(vars(self))

pkg/private/pkg_files.bzl

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -628,15 +628,17 @@ def write_manifest(ctx, manifest_file, content_map, use_short_path = False, pret
628628
manifest_file,
629629
"[\n" + ",\n".join(
630630
[
631-
_encode_manifest_entry(dst, content_map[dst], use_short_path, pretty_print)
631+
_encode_manifest_entry(ctx, dst, content_map[dst], use_short_path, pretty_print)
632632
for dst in sorted(content_map.keys())
633633
],
634634
) + "\n]\n",
635635
)
636636

637-
def _encode_manifest_entry(dest, df, use_short_path, pretty_print = False):
637+
def _encode_manifest_entry(ctx, dest, df, use_short_path, pretty_print = False):
638638
entry_type = df.entry_type if hasattr(df, "entry_type") else ENTRY_IS_FILE
639+
repository = None
639640
if df.src:
641+
repository = (df.src.owner and df.src.owner.repo_name) or ctx.workspace_name # use main repo if absent or empty
640642
src = df.src.short_path if use_short_path else df.src.path
641643
# entry_type is left as-is
642644

@@ -667,6 +669,7 @@ def _encode_manifest_entry(dest, df, use_short_path, pretty_print = False):
667669
"uid": df.uid,
668670
"gid": df.gid,
669671
"origin": origin_str,
672+
"repository": repository,
670673
}
671674

672675
if pretty_print:

tests/install/BUILD

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ py_test(
2727
data = [
2828
":test_installer",
2929
":test_installer_flag",
30+
"@mappings_test_external_repo//pkg:install_cross_repo",
3031
],
3132
imports = ["../.."],
3233
main = "test.py",

tests/install/test.py

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import unittest
2020
import stat
2121
import subprocess
22+
import tempfile
2223

2324
from python.runfiles import runfiles
2425
from pkg.private import manifest
@@ -244,5 +245,38 @@ def test_wipe(self):
244245
self.assertFalse((self.installdir / "should_be_deleted.txt").exists())
245246

246247

248+
class CrossRepoInstallTest(unittest.TestCase):
249+
"""Test external repo's pkg_install can reference main repo files."""
250+
251+
def test_external_repo_installs_file_from_main_repo(self):
252+
"""Verify source files are resolved against their own repositories (not against installer's repository),
253+
using different working directories (workspace vs elsewhere) to ensure runfiles resolution.
254+
"""
255+
runfiles_ = runfiles.Create()
256+
test_tmpdir = pathlib.Path(os.environ["TEST_TMPDIR"])
257+
258+
for case, cwd in dict(
259+
workspace_cwd=None,
260+
elsewhere_cwd=tempfile.gettempdir(),
261+
).items():
262+
with self.subTest(case):
263+
destdir = test_tmpdir / f"cross_repo_{case}"
264+
subprocess.check_call(
265+
[
266+
runfiles_.Rlocation(
267+
f"mappings_test_external_repo/pkg/install_cross_repo{PkgInstallTestBase._extension}"
268+
),
269+
f"--destdir={destdir}",
270+
"--verbose",
271+
],
272+
cwd=cwd,
273+
)
274+
275+
expected_path = destdir / "testdata/hello.txt"
276+
self.assertTrue(
277+
expected_path.exists(), f"File from main repo not found: {expected_path}"
278+
)
279+
280+
247281
if __name__ == "__main__":
248282
unittest.main()

tests/mappings/all.manifest.golden

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
[
2-
{"type": "file", "dest": "BUILD", "src": "tests/mappings/BUILD", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:BUILD"},
3-
{"type": "dir", "dest": "foodir", "src": null, "mode": "711", "user": "foo", "group": "bar", "uid": null, "gid": null, "origin": "@//tests/mappings:dirs"},
2+
{"type": "file", "dest": "BUILD", "src": "tests/mappings/BUILD", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:BUILD", "repository": "_main"},
3+
{"type": "dir", "dest": "foodir", "src": null, "mode": "711", "user": "foo", "group": "bar", "uid": null, "gid": null, "origin": "@//tests/mappings:dirs", "repository": null},
44

5-
{"type": "file", "dest": "mappings_test.bzl", "src": "tests/mappings/mappings_test.bzl", "mode": "0644", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:files"},
6-
{"type": "tree", "dest": "treeartifact", "src": "tests/mappings/treeartifact", "mode": "0644", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:directory-with-contents"}
5+
{"type": "file", "dest": "mappings_test.bzl", "src": "tests/mappings/mappings_test.bzl", "mode": "0644", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:files", "repository": "_main"},
6+
{"type": "tree", "dest": "treeartifact", "src": "tests/mappings/treeartifact", "mode": "0644", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:directory-with-contents", "repository": "_main"}
77
]
Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
[
2-
{"dest":"an_executable.runfiles/_main/tests/an_executable","gid":null,"group":null,"mode":"0755","origin":"@//tests:an_executable","src":"tests/an_executable","type":"file","uid":null,"user":null},
3-
{"dest":"an_executable.runfiles/_main/tests/foo.cc","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/foo.cc","type":"file","uid":null,"user":null},
4-
{"dest":"an_executable.runfiles/_main/tests/testdata/hello.txt","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/testdata/hello.txt","type":"file","uid":null,"user":null},
5-
{"dest":"an_executable","gid":null,"group":null,"mode":"0755","origin":"@//tests:an_executable","src":"tests/an_executable","type":"file","uid":null,"user":null},
6-
{"dest":"an_executable.repo_mapping","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/an_executable.repo_mapping","type":"file","uid":null,"user":null},
7-
{"dest":"an_executable.runfiles/_repo_mapping","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/an_executable.repo_mapping","type":"file","uid":null,"user":null},
8-
{"dest":"mappings_test.bzl","gid":null,"group":null,"mode":"","origin":"@//tests/mappings:mappings_test.bzl","src":"tests/mappings/mappings_test.bzl","type":"file","uid":null,"user":null}
2+
{"dest":"an_executable.runfiles/_main/tests/an_executable","gid":null,"group":null,"mode":"0755","origin":"@//tests:an_executable","src":"tests/an_executable","type":"file","uid":null,"user":null,"repository":"_main"},
3+
{"dest":"an_executable.runfiles/_main/tests/foo.cc","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/foo.cc","type":"file","uid":null,"user":null,"repository":"_main"},
4+
{"dest":"an_executable.runfiles/_main/tests/testdata/hello.txt","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/testdata/hello.txt","type":"file","uid":null,"user":null,"repository":"_main"},
5+
{"dest":"an_executable","gid":null,"group":null,"mode":"0755","origin":"@//tests:an_executable","src":"tests/an_executable","type":"file","uid":null,"user":null,"repository":"_main"},
6+
{"dest":"an_executable.repo_mapping","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/an_executable.repo_mapping","type":"file","uid":null,"user":null,"repository":"_main"},
7+
{"dest":"an_executable.runfiles/_repo_mapping","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/an_executable.repo_mapping","type":"file","uid":null,"user":null,"repository":"_main"},
8+
{"dest":"mappings_test.bzl","gid":null,"group":null,"mode":"","origin":"@//tests/mappings:mappings_test.bzl","src":"tests/mappings/mappings_test.bzl","type":"file","uid":null,"user":null,"repository":"_main"}
99
]
Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
[
2-
{"dest":"an_executable.exe.runfiles/_main/tests/foo.cc","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/foo.cc","type":"file","uid":null,"user":null},
3-
{"dest":"an_executable.exe.runfiles/_main/tests/an_executable.exe","gid":null,"group":null,"mode":"0755","origin":"@//tests:an_executable","src":"tests/an_executable.exe","type":"file","uid":null,"user":null},
4-
{"dest":"an_executable.exe.runfiles/_main/tests/testdata/hello.txt","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/testdata/hello.txt","type":"file","uid":null,"user":null},
5-
{"dest":"an_executable.exe","gid":null,"group":null,"mode":"0755","origin":"@//tests:an_executable","src":"tests/an_executable.exe","type":"file","uid":null,"user":null},
6-
{"dest":"an_executable.exe.repo_mapping","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src": "tests/an_executable.exe.repo_mapping","type": "file","uid":null,"user":null},
7-
{"dest":"an_executable.exe.runfiles/_repo_mapping","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src": "tests/an_executable.exe.repo_mapping","type": "file","uid":null,"user":null},
8-
{"dest":"mappings_test.bzl","gid":null,"group":null,"mode":"","origin":"@//tests/mappings:mappings_test.bzl","src":"tests/mappings/mappings_test.bzl","type":"file","uid":null,"user":null}
2+
{"dest":"an_executable.exe.runfiles/_main/tests/foo.cc","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/foo.cc","type":"file","uid":null,"user":null,"repository":"_main"},
3+
{"dest":"an_executable.exe.runfiles/_main/tests/an_executable.exe","gid":null,"group":null,"mode":"0755","origin":"@//tests:an_executable","src":"tests/an_executable.exe","type":"file","uid":null,"user":null,"repository":"_main"},
4+
{"dest":"an_executable.exe.runfiles/_main/tests/testdata/hello.txt","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src":"tests/testdata/hello.txt","type":"file","uid":null,"user":null,"repository":"_main"},
5+
{"dest":"an_executable.exe","gid":null,"group":null,"mode":"0755","origin":"@//tests:an_executable","src":"tests/an_executable.exe","type":"file","uid":null,"user":null,"repository":"_main"},
6+
{"dest":"an_executable.exe.repo_mapping","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src": "tests/an_executable.exe.repo_mapping","type": "file","uid":null,"user":null,"repository":"_main"},
7+
{"dest":"an_executable.exe.runfiles/_repo_mapping","gid":null,"group":null,"mode":"","origin":"@//tests:an_executable","src": "tests/an_executable.exe.repo_mapping","type": "file","uid":null,"user":null,"repository":"_main"},
8+
{"dest":"mappings_test.bzl","gid":null,"group":null,"mode":"","origin":"@//tests/mappings:mappings_test.bzl","src":"tests/mappings/mappings_test.bzl","type":"file","uid":null,"user":null,"repository":"_main"}
99
]

tests/mappings/external_repo/pkg/BUILD

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
# See the License for the specific language governing permissions and
1313
# limitations under the License.
1414

15+
load("@//pkg:install.bzl", "pkg_install")
1516
load("@//pkg:mappings.bzl", "pkg_files", "strip_prefix")
1617
load("@//pkg:tar.bzl", "pkg_tar")
1718
load("@//pkg:verify_archive.bzl", "verify_archive_test")
@@ -56,3 +57,17 @@ verify_archive_test(
5657
must_contain = ["extproj.sh"],
5758
target = ":external_archive",
5859
)
60+
61+
# Generate an installer that references a file from the main repo.
62+
# This verifies that files resolve against their own repository, not the installer's repository.
63+
# Tested by: //tests/install:install_test
64+
pkg_install(
65+
name = "install_cross_repo",
66+
srcs = [":file_from_main_repo"],
67+
)
68+
69+
pkg_files(
70+
name = "file_from_main_repo",
71+
srcs = ["@//tests:testdata/hello.txt"],
72+
strip_prefix = strip_prefix.from_pkg(),
73+
)
Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
[
2-
{"type": "file", "dest": "file_with_a_ridiculously_long_name_consectetur_adipiscing_elit_fusce_laoreet_lorem_neque_sed_pharetra_erat.txt", "src": "tests/testdata/file_with_a_ridiculously_long_name_consectetur_adipiscing_elit_fusce_laoreet_lorem_neque_sed_pharetra_erat.txt", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests:glob_for_texts"},
3-
{"type": "file", "dest": "hello.txt", "src": "tests/testdata/hello.txt", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests:glob_for_texts"},
4-
{"type": "file", "dest": "loremipsum.txt", "src": "tests/testdata/loremipsum.txt", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests:glob_for_texts"},
5-
{"type": "file", "dest": "test_tar_package_dir_file.txt", "src": "tests/testdata/test_tar_package_dir_file.txt", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests:glob_for_texts"}
2+
{"type": "file", "dest": "file_with_a_ridiculously_long_name_consectetur_adipiscing_elit_fusce_laoreet_lorem_neque_sed_pharetra_erat.txt", "src": "tests/testdata/file_with_a_ridiculously_long_name_consectetur_adipiscing_elit_fusce_laoreet_lorem_neque_sed_pharetra_erat.txt", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests:glob_for_texts", "repository": "_main"},
3+
{"type": "file", "dest": "hello.txt", "src": "tests/testdata/hello.txt", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests:glob_for_texts", "repository": "_main"},
4+
{"type": "file", "dest": "loremipsum.txt", "src": "tests/testdata/loremipsum.txt", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests:glob_for_texts", "repository": "_main"},
5+
{"type": "file", "dest": "test_tar_package_dir_file.txt", "src": "tests/testdata/test_tar_package_dir_file.txt", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests:glob_for_texts", "repository": "_main"}
66
]

0 commit comments

Comments
 (0)