fix: venv site packages with pkgutil packages (bazel-contrib#3268)

Currently, an error occurs if one packages files are intended to go into
a
sub-directory of another package's directory. This can happen when
pkgutil-style
namespace packages are used, which results in multiple distributions
wanting
to install the same files (pkgutil `__init__.py` files) into the same
top-level
directories. This eventually results in a Bazel error because Bazel
detects that
the one output is the prefix of another.

To fix, detect when distributions overlap in their paths and merge their
files
manually. Internally, entries are sorted from shorted venv path to
longest,
however, that's just an implementation detail.

Along the way, give agents better advice for bzl_library targets.

Fixes bazel-contrib#3204

---------

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

Author: rickeylev

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