fix(pkg_install): resolve files against own repository

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
cross-repository files.

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-repo'
```

The file should be resolved against the main repository (`_main`) but
is incorrectly looked up from the installer's repository
(`+_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 explicitly valued 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),
  denoting the main repository (`_main`, as before).

Testing:
- `//tests/install:install_test` includes new `CrossRepoInstallTest`,
- `bazel run @extrepo//:install -- --destdir=/tmp/test` case works,
- verified the above on Ubuntu Linux and Windows as well.

Author: rdesgroppes

pkg/private/install.py.tpl

@@ -36,14 +36,13 @@ from python.runfiles import runfiles
 # https://docs.bazel.build/versions/4.1.0/skylark/rules.html#tools-with-runfiles
 # https://rules-python.readthedocs.io/en/latest/api/py/runfiles/runfiles.runfiles.html
 RUNFILES = runfiles.Create()
-REPOSITORY = RUNFILES.CurrentRepository() or "{WORKSPACE_NAME}"  # the empty string denotes the "_main" repository
 
-def locate(short_path):
-    """Resolve a path relative to the current repository and return its "runfile" location.
+def locate(short_path, repository):
+    """Resolve a path relative to the given repository and return its "runfile" location.
 
     Uses `posixpath` because runfile lookups always use forward slashes, even on Windows.
     """
-    return RUNFILES.Rlocation(posixpath.normpath(posixpath.join(REPOSITORY, short_path)))
+    return RUNFILES.Rlocation(posixpath.normpath(posixpath.join(repository, short_path)))
 
 
 # This is named "NativeInstaller" because it makes use of "native" python
@@ -189,7 +188,7 @@ class NativeInstaller(object):
             # Swap out the source with the actual "runfile" location, except for
             # symbolic links as their targets denote installation paths
             if entry.type != manifest.ENTRY_IS_LINK and entry.src is not None:
-                entry.src = locate(entry.src)
+                entry.src = locate(entry.src, entry.repository)
             # Prepend the destdir path to all installation paths, if one is
             # specified.
             if self.destdir is not None:
@@ -289,7 +288,7 @@ def main(args):
         wipe_destdir=args.wipe_destdir,
     )
 
-    installer.include_manifest_path(locate("{MANIFEST_INCLUSION}"))
+    installer.include_manifest_path(locate("{MANIFEST_INCLUSION}", "{WORKSPACE_NAME}"))
     installer.do_the_thing()
 
 

pkg/private/manifest.py

@@ -37,8 +37,9 @@ class ManifestEntry(object):
     uid: int
     gid: int
     origin: str = None
+    repository: str = None
 
-    def __init__(self, type, dest, src, mode, user, group, uid = None, gid = None, origin = None):
+    def __init__(self, type, dest, src, mode, user, group, uid = None, gid = None, origin = None, repository = None):
         self.type = type
         self.dest = dest
         self.src = src
@@ -48,6 +49,7 @@ def __init__(self, type, dest, src, mode, user, group, uid = None, gid = None, o
         self.uid = uid
         self.gid = gid
         self.origin = origin
+        self.repository = repository
 
     def __repr__(self):
         return "ManifestEntry<{}>".format(vars(self))

pkg/private/pkg_files.bzl

@@ -628,15 +628,17 @@ def write_manifest(ctx, manifest_file, content_map, use_short_path = False, pret
         manifest_file,
         "[\n" + ",\n".join(
             [
-                _encode_manifest_entry(dst, content_map[dst], use_short_path, pretty_print)
+                _encode_manifest_entry(ctx, dst, content_map[dst], use_short_path, pretty_print)
                 for dst in sorted(content_map.keys())
             ],
         ) + "\n]\n",
     )
 
-def _encode_manifest_entry(dest, df, use_short_path, pretty_print = False):
+def _encode_manifest_entry(ctx, dest, df, use_short_path, pretty_print = False):
     entry_type = df.entry_type if hasattr(df, "entry_type") else ENTRY_IS_FILE
+    repository = None
     if df.src:
+        repository = (df.src.owner and df.src.owner.repo_name) or ctx.workspace_name
         src = df.src.short_path if use_short_path else df.src.path
         # entry_type is left as-is
 
@@ -667,6 +669,7 @@ def _encode_manifest_entry(dest, df, use_short_path, pretty_print = False):
         "uid": df.uid,
         "gid": df.gid,
         "origin": origin_str,
+        "repository": repository,
     }
 
     if pretty_print:

tests/install/BUILD

@@ -27,6 +27,7 @@ py_test(
     data = [
         ":test_installer",
         ":test_installer_flag",
+        "@mappings_test_external_repo//pkg:install_cross_repo",
     ],
     imports = ["../.."],
     main = "test.py",

tests/install/test.py

@@ -16,12 +16,13 @@
 
 import os
 import pathlib
-import unittest
 import stat
 import subprocess
+import tempfile
+import unittest
 
-from python.runfiles import runfiles
 from pkg.private import manifest
+from python.runfiles import runfiles
 
 
 class PkgInstallTestBase(unittest.TestCase):
@@ -244,5 +245,39 @@ def test_wipe(self):
         self.assertFalse((self.installdir / "should_be_deleted.txt").exists())
 
 
+class CrossRepoInstallTest(unittest.TestCase):
+    """Test external repo's pkg_install can reference main repo files."""
+
+    def test_external_repo_installs_file_from_main_repo(self):
+        """Verify source files are resolved against their own repositories (not against installer's repository),
+        using different working directories to also verify resolution happens exclusively through runfiles.
+        """
+        runfiles_ = runfiles.Create()
+        test_tmpdir = pathlib.Path(os.environ["TEST_TMPDIR"])
+
+        for case, cwd in dict(
+            default_cwd=None,  # from main's runfiles directory, where accessing short paths directly might work
+            outside_cwd=tempfile.gettempdir(),  # from elsewhere, where bypassing runfiles resolution would fail
+        ).items():
+            with self.subTest(case):
+                destdir = test_tmpdir / f"cross_repo_{case}"
+                subprocess.check_call(
+                    [
+                        runfiles_.Rlocation(
+                            f"mappings_test_external_repo/pkg/install_cross_repo{PkgInstallTestBase._extension}"
+                        ),
+                        f"--destdir={destdir}",
+                        "--verbose",
+                    ],
+                    cwd=cwd,
+                    env=runfiles_.EnvVars(),
+                )
+
+                expected_path = destdir / "testdata/hello.txt"
+                self.assertTrue(
+                    expected_path.exists(), f"File from main repo not found: {expected_path}"
+                )
+
+
 if __name__ == "__main__":
     unittest.main()

