Skip to content

Commit ede1163

Browse files
authored
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.
1 parent 47ad4d9 commit ede1163

File tree

6 files changed

+133
-127
lines changed

6 files changed

+133
-127
lines changed

CHANGELOG.md

+4
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,10 @@ A brief description of the categories of changes:
3838
be automatically deleted correctly. For example, if `python_generation_mode`
3939
is set to package, when `__init__.py` is deleted, the `py_library` generated
4040
for this package before will be deleted automatically.
41+
* (whl_library): Use `is_python_config_setting` to correctly handle multi-python
42+
version dependency select statements when the `experimental_target_platforms`
43+
includes the Python ABI. The default python version case within the select is
44+
also now handled correctly, stabilizing the implementation.
4145

4246
### Added
4347
* (rules) Precompiling Python source at build time is available. but is

python/pip_install/private/generate_whl_library_build_bazel.bzl

+33-56
Original file line numberDiff line numberDiff line change
@@ -92,12 +92,14 @@ py_library(
9292
"""
9393

9494
def _plat_label(plat):
95+
if plat.endswith("default"):
96+
return plat
9597
if plat.startswith("@//"):
9698
return "@@" + str(Label("//:BUILD.bazel")).partition("//")[0].strip("@") + plat.strip("@")
9799
elif plat.startswith("@"):
98100
return str(Label(plat))
99101
else:
100-
return ":is_" + plat
102+
return ":is_" + plat.replace("cp3", "python_3.")
101103

102104
def _render_list_and_select(deps, deps_by_platform, tmpl):
103105
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):
115117

116118
# Add the default, which means that we will be just using the dependencies in
117119
# `deps` for platforms that are not handled in a special way by the packages
118-
#
119-
# FIXME @aignas 2024-01-24: This currently works as expected only if the default
120-
# value of the @rules_python//python/config_settings:python_version is set in
121-
# the `.bazelrc`. If it is unset, then the we don't get the expected behaviour
122-
# in cases where we are using a simple `py_binary` using the default toolchain
123-
# without forcing any transitions. If the `python_version` config setting is set
124-
# via .bazelrc, then everything works correctly.
125-
deps_by_platform["//conditions:default"] = []
120+
deps_by_platform.setdefault("//conditions:default", [])
126121
deps_by_platform = render.select(deps_by_platform, value_repr = render.list)
127122

128123
if deps == "[]":
@@ -131,81 +126,63 @@ def _render_list_and_select(deps, deps_by_platform, tmpl):
131126
return "{} + {}".format(deps, deps_by_platform)
132127

133128
def _render_config_settings(dependencies_by_platform):
134-
py_version_by_os_arch = {}
129+
loads = []
130+
additional_content = []
135131
for p in dependencies_by_platform:
136132
# p can be one of the following formats:
133+
# * //conditions:default
137134
# * @platforms//os:{value}
138135
# * @platforms//cpu:{value}
139136
# * @//python/config_settings:is_python_3.{minor_version}
140137
# * {os}_{cpu}
141138
# * cp3{minor_version}_{os}_{cpu}
142-
if p.startswith("@"):
139+
if p.startswith("@") or p.endswith("default"):
143140
continue
144141

145142
abi, _, tail = p.partition("_")
146143
if not abi.startswith("cp"):
147144
tail = p
148145
abi = ""
146+
149147
os, _, arch = tail.partition("_")
150148
os = "" if os == "anyos" else os
151149
arch = "" if arch == "anyarch" else arch
152150

153-
py_version_by_os_arch.setdefault((os, arch), []).append(abi)
154-
155-
if not py_version_by_os_arch:
156-
return None, None
157-
158-
loads = []
159-
additional_content = []
160-
for (os, arch), abis in py_version_by_os_arch.items():
161151
constraint_values = []
162-
if os:
163-
constraint_values.append("@platforms//os:{}".format(os))
164152
if arch:
165153
constraint_values.append("@platforms//cpu:{}".format(arch))
154+
if os:
155+
constraint_values.append("@platforms//os:{}".format(os))
166156

167-
os_arch = (os or "anyos") + "_" + (arch or "anyarch")
168-
additional_content.append(
169-
"""\
170-
config_setting(
157+
constraint_values_str = render.indent(render.list(constraint_values)).lstrip()
158+
159+
if abi:
160+
if not loads:
161+
loads.append("""load("@rules_python//python/config_settings:config_settings.bzl", "is_python_config_setting")""")
162+
163+
additional_content.append(
164+
"""\
165+
is_python_config_setting(
171166
name = "is_{name}",
172-
constraint_values = {values},
167+
python_version = "3.{minor_version}",
168+
constraint_values = {constraint_values},
173169
visibility = ["//visibility:private"],
174170
)""".format(
175-
name = os_arch,
176-
values = render.indent(render.list(sorted([str(Label(c)) for c in constraint_values]))).strip(),
177-
),
178-
)
179-
180-
if abis == [""]:
181-
if not os or not arch:
182-
fail("BUG: both os and arch should be set in this case")
183-
continue
184-
185-
for abi in abis:
186-
if not loads:
187-
loads.append("""load("@bazel_skylib//lib:selects.bzl", "selects")""")
188-
minor_version = int(abi[len("cp3"):])
189-
setting = "@@{rules_python}//python/config_settings:is_python_3.{version}".format(
190-
rules_python = str(Label("//:BUILD.bazel")).partition("//")[0].strip("@"),
191-
version = minor_version,
171+
name = p.replace("cp3", "python_3."),
172+
minor_version = abi[len("cp3"):],
173+
constraint_values = constraint_values_str,
174+
),
192175
)
193-
settings = [
194-
":is_" + os_arch,
195-
setting,
196-
]
197-
198-
plat = "{}_{}".format(abi, os_arch)
199-
176+
else:
200177
additional_content.append(
201178
"""\
202-
selects.config_setting_group(
203-
name = "{name}",
204-
match_all = {values},
179+
config_setting(
180+
name = "is_{name}",
181+
constraint_values = {constraint_values},
205182
visibility = ["//visibility:private"],
206183
)""".format(
207-
name = _plat_label(plat).lstrip(":"),
208-
values = render.indent(render.list(sorted(settings))).strip(),
184+
name = p.replace("cp3", "python_3."),
185+
constraint_values = constraint_values_str,
209186
),
210187
)
211188

@@ -379,7 +356,7 @@ def generate_whl_library_build_bazel(
379356
contents = "\n".join(
380357
[
381358
_BUILD_TEMPLATE.format(
382-
loads = "\n".join(loads),
359+
loads = "\n".join(sorted(loads)),
383360
py_library_label = py_library_label,
384361
dependencies = render.indent(lib_dependencies, " " * 4).lstrip(),
385362
whl_file_deps = render.indent(whl_file_deps, " " * 4).lstrip(),

python/pip_install/tools/wheel_installer/arguments_test.py

-1
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,6 @@ def test_platform_aggregation(self) -> None:
5656
parser = arguments.parser()
5757
args = parser.parse_args(
5858
args=[
59-
"--platform=host",
6059
"--platform=linux_*",
6160
"--platform=osx_*",
6261
"--platform=windows_*",

python/pip_install/tools/wheel_installer/wheel.py

+26-13
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ class Arch(Enum):
5959
ppc64le = ppc
6060

6161
@classmethod
62-
def interpreter(cls) -> "OS":
62+
def interpreter(cls) -> "Arch":
6363
"Return the currently running interpreter architecture."
6464
# FIXME @aignas 2023-12-13: Hermetic toolchain on Windows 3.11.6
6565
# is returning an empty string here, so lets default to x86_64
@@ -94,12 +94,6 @@ class Platform:
9494
arch: Optional[Arch] = None
9595
minor_version: Optional[int] = None
9696

97-
def __post_init__(self):
98-
if not self.os and not self.arch and not self.minor_version:
99-
raise ValueError(
100-
"At least one of os, arch, minor_version must be specified"
101-
)
102-
10397
@classmethod
10498
def all(
10599
cls,
@@ -125,7 +119,13 @@ def host(cls) -> List["Platform"]:
125119
A list of parsed values which makes the signature the same as
126120
`Platform.all` and `Platform.from_string`.
127121
"""
128-
return [cls(os=OS.interpreter(), arch=Arch.interpreter())]
122+
return [
123+
Platform(
124+
os=OS.interpreter(),
125+
arch=Arch.interpreter(),
126+
minor_version=host_interpreter_minor_version(),
127+
)
128+
]
129129

130130
def all_specializations(self) -> Iterator["Platform"]:
131131
"""Return the platform itself and all its unambiguous specializations.
@@ -160,9 +160,9 @@ def __lt__(self, other: Any) -> bool:
160160

161161
def __str__(self) -> str:
162162
if self.minor_version is None:
163-
assert (
164-
self.os is not None
165-
), f"if minor_version is None, OS must be specified, got {repr(self)}"
163+
if self.os is None and self.arch is None:
164+
return "//conditions:default"
165+
166166
if self.arch is None:
167167
return f"@platforms//os:{self.os}"
168168
else:
@@ -207,6 +207,7 @@ def from_string(cls, platform: Union[str, List[str]]) -> List["Platform"]:
207207
minor_version=minor_version,
208208
)
209209
)
210+
210211
else:
211212
ret.update(
212213
cls.all(
@@ -252,6 +253,8 @@ def platform_system(self) -> str:
252253
return "Darwin"
253254
elif self.os == OS.windows:
254255
return "Windows"
256+
else:
257+
return ""
255258

256259
# derived from OS and Arch
257260
@property
@@ -416,7 +419,9 @@ def _maybe_add_common_dep(self, dep):
416419
if len(self._target_versions) < 2:
417420
return
418421

419-
platforms = [Platform(minor_version=v) for v in self._target_versions]
422+
platforms = [Platform()] + [
423+
Platform(minor_version=v) for v in self._target_versions
424+
]
420425

421426
# If the dep is targeting all target python versions, lets add it to
422427
# the common dependency list to simplify the select statements.
@@ -534,14 +539,22 @@ def _add_req(self, req: Requirement, extras: Set[str]) -> None:
534539
):
535540
continue
536541

537-
if match_arch:
542+
if match_arch and self._add_version_select:
543+
self._add(req.name, plat)
544+
if plat.minor_version == host_interpreter_minor_version():
545+
self._add(req.name, Platform(plat.os, plat.arch))
546+
elif match_arch:
538547
self._add(req.name, plat)
539548
elif match_os and self._add_version_select:
540549
self._add(req.name, Platform(plat.os, minor_version=plat.minor_version))
550+
if plat.minor_version == host_interpreter_minor_version():
551+
self._add(req.name, Platform(plat.os))
541552
elif match_os:
542553
self._add(req.name, Platform(plat.os))
543554
elif match_version and self._add_version_select:
544555
self._add(req.name, Platform(minor_version=plat.minor_version))
556+
if plat.minor_version == host_interpreter_minor_version():
557+
self._add(req.name, Platform())
545558
elif match_version:
546559
self._add(req.name, None)
547560

0 commit comments

Comments
 (0)