Skip to content

Commit 38d5c39

Browse files
gregmagolanclaude
gregmagolan
and
claude
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. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 8efcc57 commit 38d5c39

6 files changed

Lines changed: 109 additions & 27 deletions

File tree

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

Lines changed: 29 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,34 @@ oci_image_index(
9091
],
9192
)
9293

93-
# ── Test: cross-arch whl_install with compile_pyc (build-time) ─────────────
94+
# ── Test 1: 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, so no resolved_unpack_toolchain
99+
# workaround is needed.
100+
101+
unpack_toolchain_path(
102+
name = "unpack_path",
103+
)
104+
105+
platform_transition_filegroup(
106+
name = "unpack_path_for_arm64",
107+
srcs = [":unpack_path"],
108+
target_platform = ":arm64_linux",
109+
)
110+
111+
sh_test(
112+
name = "exec_toolchain_resolution_test",
113+
srcs = ["test_exec_toolchain.sh"],
114+
data = [
115+
":unpack_path",
116+
":unpack_path_for_arm64",
117+
],
118+
target_compatible_with = ["@platforms//os:linux"],
119+
)
120+
121+
# ── Test 2: cross-arch whl_install with compile_pyc (build-time) ─────────────
94122
#
95123
# Building arm64_layers on an amd64 host exercises whl_install with
96124
# 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 (no
6+
# resolved_unpack_toolchain workaround) because UNPACK_TOOLCHAIN is registered
7+
# with exec_compatible_with, so Bazel always selects 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
@@ -46,11 +46,7 @@ def _whl_install(ctx):
4646
arguments.add("--python", exec_runtime.interpreter)
4747
transitive_inputs.append(depset([exec_runtime.interpreter], transitive = [exec_runtime.files]))
4848

49-
# Need to read the toolchain config from the unpack target so we can grab
50-
# its bin and run it. Note that we have to do this dance in order to get the
51-
# unpack toolchain in the "exec" rather than target config. This allows us
52-
# to use unpack in crossbuild scenarios.
53-
unpack = ctx.attr._unpack[platform_common.ToolchainInfo].bin.bin
49+
unpack = ctx.toolchains[UNPACK_TOOLCHAIN].bin.bin
5450
ctx.actions.run(
5551
executable = unpack,
5652
arguments = [arguments],
@@ -119,10 +115,6 @@ lighter weight since the toolchain's files aren't inputs.
119115
values = ["checked-hash", "unchecked-hash", "timestamp"],
120116
doc = "PEP 552 invalidation mode for pre-compiled .pyc files.",
121117
),
122-
"_unpack": attr.label(
123-
default = "//py/private/toolchain:resolved_unpack_toolchain",
124-
cfg = "exec",
125-
),
126118
},
127119
toolchains = [
128120
PY_TOOLCHAIN,

0 commit comments

Comments
 (0)