tests/mappings/all.manifest.golden

@@ -1,7 +1,7 @@
 [
-    {"type": "file", "dest": "BUILD", "src": "tests/mappings/BUILD", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:BUILD"},
-    {"type": "dir", "dest": "foodir", "src": null, "mode": "711", "user": "foo", "group": "bar", "uid": null, "gid": null, "origin": "@//tests/mappings:dirs"},
+    {"type": "file", "dest": "BUILD", "src": "tests/mappings/BUILD", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:BUILD", "repository": "_main"},
+    {"type": "dir", "dest": "foodir", "src": null, "mode": "711", "user": "foo", "group": "bar", "uid": null, "gid": null, "origin": "@//tests/mappings:dirs", "repository": null},
 
-    {"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"},
-    {"type": "tree", "dest": "treeartifact", "src": "tests/mappings/treeartifact", "mode": "0644", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:directory-with-contents"}
+    {"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"},
+    {"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"}
 ]

tests/mappings/executable.manifest.golden

@@ -1,9 +1,9 @@
 [
-{"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},
-{"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},
-{"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},
-{"dest":"an_executable","gid":null,"group":null,"mode":"0755","origin":"@//tests:an_executable","src":"tests/an_executable","type":"file","uid":null,"user":null},
-{"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},
-{"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},
-{"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}
+{"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"},
+{"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"},
+{"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"},
+{"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"},
+{"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"},
+{"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"},
+{"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"}
 ]

tests/mappings/executable.manifest.windows.golden

@@ -1,9 +1,9 @@
 [
-{"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},
-{"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},
-{"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},
-{"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},
-{"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},
-{"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},
-{"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}
+{"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"},
+{"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"},
+{"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"},
+{"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"},
+{"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"},
+{"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"},
+{"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"}
 ]

tests/mappings/external_repo/pkg/BUILD

@@ -12,6 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+load("@//pkg:install.bzl", "pkg_install")
 load("@//pkg:mappings.bzl", "pkg_files", "strip_prefix")
 load("@//pkg:tar.bzl", "pkg_tar")
 load("@//pkg:verify_archive.bzl", "verify_archive_test")
@@ -56,3 +57,17 @@ verify_archive_test(
     must_contain = ["extproj.sh"],
     target = ":external_archive",
 )
+
+# Generate an installer that references a file from the main repository.
+# This verifies that files resolve against their own repository, not the installer's repository.
+# Tested by: //tests/install:install_test
+pkg_install(
+    name = "install_cross_repo",
+    srcs = [":file_from_main_repo"],
+)
+
+pkg_files(
+    name = "file_from_main_repo",
+    srcs = ["@//tests:testdata/hello.txt"],
+    strip_prefix = strip_prefix.from_pkg(),
+)

tests/mappings/glob_for_texts_manifest.golden

@@ -1,6 +1,6 @@
 [
-    {"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"},
-    {"type": "file", "dest": "hello.txt", "src": "tests/testdata/hello.txt", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests:glob_for_texts"},
-    {"type": "file", "dest": "loremipsum.txt", "src": "tests/testdata/loremipsum.txt", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests:glob_for_texts"},
-    {"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"}
+    {"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"},
+    {"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"},
+    {"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"},
+    {"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"}
 ]