address review comments and finish some todos

* fail if both imports and site_packages_root set
* doc the topological ordering more
* skip, not fail, for child-path symlinks
* Also treat .pyi files as namespace anchors

Author: rickeylev

python/private/attributes.bzl

@@ -283,11 +283,7 @@ that depend on this rule. The strings are repo-runfiles-root relative,
 Absolute paths (paths that start with `/`) and paths that references a path
 above the execution root are not allowed and will result in an error.
 
-:::{attention}
-Setting both this and the {attr}`site_packages_root` attribute may result in
-undefined behavior. Both will result in the code being importable, but from
-different sys.path (and thus `__file__`) entries.
-:::
+This attribute is mutually exclusive with the {attr}`site_packages_root` attribute.
 """,
     ),
 }
@@ -314,6 +310,17 @@ These are typically `py_library` rules.
 
 Targets that only provide data files used at runtime belong in the `data`
 attribute.
+
+:::{note}
+The order of this list can matter because it affects the order that information
+from dependencies is merged in, which can be relevant depending on the ordering
+mode of depsets that are merged.
+
+* {obj}`PyInfo.site_packages_symlinks` uses topological ordering.
+
+See {obj}`PyInfo` for more information about the ordering of its depsets and
+how its fields are merged.
+:::
 """,
         ),
         "precompile": attr.string(

python/private/common.bzl

@@ -30,13 +30,14 @@ PackageSpecificationInfo = getattr(py_internal, "PackageSpecificationInfo", None
 # Extensions without the dot
 _PYTHON_SOURCE_EXTENSIONS = ["py"]
 
-# Extensions that make a file considered importable
+# Extensions that mean a file is relevant to Python
 PYTHON_FILE_EXTENSIONS = [
-    "py",
-    "so",  # Python C modules, usually Linux
+    "dll",  # Python C modules, Windows specific
     "dylib",  # Python C modules, Mac specific
+    "py",
     "pyc",
-    "dll",  # Python C modules, Windows specific
+    "pyi",
+    "so",  # Python C modules, usually Linux
 ]
 
 def create_binary_semantics_struct(

python/private/py_executable.bzl

@@ -624,6 +624,23 @@ def _create_site_packages_symlinks(ctx, site_packages):
             if PyInfo in dep
         ],
     ).to_list()
+    link_map = _build_link_map(entries)
+
+    sp_files = []
+    for sp_dir_path, link_to in link_map.items():
+        sp_link = ctx.actions.declare_symlink(paths.join(site_packages, sp_dir_path))
+        sp_link_rf_path = runfiles_root_path(ctx, sp_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(sp_link_rf_path),
+            to = link_to,
+        )
+        ctx.actions.symlink(output = sp_link, target_path = rel_path)
+        sp_files.append(sp_link)
+    return sp_files
+
+def _build_link_map(entries):
     link_map = {}
     for link_to_runfiles_path, site_packages_path in entries:
         if site_packages_path in link_map:
@@ -635,33 +652,27 @@ def _create_site_packages_symlinks(ctx, site_packages):
 
     # An empty link_to value means to not create the site package symlink.
     # Because of the topological ordering, this allows binaries to remove
-    # entries by having an earlier dependency produce empty link_to values
+    # entries by having an earlier dependency produce empty link_to values.
     for sp_dir_path, link_to in link_map.items():
         if not link_to:
             link_map.pop(sp_dir_path)
 
-    # This is N^2; we can certainly do better by sorting and exploiting the
-    # order.
-    # A trailing slash is appended / to prevent /X matching /XY
-    sp_dirs = [x + "/" for x in link_map.keys()]
-    for search_for in sp_dirs:
-        for prefix in sp_dirs:
-            if search_for != prefix and search_for.startswith(prefix):
-                fail("sub-link: {} under {}", search_for, prefix)
+    # Remove entries that would be a child path of a created symlink.
+    # Earlier entries have precedence to match how exact matches are handled.
+    keep_link_map = {}
+    for _ in range(len(link_map)):
+        if not link_map:
+            break
+        dirname, value = link_map.popitem()
+        keep_link_map[dirname] = value
 
-    sp_files = []
-    for sp_dir_path, link_to in link_map.items():
-        sp_link = ctx.actions.declare_symlink(paths.join(site_packages, sp_dir_path))
-        sp_link_rf_path = runfiles_root_path(ctx, sp_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(sp_link_rf_path),
-            to = link_to,
-        )
-        ctx.actions.symlink(output = sp_link, target_path = rel_path)
-        sp_files.append(sp_link)
-    return sp_files
+        prefix = dirname + "/"  # Add slash to prevent /X matching /XY
+        for maybe_suffix in link_map.keys():
+            maybe_suffix += "/"  # Add slash to prevent /X matching /XY
+            if maybe_suffix.startswith(prefix) or prefix.startswith(maybe_suffix):
+                link_map.pop(maybe_suffix)
+
+    return keep_link_map
 
 def _map_each_identity(v):
     return v

python/private/py_library.bzl

@@ -62,25 +62,40 @@ LIBRARY_ATTRS = union_attrs(
             doc = """
 Package relative prefix to remove from `srcs` for site-packages layouts.
 
+This attribute is mutually exclusive with the {attr}`imports` attribute.
+
 When set, `srcs` are interpreted to have a file layout as if they were installed
 in site-packages. This attribute specifies the directory within `srcs` to treat
 as the site-packages root so the correct site-packages relative paths for
 the files can be computed.
 
-For example, given `srcs=["site-packages/foo/bar.py"]`, specifying
-`site_packages_root="site-packages/" means `foo/bar.py` is the file path
-under the binary's venv site-packages directory that should be made availble.
-
 :::{note}
 This string is relative to the target's *Bazel package*. e.g. Relative to the
 directory with the BUILD file that defines the target (the same as how e.g.
 `srcs`).
 :::
 
-:::{attention}
-Setting both this an the {attr}`imports` attribute may result in undefined
-behavior. Both will result in the code being importable, but from different
-sys.path (and thus `__file__`) entries.
+For example, given `srcs=["site-packages/foo/bar.py"]`, specifying
+`site_packages_root="site-packages/" means `foo/bar.py` is the file path
+under the binary's venv site-packages directory that should be made available.
+
+`__init__.py` files are treated specially to provide basic support for [implicit
+namespace packages](
+https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#native-namespace-packages).
+However, the *content* of the files cannot be taken into account, merely their
+presence or absense. Stated another way: [pkgutil-style namespace packages](
+https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#pkgutil-style-namespace-packages)
+won't be understood as namespace packages; they'll be seen as regular packages. This will
+likely lead to conflicts with other targets that contribute to the namespace.
+
+:::{tip}
+This attributes populates {obj}`PyInfo.site_packages_symlinks`, which is
+a topologically ordered depset. This means dependencies closer and earlier
+to a consumer have precedence. See {obj}`PyInfo.site_packages_symlinks` for
+more information.
+:::
+
+:::{versionadded} VERSION_NEXT_FEATURE
 :::
 """,
         ),
@@ -128,7 +143,18 @@ def py_library_impl(ctx, *, semantics):
     runfiles.add(collect_runfiles(ctx))
     runfiles = runfiles.build(ctx)
 
-    site_packages_symlinks = _get_site_packages_symlinks(ctx)
+    imports = []
+    site_packages_symlinks = []
+    if ctx.attr.imports and ctx.attr.site_packages_root:
+        fail(("Only one of the `imports` or `site_packages_root` attributes " +
+              "can be set: site_packages_root={}, imports={}").format(
+            ctx.attr.site_packages_root,
+            ctx.attr.imports,
+        ))
+    elif ctx.attr.site_packages_root:
+        site_packages_symlinks = _get_site_packages_symlinks(ctx)
+    elif ctx.attr.imports:
+        imports = collect_imports(ctx, semantics)
 
     cc_info = semantics.get_cc_info_for_library(ctx)
     py_info, deps_transitive_sources, builtins_py_info = create_py_info(
@@ -138,7 +164,7 @@ def py_library_impl(ctx, *, semantics):
         required_pyc_files = required_pyc_files,
         implicit_pyc_files = implicit_pyc_files,
         implicit_pyc_source_files = implicit_pyc_source_files,
-        imports = collect_imports(ctx, semantics),
+        imports = imports,
         site_packages_symlinks = site_packages_symlinks,
     )