Pyodide-build review from Claude Fable

[henryiii]

🤖 AI text below 🤖

All four reviews are complete and I've spot-checked the highest-impact claims directly in the source — they hold up. Here's the consolidated review.

Project review: pyodide-build (at e362d0a, 0.35.1)

TLDR: The codebase is in good shape overall — the agents specifically verified that archive extraction is traversal-protected, checksums are enforced, the dependency graph is race-free, and typing/ruff hygiene is already modern. But the review found one high-severity hang, a cluster of real medium-severity bugs (several reproduced live), two big performance wins in the PyPI dependency resolver, and a handful of easy cleanups.

Bugs

High

• Build hangs forever if a recipe script ends with exit 0 — pyodide_build/recipe/bash_runner.py:52-62. run_unchecked appends an env-dump command after the user's script. If the script terminates the shell itself (exit 0, or exec of a program that exits 0), bash returns rc 0 without ever writing to the pipe, and the parent blocks on self._reader.readline() indefinitely — there's no EOF because the parent holds the write fd open. The reviewing agent reproduced the hang with run_unchecked('echo hi; exit 0').

Medium

• Cached build-env dict is mutated per package — pyodide_build/recipe/bash_runner.py:109-110. get_build_environment_vars() is @functools.cached and returns the cached dict itself; env.update(extra_envs) permanently injects per-package vars (PKGDIR, PKG_VERSION, DISTDIR, plus all of os.environ) into the cached value. When one process builds several packages sequentially (pyodide build-recipes-no-deps pkg1 pkg2), later packages see the previous package's values. Fix is one character class: env = dict(get_build_environment_vars(pyodide_root)).

• continue should be break in wheel matching — pyodide_build/common.py:145-148. A wheel whose filename carries a compressed tag set matching two supported tags is yielded once per tag, so find_matching_wheel raises RuntimeError("Found multiple matching wheels") listing the same file twice. This is plausible right now given the pyodide→pyemscripten platform-tag migration (pyodide_tags() includes both). Reproduced live with a single wheel on disk.

• Python-version compatibility check is self-defeating — pyodide_build/build_env.py:95-100 with xbuildenv.py:250-268. On a marker mismatch, _init_xbuild_env calls manager.install() to fix it — but if the version directory already exists, install() skips all work and then unconditionally rewrites the version marker with the current Python version. The re-check passes, the carefully-worded error is unreachable, and builds proceed with HOSTSITEPACKAGES installed under the old Python.

• --no-isolation/-x silently dropped for dependency builds — pyodide_build/out_of_tree/pypi.py:408 (and :260). download_or_build_wheel(x.url, target_folder, compression_level) passes compression_level as the third positional and never forwards isolation/skip_dependency_check, so pyodide build -r reqs.txt --no-isolation builds dependency sdists with isolation anyway.

• find_matches only honors the last extras set — pyodide_build/out_of_tree/pypi.py:313-323. The loop rebinds the candidates generator each iteration, so when a package is required both with and without extras (foo and foo[bar]), only one (dict-order-last) extras set survives and dependencies pulled in by the others are never built.

• pyodide venv --no-download is a silent no-op — pyodide_build/cli/venv.py:24-36. The literal --no-download is registered by two options; click maps it to the later --download/--no-download pair, setting download=False (already the default) while no_download stays False. Verified with CliRunner — only --never-download actually works.

• Path traversal via server-controlled Content-Disposition — pyodide_build/recipe/builder.py:73-84, 372-374. _extract_tarballname trusts the filename= header verbatim and uses it as self.build_dir / tarballname; Path(build_dir) / "/etc/x" is /etc/x, so a malicious/compromised mirror can write outside the build tree. (Member-level tar/zip extraction is already protected by the "data" filter; this is the remaining gap.)

• Broken package index when installing via default_cross_build_env_url — pyodide_build/xbuildenv.py:215-217, 262-264. The default-URL branch sets version = _url_to_version(default_url) but leaves url = None, so the if not url guard doesn't skip _create_package_index, baking mangled hrefs like .../vhttps_example_com_xbuildenv_tar_bz2/full/... into the index that pyodide venv pip installs consume.

