Skip to content

Commit 5957450

Browse files
gregmagolangregmagolan
committed
refactor(whl_install): remove resolved_unpack_toolchain workaround
UNPACK_TOOLCHAIN is registered in repo.bzl with exec_compatible_with (via the cfg="exec" TOOL_CFGS entry), so ctx.toolchains[UNPACK_TOOLCHAIN] already resolves the exec-platform binary without any indirection. The resolved_unpack_toolchain rule and its cfg="exec" _unpack attr in whl_install were an unnecessary workaround, analogous to resolved_py_toolchain which was removed in #903. Remove resolved_unpack_toolchain from tools.bzl and BUILD.bazel, and replace the ctx.attr._unpack indirection in rule.bzl with a direct toolchain lookup. Add an analysis-time regression test (exec_toolchain_resolution_test) that confirms UNPACK_TOOLCHAIN resolves the exec-platform binary even when the target platform is arm64. The test writes the resolved binary path to a file for both native and arm64-transitioned targets and asserts they are identical, proving exec-platform resolution is honoured throughout platform transitions.
1 parent 0dd018b commit 5957450

6 files changed

Lines changed: 115 additions & 27 deletions

File tree

‎e2e/cases/uv-deps-650/crossbuild/BUILD.bazel‎

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ load("@aspect_rules_py//py/unstable:defs.bzl", "py_venv_binary")
33
load("@bazel_lib//lib:transitions.bzl", "platform_transition_filegroup")
44
load("@rules_oci//oci:defs.bzl", "oci_image", "oci_image_index")
55
load("@rules_shell//shell:sh_test.bzl", "sh_test")
6+
load(":toolchain_test.bzl", "unpack_toolchain_path")
67

78
platform(
89
name = "arm64_linux",
@@ -90,7 +91,40 @@ oci_image_index(
9091
],
9192
)
9293

93-
# ── Test: cross-arch whl_install with compile_pyc (build-time) ─────────────
94+
# ── Test: UNPACK_TOOLCHAIN resolves exec-platform binary (analysis-time) ──
95+
#
96+
# Verifies that ctx.toolchains[UNPACK_TOOLCHAIN] selects the exec-platform
97+
# binary even when the target platform is arm64. The unpack toolchain is
98+
# registered with exec_compatible_with.
99+
100+
unpack_toolchain_path(
101+
name = "unpack_path",
102+
)
103+
104+
# A second rule instance whose output is named unpack_path_for_arm64.txt.
105+
# platform_transition_filegroup re-exports its srcs' files unchanged, so the
106+
# source rule's name determines the filename visible in the test's runfiles.
107+
unpack_toolchain_path(
108+
name = "unpack_path_for_arm64",
109+
)
110+
111+
platform_transition_filegroup(
112+
name = "unpack_path_for_arm64_transition",
113+
srcs = [":unpack_path_for_arm64"],
114+
target_platform = ":arm64_linux",
115+
)
116+
117+
sh_test(
118+
name = "exec_toolchain_resolution_test",
119+
srcs = ["test_exec_toolchain.sh"],
120+
data = [
121+
":unpack_path",
122+
":unpack_path_for_arm64_transition",
123+
],
124+
target_compatible_with = ["@platforms//os:linux"],
125+
)
126+
127+
# ── Test: cross-arch whl_install with compile_pyc (build-time) ──────────────
94128
#
95129
# Building arm64_layers on an amd64 host exercises whl_install with
96130
# compile_pyc=True against a non-host target platform. The compileall action

‎e2e/cases/uv-deps-650/crossbuild/test_exec_toolchain.sh‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
#!/usr/bin/env bash
2+
# Verifies that UNPACK_TOOLCHAIN always resolves the exec-platform binary,
3+
# even when the target platform is arm64.
4+
#
5+
# The whl_install rule uses ctx.toolchains[UNPACK_TOOLCHAIN] directly because
6+
# UNPACK_TOOLCHAIN is registered with exec_compatible_with, so Bazel always selects
7+
# the exec-platform binary.
8+
#
9+
# This test confirms that: when we transition to an arm64 target platform on
10+
# an amd64 exec host, the resolved unpack binary path still refers to the amd64
11+
# binary (not an arm64 binary that would fail with "Exec format error").
12+
set -euo pipefail
13+
14+
DIR="${TEST_SRCDIR}/_main/cases/uv-deps-650/crossbuild"
15+
16+
native_path_file="$DIR/unpack_path.txt"
17+
arm64_path_file="$DIR/unpack_path_for_arm64.txt"
18+
19+
if [[ ! -f "$native_path_file" ]]; then
20+
echo "FAIL: $native_path_file not found"
21+
exit 1
22+
fi
23+
24+
if [[ ! -f "$arm64_path_file" ]]; then
25+
echo "FAIL: $arm64_path_file not found"
26+
exit 1
27+
fi
28+
29+
native_path=$(cat "$native_path_file")
30+
arm64_path=$(cat "$arm64_path_file")
31+
32+
echo "Native unpack path: $native_path"
33+
echo "Arm64-target unpack path: $arm64_path"
34+
35+
# Both paths must refer to the same exec-platform binary. If the arm64
36+
# transition incorrectly resolved a target-platform binary, the paths would
37+
# differ and the arm64 binary would fail with "Exec format error" at runtime.
38+
if [[ "$native_path" != "$arm64_path" ]]; then
39+
echo "FAIL: arm64-target transition resolved a different unpack binary."
40+
echo " Expected exec-platform binary: $native_path"
41+
echo " Got: $arm64_path"
42+
echo " This indicates toolchain resolution is using the target platform"
43+
echo " instead of the exec platform for the unpack binary."
44+
exit 1
45+
fi
46+
47+
# Additionally, the path must not reference an arm64/aarch64 binary.
48+
if [[ "$arm64_path" == *"arm64"* ]] || [[ "$arm64_path" == *"aarch64"* ]]; then
49+
echo "FAIL: unpack binary path contains arm64/aarch64 — exec platform not honoured."
50+
echo " Path: $arm64_path"
51+
exit 1
52+
fi
53+
54+
echo "PASS: UNPACK_TOOLCHAIN resolved exec-platform binary for arm64 target ($(uname -m) host)"

