refactor: explicitly define host platform ordering (#2890)
It turns out the `unsorted-dict-items` disable in versions.bzl is
load-bearing: the
precedence of what host-compatible runtime is selected depends on the
order of keys.
Hence, the keys are carefully defined such that freethreaded and musl
come after the
regular runtimes.
Make this subtle and implicit behavior explicit by having an ordering
function that
sorts keys in the order we want.
Work towards https://github.com/bazel-contrib/rules_python/issues/2081
---------
Co-authored-by: Ignas Anikevicius <240938+aignas@users.noreply.github.com>
diff --git a/python/private/python.bzl b/python/private/python.bzl
index 0f3bbee..0cc1938 100644
--- a/python/private/python.bzl
+++ b/python/private/python.bzl
@@ -21,7 +21,7 @@
load(":python_register_toolchains.bzl", "python_register_toolchains")
load(":pythons_hub.bzl", "hub_repo")
load(":repo_utils.bzl", "repo_utils")
-load(":toolchains_repo.bzl", "host_toolchain", "multi_toolchain_aliases")
+load(":toolchains_repo.bzl", "host_toolchain", "multi_toolchain_aliases", "sorted_host_platforms")
load(":util.bzl", "IS_BAZEL_6_4_OR_HIGHER")
load(":version.bzl", "version")
@@ -298,9 +298,8 @@
_internal_bzlmod_toolchain_call = True,
**kwargs
)
- host_platforms = []
- host_os_names = {}
- host_archs = {}
+
+ host_platforms = {}
for repo_name, (platform_name, platform_info) in register_result.impl_repos.items():
toolchain_impls.append(struct(
# str: The base name to use for the toolchain() target
@@ -319,17 +318,21 @@
set_python_version_constraint = is_last,
))
if _is_compatible_with_host(module_ctx, platform_info):
- host_key = str(len(host_platforms))
- host_platforms.append(platform_name)
- host_os_names[host_key] = platform_info.os_name
- host_archs[host_key] = platform_info.arch
+ host_platforms[platform_name] = platform_info
+ host_platforms = sorted_host_platforms(host_platforms)
host_toolchain(
name = toolchain_info.name + "_host",
# NOTE: Order matters. The first found to be compatible is (usually) used.
- platforms = host_platforms,
- os_names = host_os_names,
- arch_names = host_archs,
+ platforms = host_platforms.keys(),
+ os_names = {
+ str(i): platform_info.os_name
+ for i, platform_info in enumerate(host_platforms.values())
+ },
+ arch_names = {
+ str(i): platform_info.arch
+ for i, platform_info in enumerate(host_platforms.values())
+ },
python_version = full_python_version,
)
diff --git a/python/private/toolchains_repo.bzl b/python/private/toolchains_repo.bzl
index 29ac694..0fd05c6 100644
--- a/python/private/toolchains_repo.bzl
+++ b/python/private/toolchains_repo.bzl
@@ -25,6 +25,8 @@
load(
"//python:versions.bzl",
+ "FREETHREADED",
+ "MUSL",
"PLATFORMS",
"WINDOWS_NAME",
)
@@ -433,6 +435,44 @@
},
)
+def sorted_host_platforms(platform_map):
+ """Sort the keys in the platform map to give correct precedence.
+
+ The order of keys in the platform mapping matters for the host toolchain
+ selection. When multiple runtimes are compatible with the host, we take the
+ first that is compatible (usually; there's also the
+ `RULES_PYTHON_REPO_TOOLCHAIN_*` environment variables). The historical
+ behavior carefully constructed the ordering of platform keys such that
+ the ordering was:
+ * Regular platforms
+ * The "-freethreaded" suffix
+ * The "-musl" suffix
+
+ Here, we formalize that so it isn't subtly encoded in the ordering of keys
+ in a dict that autoformatters like to clobber and whose only documentation
+ is an innocous looking formatter disable directive.
+
+ Args:
+ platform_map: a mapping of platforms and their metadata.
+
+ Returns:
+ dict; the same values, but with the keys inserted in the desired
+ order so that iteration happens in the desired order.
+ """
+
+ def platform_keyer(name):
+ # Ascending sort: lower is higher precedence
+ return (
+ 1 if MUSL in name else 0,
+ 1 if FREETHREADED in name else 0,
+ )
+
+ sorted_platform_keys = sorted(platform_map.keys(), key = platform_keyer)
+ return {
+ key: platform_map[key]
+ for key in sorted_platform_keys
+ }
+
def _get_host_platform(*, rctx, logger, python_version, os_name, cpu_name, platforms):
"""Gets the host platform.
@@ -455,7 +495,7 @@
arch = rctx.attr.arch_names[key],
)
else:
- platform_map = PLATFORMS
+ platform_map = sorted_host_platforms(PLATFORMS)
candidates = []
for platform in platforms:
diff --git a/python/versions.bzl b/python/versions.bzl
index 4a2a4cb..166cc98 100644
--- a/python/versions.bzl
+++ b/python/versions.bzl
@@ -19,7 +19,9 @@
MACOS_NAME = "osx"
LINUX_NAME = "linux"
WINDOWS_NAME = "windows"
-FREETHREADED = "freethreaded"
+
+FREETHREADED = "-freethreaded"
+MUSL = "-musl"
INSTALL_ONLY = "install_only"
DEFAULT_RELEASE_BASE_URL = "https://github.com/astral-sh/python-build-standalone/releases/download"
@@ -845,7 +847,7 @@
for p, v in platforms.items()
for suffix, freethreadedness in {
"": is_freethreaded_no,
- "-" + FREETHREADED: is_freethreaded_yes,
+ FREETHREADED: is_freethreaded_yes,
}.items()
}
@@ -879,11 +881,11 @@
release_filename = None
rendered_urls = []
for u in url:
- p, _, _ = platform.partition("-" + FREETHREADED)
+ p, _, _ = platform.partition(FREETHREADED)
- if FREETHREADED in platform:
+ if FREETHREADED.lstrip("-") in platform:
build = "{}+{}-full".format(
- FREETHREADED,
+ FREETHREADED.lstrip("-"),
{
"aarch64-apple-darwin": "pgo+lto",
"aarch64-unknown-linux-gnu": "lto",
