Skip to content

Commit b4b52fc

Browse files
aignasaignas
authored
refactor/fix: store dists in parse_requirements output (bazel-contrib#1917)
This moves some of the code out of the `pip.bzl` extension and changes the layout of the code to prepare for multi-platform whl support. Summary: * parse_requirements: add whls and sdists attribute, so that we can use a function to populate the lists. Not sure if there is a better way to do this. * parse_requirements: add an extra code to ensure that we are handling the target platform filtering correctly. * select_whl: split the `select_whl` into `select_whls`, which filters the whls (this can be used later in multi-platform selects) and select_whl , which just is used get the most appropriate whl for the host platform. * Additionally fix the logic in `select_whl`, which would result in Python 3.12 wheels being selected on Python 3.11 interpreters because we were not taking into account the interpreter tag when doing the filtering. Fixes bazel-contrib#1930
1 parent c84cdf5 commit b4b52fc

9 files changed

Lines changed: 655 additions & 194 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,8 @@ A brief description of the categories of changes:
4949
"panic: runtime error: invalid memory address or nil pointer dereference"
5050
* (bzlmod) remove `pip.parse(annotations)` attribute as it is unused and has been
5151
replaced by whl_modifications.
52+
* (pip) Correctly select wheels when the python tag includes minor versions.
53+
See ([#1930](https://github.com/bazelbuild/rules_python/issues/1930))
5254

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

‎python/pip_install/pip_repository.bzl‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -584,7 +584,17 @@ python_interpreter. An example value: "@python3_x86_64-unknown-linux-gnu//:pytho
584584
),
585585
"quiet": attr.bool(
586586
default = True,
587-
doc = "If True, suppress printing stdout and stderr output to the terminal.",
587+
doc = """\
588+
If True, suppress printing stdout and stderr output to the terminal.
589+
590+
If you would like to get more diagnostic output, please use:
591+
592+
RULES_PYTHON_REPO_DEBUG=1
593+
594+
or
595+
596+
RULES_PYTHON_REPO_DEBUG_VERBOSITY=<INFO|DEBUG|TRACE>
597+
""",
588598
),
589599
"repo_prefix": attr.string(
590600
doc = """

‎python/private/bzlmod/pip.bzl‎

Lines changed: 34 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ load("//python/private:parse_requirements.bzl", "host_platform", "parse_requirem
2828
load("//python/private:parse_whl_name.bzl", "parse_whl_name")
2929
load("//python/private:pypi_index.bzl", "simpleapi_download")
3030
load("//python/private:render_pkg_aliases.bzl", "whl_alias")
31+
load("//python/private:repo_utils.bzl", "repo_utils")
3132
load("//python/private:version_label.bzl", "version_label")
3233
load("//python/private:whl_target_platforms.bzl", "select_whl")
3334
load(":pip_repository.bzl", "pip_repository")
@@ -100,6 +101,7 @@ You cannot use both the additive_build_content and additive_build_content_file a
100101
)
101102

102103
def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, simpleapi_cache):
104+
logger = repo_utils.logger(module_ctx)
103105
python_interpreter_target = pip_attr.python_interpreter_target
104106

105107
# if we do not have the python_interpreter set in the attributes
@@ -160,32 +162,18 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, s
160162

161163
# Create a new wheel library for each of the different whls
162164

163-
requirements_by_platform = parse_requirements(
164-
module_ctx,
165-
requirements_by_platform = pip_attr.requirements_by_platform,
166-
requirements_linux = pip_attr.requirements_linux,
167-
requirements_lock = pip_attr.requirements_lock,
168-
requirements_osx = pip_attr.requirements_darwin,
169-
requirements_windows = pip_attr.requirements_windows,
170-
extra_pip_args = pip_attr.extra_pip_args,
171-
)
172-
173-
index_urls = {}
165+
get_index_urls = None
174166
if pip_attr.experimental_index_url:
175167
if pip_attr.download_only:
176168
fail("Currently unsupported to use `download_only` and `experimental_index_url`")
177169

178-
index_urls = simpleapi_download(
179-
module_ctx,
170+
get_index_urls = lambda ctx, distributions: simpleapi_download(
171+
ctx,
180172
attr = struct(
181173
index_url = pip_attr.experimental_index_url,
182174
extra_index_urls = pip_attr.experimental_extra_index_urls or [],
183175
index_url_overrides = pip_attr.experimental_index_url_overrides or {},
184-
sources = list({
185-
req.distribution: None
186-
for reqs in requirements_by_platform.values()
187-
for req in reqs
188-
}),
176+
sources = distributions,
189177
envsubst = pip_attr.envsubst,
190178
# Auth related info
191179
netrc = pip_attr.netrc,
@@ -195,6 +183,19 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, s
195183
parallel_download = pip_attr.parallel_download,
196184
)
197185

186+
requirements_by_platform = parse_requirements(
187+
module_ctx,
188+
requirements_by_platform = pip_attr.requirements_by_platform,
189+
requirements_linux = pip_attr.requirements_linux,
190+
requirements_lock = pip_attr.requirements_lock,
191+
requirements_osx = pip_attr.requirements_darwin,
192+
requirements_windows = pip_attr.requirements_windows,
193+
extra_pip_args = pip_attr.extra_pip_args,
194+
get_index_urls = get_index_urls,
195+
python_version = major_minor,
196+
logger = logger,
197+
)
198+
198199
repository_platform = host_platform(module_ctx.os)
199200
for whl_name, requirements in requirements_by_platform.items():
200201
requirement = select_requirement(
@@ -255,37 +256,22 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, s
255256
)
256257
whl_library_args.update({k: v for k, (v, default) in maybe_args_with_default.items() if v == default})
257258

258-
if index_urls:
259-
whls = []
260-
sdist = None
261-
for sha256 in requirement.srcs.shas:
262-
# For now if the artifact is marked as yanked we just ignore it.
263-
#
264-
# See https://packaging.python.org/en/latest/specifications/simple-repository-api/#adding-yank-support-to-the-simple-api
265-
266-
maybe_whl = index_urls[whl_name].whls.get(sha256)
267-
if maybe_whl and not maybe_whl.yanked:
268-
whls.append(maybe_whl)
269-
continue
270-
271-
maybe_sdist = index_urls[whl_name].sdists.get(sha256)
272-
if maybe_sdist and not maybe_sdist.yanked:
273-
sdist = maybe_sdist
274-
continue
275-
276-
print("WARNING: Could not find a whl or an sdist with sha256={}".format(sha256)) # buildifier: disable=print
277-
259+
if requirement.whls or requirement.sdist:
260+
logger.debug(lambda: "Selecting a compatible dist for {} from dists:\n{}".format(
261+
repository_platform,
262+
json.encode(
263+
struct(
264+
whls = requirement.whls,
265+
sdist = requirement.sdist,
266+
),
267+
),
268+
))
278269
distribution = select_whl(
279-
whls = whls,
280-
want_abis = [
281-
"none",
282-
"abi3",
283-
"cp" + major_minor.replace(".", ""),
284-
# Older python versions have wheels for the `*m` ABI.
285-
"cp" + major_minor.replace(".", "") + "m",
286-
],
270+
whls = requirement.whls,
287271
want_platform = repository_platform,
288-
) or sdist
272+
) or requirement.sdist
273+
274+
logger.debug(lambda: "Selected: {}".format(distribution))
289275

290276
if distribution:
291277
whl_library_args["requirement"] = requirement.srcs.requirement
@@ -303,7 +289,7 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, s
303289
# This is no-op because pip is not used to download the wheel.
304290
whl_library_args.pop("download_only", None)
305291
else:
306-
print("WARNING: falling back to pip for installing the right file for {}".format(requirement.requirement_line)) # buildifier: disable=print
292+
logger.warn("falling back to pip for installing the right file for {}".format(requirement.requirement_line))
307293

308294
# We sort so that the lock-file remains the same no matter the order of how the
309295
# args are manipulated in the code going before.

‎python/private/parse_requirements.bzl‎

Lines changed: 109 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ behavior.
2929
load("//python/pip_install:requirements_parser.bzl", "parse")
3030
load(":normalize_name.bzl", "normalize_name")
3131
load(":pypi_index_sources.bzl", "get_simpleapi_sources")
32-
load(":whl_target_platforms.bzl", "whl_target_platforms")
32+
load(":whl_target_platforms.bzl", "select_whls", "whl_target_platforms")
3333

3434
# This includes the vendored _translate_cpu and _translate_os from
3535
# @platforms//host:extension.bzl at version 0.0.9 so that we don't
@@ -84,6 +84,11 @@ def _default_platforms(*, filter):
8484
if not filter:
8585
fail("Must specific a filter string, got: {}".format(filter))
8686

87+
if filter.startswith("cp3"):
88+
# TODO @aignas 2024-05-23: properly handle python versions in the filter.
89+
# For now we are just dropping it to ensure that we don't fail.
90+
_, _, filter = filter.partition("_")
91+
8792
sanitized = filter.replace("*", "").replace("_", "")
8893
if sanitized and not sanitized.isalnum():
8994
fail("The platform filter can only contain '*', '_' and alphanumerics")
@@ -142,6 +147,9 @@ def parse_requirements(
142147
requirements_lock = None,
143148
requirements_windows = None,
144149
extra_pip_args = [],
150+
get_index_urls = None,
151+
python_version = None,
152+
logger = None,
145153
fail_fn = fail):
146154
"""Get the requirements with platforms that the requirements apply to.
147155
@@ -156,6 +164,12 @@ def parse_requirements(
156164
requirements_windows (label): The requirements file for windows OS.
157165
extra_pip_args (string list): Extra pip arguments to perform extra validations and to
158166
be joined with args fined in files.
167+
get_index_urls: Callable[[ctx, list[str]], dict], a callable to get all
168+
of the distribution URLs from a PyPI index. Accepts ctx and
169+
distribution names to query.
170+
python_version: str or None. This is needed when the get_index_urls is
171+
specified. It should be of the form "3.x.x",
172+
logger: repo_utils.logger or None, a simple struct to log diagnostic messages.
159173
fail_fn (Callable[[str], None]): A failure function used in testing failure cases.
160174
161175
Returns:
@@ -312,20 +326,46 @@ def parse_requirements(
312326
)
313327
for_req.target_platforms.append(target_platform)
314328

315-
return {
316-
whl_name: [
317-
struct(
318-
distribution = r.distribution,
319-
srcs = r.srcs,
320-
requirement_line = r.requirement_line,
321-
target_platforms = sorted(r.target_platforms),
322-
extra_pip_args = r.extra_pip_args,
323-
download = r.download,
329+
index_urls = {}
330+
if get_index_urls:
331+
if not python_version:
332+
fail_fn("'python_version' must be provided")
333+
return None
334+
335+
index_urls = get_index_urls(
336+
ctx,
337+
# Use list({}) as a way to have a set
338+
list({
339+
req.distribution: None
340+
for reqs in requirements_by_platform.values()
341+
for req in reqs.values()
342+
}),
343+
)
344+
345+
ret = {}
346+
for whl_name, reqs in requirements_by_platform.items():
347+
for r in sorted(reqs.values(), key = lambda r: r.requirement_line):
348+
whls, sdist = _add_dists(
349+
r,
350+
index_urls.get(whl_name),
351+
python_version = python_version,
352+
logger = logger,
324353
)
325-
for r in sorted(reqs.values(), key = lambda r: r.requirement_line)
326-
]
327-
for whl_name, reqs in requirements_by_platform.items()
328-
}
354+
355+
ret.setdefault(whl_name, []).append(
356+
struct(
357+
distribution = r.distribution,
358+
srcs = r.srcs,
359+
requirement_line = r.requirement_line,
360+
target_platforms = sorted(r.target_platforms),
361+
extra_pip_args = r.extra_pip_args,
362+
download = r.download,
363+
whls = whls,
364+
sdist = sdist,
365+
),
366+
)
367+
368+
return ret
329369

330370
def select_requirement(requirements, *, platform):
331371
"""A simple function to get a requirement for a particular platform.
@@ -372,3 +412,58 @@ def host_platform(repository_os):
372412
_translate_os(repository_os.name.lower()),
373413
_translate_cpu(repository_os.arch.lower()),
374414
)
415+
416+
def _add_dists(requirement, index_urls, python_version, logger = None):
417+
"""Populate dists based on the information from the PyPI index.
418+
419+
This function will modify the given requirements_by_platform data structure.
420+
421+
Args:
422+
requirement: The result of parse_requirements function.
423+
index_urls: The result of simpleapi_download.
424+
python_version: The version of the python interpreter.
425+
logger: A logger for printing diagnostic info.
426+
"""
427+
if not index_urls:
428+
return [], None
429+
430+
whls = []
431+
sdist = None
432+
433+
# TODO @aignas 2024-05-22: it is in theory possible to add all
434+
# requirements by version instead of by sha256. This may be useful
435+
# for some projects.
436+
for sha256 in requirement.srcs.shas:
437+
# For now if the artifact is marked as yanked we just ignore it.
438+
#
439+
# See https://packaging.python.org/en/latest/specifications/simple-repository-api/#adding-yank-support-to-the-simple-api
440+
441+
maybe_whl = index_urls.whls.get(sha256)
442+
if maybe_whl and not maybe_whl.yanked:
443+
whls.append(maybe_whl)
444+
continue
445+
446+
maybe_sdist = index_urls.sdists.get(sha256)
447+
if maybe_sdist and not maybe_sdist.yanked:
448+
sdist = maybe_sdist
449+
continue
450+
451+
if logger:
452+
logger.warn("Could not find a whl or an sdist with sha256={}".format(sha256))
453+
454+
# Filter out the wheels that are incompatible with the target_platforms.
455+
whls = select_whls(
456+
whls = whls,
457+
want_abis = [
458+
"none",
459+
"abi3",
460+
"cp" + python_version.replace(".", ""),
461+
# Older python versions have wheels for the `*m` ABI.
462+
"cp" + python_version.replace(".", "") + "m",
463+
],
464+
want_platforms = requirement.target_platforms,
465+
want_python_version = python_version,
466+
logger = logger,
467+
)
468+
469+
return whls, sdist

‎python/private/parse_whl_name.bzl‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,30 @@
1616
A starlark implementation of a Wheel filename parsing.
1717
"""
1818

19+
# Taken from https://peps.python.org/pep-0600/
20+
_LEGACY_ALIASES = {
21+
"manylinux1_i686": "manylinux_2_5_i686",
22+
"manylinux1_x86_64": "manylinux_2_5_x86_64",
23+
"manylinux2010_i686": "manylinux_2_12_i686",
24+
"manylinux2010_x86_64": "manylinux_2_12_x86_64",
25+
"manylinux2014_aarch64": "manylinux_2_17_aarch64",
26+
"manylinux2014_armv7l": "manylinux_2_17_armv7l",
27+
"manylinux2014_i686": "manylinux_2_17_i686",
28+
"manylinux2014_ppc64": "manylinux_2_17_ppc64",
29+
"manylinux2014_ppc64le": "manylinux_2_17_ppc64le",
30+
"manylinux2014_s390x": "manylinux_2_17_s390x",
31+
"manylinux2014_x86_64": "manylinux_2_17_x86_64",
32+
}
33+
34+
def normalize_platform_tag(tag):
35+
"""Resolve legacy aliases to modern equivalents for easier parsing elsewhere."""
36+
return ".".join(list({
37+
# The `list({})` usage here is to use it as a string set, where we will
38+
# deduplicate, but otherwise retain the order of the tags.
39+
_LEGACY_ALIASES.get(p, p): None
40+
for p in tag.split(".")
41+
}))
42+
1943
def parse_whl_name(file):
2044
"""Parse whl file name into a struct of constituents.
2145
@@ -68,5 +92,5 @@ def parse_whl_name(file):
6892
build_tag = build_tag,
6993
python_tag = python_tag,
7094
abi_tag = abi_tag,
71-
platform_tag = platform_tag,
95+
platform_tag = normalize_platform_tag(platform_tag),
7296
)

0 commit comments

Comments
 (0)