‎e2e/cases/uv-deps-650/crossbuild/toolchain_test.bzl‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
"""Analysis-time toolchain resolution test utilities.
2+
3+
Provides a rule that materialises the resolved unpack toolchain binary path
4+
into a file, so sh_test scripts can inspect which binary was selected.
5+
"""
6+
7+
UNPACK_TOOLCHAIN = "@aspect_rules_py//py/private/toolchain:unpack_toolchain_type"
8+
9+
def _unpack_toolchain_path_impl(ctx):
10+
unpack_bin = ctx.toolchains[UNPACK_TOOLCHAIN].bin.bin
11+
out = ctx.actions.declare_file(ctx.label.name + ".txt")
12+
ctx.actions.write(out, unpack_bin.path)
13+
return [DefaultInfo(files = depset([out]))]
14+
15+
unpack_toolchain_path = rule(
16+
doc = """Writes the resolved unpack toolchain binary path to a text file.
17+
18+
Used to verify that toolchain resolution picks the exec-platform binary even
19+
when the target platform differs (cross-compilation scenario).
20+
""",
21+
implementation = _unpack_toolchain_path_impl,
22+
toolchains = [UNPACK_TOOLCHAIN],
23+
)

‎py/private/toolchain/BUILD.bazel‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
load("@bazel_lib//:bzl_library.bzl", "bzl_library")
2-
load(":tools.bzl", "dummy_toolchain", "resolved_unpack_toolchain", "resolved_venv_toolchain")
2+
load(":tools.bzl", "dummy_toolchain", "resolved_venv_toolchain")
33

44
exports_files(
55
["python.sh"],
@@ -26,11 +26,6 @@ resolved_venv_toolchain(
2626
visibility = ["//visibility:public"],
2727
)
2828

29-
resolved_unpack_toolchain(
30-
name = "resolved_unpack_toolchain",
31-
visibility = ["//visibility:public"],
32-
)
33-
3429
# Implementation detail of the target exec toolchain
3530
dummy_toolchain(
3631
name = "empty",

‎py/private/toolchain/tools.bzl‎

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
"""Declaration of concrete toolchains for our Rust tools"""
22

33
load("@bazel_skylib//lib:structs.bzl", "structs")
4-
load(":types.bzl", "PyToolInfo", "UNPACK_TOOLCHAIN", "VENV_TOOLCHAIN")
4+
load(":types.bzl", "PyToolInfo", "VENV_TOOLCHAIN")
55

66
def PrebuiltToolConfig(
77
target,
@@ -138,16 +138,6 @@ resolved_venv_toolchain = rule(
138138
toolchains = [VENV_TOOLCHAIN],
139139
)
140140

141-
# FIXME: Clean up this copypasta somehow
142-
def _resolved_unpack_impl(ctx):
143-
toolchain_info = ctx.toolchains[UNPACK_TOOLCHAIN]
144-
return [toolchain_info] + structs.to_dict(toolchain_info).values()
145-
146-
resolved_unpack_toolchain = rule(
147-
implementation = _resolved_unpack_impl,
148-
toolchains = [UNPACK_TOOLCHAIN],
149-
)
150-
151141
def _dummy_toolchain_impl(ctx):
152142
toolchain_info = platform_common.ToolchainInfo(
153143
dummy = True,

‎uv/private/whl_install/rule.bzl‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -50,11 +50,7 @@ def _whl_install(ctx):
5050
arguments.add("--python", exec_runtime.interpreter)
5151
transitive_inputs.append(depset([exec_runtime.interpreter], transitive = [exec_runtime.files]))
5252

53-
# Need to read the toolchain config from the unpack target so we can grab
54-
# its bin and run it. Note that we have to do this dance in order to get the
55-
# unpack toolchain in the "exec" rather than target config. This allows us
56-
# to use unpack in crossbuild scenarios.
57-
unpack = ctx.attr._unpack[platform_common.ToolchainInfo].bin.bin
53+
unpack = ctx.toolchains[UNPACK_TOOLCHAIN].bin.bin
5854
ctx.actions.run(
5955
mnemonic = "WhlInstall",
6056
executable = unpack,
@@ -130,10 +126,6 @@ lighter weight since the toolchain's files aren't inputs.
130126
values = ["checked-hash", "unchecked-hash", "timestamp"],
131127
doc = "PEP 552 invalidation mode for pre-compiled .pyc files.",
132128
),
133-
"_unpack": attr.label(
134-
default = "//py/private/toolchain:resolved_unpack_toolchain",
135-
cfg = "exec",
136-
),
137129
},
138130
toolchains = [
139131
PY_TOOLCHAIN,

0 commit comments

Comments
 (0)