envoyproxy / envoyproxy/toolshed

emsdk: resolve the emcc/emar Python interpreter from the py toolchain (drop @python_3_12//:files literal and env.sh glob)

Open
#4,963 0 comments 1 reaction 2 assignees Claimed by @phlax View on GitHub
Dominant language
Python
Stars
12
Forks
24
Avg merge
6h 37m
Merged PRs (30d)
92

Description

Follow-up to #4960 / PR #4962. That PR switched emsdk to Python 3.12 and unblocked the build, but it resolves the interpreter the **wrong way**. The emscripten toolchain already has a Python toolchain available — it should consume it properly instead of hardcoding a named python repo and globbing the execroot.

## What's wrong with the current approach

In `bazel-registry/modules/emsdk/4.0.23.envoy/patches/emsdk_bzlmod.patch`:

1. **`bazel/remote_emscripten_repository.bzl`** still hardcodes a **named python repo** in the toolchain `files`:
```
"@python_3_12//:files",
```
This bakes in the `3_12` version. If the resolved/default Python moves, this breaks again — same fragility class as the original `python_3_13` bug, just kicked down the road.

2. **`bazel/emscripten_toolchain/env.sh`** globs the execroot to find the interpreter:
```sh
for _em_python3 in "$ROOT_DIR"/external/*python*3_12*/bin/python3; do ...
```
This still hardcodes `3_12` and relies on string-matching the external repo directory layout. It's "hardcoding by another name" and exactly what the interpreter resolution should avoid.

## What it should do instead

The toolchain already resolves a Python toolchain — **use it properly** rather than scraping `//:files` from a named repo and globbing for the binary:

- Consume the resolved Python toolchain via `@rules_python//python:current_py_toolchain` (toolchain type `@rules_python//python:toolchain_type`).
- Get the interpreter `File` from the provider, e.g. `ctx.toolchains["@rules_python//python:toolchain_type"].py3_runtime.interpreter`.
- Add that interpreter (and its runfiles) to the toolchain action inputs, and pass its path into the emcc/emar/link scripts via an env var (e.g. `EM_PYTHON3`) or args — resolved by Bazel, not by a shell glob.

Result:
- **No `@python_3_XX//:files` literal** anywhere.
- **No `env.sh` glob** — `env.sh` receives the interpreter path from the toolchain.
- **No `use_repo(python, "python_3_12")`** needed in the registry `MODULE.bazel` purely to expose the interpreter (keep only if genuinely required for something else).
- Fully version- and arch-agnostic; follows whatever Python the toolchain resolves (envoy default 3.12).

## Constraints (unchanged from #4960)

- All changes confined to `bazel-registry/modules/emsdk/`.
- The patch must **not** touch upstream `bazel/MODULE.bazel` for python config.
- Preserve the hermetic intent (bazel-provided Python, not system `python3`).
- Preserve the unrelated `dwp_files` hunks.
- Recompute `source.json` patch integrity after editing the patch (keep `patch_strip: 2` unless the rework requires otherwise).
- Registry versions are immutable — bump to a new emsdk version dir if the current one is already consumed, and update `metadata.json`.

## Investigation note
`bazel/remote_emscripten_repository.bzl`'s `create_toolchains(...)` is where `@python_3_12//:files` is currently injected. Confirm how the emscripten toolchain rule is wired (it's a cc-toolchain-style setup) and thread the interpreter from `current_py_toolchain` / `py3_runtime.interpreter` through to the toolchain files + `env.sh`, rather than a repo-name literal.

## Validation
- No `python_3_12` / `python_3_13` (or any `@python_3_XX`) literal remains in the emsdk module.
- No execroot glob for the interpreter in `env.sh`.
- Interpreter is obtained from the resolved py toolchain and passed to emcc/emar/link via Bazel-provided path.
- Build succeeds on x86_64 and is not arch- or py-version-pinned.
- Hermetic behavior preserved.
- `source.json` integrity matches the recomputed patch hash.

## Follow-up (do not do here)
Bump envoy's root `MODULE.bazel` `bazel_dep(name = "emsdk", version = "...")` if a new emsdk version dir is created.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.