Skip to content

Commit d13c4c2

Browse files
arrdemarrdem
authored
fix(release): Configure the interpreter shim via a prebuilt toolchain (#568)
This fixes a breaking regression in v1.5.0 caused by the new interpreter shim being referenced by source label and not via a toolchain which can be supplied with prebuild artifacts. Included in this change is a refactor consolidating our source of truth for the list of prebuilt tools to configure down to `tools.bzl` which is now shared with `tools/release` among other things, preventing how I got myself into this mess. As a further bit of paranoia this changeset also reworks the integration tests so that instead of doing e2e testing with assets from a checkpoint release (previously 1.1.0) we instead do a full build of the release candidate assets, update the asset integrity file, run a local fileserver hosting them and then do builds using the locally hosted assets. ### Changes are visible to end-users: yes - Searched for relevant documentation and updated as needed: yes/no - Breaking change (forces users to change their own code or config): yes/no - Suggested release notes appear below: yes/no ### Test plan - [x] Existing test cases still green - [x] Preexisting and inadequate e2e tests updated to cover this case
1 parent 5dae011 commit d13c4c2

File tree

13 files changed

+245
-93
lines changed

13 files changed

+245
-93
lines changed

‎e2e/use_release/minimal_download_test.sh‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,41 @@ if [ "$ARCH" == "x86_64" ]; then
99
ALLOWED="rules_py_tools.${OS}_amd64"
1010
fi
1111

12+
# FIXME: Find a port we can bind.
13+
PORT=7654
14+
touch devserver.pid
15+
PIDFILE=$(realpath ./devserver.pid)
16+
17+
(
18+
cd ../../
19+
20+
# First we produce a release artifact matrix
21+
mkdir artifacts
22+
DEST=$(realpath artifacts) bazel run //tools/release:copy_release_artifacts
23+
24+
# We kick off a dev http server on localhost
25+
#
26+
# Bazel will block until the devserver starts, then it forks to the background
27+
# which will unblock the Bazel server & shell execution.
28+
bazel run //tools/e2e:fileserver -- --port=$PORT --dir="$(realpath ./artifacts)" --background --pidfile="$PIDFILE"
29+
30+
# Now we need to update the integrity file
31+
bazel run //tools/e2e:integrity -- --dir="$(realpath ./artifacts)" --target="$(realpath ./tools/integrity.bzl)"
32+
33+
# Note that we don't have to scrub the bazel server because we're using a separate output_base below for the cases.
34+
)
35+
36+
# Set an environment flag which will make rules_py treat localhost as a mirror/artifact source
37+
export RULES_PY_RELEASE_URL="http://localhost:$PORT/{filename}"
38+
1239
#############
1340
# Test bzlmod
1441
(
1542
cd ../..
1643
# Create the .orig file, whether there's a mismatch or not
1744
patch -p1 --backup < .bcr/patches/*.patch
1845
)
46+
1947
OUTPUT_BASE=$(mktemp -d)
2048
output=$(bazel "--output_base=$OUTPUT_BASE" run --enable_bzlmod //src:main)
2149
if [[ "$output" != "hello world" ]]; then
@@ -77,3 +105,6 @@ bazel test --test_output=streamed //...
77105
bazel run //examples/py_venv:external_venv
78106
bazel run --stamp //examples/py_venv:external_venv
79107
)
108+
109+
# Shut down the devserver
110+
kill "$(cat $PIDFILE)"

‎py/private/py_venv/py_venv.bzl‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ load("@aspect_bazel_lib//lib:paths.bzl", "BASH_RLOCATION_FUNCTION", "to_rlocatio
55
load("//py/private:py_library.bzl", _py_library = "py_library_utils")
66
load("//py/private:py_semantics.bzl", _py_semantics = "semantics")
77
load("//py/private:transitions.bzl", "python_version_transition")
8-
load("//py/private/toolchain:types.bzl", "PY_TOOLCHAIN")
8+
load("//py/private/toolchain:types.bzl", "PY_TOOLCHAIN", "SHIM_TOOLCHAIN")
99

1010
VirtualenvInfo = provider(
1111
doc = """
@@ -74,6 +74,8 @@ def _py_venv_base_impl(ctx):
7474

7575
py_toolchain = _py_semantics.resolve_toolchain(ctx)
7676

77+
py_shim = ctx.toolchains[SHIM_TOOLCHAIN]
78+
7779
# Check for duplicate virtual dependency names. Those that map to the same resolution target would have been merged by the depset for us.
7880
virtual_resolution = _py_library.resolve_virtuals(ctx)
7981
imports_depset = _py_library.make_imports_depset(ctx, extra_imports_depsets = virtual_resolution.imports)
@@ -142,7 +144,7 @@ def _py_venv_base_impl(ctx):
142144
executable = ctx.file._venv_tool,
143145
arguments = [
144146
"--location=" + venv_dir.path,
145-
"--python=" + ctx.file._interpreter_shim.path,
147+
"--python=" + py_shim.bin.path,
146148
"--pth-file=" + site_packages_pth_file.path,
147149
"--env-file=" + env_file.path,
148150
"--bin-dir=" + ctx.bin_dir.path,
@@ -158,9 +160,9 @@ def _py_venv_base_impl(ctx):
158160
ctx.runfiles(files = [
159161
site_packages_pth_file,
160162
env_file,
161-
ctx.file._interpreter_shim,
162163
ctx.file._venv_tool,
163164
]),
165+
py_shim.default_info.default_runfiles,
164166
]).files,
165167
outputs = [
166168
venv_dir,
@@ -300,11 +302,6 @@ A collision can occur when multiple packages providing the same file are install
300302
"_interpreter_version_flag": attr.label(
301303
default = "//py:interpreter_version",
302304
),
303-
"_interpreter_shim": attr.label(
304-
allow_single_file = True,
305-
doc = "An interpreter shim to use. Should be a single file executable.",
306-
default = "//py/tools/venv_shim",
307-
),
308305
# Required for py_version attribute
309306
"_allowlist_function_transition": attr.label(
310307
default = "@bazel_tools//tools/allowlists/function_transition_allowlist",
@@ -367,6 +364,7 @@ py_venv_base = struct(
367364
test_attrs = _test_attrs,
368365
toolchains = [
369366
PY_TOOLCHAIN,
367+
SHIM_TOOLCHAIN,
370368
],
371369
cfg = python_version_transition,
372370
)

‎py/private/toolchain/BUILD.bazel‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@ toolchain_type(
1515
visibility = ["//visibility:public"],
1616
)
1717

18+
toolchain_type(
19+
name = "shim_toolchain_type",
20+
visibility = ["//visibility:public"],
21+
)
22+
1823
bzl_library(
1924
name = "autodetecting",
2025
srcs = ["autodetecting.bzl"],
@@ -25,7 +30,9 @@ bzl_library(
2530
bzl_library(
2631
name = "tools",
2732
srcs = ["tools.bzl"],
28-
visibility = ["//py:__subpackages__"],
33+
visibility = [
34+
"//py:__subpackages__",
35+
],
2936
)
3037

3138
bzl_library(

‎py/private/toolchain/repo.bzl‎

Lines changed: 61 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,9 @@ This guidance tells us how to avoid that: we put the toolchain targets in the al
1717
with only the toolchain attribute pointing into the platform-specific repositories.
1818
"""
1919

20-
load("//py/private/toolchain:tools.bzl", "RUST_BIN_CFG", "TOOLCHAIN_PLATFORMS")
20+
load("//py/private/toolchain:tools.bzl", "TOOLCHAIN_PLATFORMS", "TOOL_CFGS")
21+
load("//tools:integrity.bzl", "RELEASED_BINARY_INTEGRITY")
22+
load("//tools:version.bzl", "VERSION")
2123

2224
def _toolchains_repo_impl(repository_ctx):
2325
build_content = """# Generated by toolchains_repo.bzl
@@ -27,7 +29,7 @@ def _toolchains_repo_impl(repository_ctx):
2729
# so you don't normally need to interact with these targets.
2830
2931
"""
30-
for tool, cfg in RUST_BIN_CFG.items():
32+
for bin in TOOL_CFGS:
3133
for [platform, meta] in TOOLCHAIN_PLATFORMS.items():
3234
build_content += """
3335
# Declare a toolchain Bazel will select for running {tool} on the {cfg} platform.
@@ -37,11 +39,12 @@ toolchain(
3739
# Bazel does not follow this attribute during analysis, so the referenced repo
3840
# will only be fetched if this toolchain is selected.
3941
toolchain = "@{user_repository_name}.{platform}//:{tool}_toolchain",
40-
toolchain_type = "@aspect_rules_py//py/private/toolchain:{tool}_toolchain_type",
42+
toolchain_type = "{toolchain_type}",
4143
)
4244
""".format(
43-
cfg = cfg,
44-
tool = tool,
45+
cfg = bin.cfg,
46+
tool = bin.name,
47+
toolchain_type = bin.toolchain_type,
4548
platform = platform,
4649
user_repository_name = repository_ctx.attr.user_repository_name,
4750
compatible_with = meta.compatible_with,
@@ -75,3 +78,56 @@ prerelease_toolchains_repo = repository_rule(
7578
By doing this, we can avoid those register_toolchains callsites needing to be conditional on IS_PRERELEASE
7679
""",
7780
)
81+
82+
def _prebuilt_tool_repo_impl(rctx):
83+
build_content = """\
84+
# Generated by @aspect_rules_py//py/private/toolchain:tools.bzl
85+
load("@aspect_rules_py//py/private/toolchain:tools.bzl", "py_tool_toolchain")
86+
87+
package(default_visibility = ["//visibility:public"])
88+
"""
89+
90+
# For manual testing, override these environment variables
91+
# TODO: use rctx.getenv when available, see https://github.com/bazelbuild/bazel/pull/20944
92+
release_fork = "aspect-build"
93+
release_version = VERSION
94+
if "RULES_PY_RELEASE_FORK" in rctx.os.environ:
95+
release_fork = rctx.os.environ["RULES_PY_RELEASE_FORK"]
96+
if "RULES_PY_RELEASE_VERSION" in rctx.os.environ:
97+
release_version = rctx.os.environ["RULES_PY_RELEASE_VERSION"]
98+
99+
url_template = "https://github.com/{release_fork}/rules_py/releases/download/v{release_version}/{filename}"
100+
if "RULES_PY_RELEASE_URL" in rctx.os.environ:
101+
url_template = rctx.os.environ["RULES_PY_RELEASE_URL"]
102+
103+
for tool in TOOL_CFGS:
104+
filename = "-".join([
105+
tool.name,
106+
TOOLCHAIN_PLATFORMS[rctx.attr.platform].arch,
107+
TOOLCHAIN_PLATFORMS[rctx.attr.platform].vendor_os_abi,
108+
])
109+
url = url_template.format(
110+
release_fork = release_fork,
111+
release_version = release_version,
112+
filename = filename,
113+
)
114+
rctx.download(
115+
url = url,
116+
sha256 = RELEASED_BINARY_INTEGRITY[filename],
117+
executable = True,
118+
output = tool.name,
119+
)
120+
build_content += """py_tool_toolchain(name = "{tool}_toolchain", bin = "{tool}", template_var = "{tool_upper}_BIN")\n""".format(
121+
tool = tool.name,
122+
tool_upper = tool.name.upper(),
123+
)
124+
125+
rctx.file("BUILD.bazel", build_content)
126+
127+
prebuilt_tool_repo = repository_rule(
128+
doc = "Download pre-built binary tools and create concrete toolchains for them",
129+
implementation = _prebuilt_tool_repo_impl,
130+
attrs = {
131+
"platform": attr.string(mandatory = True, values = TOOLCHAIN_PLATFORMS.keys()),
132+
},
133+
)

‎py/private/toolchain/shim/BUILD.bazel‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
load("//py/private/toolchain:tools.bzl", "source_toolchain")
2+
3+
source_toolchain(
4+
name = "shim",
5+
bin = "//py/tools/venv_shim",
6+
# FIXME: Use the target cfg?
7+
# See notes in the venv setup
8+
toolchain_type = "//py/private/toolchain:shim_toolchain_type",
9+
)

‎py/private/toolchain/tools.bzl‎

Lines changed: 28 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,35 @@
11
"""Declaration of concrete toolchains for our Rust tools"""
22

3-
load("//tools:integrity.bzl", "RELEASED_BINARY_INTEGRITY")
4-
load("//tools:version.bzl", "VERSION")
3+
def PrebuiltToolConfig(
4+
target,
5+
cfg = "target",
6+
name = None,
7+
toolchain = None,
8+
toolchain_type = None):
9+
name = name or Label(target).name
10+
11+
# FIXME: The source_toolchain macro creates two targets, so we need to match them both
12+
# But that makes this not really a label which is weird
13+
toolchain = toolchain or "@aspect_rules_py//py/private/toolchain/{}/...".format(name)
14+
toolchain_type = toolchain_type or "@aspect_rules_py//py/private/toolchain:{}_toolchain_type".format(name)
15+
16+
return struct(
17+
target = target,
18+
cfg = cfg,
19+
name = name,
20+
toolchain = toolchain,
21+
toolchain_type = toolchain_type,
22+
)
523

624
# The expected config for each tool, whether it runs in an action or at runtime
7-
RUST_BIN_CFG = {
8-
# unpack wheels happens inside an action
9-
"unpack": "exec",
10-
# creating the virtualenv happens when the binary is running
11-
"venv": "target",
12-
}
25+
#
26+
# Note that this is the source of truth for how toolchains get registered and
27+
# for how they get prebuilt/patched in.
28+
TOOL_CFGS = [
29+
PrebuiltToolConfig("//py/tools/unpack_bin:unpack", cfg = "exec"),
30+
PrebuiltToolConfig("//py/tools/venv_bin:venv"),
31+
PrebuiltToolConfig("//py/tools/venv_shim:shim"),
32+
]
1333

1434
TOOLCHAIN_PLATFORMS = {
1535
"darwin_amd64": struct(
@@ -102,52 +122,3 @@ def source_toolchain(name, toolchain_type, bin):
102122
toolchain = toolchain_rule,
103123
toolchain_type = toolchain_type,
104124
)
105-
106-
def _prebuilt_tool_repo_impl(rctx):
107-
build_content = """\
108-
# Generated by @aspect_rules_py//py/private/toolchain:tools.bzl
109-
load("@aspect_rules_py//py/private/toolchain:tools.bzl", "py_tool_toolchain")
110-
111-
package(default_visibility = ["//visibility:public"])
112-
"""
113-
114-
# For manual testing, override these environment variables
115-
# TODO: use rctx.getenv when available, see https://github.com/bazelbuild/bazel/pull/20944
116-
release_fork = "aspect-build"
117-
release_version = VERSION
118-
if "RULES_PY_RELEASE_FORK" in rctx.os.environ:
119-
release_fork = rctx.os.environ["RULES_PY_RELEASE_FORK"]
120-
if "RULES_PY_RELEASE_VERSION" in rctx.os.environ:
121-
release_version = rctx.os.environ["RULES_PY_RELEASE_VERSION"]
122-
123-
for tool in RUST_BIN_CFG.keys():
124-
filename = "-".join([
125-
tool,
126-
TOOLCHAIN_PLATFORMS[rctx.attr.platform].arch,
127-
TOOLCHAIN_PLATFORMS[rctx.attr.platform].vendor_os_abi,
128-
])
129-
url = "https://github.com/{}/rules_py/releases/download/v{}/{}".format(
130-
release_fork,
131-
release_version,
132-
filename,
133-
)
134-
rctx.download(
135-
url = url,
136-
sha256 = RELEASED_BINARY_INTEGRITY[filename],
137-
executable = True,
138-
output = tool,
139-
)
140-
build_content += """py_tool_toolchain(name = "{tool}_toolchain", bin = "{tool}", template_var = "{tool_upper}_BIN")\n""".format(
141-
tool = tool,
142-
tool_upper = tool.upper(),
143-
)
144-
145-
rctx.file("BUILD.bazel", build_content)
146-
147-
prebuilt_tool_repo = repository_rule(
148-
doc = "Download pre-built binary tools and create concrete toolchains for them",
149-
implementation = _prebuilt_tool_repo_impl,
150-
attrs = {
151-
"platform": attr.string(mandatory = True, values = TOOLCHAIN_PLATFORMS.keys()),
152-
},
153-
)

‎py/private/toolchain/types.bzl‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,5 +4,6 @@ PY_TOOLCHAIN = "@bazel_tools//tools/python:toolchain_type"
44
SH_TOOLCHAIN = "@bazel_tools//tools/sh:toolchain_type"
55

66
# Toolchain type for the virtual env creation tools.
7-
VENV_TOOLCHAIN = "@aspect_rules_py//py/private/toolchain:venv_toolchain_type"
7+
SHIM_TOOLCHAIN = "@aspect_rules_py//py/private/toolchain:shim_toolchain_type"
88
UNPACK_TOOLCHAIN = "@aspect_rules_py//py/private/toolchain:unpack_toolchain_type"
9+
VENV_TOOLCHAIN = "@aspect_rules_py//py/private/toolchain:venv_toolchain_type"

‎py/toolchains.bzl‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,8 @@
33
load("@aspect_bazel_lib//lib:repositories.bzl", "register_tar_toolchains")
44
load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_file")
55
load("//py/private/toolchain:autodetecting.bzl", _register_autodetecting_python_toolchain = "register_autodetecting_python_toolchain")
6-
load("//py/private/toolchain:repo.bzl", "prerelease_toolchains_repo", "toolchains_repo")
7-
load("//py/private/toolchain:tools.bzl", "TOOLCHAIN_PLATFORMS", "prebuilt_tool_repo")
6+
load("//py/private/toolchain:repo.bzl", "prebuilt_tool_repo", "prerelease_toolchains_repo", "toolchains_repo")
7+
load("//py/private/toolchain:tools.bzl", "TOOLCHAIN_PLATFORMS", "TOOL_CFGS")
88
load("//tools:version.bzl", "IS_PRERELEASE")
99

1010
register_autodetecting_python_toolchain = _register_autodetecting_python_toolchain
@@ -25,10 +25,8 @@ def rules_py_toolchains(name = DEFAULT_TOOLS_REPOSITORY, register = True, is_pre
2525
if is_prerelease:
2626
prerelease_toolchains_repo(name = name)
2727
if register:
28-
native.register_toolchains(
29-
"@aspect_rules_py//py/private/toolchain/venv/...",
30-
"@aspect_rules_py//py/private/toolchain/unpack/...",
31-
)
28+
for tool in TOOL_CFGS:
29+
native.register_toolchains(tool.toolchain)
3230
else:
3331
for platform in TOOLCHAIN_PLATFORMS.keys():
3432
prebuilt_tool_repo(name = ".".join([name, platform]), platform = platform)

‎tools/e2e/BUILD.bazel‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
load("//py:defs.bzl", "py_binary")
2+
3+
py_binary(
4+
name = "fileserver",
5+
srcs = ["devserver.py"],
6+
main = "devserver.py",
7+
)
8+
9+
py_binary(
10+
name = "integrity",
11+
srcs = ["devintegrity.py"],
12+
main = "devintegrity.py",
13+
)

0 commit comments

Comments
 (0)