fix(zipapp): store venv runfiles symlinks as symlinks, not copies

news/zipapp-venv-symlinks.fixed.md

@@ -0,0 +1,3 @@
+(zipapp) {obj}`py_zipapp_binary` stores runfiles symlinks whose target is also
+packed in the zipapp, such as the venv `site-packages` entries that point into
+pip repositories, as relative symlinks instead of a second copy of the file.

python/private/zipapp/py_zipapp_rule.bzl

@@ -93,6 +93,34 @@ def _map_zip_root_symlinks(entry):
 def _map_explicit_symlinks(entry):
     return "symlink|" + entry.runfiles_path + "|" + entry.link_to_path
 
+def _map_runfiles_symlinks(ctx, runfiles):
+    """Maps runfiles symlinks, avoiding duplicate copies of packed files.
+
+    A runfiles symlink whose target file is also stored at its own runfiles
+    path (e.g. a venv site-packages entry, which points into a pip repo) is
+    stored as a relative symlink to that file instead of a second copy of its
+    content.
+    """
+    packed = {f: None for f in runfiles.files.to_list()}
+    lines = []
+    for prefix, entries, map_entry in [
+        (ctx.workspace_name, runfiles.symlinks, _map_zip_symlinks),
+        ("", runfiles.root_symlinks, _map_zip_root_symlinks),
+    ]:
+        for entry in entries.to_list():
+            link_path = paths.normalize(paths.join(prefix, entry.path))
+            target_path = paths.normalize(
+                runfiles_root_path(ctx, entry.target_file.short_path),
+            )
+
+            # A link at the target's own path would point to itself and take
+            # the place of the file, so keep storing the file there.
+            if entry.target_file in packed and link_path != target_path:
+                lines.append("symlink|{}|{}".format(link_path, target_path))
+            else:
+                lines.append(map_entry(entry))
+    return lines
+
 def _build_manifest(ctx, manifest, runfiles, explicit_symlinks, inputs):
     manifest.add_all(
         # NOTE: Accessing runfiles.empty_filenames materializes them. A lambda
@@ -103,8 +131,14 @@ def _build_manifest(ctx, manifest, runfiles, explicit_symlinks, inputs):
     )
 
     manifest.add_all(runfiles.files, map_each = _map_zip_runfiles)
-    manifest.add_all(runfiles.symlinks, map_each = _map_zip_symlinks)
-    manifest.add_all(runfiles.root_symlinks, map_each = _map_zip_root_symlinks)
+
+    # On Windows, creating symlinks when extracting needs extra privileges, so
+    # only the explicit venv symlinks are stored as symlinks there.
+    if is_windows_platform(ctx):
+        manifest.add_all(runfiles.symlinks, map_each = _map_zip_symlinks)
+        manifest.add_all(runfiles.root_symlinks, map_each = _map_zip_root_symlinks)
+    else:
+        manifest.add_all(_map_runfiles_symlinks(ctx, runfiles))
     manifest.add_all(explicit_symlinks, map_each = _map_explicit_symlinks)
 
     inputs.add(runfiles.files)

tests/py_zipapp/BUILD.bazel

@@ -5,6 +5,7 @@ load("//python:py_test.bzl", "py_test")
 load("//python/private:bzlmod_enabled.bzl", "BZLMOD_ENABLED")  # buildifier: disable=bzl-visibility
 load("//python/zipapp:py_zipapp_binary.bzl", "py_zipapp_binary")
 load("//tests/support:support.bzl", "SUPPORTS_BOOTSTRAP_SCRIPT")
+load(":self_root_symlink.bzl", "self_root_symlink")
 
 py_binary(
     name = "venv_bin",
@@ -17,12 +18,20 @@ py_binary(
             "//python/config_settings:bootstrap_impl": "script",
         },
     }),
+    data = [":self_root_symlink"],
     main = "main.py",
     # Pyrefly's bazel-check validator rejects explicit import paths with '.' components from :bin_deps.
     tags = ["no-pyrefly"],
     deps = [":bin_deps"],
 )
 
