Repository navigation
fix: make the wasm32 target compile again, and keep it compiling - #156
Open
lets-order-some-fries wants to merge 1 commit into
Open
lets-order-some-fries wants to merge 1 commit into
lets-order-some-fries wants to merge 1 commit into
Conversation
`cargo build --target wasm32-unknown-unknown` fails on main with five errors. Two causes, both from cfg branches that nothing ever built: 1. registry.rs duplicated the whole `HarmonyEncoding` struct literal across the native and wasm32 loaders. d00ac3d ("make HarmonyEncoding usable concurrently", 2025-08-05) removed the `conversation_has_function_tools` field and updated only the native copy, so the wasm32 copy kept setting a field that no longer exists, against an `AtomicBool` that was no longer imported. E0560 + E0433. 2. The wasm32 `Encoding::load` calls `load_encoding_from_bytes`, which was never defined -- only `load_encoding_from_file` exists, and there is no filesystem to read on that target. E0425, three times. Rather than re-sync the two struct literals and leave them to drift again, the shared construction moves into `build_harmony_encoding`, so the two loaders now differ only in how they obtain the tokenizer (blocking vs async). Same for the vocab side: `build_core_bpe` holds what `load_encoding_from_file` and the new `load_encoding_from_bytes` share. `load_encoding_from_bytes` is gated to wasm32 because `public_encodings` is a private module, so an unreachable `pub fn` trips dead_code under `-D warnings`. It is useful off wasm32 too, for an embedded or air-gapped vocabulary (cf. openai#101), but exposing it there is an API decision and is left to you. Adds a wasm32 workflow. Without one this just rots again -- it broke in August 2025 and no build noticed for over a year. It is a separate file because CI.yml is generated by `maturin generate-ci github` and a job added there would be lost on the next regeneration. Fixes openai#77 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
Could a maintainer approve the two workflow runs here when convenient, CI and the wasm32 check this PR adds? Both have been waiting for approval since 2026-09-23, so neither has run yet. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
cargo build --target wasm32-unknown-unknownfails onmainwith five errors.This fixes them, removes the duplication that caused them, and adds a wasm32 CI
check so it does not silently rot again.
Fixes #77.
The two causes
Both are
cfgbranches that nothing ever built.1.
registry.rs— a field removed a year ago. The native and wasm32loaders each carried their own full copy of the
HarmonyEncodingstructliteral. d00ac3d ("make
HarmonyEncodingusable concurrently", 2025-08-05)removed the
conversation_has_function_toolsfield and updated only the nativecopy. The wasm32 copy has been setting a field that no longer exists, against an
AtomicBoolthat is no longer imported, ever since.2.
public_encodings.rs— a function that was never written. The wasm32Encoding::loadcallsload_encoding_from_bytes; onlyload_encoding_from_fileexists, and there is no filesystem to read on that target.
Details
Re-syncing the two struct literals would fix the build and leave them free to
drift apart again, so instead:
build_harmony_encodingholds the shared construction. The two loaders nowdiffer only in how they obtain the tokenizer — blocking vs
.await— sothere is no second copy to fall behind.
build_core_bpedoes the same for the vocab side, shared byload_encoding_from_fileand the newload_encoding_from_bytes.load_encoding_from_bytesis#[cfg(target_arch = "wasm32")]becausepublic_encodingsis a private module (mod public_encodings;), so anunreachable
pub fntripsdead_codeunder-D warningson other targets.Loading a vocabulary you already hold is useful off wasm32 too — an embedded or
air-gapped copy, cf. #101 — but exposing it there is an API decision, so I left
it to you rather than widening this PR.
The CI job. Without one this just breaks again: it broke in August 2025 and
no build noticed for over a year. It is a separate workflow file rather than
a job in
CI.yml, because that file is generated ("This file is autogeneratedby maturin v1.8.7 — to update, run
maturin generate-ci github") and a jobadded there would be lost on the next regeneration.
How to Validate
rustup target add wasm32-unknown-unknown cargo build --target wasm32-unknown-unknown # fails on main with 5 errorsOn this branch, observed locally (macOS, aarch64, rustc 1.98.1):
cargo check --target wasm32-unknown-unknown --lib(the CI job)cargo testcargo fmt --checkcargo clippy --all-targets -- -D warningsThose two clippy errors are pre-existing on
mainwith this toolchain(
unneeded late initializationatsrc/encoding.rs:1413andcan be more succinctly written as a byte stratsrc/tiktoken.rs:312) — I verified bystashing this change and re-running. They are untouched here; your CI presumably
pins an older toolchain where those lints do not fire. Say the word if you would
like them in a separate PR.
The wasm32 build still emits
dead_codewarnings for the file/cache helpersthat only apply off-wasm. The new job does not use
-D warnings, so thosedo not fail it; cleaning them up is a follow-up, not this PR.
🤖 Generated with Claude Code