• Dead skeleton fallback + unbuildable generated recipes — pyodide_build/recipe/skeleton.py:358, 408-416 makes the wheel/sdist format-fallback unreachable (source_fmt = source_fmt or old_fmt means it's never falsy), so updating an sdist recipe for a release that ships only wheels raises instead of falling back. Relatedly, skeleton.py:123-124 substitutes a predictable {name}-{version}.tar.gz URL without checking the real filename, producing 404 URLs for .zip or legacy-named sdists while storing the real file's sha256.

Lower severity (still worth fixing)

• pyodide_build/xbuildenv.py:160-172 — use_version() crashes with FileExistsError on a dangling xbuildenv symlink (exists() follows symlinks; needs an is_symlink() check), and once dangling, every install/use fails until removed by hand.
• pyodide_build/xbuildenv.py:269-272 — the except Exception: shutil.rmtree(download_path) cleanup also fires when the directory pre-existed this call, so a failure in use_version() (e.g. Windows symlink-privilege OSError) wipes a previously good cached xbuildenv. out_of_tree/venv.py:564-573 has the same delete-what-we-didn't-create pattern for venv destinations.
• pyodide_build/config.py:72 — skip_emscripten_version_check = true (natural TOML) crashes every CLI invocation with an opaque TypeError from re.sub, with no hint which key is at fault.
• pyodide_build/out_of_tree/pypi.py:244-254 — .zip/.tar.bz2 sdist URLs hit UnboundLocalError instead of a real error message.
• pyodide_build/cli/build.py:229-233 — pkg[extra]==1.0 silently discards ==1.0; pkg[a,b] extracts no extras at all.
• pyodide_build/recipe/cleanup.py:21-28 — pyodide clean recipes foo also cleans every always-tagged package, contradicting its own docstring.
• pyodide_build/out_of_tree/venv.py:481 — Path("pip3.12").with_suffix("") is pip3, so the versioned pip3.12 entry point silently disappears from created venvs.
• pyodide_build/recipe/graph_builder.py:271-277 — elapsed-time formatting via datetime.fromtimestamp drops hours for builds ≥ 1h.
• pyodide_build/xbuildenv.py:288-303 — the "newest/oldest supported Python" diagnostic indexes a list sorted by Pyodide version as if sorted by Python version, and raises IndexError on empty release metadata.

Performance

The two big ones are both in the out-of-tree PyPI resolver:

1. Entire wheels downloaded just to read METADATA, with no cross-round caching — out_of_tree/pypi.py:257-274. During pyodide build --build-dependencies, resolvelib recreates Candidate objects on every find_matches, so the per-instance metadata cache never helps and the same tens-of-MB wheel can be downloaded repeatedly across backtracking rounds. Fix: cache by URL, and prefer PyPI's PEP 658 sidecar (<wheel-url>.metadata) before falling back to a full download. This is minutes and GBs on real resolutions.
2. PyPI simple index re-fetched and PackageFinder rebuilt on every find_matches call — out_of_tree/pypi.py:223-234, 301-323. New HTTP session each time, no connection reuse. Cache the candidate list per package name and share one finder.

Medium-impact:

3. Per-package subprocess overhead in recipe builds — recipe/graph_builder.py:163-206 spawns pyodide build-recipes-no-deps per package; each child re-runs make -f Makefile.envs, emcc -v, and (in-tree) re-parses all ~250 recipes, since the @functools.caches are per-process. Roughly 0.5–1.5s of pure overhead per package — minutes aggregate on a full distribution build. The parent already has the computed env; pass it down.
4. Serial post-build phase — recipe/graph_builder.py:916-935: every artifact is repacked (full decompress/recompress, members loaded wholly into memory), unvendored, and sha256-hashed in the main thread after the parallel build finishes. A thread pool plus skipping repack_zip_archive when compression already matches would recover tens of seconds to minutes.
5. Whole archives buffered in RAM — common.py:631-647 reads entire xbuildenv/emsdk archives (hundreds of MB) with resp.read() before writing to disk; stream with shutil.copyfileobj instead.

Smaller: out_of_tree/build.py:91-92 unpacks/repacks every built wheel even pure-Python ones with no .so to retag; cli/build.py:236 and cli/xbuildenv.py:17 run default_xbuildenv_path() (config build + directory walk + write probes) at import time on every pyodide invocation — also an import-time crash risk; the rust busy-wait loop in graph_builder.py:621-640 burns minor CPU on a 10ms poll.

The agent also confirmed the good news: in-process caching of Makefile.envs parsing, recipe loading, and tag computation is otherwise correct, hashing is chunked, and graph construction has no O(n²) hot spots.

Simplifications & modernizations

The typing story is already clean (ruff UP rules with a 3.12 target), so the remaining items are small:

• Dead code: recipe/cleanup.py:31-50 _remove_path is never called anywhere (it also duplicates RecipeBuilder.clean). pypabuild.py:308-317 _create_symlink_dir has a never-used env parameter and a self-acknowledged obsolete context manager. recipe/builder.py:54-70's shutil.register_archive_format("whl", ...) registrations (which reach into private shutil._make_zipfile) appear unused, though that one deserves a careful check before deleting.
• Stdlib replacements: common.py:336-343 chdir is exactly contextlib.chdir (3.11+); common.py:407-428 _get_sha256_checksum is exactly hashlib.file_digest (3.11+).
• Latent dataclass hazard: graph_builder.py:215-257 — the BasePackage subclasses use plain @dataclass, which regenerates __eq__ and sets __hash__ = None, silently discarding the base class's hand-written eq=False hash/eq. Nothing hashes the instances today, so it's latent, but _PackagePriorityWrapper.__hash__ would blow up the moment it's used. Drop or eq=False the subclass decorators.
• Trivia: common.py:152 has an implicit-Optional version: str = None; the compression-level→ZIP_DEFLATED mapping is copy-pasted in three places (common.py:356-404, _py_compile.py:119-122).

One stale-comment note worth keeping: the "remove once packaging 22.0" comment at build_env.py:280-281 is outdated, but the manual cpXY-none-any tag it guards is still required — packaging 26.1 doesn't emit it, so don't remove it.

---

If you'd like, I can start fixing these — the natural first batch is the verified high/medium bugs (the exit 0 hang, the cached-env mutation, the continue→break, the dropped build flags, and the --no-download option collision), each of which is small and testable in isolation.