fix(pkg_install): resolve files against own repository

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"}
 ]

tests/mappings/node_modules_manifest.golden

@@ -1,9 +1,9 @@
 [
-    {"type": "symlink", "dest": "node_modules/.pnpm/bar@1.0.0/node_modules/bar", "src": "STORE/bar", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:node_modules"},
-    {"type": "symlink", "dest": "node_modules/.pnpm/bar@1.0.0/node_modules/qar", "src": "../../qar@2.0.0/node_modules/qar", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:node_modules"},
-    {"type": "symlink", "dest": "node_modules/.pnpm/foo@1.0.0/node_modules/bar", "src": "../../bar@1.0.0/node_modules/bar", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:node_modules"},
-    {"type": "symlink", "dest": "node_modules/.pnpm/foo@1.0.0/node_modules/foo", "src": "STORE/foo", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:node_modules"},
-    {"type": "symlink", "dest": "node_modules/.pnpm/foo@1.0.0/node_modules/qar", "src": "../../qar@2.0.0/node_modules/qar", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:node_modules"},
-    {"type": "symlink", "dest": "node_modules/.pnpm/qar@2.0.0/node_modules/qar", "src": "STORE/qar", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:node_modules"},
-    {"type": "symlink", "dest": "node_modules/foo", "src": ".pnpm/foo@1.0.0/node_modules/foo", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:node_modules"}
+    {"type": "symlink", "dest": "node_modules/.pnpm/bar@1.0.0/node_modules/bar", "src": "STORE/bar", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:node_modules", "repository": null},
+    {"type": "symlink", "dest": "node_modules/.pnpm/bar@1.0.0/node_modules/qar", "src": "../../qar@2.0.0/node_modules/qar", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:node_modules", "repository": null},
+    {"type": "symlink", "dest": "node_modules/.pnpm/foo@1.0.0/node_modules/bar", "src": "../../bar@1.0.0/node_modules/bar", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:node_modules", "repository": null},
+    {"type": "symlink", "dest": "node_modules/.pnpm/foo@1.0.0/node_modules/foo", "src": "STORE/foo", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:node_modules", "repository": null},
+    {"type": "symlink", "dest": "node_modules/.pnpm/foo@1.0.0/node_modules/qar", "src": "../../qar@2.0.0/node_modules/qar", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:node_modules", "repository": null},
+    {"type": "symlink", "dest": "node_modules/.pnpm/qar@2.0.0/node_modules/qar", "src": "STORE/qar", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:node_modules", "repository": null},
+    {"type": "symlink", "dest": "node_modules/foo", "src": ".pnpm/foo@1.0.0/node_modules/foo", "mode": "", "user": null, "group": null, "uid": null, "gid": null, "origin": "@//tests/mappings:node_modules", "repository": null}
 ]