fix(pypi) backport python_full_version fix to Python (#2833)

Handling of `python_full_version` correctly has been fixed in the
Starlark
implementation in #2793 and in this PR I am backporting the changes to
handle
the full python version target platform strings so that we can have the
same
behaviour for now.

At the same time I have simplified and got rid of the specialization
handling
in the Python algorithm just like I did in the starlark, which
simplifies the
tests and makes the algorithm more correct.

Summary:
* Handle `cp3x.y_os_arch` strings in the `platform.py`
* Produce correct strings when the `micro_version` is unset. Note, that
we use version `0` in evaluating but we use the default version in the
config setting. This is to keep compatibility with the current behaviour
when the target platform is not fully specified (which would be the case
for WORKSPACE users).
* Adjust the tests and the code to be more similar to the starlark impl.

Work towards #2830

Author: aignas

python/private/pypi/whl_installer/platform.py

@@ -18,7 +18,7 @@
 import sys
 from dataclasses import dataclass
 from enum import Enum
-from typing import Any, Dict, Iterator, List, Optional, Union
+from typing import Any, Dict, Iterator, List, Optional, Tuple, Union
 
 
 class OS(Enum):
@@ -77,25 +77,32 @@ def _as_int(value: Optional[Union[OS, Arch]]) -> int:
     return int(value.value)
 
 
-def host_interpreter_minor_version() -> int:
-    return sys.version_info.minor
+def host_interpreter_version() -> Tuple[int, int]:
+    return (sys.version_info.minor, sys.version_info.micro)
 
 
 @dataclass(frozen=True)
 class Platform:
     os: Optional[OS] = None
     arch: Optional[Arch] = None
     minor_version: Optional[int] = None
+    micro_version: Optional[int] = None
 
     @classmethod
     def all(
         cls,
         want_os: Optional[OS] = None,
         minor_version: Optional[int] = None,
+        micro_version: Optional[int] = None,
     ) -> List["Platform"]:
         return sorted(
             [
-                cls(os=os, arch=arch, minor_version=minor_version)
+                cls(
+                    os=os,
+                    arch=arch,
+                    minor_version=minor_version,
+                    micro_version=micro_version,
+                )
                 for os in OS
                 for arch in Arch
                 if not want_os or want_os == os
@@ -112,32 +119,16 @@ def host(cls) -> List["Platform"]:
             A list of parsed values which makes the signature the same as
             `Platform.all` and `Platform.from_string`.
         """
+        minor, micro = host_interpreter_version()
         return [
             Platform(
                 os=OS.interpreter(),
                 arch=Arch.interpreter(),
-                minor_version=host_interpreter_minor_version(),
+                minor_version=minor,
+                micro_version=micro,
             )
         ]
 
-    def all_specializations(self) -> Iterator["Platform"]:
-        """Return the platform itself and all its unambiguous specializations.
-
-        For more info about specializations see
-        https://bazel.build/docs/configurable-attributes
-        """
-        yield self
-        if self.arch is None:
-            for arch in Arch:
-                yield Platform(os=self.os, arch=arch, minor_version=self.minor_version)
-        if self.os is None:
-            for os in OS:
-                yield Platform(os=os, arch=self.arch, minor_version=self.minor_version)
-        if self.arch is None and self.os is None:
-            for os in OS:
-                for arch in Arch:
-                    yield Platform(os=os, arch=arch, minor_version=self.minor_version)
-
     def __lt__(self, other: Any) -> bool:
         """Add a comparison method, so that `sorted` returns the most specialized platforms first."""
         if not isinstance(other, Platform) or other is None:
@@ -153,24 +144,15 @@ def __lt__(self, other: Any) -> bool:
 
     def __str__(self) -> str:
         if self.minor_version is None:
-            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:
-                return f"{self.os}_{self.arch}"
-
-        if self.arch is None and self.os is None:
-            return f"@//python/config_settings:is_python_3.{self.minor_version}"
+            return f"{self.os}_{self.arch}"
 
-        if self.arch is None:
-            return f"cp3{self.minor_version}_{self.os}_anyarch"
+        minor_version = self.minor_version
+        micro_version = self.micro_version
 
-        if self.os is None:
-            return f"cp3{self.minor_version}_anyos_{self.arch}"
-
-        return f"cp3{self.minor_version}_{self.os}_{self.arch}"
+        if micro_version is None:
+            return f"cp3{minor_version}_{self.os}_{self.arch}"
+        else:
+            return f"cp3{minor_version}.{micro_version}_{self.os}_{self.arch}"
 
     @classmethod
     def from_string(cls, platform: Union[str, List[str]]) -> List["Platform"]:
@@ -190,14 +172,25 @@ def from_string(cls, platform: Union[str, List[str]]) -> List["Platform"]:
             os, _, arch = tail.partition("_")
             arch = arch or "*"
 
-            minor_version = int(abi[len("cp3") :]) if abi else None
+            if abi:
+                tail = abi[len("cp3") :]
+                minor_version, _, micro_version = tail.partition(".")
+                minor_version = int(minor_version)
+                if micro_version == "":
+                    micro_version = None
+                else:
+                    micro_version = int(micro_version)
+            else:
+                minor_version = None
+                micro_version = None
 
             if arch != "*":
                 ret.add(
                     cls(
                         os=OS[os] if os != "*" else None,
                         arch=Arch[arch],
                         minor_version=minor_version,
+                        micro_version=micro_version,
                     )
                 )
 
@@ -206,6 +199,7 @@ def from_string(cls, platform: Union[str, List[str]]) -> List["Platform"]:
                     cls.all(
                         want_os=OS[os] if os != "*" else None,
                         minor_version=minor_version,
+                        micro_version=micro_version,
                     )
                 )
 
@@ -282,7 +276,12 @@ def platform_machine(self) -> str:
 
     def env_markers(self, extra: str) -> Dict[str, str]:
         # If it is None, use the host version
-        minor_version = self.minor_version or host_interpreter_minor_version()
+        if self.minor_version is None:
+            minor, micro = host_interpreter_version()
+        else:
+            minor, micro = self.minor_version, self.micro_version
+
+        micro = micro or 0
 
         return {
             "extra": extra,
@@ -292,12 +291,9 @@ def env_markers(self, extra: str) -> Dict[str, str]:
             "platform_system": self.platform_system,
             "platform_release": "",  # unset
             "platform_version": "",  # unset
-            "python_version": f"3.{minor_version}",
-            # FIXME @aignas 2024-01-14: is putting zero last a good idea? Maybe we should
-            # use `20` or something else to avoid having weird issues where the full version is used for
-            # matching and the author decides to only support 3.y.5 upwards.
-            "implementation_version": f"3.{minor_version}.0",
-            "python_full_version": f"3.{minor_version}.0",
+            "python_version": f"3.{minor}",
+            "implementation_version": f"3.{minor}.{micro}",
+            "python_full_version": f"3.{minor}.{micro}",
             # we assume that the following are the same as the interpreter used to setup the deps:
             # "implementation_name": "cpython"
             # "platform_python_implementation: "CPython",

python/private/pypi/whl_installer/wheel.py

@@ -27,7 +27,7 @@
 
 from python.private.pypi.whl_installer.platform import (
     Platform,
-    host_interpreter_minor_version,
+    host_interpreter_version,
 )
 
 
@@ -62,12 +62,13 @@ def __init__(
         """
         self.name: str = Deps._normalize(name)
         self._platforms: Set[Platform] = platforms or set()
-        self._target_versions = {p.minor_version for p in platforms or {}}
-        self._default_minor_version = None
-        if platforms and len(self._target_versions) > 2:
+        self._target_versions = {(p.minor_version, p.micro_version) for p in platforms or {}}
+        if platforms and len(self._target_versions) > 1:
             # TODO @aignas 2024-06-23: enable this to be set via a CLI arg
             # for being more explicit.
-            self._default_minor_version = host_interpreter_minor_version()
+            self._default_minor_version, _ = host_interpreter_version()
+        else:
+            self._default_minor_version = None
 
         if None in self._target_versions and len(self._target_versions) > 2:
             raise ValueError(
@@ -88,8 +89,13 @@ def __init__(
         # Then add all of the requirements in order
         self._deps: Set[str] = set()
         self._select: Dict[Platform, Set[str]] = defaultdict(set)
+
+        reqs_by_name = {}
         for req in reqs:
-            self._add_req(req, want_extras)
+            reqs_by_name.setdefault(req.name, []).append(req)
+
+        for reqs in reqs_by_name.values():
+            self._add_req(reqs, want_extras)
 
     def _add(self, dep: str, platform: Optional[Platform]):
         dep = Deps._normalize(dep)
@@ -123,50 +129,6 @@ def _add(self, dep: str, platform: Optional[Platform]):
         # Add the platform-specific dep
         self._select[platform].add(dep)
 
-        # Add the dep to specializations of the given platform if they
-        # exist in the select statement.
-        for p in platform.all_specializations():
-            if p not in self._select:
-                continue
-
-            self._select[p].add(dep)
-
-        if len(self._select[platform]) == 1:
-            # We are adding a new item to the select and we need to ensure that
-            # existing dependencies from less specialized platforms are propagated
-            # to the newly added dependency set.
-            for p, deps in self._select.items():
-                # Check if the existing platform overlaps with the given platform
-                if p == platform or platform not in p.all_specializations():
-                    continue
-
-                self._select[platform].update(self._select[p])
-
-    def _maybe_add_common_dep(self, dep):
-        if len(self._target_versions) < 2:
-            return
-
-        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.
-        for p in platforms:
-            if p not in self._select:
-                return
-
-            if dep not in self._select[p]:
-                return
-
-        # All of the python version-specific branches have the dep, so lets add
-        # it to the common deps.
-        self._deps.add(dep)
-        for p in platforms:
-            self._select[p].remove(dep)
-            if not self._select[p]:
-                self._select.pop(p)
-
     @staticmethod
     def _normalize(name: str) -> str:
         return re.sub(r"[-_.]+", "_", name).lower()
@@ -227,66 +189,40 @@ def _resolve_extras(
 
         return extras
 
-    def _add_req(self, req: Requirement, extras: Set[str]) -> None:
-        if req.marker is None:
-            self._add(req.name, None)
-            return
+    def _add_req(self, reqs: List[Requirement], extras: Set[str]) -> None:
+        platforms_to_add = set()
+        for req in reqs:
+            if req.marker is None:
+                self._add(req.name, None)
+                return
 
-        marker_str = str(req.marker)
+            for plat in self._platforms:
+                if plat in platforms_to_add:
+                    # marker evaluation is more expensive than this check
+                    continue
 
-        if not self._platforms:
-            if any(req.marker.evaluate({"extra": extra}) for extra in extras):
-                self._add(req.name, None)
-            return
+                added = False
+                for extra in extras:
+                    if added:
+                        break
 
-        # NOTE @aignas 2023-12-08: in order to have reasonable select statements
-        # we do have to have some parsing of the markers, so it begs the question
-        # if packaging should be reimplemented in Starlark to have the best solution
-        # for now we will implement it in Python and see what the best parsing result
-        # can be before making this decision.
-        match_os = any(
-            tag in marker_str
-            for tag in [
-                "os_name",
-                "sys_platform",
-                "platform_system",
-            ]
-        )
-        match_arch = "platform_machine" in marker_str
-        match_version = "version" in marker_str
+                    if req.marker.evaluate(plat.env_markers(extra)):
+                        platforms_to_add.add(plat)
+                        added = True
+                        break
 
-        if not (match_os or match_arch or match_version):
-            if any(req.marker.evaluate({"extra": extra}) for extra in extras):
-                self._add(req.name, None)
+        if len(platforms_to_add) == len(self._platforms):
+            # the dep is in all target platforms, let's just add it to the regular
+            # list
+            self._add(req.name, None)
             return
 
-        for plat in self._platforms:
-            if not any(
-                req.marker.evaluate(plat.env_markers(extra)) for extra in extras
-            ):
-                continue
-
-            if match_arch and self._default_minor_version:
+        for plat in platforms_to_add:
+            if self._default_minor_version is not None:
                 self._add(req.name, plat)
-                if plat.minor_version == self._default_minor_version:
-                    self._add(req.name, Platform(plat.os, plat.arch))
-            elif match_arch:
-                self._add(req.name, Platform(plat.os, plat.arch))
-            elif match_os and self._default_minor_version:
-                self._add(req.name, Platform(plat.os, minor_version=plat.minor_version))
-                if plat.minor_version == self._default_minor_version:
-                    self._add(req.name, Platform(plat.os))
-            elif match_os:
-                self._add(req.name, Platform(plat.os))
-            elif match_version and self._default_minor_version:
-                self._add(req.name, Platform(minor_version=plat.minor_version))
-                if plat.minor_version == self._default_minor_version:
-                    self._add(req.name, Platform())
-            elif match_version:
-                self._add(req.name, None)
 
-        # Merge to common if possible after processing all platforms
-        self._maybe_add_common_dep(req.name)
+            if self._default_minor_version is None or plat.minor_version == self._default_minor_version:
+                self._add(req.name, Platform(os = plat.os, arch = plat.arch))
 
     def build(self) -> FrozenDeps:
         return FrozenDeps(