Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 4a262fa

Browse filesBrowse files
aignasaignas
authored
fix(pypi): fix the whl selection algorithm after bazel-contrib#2069 (bazel-contrib#2078)
It seems that a few things broke in recent commits: - We are not using the `MODULE.bazel.lock` file and it seems that it is easy to miss when the components in the PyPI extension stop integrating well together. This happened during the switch to `{abi}_{os}_{plat}` target platform passing within the code. - The logger code stopped working in the extension after the recent additions to add the `rule_name`. - `repo_utils.getenv` was always getting `PATH` env var on bazel `6.x`. This PR fixes both cases and updates docs to serve as a better reminder. By fixing the `select_whls` code and we can just rely on target platform triples (ABI, OS, CPU). This gets one step closer to maybe supporting optional `python_version` which would address bazel-contrib#1708. Whilst at it we are also adding different status messages for building the wheel from `sdist` vs just extracting or downloading the wheel. Tests: - Added more unit tests and brought them in line with the rest of the code. - Checked manually for differences between the `MODULE.bazel.lock` files in our `rules_python` extension before bazel-contrib#2069 and after this PR and there are no differences except in the `experimental_target_platforms` attribute in `whl_library`. Before this PR you would see that we do not select any wheels for e.g. `MarkupSafe` and we are always building from `sdist`. Work towards bazel-contrib#260.
1 parent ae2eb70 commit 4a262fa
Copy full SHA for 4a262fa

File tree

Expand file treeCollapse file tree

8 files changed

+110
-119
lines changed
Filter options
Expand file treeCollapse file tree

8 files changed

+110
-119
lines changed

‎CHANGELOG.md

Copy file name to clipboardExpand all lines: CHANGELOG.md
+2-1Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@ A brief description of the categories of changes:
2525
[x.x.x]: https://github.com/bazelbuild/rules_python/releases/tag/x.x.x
2626

2727
### Changed
28-
* Nothing yet
28+
* (whl_library) A better log message when the wheel is built from an sdist or
29+
when the wheel is downloaded using `download_only` feature to aid debugging.
2930

3031
### Fixed
3132
* (rules) Signals are properly received when using {obj}`--bootstrap_impl=script`

‎python/private/pypi/extension.bzl

Copy file name to clipboardExpand all lines: python/private/pypi/extension.bzl
+1-2Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ You cannot use both the additive_build_content and additive_build_content_file a
9999
)
100100

101101
def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, simpleapi_cache, exposed_packages):
102-
logger = repo_utils.logger(module_ctx)
102+
logger = repo_utils.logger(module_ctx, "pypi:create_whl_repos")
103103
python_interpreter_target = pip_attr.python_interpreter_target
104104
is_hub_reproducible = True
105105

@@ -195,7 +195,6 @@ def _create_whl_repos(module_ctx, pip_attr, whl_map, whl_overrides, group_map, s
195195
logger = logger,
196196
),
197197
get_index_urls = get_index_urls,
198-
python_version = major_minor,
199198
logger = logger,
200199
)
201200

‎python/private/pypi/parse_requirements.bzl