+# A runfiles root symlink at its target's own runfiles path. The zipapp must
+# store the file there, not a symlink to itself.
+self_root_symlink(
+    name = "self_root_symlink",
+    src = "self_root_symlink.txt",
+)
+
 py_zipapp_binary(
     name = "venv_zipapp",
     binary = ":venv_bin",
@@ -103,6 +112,10 @@ py_library(
     deps = [
         ":pkgdep",
         ":some_dep",
+        # A pip dep with a native extension. The venv links each shared
+        # library file individually, so it's a runfiles symlink to a file
+        # that's also packed in the zipapp.
+        "@dev_pip//markupsafe",
     ],
 )
 

tests/py_zipapp/main.py

@@ -51,6 +51,9 @@ def main():
         import pkgdep.pkgmod
 
         print(f"dep: {pkgdep.pkgmod}")
+        import markupsafe._speedups
+
+        print(f"native dep: {markupsafe._speedups}")
     except ImportError as e:
         e.add_note(
             "Failed to import a dependency.\n" + "sys.path:\n" + "\n".join(sys.path)

tests/py_zipapp/self_root_symlink.bzl

@@ -0,0 +1,15 @@
+"""A rule whose runfiles root symlink is at its target file's own path."""
+
+def _self_root_symlink_impl(ctx):
+    f = ctx.file.src
+    return [DefaultInfo(runfiles = ctx.runfiles(
+        files = [f],
+        root_symlinks = {ctx.workspace_name + "/" + f.short_path: f},
+    ))]
+
+self_root_symlink = rule(
+    implementation = _self_root_symlink_impl,
+    attrs = {
+        "src": attr.label(allow_single_file = True, mandatory = True),
+    },
+)

tests/py_zipapp/self_root_symlink.txt

@@ -0,0 +1 @@
+A file whose runfiles root symlink is at its own path.

tests/py_zipapp/venv_zipapp_test.py

@@ -1,5 +1,6 @@
 import contextlib
 import os
+import posixpath
 import subprocess
 import unittest
 import zipfile
@@ -26,6 +27,7 @@ def test_zipapp_runnable(self):
             )
         self.assertIn("Hello from zipapp", output)
         self.assertIn("dep:", output)
+        self.assertIn("native dep:", output)
 
     def assertHasPathMatchingSuffix(self, namelist, suffix, msg=None):
         if not any(name.endswith(suffix) for name in namelist):
@@ -105,6 +107,49 @@ def test_zipapp_structure(self):
                 python_bin = f"{venv_root}/bin/python3"
                 self.assertZipEntryIsSymlink(zf, python_bin)
 
+                # The venv links a pip dep's shared library individually. It's
+                # stored as a relative symlink to the copy packed from the pip
+                # repo, not as a second copy of the library.
+                venv_file = next(
+                    (
+                        name
+                        for name in namelist
+                        if name.startswith(venv_root + "/lib/")
+                        and "/site-packages/markupsafe/_speedups." in name
+                        and name.endswith(".so")
+                    ),
+                    None,
+                )
+                self.assertIsNotNone(venv_file, "\n".join(namelist))
+                self.assertZipEntryIsSymlink(zf, venv_file)
+                target = posixpath.normpath(
+                    posixpath.join(
+                        posixpath.dirname(venv_file),
+                        zf.read(venv_file).decode("utf-8"),
+                    )
+                )
+                self.assertIn(target, namelist)
+                self.assertNotIn(".venv/", target)
+                self.assertTrue(
+                    (zf.getinfo(target).external_attr >> 16) & 0o170000 != 0o120000,
+                    f"{target} should be the packed file, not another symlink",
+                )
+
+            # A runfiles root symlink at its target's own path is stored as the
+            # file, not as a symlink to itself.
+            self_link = next(
+                (n for n in namelist if n.endswith("/py_zipapp/self_root_symlink.txt")),
+                None,
+            )
+            self.assertIsNotNone(self_link, "\n".join(namelist))
+            info = zf.getinfo(self_link)
+            self.assertNotEqual(
+                (info.external_attr >> 16) & 0o170000,
+                0o120000,
+                f"{self_link} should be a file, not a symlink",
+            )
+            self.assertIn(b"its own path", zf.read(self_link))
+
             # Verify _bazel_site_init.py exists in site-packages
             self.assertHasPathMatchingSuffix(
                 namelist, "/site-packages/_bazel_site_init.py"