fix(whl_library): fix the dependency generation for multi-python depe…

…nency closures (#1875)

We start using the recently introduced `is_python_config_setting` to
make
it possible to have a working select statement when multiple python
version selection needs to happen in a `whl_library`.

This adds further fixes so that the correct dependencies are pulled in
when the
`python_version` string flag is unset thus making this implementation
suitable
for `bzlmod` use case where we would use a single `whl_library` instance
for
multiple python versions within the hub.

Work towards #735.

CHANGELOG.md

@@ -38,6 +38,10 @@ A brief description of the categories of changes:
   be automatically deleted correctly. For example, if `python_generation_mode`
   is set to package, when `__init__.py` is deleted, the `py_library` generated
   for this package before will be deleted automatically.
+* (whl_library): Use `is_python_config_setting` to correctly handle multi-python
+  version dependency select statements when the `experimental_target_platforms`
+  includes the Python ABI. The default python version case within the select is
+  also now handled correctly, stabilizing the implementation.
 
 ### Added
 * (rules) Precompiling Python source at build time is available. but is

python/pip_install/private/generate_whl_library_build_bazel.bzl

@@ -92,12 +92,14 @@ py_library(
 """
 
 def _plat_label(plat):
+    if plat.endswith("default"):
+        return plat
     if plat.startswith("@//"):
         return "@@" + str(Label("//:BUILD.bazel")).partition("//")[0].strip("@") + plat.strip("@")
     elif plat.startswith("@"):
         return str(Label(plat))
     else:
-        return ":is_" + plat
+        return ":is_" + plat.replace("cp3", "python_3.")
 
 def _render_list_and_select(deps, deps_by_platform, tmpl):
     deps = render.list([tmpl.format(d) for d in sorted(deps)])
@@ -115,14 +117,7 @@ def _render_list_and_select(deps, deps_by_platform, tmpl):
 
     # Add the default, which means that we will be just using the dependencies in
     # `deps` for platforms that are not handled in a special way by the packages
-    #
-    # FIXME @aignas 2024-01-24: This currently works as expected only if the default
-    # value of the @rules_python//python/config_settings:python_version is set in
-    # the `.bazelrc`. If it is unset, then the we don't get the expected behaviour
-    # in cases where we are using a simple `py_binary` using the default toolchain
-    # without forcing any transitions. If the `python_version` config setting is set
-    # via .bazelrc, then everything works correctly.
-    deps_by_platform["//conditions:default"] = []
+    deps_by_platform.setdefault("//conditions:default", [])
     deps_by_platform = render.select(deps_by_platform, value_repr = render.list)
 
     if deps == "[]":
@@ -131,81 +126,63 @@ def _render_list_and_select(deps, deps_by_platform, tmpl):
         return "{} + {}".format(deps, deps_by_platform)
 
 def _render_config_settings(dependencies_by_platform):
-    py_version_by_os_arch = {}
+    loads = []
+    additional_content = []
     for p in dependencies_by_platform:
         # p can be one of the following formats:
+        # * //conditions:default
         # * @platforms//os:{value}
         # * @platforms//cpu:{value}
         # * @//python/config_settings:is_python_3.{minor_version}
         # * {os}_{cpu}
         # * cp3{minor_version}_{os}_{cpu}
-        if p.startswith("@"):
+        if p.startswith("@") or p.endswith("default"):
             continue
 
         abi, _, tail = p.partition("_")
         if not abi.startswith("cp"):
             tail = p
             abi = ""
+
         os, _, arch = tail.partition("_")
         os = "" if os == "anyos" else os
         arch = "" if arch == "anyarch" else arch
 
-        py_version_by_os_arch.setdefault((os, arch), []).append(abi)
-
-    if not py_version_by_os_arch:
-        return None, None
-
-    loads = []
-    additional_content = []
-    for (os, arch), abis in py_version_by_os_arch.items():
         constraint_values = []
-        if os:
-            constraint_values.append("@platforms//os:{}".format(os))
         if arch:
             constraint_values.append("@platforms//cpu:{}".format(arch))
+        if os:
+            constraint_values.append("@platforms//os:{}".format(os))
 
-        os_arch = (os or "anyos") + "_" + (arch or "anyarch")
-        additional_content.append(
-            """\
-config_setting(
+        constraint_values_str = render.indent(render.list(constraint_values)).lstrip()
+
+        if abi:
+            if not loads:
+                loads.append("""load("@rules_python//python/config_settings:config_settings.bzl", "is_python_config_setting")""")
+
+            additional_content.append(
+                """\
+is_python_config_setting(
     name = "is_{name}",
-    constraint_values = {values},
+    python_version = "3.{minor_version}",
+    constraint_values = {constraint_values},
     visibility = ["//visibility:private"],
 )""".format(
-                name = os_arch,
-                values = render.indent(render.list(sorted([str(Label(c)) for c in constraint_values]))).strip(),
-            ),
-        )
-
-        if abis == [""]:
-            if not os or not arch:
-                fail("BUG: both os and arch should be set in this case")
-            continue
-
-        for abi in abis:
-            if not loads:
-                loads.append("""load("@bazel_skylib//lib:selects.bzl", "selects")""")
-            minor_version = int(abi[len("cp3"):])
-            setting = "@@{rules_python}//python/config_settings:is_python_3.{version}".format(
-                rules_python = str(Label("//:BUILD.bazel")).partition("//")[0].strip("@"),
-                version = minor_version,
+                    name = p.replace("cp3", "python_3."),
+                    minor_version = abi[len("cp3"):],
+                    constraint_values = constraint_values_str,
+                ),
             )
-            settings = [
-                ":is_" + os_arch,
-                setting,
-            ]
-
-            plat = "{}_{}".format(abi, os_arch)
-
+        else:
             additional_content.append(
                 """\
-selects.config_setting_group(
-    name = "{name}",
-    match_all = {values},
+config_setting(
+    name = "is_{name}",
+    constraint_values = {constraint_values},
     visibility = ["//visibility:private"],
 )""".format(
-                    name = _plat_label(plat).lstrip(":"),
-                    values = render.indent(render.list(sorted(settings))).strip(),
+                    name = p.replace("cp3", "python_3."),
+                    constraint_values = constraint_values_str,
                 ),
             )
 
@@ -379,7 +356,7 @@ def generate_whl_library_build_bazel(
     contents = "\n".join(
         [
             _BUILD_TEMPLATE.format(
-                loads = "\n".join(loads),
+                loads = "\n".join(sorted(loads)),
                 py_library_label = py_library_label,
                 dependencies = render.indent(lib_dependencies, " " * 4).lstrip(),
                 whl_file_deps = render.indent(whl_file_deps, " " * 4).lstrip(),

python/pip_install/tools/wheel_installer/arguments_test.py

@@ -56,7 +56,6 @@ def test_platform_aggregation(self) -> None:
         parser = arguments.parser()
         args = parser.parse_args(
             args=[
-                "--platform=host",
                 "--platform=linux_*",
                 "--platform=osx_*",
                 "--platform=windows_*",

python/pip_install/tools/wheel_installer/wheel.py

@@ -59,7 +59,7 @@ class Arch(Enum):
     ppc64le = ppc
 
     @classmethod
-    def interpreter(cls) -> "OS":
+    def interpreter(cls) -> "Arch":
         "Return the currently running interpreter architecture."
         # FIXME @aignas 2023-12-13: Hermetic toolchain on Windows 3.11.6
         # is returning an empty string here, so lets default to x86_64
@@ -94,12 +94,6 @@ class Platform:
     arch: Optional[Arch] = None
     minor_version: Optional[int] = None
 
-    def __post_init__(self):
-        if not self.os and not self.arch and not self.minor_version:
-            raise ValueError(
-                "At least one of os, arch, minor_version must be specified"
-            )
-
     @classmethod
     def all(
         cls,
@@ -125,7 +119,13 @@ def host(cls) -> List["Platform"]:
             A list of parsed values which makes the signature the same as
             `Platform.all` and `Platform.from_string`.
         """
-        return [cls(os=OS.interpreter(), arch=Arch.interpreter())]
+        return [
+            Platform(
+                os=OS.interpreter(),
+                arch=Arch.interpreter(),
+                minor_version=host_interpreter_minor_version(),
+            )
+        ]
 
     def all_specializations(self) -> Iterator["Platform"]:
         """Return the platform itself and all its unambiguous specializations.
@@ -160,9 +160,9 @@ def __lt__(self, other: Any) -> bool:
 
     def __str__(self) -> str:
         if self.minor_version is None:
-            assert (
-                self.os is not None
-            ), f"if minor_version is None, OS must be specified, got {repr(self)}"
+            if self.os is None and self.arch is None:
+                return "//conditions:default"
+
             if self.arch is None:
                 return f"@platforms//os:{self.os}"
             else:
@@ -207,6 +207,7 @@ def from_string(cls, platform: Union[str, List[str]]) -> List["Platform"]:
                         minor_version=minor_version,
                     )
                 )
+
             else:
                 ret.update(
                     cls.all(
@@ -252,6 +253,8 @@ def platform_system(self) -> str:
             return "Darwin"
         elif self.os == OS.windows:
             return "Windows"
+        else:
+            return ""
 
     # derived from OS and Arch
     @property
@@ -416,7 +419,9 @@ def _maybe_add_common_dep(self, dep):
         if len(self._target_versions) < 2:
             return
 
-        platforms = [Platform(minor_version=v) for v in self._target_versions]
+        platforms = [Platform()] + [
+            Platform(minor_version=v) for v in self._target_versions
+        ]
 
         # If the dep is targeting all target python versions, lets add it to
         # the common dependency list to simplify the select statements.
@@ -534,14 +539,22 @@ def _add_req(self, req: Requirement, extras: Set[str]) -> None:
             ):
                 continue
 
-            if match_arch:
+            if match_arch and self._add_version_select:
+                self._add(req.name, plat)
+                if plat.minor_version == host_interpreter_minor_version():
+                    self._add(req.name, Platform(plat.os, plat.arch))
+            elif match_arch:
                 self._add(req.name, plat)
             elif match_os and self._add_version_select:
                 self._add(req.name, Platform(plat.os, minor_version=plat.minor_version))
+                if plat.minor_version == host_interpreter_minor_version():
+                    self._add(req.name, Platform(plat.os))
             elif match_os:
                 self._add(req.name, Platform(plat.os))
             elif match_version and self._add_version_select:
                 self._add(req.name, Platform(minor_version=plat.minor_version))
+                if plat.minor_version == host_interpreter_minor_version():
+                    self._add(req.name, Platform())
             elif match_version:
                 self._add(req.name, None)