Copy file name to clipboardExpand all lines: python/private/pypi/parse_requirements.bzl
+5-28Lines changed: 5 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,7 @@ def parse_requirements(
3838
requirements_by_platform = {},
3939
extra_pip_args = [],
4040
get_index_urls = None,
41-
python_version = None,
42-
logger = None,
43-
fail_fn = fail):
41+
logger = None):
4442
"""Get the requirements with platforms that the requirements apply to.
4543
4644
Args:
@@ -53,10 +51,7 @@ def parse_requirements(
5351
get_index_urls: Callable[[ctx, list[str]], dict], a callable to get all
5452
of the distribution URLs from a PyPI index. Accepts ctx and
5553
distribution names to query.
56-
python_version: str or None. This is needed when the get_index_urls is
57-
specified. It should be of the form "3.x.x",
5854
logger: repo_utils.logger or None, a simple struct to log diagnostic messages.
59-
fail_fn (Callable[[str], None]): A failure function used in testing failure cases.
6055
6156
Returns:
6257
A tuple where the first element a dict of dicts where the first key is
@@ -137,10 +132,6 @@ def parse_requirements(
137132

138133
index_urls = {}
139134
if get_index_urls:
140-
if not python_version:
141-
fail_fn("'python_version' must be provided")
142-
return None
143-
144135
index_urls = get_index_urls(
145136
ctx,
146137
# Use list({}) as a way to have a set
@@ -168,9 +159,8 @@ def parse_requirements(
168159

169160
for r in sorted(reqs.values(), key = lambda r: r.requirement_line):
170161
whls, sdist = _add_dists(
171-
r,
172-
index_urls.get(whl_name),
173-
python_version = python_version,
162+
requirement = r,
163+
index_urls = index_urls.get(whl_name),
174164
logger = logger,
175165
)
176166

@@ -238,15 +228,14 @@ def host_platform(ctx):
238228
repo_utils.get_platforms_cpu_name(ctx),
239229
)
240230

241-
def _add_dists(requirement, index_urls, python_version, logger = None):
231+
def _add_dists(*, requirement, index_urls, logger = None):
242232
"""Populate dists based on the information from the PyPI index.
243233
244234
This function will modify the given requirements_by_platform data structure.
245235
246236
Args:
247237
requirement: The result of parse_requirements function.
248238
index_urls: The result of simpleapi_download.
249-
python_version: The version of the python interpreter.
250239
logger: A logger for printing diagnostic info.
251240
"""
252241
if not index_urls:
@@ -289,18 +278,6 @@ def _add_dists(requirement, index_urls, python_version, logger = None):
289278
]))
290279

291280
# Filter out the wheels that are incompatible with the target_platforms.
292-
whls = select_whls(
293-
whls = whls,
294-
want_abis = [
295-
"none",
296-
"abi3",
297-
"cp" + python_version.replace(".", ""),
298-
# Older python versions have wheels for the `*m` ABI.
299-
"cp" + python_version.replace(".", "") + "m",
300-
],
301-
want_platforms = requirement.target_platforms,
302-
want_python_version = python_version,
303-
logger = logger,
304-
)
281+
whls = select_whls(whls = whls, want_platforms = requirement.target_platforms, logger = logger)
305282

306283
return whls, sdist

‎python/private/pypi/whl_library.bzl

Copy file name to clipboardExpand all lines: python/private/pypi/whl_library.bzl
+8-1Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,9 +231,16 @@ def _whl_library_impl(rctx):
231231
args = _parse_optional_attrs(rctx, args, extra_pip_args)
232232

233233
if not whl_path:
234+
if rctx.attr.urls:
235+
op_tmpl = "whl_library.BuildWheelFromSource({name}, {requirement})"
236+
elif rctx.attr.download_only:
237+
op_tmpl = "whl_library.DownloadWheel({name}, {requirement})"
238+
else:
239+
op_tmpl = "whl_library.ResolveRequirement({name}, {requirement})"
240+
234241
repo_utils.execute_checked(
235242
rctx,
236-
op = "whl_library.ResolveRequirement({}, {})".format(rctx.attr.name, rctx.attr.requirement),
243+
op = op_tmpl.format(name = rctx.attr.name, requirement = rctx.attr.requirement),
237244
arguments = args,
238245
environment = environment,
239246
quiet = rctx.attr.quiet,

‎python/private/pypi/whl_target_platforms.bzl

Copy file name to clipboardExpand all lines: python/private/pypi/whl_target_platforms.bzl
+32-9Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -46,15 +46,13 @@ _OS_PREFIXES = {
4646
"win": "windows",
4747
} # buildifier: disable=unsorted-dict-items
4848

49-
def select_whls(*, whls, want_python_version = "3.0", want_abis = [], want_platforms = [], logger = None):
49+
def select_whls(*, whls, want_platforms = [], logger = None):
5050
"""Select a subset of wheels suitable for target platforms from a list.
5151
5252
Args:
5353
whls(list[struct]): A list of candidates which have a `filename`
5454
attribute containing the `whl` filename.
55-
want_python_version(str): An optional parameter to filter whls by python version. Defaults to '3.0'.
56-
want_abis(list[str]): A list of ABIs that are supported.
57-
want_platforms(str): The platforms
55+
want_platforms(str): The platforms in "{abi}_{os}_{cpu}" or "{os}_{cpu}" format.
5856
logger: A logger for printing diagnostic messages.
5957
6058
Returns:
@@ -64,9 +62,34 @@ def select_whls(*, whls, want_python_version = "3.0", want_abis = [], want_platf
6462
if not whls:
6563
return []
6664

67-
version_limit = -1
68-
if want_python_version:
69-
version_limit = int(want_python_version.split(".")[1])
65+
want_abis = {
66+
"abi3": None,
67+
"none": None,
68+
}
69+
70+
_want_platforms = {}
71+
version_limit = None
72+
73+
for p in want_platforms:
74+
if not p.startswith("cp3"):
75+
fail("expected all platforms to start with ABI, but got: {}".format(p))
76+
77+
abi, _, os_cpu = p.partition("_")
78+
_want_platforms[os_cpu] = None
79+
_want_platforms[p] = None
80+
81+
version_limit_candidate = int(abi[3:])
82+
if not version_limit:
83+
version_limit = version_limit_candidate
84+
if version_limit and version_limit != version_limit_candidate:
85+
fail("Only a single python version is supported for now")
86+
87+
# For some legacy implementations the wheels may target the `cp3xm` ABI
88+
_want_platforms["{}m_{}".format(abi, os_cpu)] = None
89+
want_abis[abi] = None
90+
want_abis[abi + "m"] = None
91+
92+
want_platforms = sorted(_want_platforms)
7093

7194
candidates = {}
7295
for whl in whls:
@@ -101,7 +124,7 @@ def select_whls(*, whls, want_python_version = "3.0", want_abis = [], want_platf
101124
logger.trace(lambda: "Discarding the whl because the whl abi did not match")
102125
continue
103126

104-
if version_limit != -1 and whl_version_min > version_limit:
127+
if whl_version_min > version_limit:
105128
if logger:
106129
logger.trace(lambda: "Discarding the whl because the whl supported python version is too high")
107130
continue
@@ -110,7 +133,7 @@ def select_whls(*, whls, want_python_version = "3.0", want_abis = [], want_platf
110133
if parsed.platform_tag == "any":
111134
compatible = True
112135
else:
113-
for p in whl_target_platforms(parsed.platform_tag):
136+
for p in whl_target_platforms(parsed.platform_tag, abi_tag = parsed.abi_tag.strip("m") if parsed.abi_tag.startswith("cp") else None):
114137
if p.target_platform in want_platforms:
115138
compatible = True
116139
break

‎python/private/repo_utils.bzl

Copy file name to clipboardExpand all lines: python/private/repo_utils.bzl
+18-15Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -42,22 +42,23 @@ def _debug_print(rctx, message_cb):
4242
if _is_repo_debug_enabled(rctx):
4343
print(message_cb()) # buildifier: disable=print
4444

45-
def _logger(rctx):
45+
def _logger(ctx, name = None):
4646
"""Creates a logger instance for printing messages.
4747
4848
Args:
49-
rctx: repository_ctx object. If the attribute `_rule_name` is
50-
present, it will be included in log messages.
49+
ctx: repository_ctx or module_ctx object. If the attribute
50+
`_rule_name` is present, it will be included in log messages.
51+
name: name for the logger. Optional for repository_ctx usage.
5152
5253
Returns:
5354
A struct with attributes logging: trace, debug, info, warn, fail.
5455
"""
55-
if _is_repo_debug_enabled(rctx):
56+
if _is_repo_debug_enabled(ctx):
5657
verbosity_level = "DEBUG"
5758
else:
5859
verbosity_level = "WARN"
5960

60-
env_var_verbosity = rctx.os.environ.get(REPO_VERBOSITY_ENV_VAR)
61+
env_var_verbosity = _getenv(ctx, REPO_VERBOSITY_ENV_VAR)
6162
verbosity_level = env_var_verbosity or verbosity_level
6263

6364
verbosity = {
@@ -66,18 +67,23 @@ def _logger(rctx):
6667
"TRACE": 3,
6768
}.get(verbosity_level, 0)
6869

70+
if hasattr(ctx, "attr"):
71+
# This is `repository_ctx`.
72+
name = name or "{}(@@{})".format(getattr(ctx.attr, "_rule_name", "?"), ctx.name)
73+
elif not name:
74+
fail("The name has to be specified when using the logger with `module_ctx`")
75+
6976
def _log(enabled_on_verbosity, level, message_cb_or_str):
7077
if verbosity < enabled_on_verbosity:
7178
return
72-
rule_name = getattr(rctx.attr, "_rule_name", "?")
79+
7380
if type(message_cb_or_str) == "string":
7481
message = message_cb_or_str
7582
else:
7683
message = message_cb_or_str()
7784

78-
print("\nrules_python:{}(@@{}) {}:".format(
79-
rule_name,
80-
rctx.name,
85+
print("\nrules_python:{} {}:".format(
86+
name,
8187
level.upper(),
8288
), message) # buildifier: disable=print
8389

@@ -278,12 +284,9 @@ def _which_describe_failure(binary_name, path):
278284
path = path,
279285
)
280286

281-
def _getenv(rctx, name, default = None):
282-
# Bazel 7+ API
283-
if hasattr(rctx, "getenv"):
284-
return rctx.getenv(name, default)
285-
else:
286-
return rctx.os.environ.get("PATH", default)
287+
def _getenv(ctx, name, default = None):
288+
# Bazel 7+ API has ctx.getenv
289+
return getattr(ctx, "getenv", ctx.os.environ.get)(name, default)
287290

288291
def _args_to_str(arguments):
289292
return " ".join([_arg_repr(a) for a in arguments])

‎tests/pypi/parse_requirements/parse_requirements_tests.bzl

Copy file name to clipboardExpand all lines: tests/pypi/parse_requirements/parse_requirements_tests.bzl
-14Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -197,20 +197,6 @@ def _test_select_requirement_none_platform(env):
197197

198198
_tests.append(_test_select_requirement_none_platform)
199199

200-
def _test_fail_no_python_version(env):
201-
errors = []
202-
parse_requirements(
203-
ctx = _mock_ctx(),
204-
requirements_by_platform = {
205-
"requirements_lock": [""],
206-
},
207-
get_index_urls = lambda _, __: {},
208-
fail_fn = errors.append,
209-
)
210-
env.expect.that_str(errors[0]).equals("'python_version' must be provided")
211-
212-
_tests.append(_test_fail_no_python_version)
213-
214200
def parse_requirements_test_suite(name):
215201
"""Create the test suite.
216202

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.