diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e993a7db..9737a0e1e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,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 diff --git a/python/pip_install/private/generate_whl_library_build_bazel.bzl b/python/pip_install/private/generate_whl_library_build_bazel.bzl index 8010ccbad..f3ddd3bca 100644 --- a/python/pip_install/private/generate_whl_library_build_bazel.bzl +++ b/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(), diff --git a/python/pip_install/tools/wheel_installer/arguments_test.py b/python/pip_install/tools/wheel_installer/arguments_test.py index cafb85f8e..fa018da40 100644 --- a/python/pip_install/tools/wheel_installer/arguments_test.py +++ b/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_*", diff --git a/python/pip_install/tools/wheel_installer/wheel.py b/python/pip_install/tools/wheel_installer/wheel.py index f7c686d9b..d355bfe69 100644 --- a/python/pip_install/tools/wheel_installer/wheel.py +++ b/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) diff --git a/python/pip_install/tools/wheel_installer/wheel_test.py b/python/pip_install/tools/wheel_installer/wheel_test.py index 20141e286..acf2315ee 100644 --- a/python/pip_install/tools/wheel_installer/wheel_test.py +++ b/python/pip_install/tools/wheel_installer/wheel_test.py @@ -1,5 +1,6 @@ import unittest from random import shuffle +from unittest import mock from python.pip_install.tools.wheel_installer import wheel @@ -216,22 +217,28 @@ def test_can_get_deps_based_on_specific_python_version(self): self.assertEqual(["bar"], py38_deps.deps) self.assertEqual({"@platforms//os:linux": ["posix_dep"]}, py38_deps.deps_select) - def test_can_get_version_select(self): + @mock.patch( + "python.pip_install.tools.wheel_installer.wheel.host_interpreter_minor_version" + ) + def test_can_get_version_select(self, mock_host_interpreter_version): requires_dist = [ "bar", "baz; python_version < '3.8'", + "baz_new; python_version >= '3.8'", "posix_dep; os_name=='posix'", "posix_dep_with_version; os_name=='posix' and python_version >= '3.8'", ] + mock_host_interpreter_version.return_value = 7 + + self.maxDiff = None deps = wheel.Deps( "foo", requires_dist=requires_dist, platforms=[ - wheel.Platform( - os=wheel.OS.linux, arch=wheel.Arch.x86_64, minor_version=minor - ) + wheel.Platform(os=os, arch=wheel.Arch.x86_64, minor_version=minor) for minor in [7, 8, 9] + for os in [wheel.OS.linux, wheel.OS.windows] ], ) got = deps.build() @@ -239,20 +246,38 @@ def test_can_get_version_select(self): self.assertEqual(["bar"], got.deps) self.assertEqual( { + "//conditions:default": ["baz"], "@//python/config_settings:is_python_3.7": ["baz"], + "@//python/config_settings:is_python_3.8": ["baz_new"], + "@//python/config_settings:is_python_3.9": ["baz_new"], + "@platforms//os:linux": ["baz", "posix_dep"], "cp37_linux_anyarch": ["baz", "posix_dep"], - "cp38_linux_anyarch": ["posix_dep", "posix_dep_with_version"], - "cp39_linux_anyarch": ["posix_dep", "posix_dep_with_version"], + "cp38_linux_anyarch": [ + "baz_new", + "posix_dep", + "posix_dep_with_version", + ], + "cp39_linux_anyarch": [ + "baz_new", + "posix_dep", + "posix_dep_with_version", + ], }, got.deps_select, ) - def test_deps_spanning_all_target_py_versions_are_added_to_common(self): + @mock.patch( + "python.pip_install.tools.wheel_installer.wheel.host_interpreter_minor_version" + ) + def test_deps_spanning_all_target_py_versions_are_added_to_common( + self, mock_host_version + ): requires_dist = [ "bar", "baz (<2,>=1.11) ; python_version < '3.8'", "baz (<2,>=1.14) ; python_version >= '3.8'", ] + mock_host_version.return_value = 8 deps = wheel.Deps( "foo", @@ -264,7 +289,12 @@ def test_deps_spanning_all_target_py_versions_are_added_to_common(self): self.assertEqual(["bar", "baz"], got.deps) self.assertEqual({}, got.deps_select) - def test_deps_are_not_duplicated(self): + @mock.patch( + "python.pip_install.tools.wheel_installer.wheel.host_interpreter_minor_version" + ) + def test_deps_are_not_duplicated(self, mock_host_version): + mock_host_version.return_value = 7 + # See an example in # https://files.pythonhosted.org/packages/76/9e/db1c2d56c04b97981c06663384f45f28950a73d9acf840c4006d60d0a1ff/opencv_python-4.9.0.80-cp37-abi3-win32.whl.metadata requires_dist = [ @@ -281,14 +311,21 @@ def test_deps_are_not_duplicated(self): deps = wheel.Deps( "foo", requires_dist=requires_dist, - platforms=wheel.Platform.from_string(["cp310_*"]), + platforms=wheel.Platform.from_string(["cp37_*", "cp310_*"]), ) got = deps.build() self.assertEqual(["bar"], got.deps) self.assertEqual({}, got.deps_select) - def test_deps_are_not_duplicated_when_encountering_platform_dep_first(self): + @mock.patch( + "python.pip_install.tools.wheel_installer.wheel.host_interpreter_minor_version" + ) + def test_deps_are_not_duplicated_when_encountering_platform_dep_first( + self, mock_host_version + ): + mock_host_version.return_value = 7 + # Note, that we are sorting the incoming `requires_dist` and we need to ensure that we are not getting any # issues even if the platform-specific line comes first. requires_dist = [ @@ -299,7 +336,7 @@ def test_deps_are_not_duplicated_when_encountering_platform_dep_first(self): deps = wheel.Deps( "foo", requires_dist=requires_dist, - platforms=wheel.Platform.from_string(["cp310_*"]), + platforms=wheel.Platform.from_string(["cp37_*", "cp310_*"]), ) got = deps.build() diff --git a/tests/pip_install/whl_library/generate_build_bazel_tests.bzl b/tests/pip_install/whl_library/generate_build_bazel_tests.bzl index 66126cf6f..62858afc9 100644 --- a/tests/pip_install/whl_library/generate_build_bazel_tests.bzl +++ b/tests/pip_install/whl_library/generate_build_bazel_tests.bzl @@ -21,8 +21,8 @@ _tests = [] def _test_simple(env): want = """\ -load("@rules_python//python:defs.bzl", "py_library", "py_binary") load("@bazel_skylib//rules:copy_file.bzl", "copy_file") +load("@rules_python//python:defs.bzl", "py_library", "py_binary") package(default_visibility = ["//visibility:public"]) @@ -86,9 +86,9 @@ _tests.append(_test_simple) def _test_dep_selects(env): want = """\ -load("@rules_python//python:defs.bzl", "py_library", "py_binary") load("@bazel_skylib//rules:copy_file.bzl", "copy_file") -load("@bazel_skylib//lib:selects.bzl", "selects") +load("@rules_python//python/config_settings:config_settings.bzl", "is_python_config_setting") +load("@rules_python//python:defs.bzl", "py_library", "py_binary") package(default_visibility = ["//visibility:public"]) @@ -113,9 +113,9 @@ filegroup( "@//python/config_settings:is_python_3.9": ["@pypi_py39_dep//:whl"], "@platforms//cpu:aarch64": ["@pypi_arm_dep//:whl"], "@platforms//os:windows": ["@pypi_win_dep//:whl"], - ":is_cp310_linux_ppc": ["@pypi_py310_linux_ppc_dep//:whl"], - ":is_cp39_anyos_aarch64": ["@pypi_py39_arm_dep//:whl"], - ":is_cp39_linux_anyarch": ["@pypi_py39_linux_dep//:whl"], + ":is_python_3.10_linux_ppc": ["@pypi_py310_linux_ppc_dep//:whl"], + ":is_python_3.9_anyos_aarch64": ["@pypi_py39_arm_dep//:whl"], + ":is_python_3.9_linux_anyarch": ["@pypi_py39_linux_dep//:whl"], ":is_linux_x86_64": ["@pypi_linux_intel_dep//:whl"], "//conditions:default": [], }, @@ -147,9 +147,9 @@ py_library( "@//python/config_settings:is_python_3.9": ["@pypi_py39_dep//:pkg"], "@platforms//cpu:aarch64": ["@pypi_arm_dep//:pkg"], "@platforms//os:windows": ["@pypi_win_dep//:pkg"], - ":is_cp310_linux_ppc": ["@pypi_py310_linux_ppc_dep//:pkg"], - ":is_cp39_anyos_aarch64": ["@pypi_py39_arm_dep//:pkg"], - ":is_cp39_linux_anyarch": ["@pypi_py39_linux_dep//:pkg"], + ":is_python_3.10_linux_ppc": ["@pypi_py310_linux_ppc_dep//:pkg"], + ":is_python_3.9_anyos_aarch64": ["@pypi_py39_arm_dep//:pkg"], + ":is_python_3.9_linux_anyarch": ["@pypi_py39_linux_dep//:pkg"], ":is_linux_x86_64": ["@pypi_linux_intel_dep//:pkg"], "//conditions:default": [], }, @@ -158,8 +158,9 @@ py_library( visibility = ["//visibility:public"], ) -config_setting( - name = "is_linux_ppc", +is_python_config_setting( + name = "is_python_3.10_linux_ppc", + python_version = "3.10", constraint_values = [ "@platforms//cpu:ppc", "@platforms//os:linux", @@ -167,45 +168,20 @@ config_setting( visibility = ["//visibility:private"], ) -selects.config_setting_group( - name = "is_cp310_linux_ppc", - match_all = [ - ":is_linux_ppc", - "@//python/config_settings:is_python_3.10", - ], - visibility = ["//visibility:private"], -) - -config_setting( - name = "is_anyos_aarch64", +is_python_config_setting( + name = "is_python_3.9_anyos_aarch64", + python_version = "3.9", constraint_values = ["@platforms//cpu:aarch64"], visibility = ["//visibility:private"], ) -selects.config_setting_group( - name = "is_cp39_anyos_aarch64", - match_all = [ - ":is_anyos_aarch64", - "@//python/config_settings:is_python_3.9", - ], - visibility = ["//visibility:private"], -) - -config_setting( - name = "is_linux_anyarch", +is_python_config_setting( + name = "is_python_3.9_linux_anyarch", + python_version = "3.9", constraint_values = ["@platforms//os:linux"], visibility = ["//visibility:private"], ) -selects.config_setting_group( - name = "is_cp39_linux_anyarch", - match_all = [ - ":is_linux_anyarch", - "@//python/config_settings:is_python_3.9", - ], - visibility = ["//visibility:private"], -) - config_setting( name = "is_linux_x86_64", constraint_values = [ @@ -239,8 +215,8 @@ _tests.append(_test_dep_selects) def _test_with_annotation(env): want = """\ -load("@rules_python//python:defs.bzl", "py_library", "py_binary") load("@bazel_skylib//rules:copy_file.bzl", "copy_file") +load("@rules_python//python:defs.bzl", "py_library", "py_binary") package(default_visibility = ["//visibility:public"]) @@ -327,8 +303,8 @@ _tests.append(_test_with_annotation) def _test_with_entry_points(env): want = """\ -load("@rules_python//python:defs.bzl", "py_library", "py_binary") load("@bazel_skylib//rules:copy_file.bzl", "copy_file") +load("@rules_python//python:defs.bzl", "py_library", "py_binary") package(default_visibility = ["//visibility:public"]) @@ -401,8 +377,8 @@ _tests.append(_test_with_entry_points) def _test_group_member(env): want = """\ -load("@rules_python//python:defs.bzl", "py_library", "py_binary") load("@bazel_skylib//rules:copy_file.bzl", "copy_file") +load("@rules_python//python:defs.bzl", "py_library", "py_binary") package(default_visibility = ["//visibility:public"]) @@ -503,8 +479,8 @@ _tests.append(_test_group_member) def _test_group_member_deps_to_hub(env): want = """\ -load("@rules_python//python:defs.bzl", "py_library", "py_binary") load("@bazel_skylib//rules:copy_file.bzl", "copy_file") +load("@rules_python//python:defs.bzl", "py_library", "py_binary") package(default_visibility = ["//visibility:public"])