fix: venv site packages with pkgutil packages

AGENTS.md

@@ -21,10 +21,16 @@ into the sentence, not verbatim.
 
 ### bzl_library targets for bzl source files
 
-* `.bzl` files should have `bzl_library` defined for them.
+* A `bzl_library` target should be defined for every `.bzl` file outside
+  of the `tests/` directory.
 * They should have a single `srcs` file and be named after the file with `_bzl`
   appended.
-* Their deps should be based on the `load()` statements in the source file.
+* Their deps should be based on the `load()` statements in the source file
+  and refer to the `bzl_library` target containing the loaded file.
+  * For files in rules_python: replace `.bzl` with `_bzl`.
+    e.g. given `load("//foo:bar.bzl", ...)`, the target is `//foo:bar_bzl`.
+  * For files outside rules_python: remove the `.bzl` suffix. e.g. given
+    `load("@foo//foo:bar.bzl", ...)`, the target is `@foo//foo:bar`.
 * `bzl_library()` targets should be kept in alphabetical order by name.
 
 Example:

CHANGELOG.md

@@ -86,6 +86,9 @@ END_UNRELEASED_TEMPLATE
   {attr}`pip.defaults.whl_platform_tags` attribute to configure that. If
   `musllinux_*_x86_64` is specified, we will chose the lowest available
   wheel version. Fixes [#3250](https://github.com/bazel-contrib/rules_python/issues/3250).
+* (venvs) {obj}`--vens_site_packages=yes` no longer errors when packages with
+  overlapping files or directories are used together.
+  ([#3204](https://github.com/bazel-contrib/rules_python/issues/3204)).
 
 {#v0-0-0-added}
 ### Added

python/private/BUILD.bazel

@@ -419,6 +419,7 @@ bzl_library(
         ":rules_cc_srcs_bzl",
         ":toolchain_types_bzl",
         ":transition_labels_bzl",
+        ":venv_runfiles_bzl",
         "@bazel_skylib//lib:dicts",
         "@bazel_skylib//lib:paths",
         "@bazel_skylib//lib:structs",
@@ -468,6 +469,7 @@ bzl_library(
         ":py_internal_bzl",
         ":rule_builders_bzl",
         ":toolchain_types_bzl",
+        ":venv_runfiles_bzl",
         ":version_bzl",
         "@bazel_skylib//lib:dicts",
         "@bazel_skylib//rules:common_settings",
@@ -727,6 +729,16 @@ bzl_library(
     ],
 )
 
+bzl_library(
+    name = "venv_runfiles_bzl",
+    srcs = ["venv_runfiles.bzl"],
+    deps = [
+        ":common_bzl",
+        ":py_info.bzl",
+        "@bazel_skylib//lib:paths",
+    ],
+)
+
 # Needed to define bzl_library targets for docgen. (We don't define the
 # bzl_library target here because it'd give our users a transitive dependency
 # on Skylib.)

python/private/common.bzl

@@ -495,6 +495,9 @@ _BOOL_TYPE = type(True)
 def is_bool(v):
     return type(v) == _BOOL_TYPE
 
+def is_file(v):
+    return type(v) == "File"
+
 def target_platform_has_any_constraint(ctx, constraints):
     """Check if target platform has any of a list of constraints.
 
@@ -511,6 +514,37 @@ def target_platform_has_any_constraint(ctx, constraints):
             return True
     return False
 
+def relative_path(from_, to):
+    """Compute a relative path from one path to another.
+
+    Args:
+        from_: {type}`str` the starting directory. Note that it should be
+            a directory because relative-symlinks are relative to the
+            directory the symlink resides in.
+        to: {type}`str` the path that `from_` wants to point to
+
+    Returns:
+        {type}`str` a relative path
+    """
+    from_parts = from_.split("/")
+    to_parts = to.split("/")
+
+    # Strip common leading parts from both paths
+    n = min(len(from_parts), len(to_parts))
+    for _ in range(n):
+        if from_parts[0] == to_parts[0]:
+            from_parts.pop(0)
+            to_parts.pop(0)
+        else:
+            break
+
+    # Impossible to compute a relative path without knowing what ".." is
+    if from_parts and from_parts[0] == "..":
+        fail("cannot compute relative path from '%s' to '%s'", from_, to)
+
+    parts = ([".."] * len(from_parts)) + to_parts
+    return paths.join(*parts)
+
 def runfiles_root_path(ctx, short_path):
     """Compute a runfiles-root relative path from `File.short_path`
 

python/private/py_executable.bzl

@@ -48,6 +48,7 @@ load(
     "filter_to_py_srcs",
     "get_imports",
     "is_bool",
+    "relative_path",
     "runfiles_root_path",
     "target_platform_has_any_constraint",
 )
@@ -63,6 +64,7 @@ load(":reexports.bzl", "BuiltinPyInfo", "BuiltinPyRuntimeInfo")
 load(":rule_builders.bzl", "ruleb")
 load(":toolchain_types.bzl", "EXEC_TOOLS_TOOLCHAIN_TYPE", "TARGET_TOOLCHAIN_TYPE", TOOLCHAIN_TYPE = "TARGET_TOOLCHAIN_TYPE")
 load(":transition_labels.bzl", "TRANSITION_LABELS")
+load(":venv_runfiles.bzl", "create_venv_app_files")
 
 _py_builtins = py_internal
 _EXTERNAL_PATH_PREFIX = "external"
@@ -499,37 +501,6 @@ def _create_zip_main(ctx, *, stage2_bootstrap, runtime_details, venv):
     )
     return output
 
-def relative_path(from_, to):
-    """Compute a relative path from one path to another.
-
-    Args:
-        from_: {type}`str` the starting directory. Note that it should be
-            a directory because relative-symlinks are relative to the
-            directory the symlink resides in.
-        to: {type}`str` the path that `from_` wants to point to
-
-    Returns:
-        {type}`str` a relative path
-    """
-    from_parts = from_.split("/")
-    to_parts = to.split("/")
-
-    # Strip common leading parts from both paths
-    n = min(len(from_parts), len(to_parts))
-    for _ in range(n):
-        if from_parts[0] == to_parts[0]:
-            from_parts.pop(0)
-            to_parts.pop(0)
-        else:
-            break
-
-    # Impossible to compute a relative path without knowing what ".." is
-    if from_parts and from_parts[0] == "..":
-        fail("cannot compute relative path from '%s' to '%s'", from_, to)
-
-    parts = ([".."] * len(from_parts)) + to_parts
-    return paths.join(*parts)
-
 # Create a venv the executable can use.
 # For venv details and the venv startup process, see:
 # * https://docs.python.org/3/library/venv.html
@@ -636,9 +607,9 @@ def _create_venv(ctx, output_prefix, imports, runtime_details):
         VenvSymlinkKind.BIN: bin_dir,
         VenvSymlinkKind.LIB: site_packages,
     }
-    venv_symlinks = _create_venv_symlinks(ctx, venv_dir_map)
+    venv_app_files = create_venv_app_files(ctx, ctx.attr.deps, venv_dir_map)
 
-    files_without_interpreter = [pth, site_init] + venv_symlinks
+    files_without_interpreter = [pth, site_init] + venv_app_files
     if pyvenv_cfg:
         files_without_interpreter.append(pyvenv_cfg)
 
@@ -663,94 +634,6 @@ def _create_venv(ctx, output_prefix, imports, runtime_details):
         ),
     )
 
-def _create_venv_symlinks(ctx, venv_dir_map):
-    """Creates symlinks within the venv.
-
-    Args:
-        ctx: current rule ctx
-        venv_dir_map: mapping of VenvSymlinkKind constants to the
-            venv path.
-
-    Returns:
-        {type}`list[File]` list of the File symlink objects created.
-    """
-
-    # maps venv-relative path to the runfiles path it should point to
-    entries = depset(
-        transitive = [
-            dep[PyInfo].venv_symlinks
-            for dep in ctx.attr.deps
-            if PyInfo in dep
-        ],
-    ).to_list()
-
-    link_map = _build_link_map(entries)
-    venv_files = []
-    for kind, kind_map in link_map.items():
-        base = venv_dir_map[kind]
-        for venv_path, link_to in kind_map.items():
-            venv_link = ctx.actions.declare_symlink(paths.join(base, venv_path))
-            venv_link_rf_path = runfiles_root_path(ctx, venv_link.short_path)
-            rel_path = relative_path(
-                # dirname is necessary because a relative symlink is relative to
-                # the directory the symlink resides within.
-                from_ = paths.dirname(venv_link_rf_path),
-                to = link_to,
-            )
-            ctx.actions.symlink(output = venv_link, target_path = rel_path)
-            venv_files.append(venv_link)
-
-    return venv_files
-
-def _build_link_map(entries):
-    # dict[str package, dict[str kind, dict[str rel_path, str link_to_path]]]
-    pkg_link_map = {}
-
-    # dict[str package, str version]
-    version_by_pkg = {}
-
-    for entry in entries:
-        link_map = pkg_link_map.setdefault(entry.package, {})
-        kind_map = link_map.setdefault(entry.kind, {})
-
-        if version_by_pkg.setdefault(entry.package, entry.version) != entry.version:
-            # We ignore duplicates by design.
-            continue
-        elif entry.venv_path in kind_map:
-            # We ignore duplicates by design.
-            continue
-        else:
-            kind_map[entry.venv_path] = entry.link_to_path
-
-    # An empty link_to value means to not create the site package symlink. Because of the
-    # ordering, this allows binaries to remove entries by having an earlier dependency produce
-    # empty link_to values.
-    for link_map in pkg_link_map.values():
-        for kind, kind_map in link_map.items():
-            for dir_path, link_to in kind_map.items():
-                if not link_to:
-                    kind_map.pop(dir_path)
-
-    # dict[str kind, dict[str rel_path, str link_to_path]]
-    keep_link_map = {}
-
-    # Remove entries that would be a child path of a created symlink.
-    # Earlier entries have precedence to match how exact matches are handled.
-    for link_map in pkg_link_map.values():
-        for kind, kind_map in link_map.items():
-            keep_kind_map = keep_link_map.setdefault(kind, {})
-            for _ in range(len(kind_map)):
-                if not kind_map:
-                    break
-                dirname, value = kind_map.popitem()
-                keep_kind_map[dirname] = value
-                prefix = dirname + "/"  # Add slash to prevent /X matching /XY
-                for maybe_suffix in kind_map.keys():
-                    maybe_suffix += "/"  # Add slash to prevent /X matching /XY
-                    if maybe_suffix.startswith(prefix) or prefix.startswith(maybe_suffix):
-                        kind_map.pop(maybe_suffix)
-    return keep_link_map
-
 def _map_each_identity(v):
     return v
 

python/private/py_info.bzl

@@ -56,6 +56,14 @@ VenvSymlinkEntry = provider(
 An entry in `PyInfo.venv_symlinks`
 """,
     fields = {
+        "files": """
+:type: depset[File]
+
+Files under `link_to_path`.
+
+This is only used when multiple targets have overlapping `venv_path` paths. e.g.
+if one adds files to `venv_path=a/` and another adds files to `venv_path=a/b/`.
+""",
         "kind": """
 :type: str