fix(whl_library): fix the dependency generation for multi-python depenency 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.
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 61e9d95..1040ded 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -38,6 +38,10 @@
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
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 8010ccb..f3ddd3b 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 @@
"""
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 @@
# 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 @@
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(
- name = "is_{name}",
- constraint_values = {values},
- visibility = ["//visibility:private"],
-)""".format(
- name = os_arch,
- values = render.indent(render.list(sorted([str(Label(c)) for c in constraint_values]))).strip(),
- ),
- )
+ constraint_values_str = render.indent(render.list(constraint_values)).lstrip()
- 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 abi:
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,
- )
- settings = [
- ":is_" + os_arch,
- setting,
- ]
-
- plat = "{}_{}".format(abi, os_arch)
+ loads.append("""load("@rules_python//python/config_settings:config_settings.bzl", "is_python_config_setting")""")
additional_content.append(
"""\
-selects.config_setting_group(
- name = "{name}",
- match_all = {values},
+is_python_config_setting(
+ name = "is_{name}",
+ python_version = "3.{minor_version}",
+ 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."),
+ minor_version = abi[len("cp3"):],
+ constraint_values = constraint_values_str,
+ ),
+ )
+ else:
+ additional_content.append(
+ """\
+config_setting(
+ name = "is_{name}",
+ constraint_values = {constraint_values},
+ visibility = ["//visibility:private"],
+)""".format(
+ name = p.replace("cp3", "python_3."),
+ constraint_values = constraint_values_str,
),
)
@@ -379,7 +356,7 @@
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 cafb85f..fa018da 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 @@
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 f7c686d..d355bfe 100644
--- a/python/pip_install/tools/wheel_installer/wheel.py
+++ b/python/pip_install/tools/wheel_installer/wheel.py
@@ -59,7 +59,7 @@
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 @@
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 @@
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 __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 @@
minor_version=minor_version,
)
)
+
else:
ret.update(
cls.all(
@@ -252,6 +253,8 @@
return "Darwin"
elif self.os == OS.windows:
return "Windows"
+ else:
+ return ""
# derived from OS and Arch
@property
@@ -416,7 +419,9 @@
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 @@
):
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 20141e2..acf2315 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 @@
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 @@
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 @@
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 @@
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 @@
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 66126cf..62858af 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 @@
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 @@
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 @@
"@//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 @@
"@//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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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"])
