Skip to content

Commit 3d98aee

Browse files
authored
fix(toolchains): correctly order the toolchains (#2735)
Since toolchain matching is done by matching the first target that matches target settings, the `minor_mapping` config setting is special, because e.g. all `3.11.X` toolchains match the `python_version = "3.11"` setting. This just reshuffles the list so that we have toolchains that are in the `minor_mapping` before the rest. At the same time remove the workaround from the `lock.bzl` where the bug was initially discovered. Fixes #2685
1 parent 965dd51 commit 3d98aee

File tree

8 files changed

+269
-26
lines changed

8 files changed

+269
-26
lines changed

CHANGELOG.md

+3
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,9 @@ Unreleased changes template.
7878
* (toolchains) Do not try to run `chmod` when downloading non-windows hermetic toolchain
7979
repositories on Windows. Fixes
8080
[#2660](https://github.com/bazel-contrib/rules_python/issues/2660).
81+
* (toolchains) The toolchain matching is has been fixed when writing
82+
transitions transitioning on the `python_version` flag.
83+
Fixes [#2685](https://github.com/bazel-contrib/rules_python/issues/2685).
8184

8285
{#v0-0-0-added}
8386
### Added

python/private/python.bzl

+16-1
Original file line numberDiff line numberDiff line change
@@ -243,10 +243,25 @@ def parse_modules(*, module_ctx, _fail = fail):
243243
if len(toolchains) > _MAX_NUM_TOOLCHAINS:
244244
fail("more than {} python versions are not supported".format(_MAX_NUM_TOOLCHAINS))
245245

246+
# sort the toolchains so that the toolchain versions that are in the
247+
# `minor_mapping` are coming first. This ensures that `python_version =
248+
# "3.X"` transitions work as expected.
249+
minor_version_toolchains = []
250+
other_toolchains = []
251+
minor_mapping = list(config.minor_mapping.values())
252+
for t in toolchains:
253+
# FIXME @aignas 2025-04-04: How can we unit test that this ordering is
254+
# consistent with what would actually work?
255+
if config.minor_mapping.get(t.python_version, t.python_version) in minor_mapping:
256+
minor_version_toolchains.append(t)
257+
else:
258+
other_toolchains.append(t)
259+
toolchains = minor_version_toolchains + other_toolchains
260+
246261
return struct(
247262
config = config,
248263
debug_info = debug_info,
249-
default_python_version = toolchains[-1].python_version,
264+
default_python_version = default_toolchain.python_version,
250265
toolchains = [
251266
struct(
252267
python_version = t.python_version,

python/uv/private/BUILD.bazel

-2
Original file line numberDiff line numberDiff line change
@@ -43,10 +43,8 @@ bzl_library(
4343
":toolchain_types_bzl",
4444
"//python:py_binary_bzl",
4545
"//python/private:bzlmod_enabled_bzl",
46-
"//python/private:full_version_bzl",
4746
"//python/private:toolchain_types_bzl",
4847
"@bazel_skylib//lib:shell",
49-
"@pythons_hub//:versions_bzl",
5048
],
5149
)
5250

python/uv/private/lock.bzl

+9-22
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,8 @@
1616
"""
1717

1818
load("@bazel_skylib//lib:shell.bzl", "shell")
19-
load("@pythons_hub//:versions.bzl", "DEFAULT_PYTHON_VERSION", "MINOR_MAPPING")
2019
load("//python:py_binary.bzl", "py_binary")
2120
load("//python/private:bzlmod_enabled.bzl", "BZLMOD_ENABLED") # buildifier: disable=bzl-visibility
22-
load("//python/private:full_version.bzl", "full_version")
2321
load("//python/private:toolchain_types.bzl", "EXEC_TOOLS_TOOLCHAIN_TYPE") # buildifier: disable=bzl-visibility
2422
load(":toolchain_types.bzl", "UV_TOOLCHAIN_TYPE")
2523

@@ -75,15 +73,15 @@ def _args(ctx):
7573

7674
def _lock_impl(ctx):
7775
srcs = ctx.files.srcs
78-
python_version = full_version(
79-
version = ctx.attr.python_version or DEFAULT_PYTHON_VERSION,
80-
minor_mapping = MINOR_MAPPING,
81-
)
82-
output = ctx.actions.declare_file("{}.{}.out".format(
83-
ctx.label.name,
84-
python_version.replace(".", "_"),
85-
))
76+
fname = "{}.out".format(ctx.label.name)
77+
python_version = ctx.attr.python_version
78+
if python_version:
79+
fname = "{}.{}.out".format(
80+
ctx.label.name,
81+
python_version.replace(".", "_"),
82+
)
8683

84+
output = ctx.actions.declare_file(fname)
8785
toolchain_info = ctx.toolchains[UV_TOOLCHAIN_TYPE]
8886
uv = toolchain_info.uv_toolchain_info.uv[DefaultInfo].files_to_run.executable
8987

@@ -166,15 +164,7 @@ def _transition_impl(input_settings, attr):
166164
_PYTHON_VERSION_FLAG: input_settings[_PYTHON_VERSION_FLAG],
167165
}
168166
if attr.python_version:
169-
# FIXME @aignas 2025-03-20: using `full_version` is a workaround for a bug in
170-
# how we order toolchains in bazel. If I set the `python_version` flag
171-
# to `3.12`, I would expect the latest version to be selected, i.e. the
172-
# one that is in MINOR_MAPPING, but it seems that 3.12.0 is selected,
173-
# because of how the targets are ordered.
174-
settings[_PYTHON_VERSION_FLAG] = full_version(
175-
version = attr.python_version,
176-
minor_mapping = MINOR_MAPPING,
177-
)
167+
settings[_PYTHON_VERSION_FLAG] = attr.python_version
178168
return settings
179169

180170
_python_version_transition = transition(
@@ -436,9 +426,6 @@ def lock(
436426
if not BZLMOD_ENABLED:
437427
kwargs["target_compatible_with"] = ["@platforms//:incompatible"]
438428

439-
# FIXME @aignas 2025-03-17: should we have one more target that transitions
440-
# the python_version to ensure that if somebody calls `bazel build
441-
# :requirements` that it is locked with the right `python_version`?
442429
_lock(
443430
name = name,
444431
args = args,

tests/config_settings/transition/multi_version_tests.bzl

+2-1
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
# limitations under the License.
1414
"""Tests for py_test."""
1515

16+
load("@pythons_hub//:versions.bzl", "DEFAULT_PYTHON_VERSION")
1617
load("@rules_testing//lib:analysis_test.bzl", "analysis_test")
1718
load("@rules_testing//lib:test_suite.bzl", "test_suite")
1819
load("@rules_testing//lib:util.bzl", "TestingAspectInfo", rt_util = "util")
@@ -29,7 +30,7 @@ load("//tests/support:support.bzl", "CC_TOOLCHAIN")
2930
# If the toolchain is not resolved then you will have a weird message telling
3031
# you that your transition target does not have a PyRuntime provider, which is
3132
# caused by there not being a toolchain detected for the target.
32-
_PYTHON_VERSION = "3.11"
33+
_PYTHON_VERSION = DEFAULT_PYTHON_VERSION
3334

3435
_tests = []
3536

tests/python/python_tests.bzl

+52
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,58 @@ def _test_default_non_rules_python_ignore_root_user_error_non_root_module(env):
284284

285285
_tests.append(_test_default_non_rules_python_ignore_root_user_error_non_root_module)
286286

287+
def _test_toolchain_ordering(env):
288+
py = parse_modules(
289+
module_ctx = _mock_mctx(
290+
_mod(
291+
name = "my_module",
292+
toolchain = [
293+
_toolchain("3.10"),
294+
_toolchain("3.10.15"),
295+
_toolchain("3.10.16"),
296+
_toolchain("3.10.11"),
297+
_toolchain("3.11.1"),
298+
_toolchain("3.11.10"),
299+
_toolchain("3.11.11", is_default = True),
300+
],
301+
),
302+
_mod(name = "rules_python", toolchain = [_toolchain("3.11")]),
303+
),
304+
)
305+
got_versions = [
306+
t.python_version
307+
for t in py.toolchains
308+
]
309+
310+
env.expect.that_str(py.default_python_version).equals("3.11.11")
311+
env.expect.that_dict(py.config.minor_mapping).contains_exactly({
312+
"3.10": "3.10.16",
313+
"3.11": "3.11.11",
314+
"3.12": "3.12.9",
315+
"3.13": "3.13.2",
316+
"3.8": "3.8.20",
317+
"3.9": "3.9.21",
318+
})
319+
env.expect.that_collection(got_versions).contains_exactly([
320+
# First the full-version toolchains that are in minor_mapping
321+
# so that they get matched first if only the `python_version` is in MINOR_MAPPING
322+
#
323+
# The default version is always set in the `python_version` flag, so know, that
324+
# the default match will be somewhere in the first bunch.
325+
"3.10",
326+
"3.10.16",
327+
"3.11",
328+
"3.11.11",
329+
# Next, the rest, where we will match things based on the `python_version` being
330+
# the same
331+
"3.10.15",
332+
"3.10.11",
333+
"3.11.1",
334+
"3.11.10",
335+
]).in_order()
336+
337+
_tests.append(_test_toolchain_ordering)
338+
287339
def _test_default_from_defaults(env):
288340
py = parse_modules(
289341
module_ctx = _mock_mctx(
+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
load(":transitions_tests.bzl", "transitions_test_suite")
2+
3+
transitions_test_suite(
4+
name = "transitions_tests",
5+
)
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,182 @@
1+
# Copyright 2022 The Bazel Authors. All rights reserved.
2+
#
3+
# Licensed under the Apache License, Version 2.0 (the "License");
4+
# you may not use this file except in compliance with the License.
5+
# You may obtain a copy of the License at
6+
#
7+
# http://www.apache.org/licenses/LICENSE-2.0
8+
#
9+
# Unless required by applicable law or agreed to in writing, software
10+
# distributed under the License is distributed on an "AS IS" BASIS,
11+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
# See the License for the specific language governing permissions and
13+
# limitations under the License.
14+
15+
""
16+
17+
load("@pythons_hub//:versions.bzl", "DEFAULT_PYTHON_VERSION", "MINOR_MAPPING")
18+
load("@rules_testing//lib:analysis_test.bzl", "analysis_test")
19+
load("@rules_testing//lib:test_suite.bzl", "test_suite")
20+
load("@rules_testing//lib:util.bzl", rt_util = "util")
21+
load("//python:versions.bzl", "TOOL_VERSIONS")
22+
load("//python/private:bzlmod_enabled.bzl", "BZLMOD_ENABLED") # buildifier: disable=bzl-visibility
23+
load("//python/private:full_version.bzl", "full_version") # buildifier: disable=bzl-visibility
24+
load("//python/private:toolchain_types.bzl", "EXEC_TOOLS_TOOLCHAIN_TYPE") # buildifier: disable=bzl-visibility
25+
load("//tests/support:support.bzl", "PYTHON_VERSION")
26+
27+
_analysis_tests = []
28+
29+
def _transition_impl(input_settings, attr):
30+
"""Transition based on python_version flag.
31+
32+
This is a simple transition impl that a user of rules_python may implement
33+
for their own rule.
34+
"""
35+
settings = {
36+
PYTHON_VERSION: input_settings[PYTHON_VERSION],
37+
}
38+
if attr.python_version:
39+
settings[PYTHON_VERSION] = attr.python_version
40+
return settings
41+
42+
_python_version_transition = transition(
43+
implementation = _transition_impl,
44+
inputs = [PYTHON_VERSION],
45+
outputs = [PYTHON_VERSION],
46+
)
47+
48+
TestInfo = provider(
49+
doc = "A simple test provider to forward the values for the assertion.",
50+
fields = {"got": "", "want": ""},
51+
)
52+
53+
def _impl(ctx):
54+
if ctx.attr.skip:
55+
return [TestInfo(got = "", want = "")]
56+
57+
exec_tools = ctx.toolchains[EXEC_TOOLS_TOOLCHAIN_TYPE].exec_tools
58+
got_version = exec_tools.exec_interpreter[platform_common.ToolchainInfo].py3_runtime.interpreter_version_info
59+
60+
return [
61+
TestInfo(
62+
got = "{}.{}.{}".format(
63+
got_version.major,
64+
got_version.minor,
65+
got_version.micro,
66+
),
67+
want = ctx.attr.want_version,
68+
),
69+
]
70+
71+
_simple_transition = rule(
72+
implementation = _impl,
73+
attrs = {
74+
"python_version": attr.string(
75+
doc = "The input python version which we transition on.",
76+
),
77+
"skip": attr.bool(
78+
doc = "Whether to skip the test",
79+
),
80+
"want_version": attr.string(
81+
doc = "The python version that we actually expect to receive.",
82+
),
83+
"_allowlist_function_transition": attr.label(
84+
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
85+
),
86+
},
87+
toolchains = [
88+
config_common.toolchain_type(
89+
EXEC_TOOLS_TOOLCHAIN_TYPE,
90+
mandatory = False,
91+
),
92+
],
93+
cfg = _python_version_transition,
94+
)
95+
96+
def _test_transitions(*, name, tests, skip = False):
97+
"""A reusable rule so that we can split the tests."""
98+
targets = {}
99+
for test_name, (input_version, want_version) in tests.items():
100+
target_name = "{}_{}".format(name, test_name)
101+
targets["python_" + test_name] = target_name
102+
rt_util.helper_target(
103+
_simple_transition,
104+
name = target_name,
105+
python_version = input_version,
106+
want_version = want_version,
107+
skip = skip,
108+
)
109+
110+
analysis_test(
111+
name = name,
112+
impl = _test_transition_impl,
113+
targets = targets,
114+
)
115+
116+
def _test_transition_impl(env, targets):
117+
# Check that the forwarded version from the PyRuntimeInfo is correct
118+
for target in dir(targets):
119+
if not target.startswith("python"):
120+
# Skip other attributes that might be not the ones we set (e.g. to_json, to_proto).
121+
continue
122+
123+
test_info = env.expect.that_target(getattr(targets, target)).provider(
124+
TestInfo,
125+
factory = lambda v, meta: v,
126+
)
127+
env.expect.that_str(test_info.got).equals(test_info.want)
128+
129+
def _test_full_version(name):
130+
"""Check that python_version transitions work.
131+
132+
Expectation is to get the same full version that we input.
133+
"""
134+
_test_transitions(
135+
name = name,
136+
tests = {
137+
v.replace(".", "_"): (v, v)
138+
for v in TOOL_VERSIONS
139+
},
140+
)
141+
142+
_analysis_tests.append(_test_full_version)
143+
144+
def _test_minor_versions(name):
145+
"""Ensure that MINOR_MAPPING versions are correctly selected."""
146+
_test_transitions(
147+
name = name,
148+
skip = not BZLMOD_ENABLED,
149+
tests = {
150+
minor.replace(".", "_"): (minor, full)
151+
for minor, full in MINOR_MAPPING.items()
152+
},
153+
)
154+
155+
_analysis_tests.append(_test_minor_versions)
156+
157+
def _test_default(name):
158+
"""Check the default version.
159+
160+
Lastly, if we don't provide any version to the transition, we should
161+
get the default version
162+
"""
163+
default_version = full_version(
164+
version = DEFAULT_PYTHON_VERSION,
165+
minor_mapping = MINOR_MAPPING,
166+
) if DEFAULT_PYTHON_VERSION else ""
167+
168+
_test_transitions(
169+
name = name,
170+
skip = not BZLMOD_ENABLED,
171+
tests = {
172+
"default": (None, default_version),
173+
},
174+
)
175+
176+
_analysis_tests.append(_test_default)
177+
178+
def transitions_test_suite(name):
179+
test_suite(
180+
name = name,
181+
tests = _analysis_tests,
182+
)

0 commit comments

Comments
 (0)