Draft
Conversation
cderv
force-pushed
the
feature/wasm-testing-and-cleanup
branch
2 times, most recently
from
52923e7 to
60df573
Compare
cderv
force-pushed
the
feature/wasm-testing-and-cleanup
branch
from
8c6bfe2 to
89d90e0
Compare
cderv
changed the title
cderv
force-pushed
the
feature/wasm-testing-and-cleanup
branch
from
267f27a to
ba0ec15
Compare
cderv
force-pushed
the
fix/wasm-cfg-proxy-and-cleanup
branch
from
6009f60 to
2db6f1e
Compare
The wasm-qmd-parser crate was superseded by wasm-quarto-hub-client but never cleaned up. Remove it along with its dedicated build-wasm.yml workflow and wasm-pack install steps in hub-client-e2e.yml. Update READMEs and CLAUDE.md to remove stale references.
rust-toolchain.toml already specifies the full toolchain configuration (nightly channel, components, targets). The dtolnay/rust-toolchain action was a redundant wrapper — rustup reads rust-toolchain.toml natively. Replace with `rustup show active-toolchain` which triggers auto-install from rust-toolchain.toml and displays the resolved toolchain. One fewer third-party action dependency across all CI workflows.
Design for bd-itj9: replace cfg proxy in Lua VM setup with real WASM tests, clean up stale wasm-qmd-parser crate, and migrate away from deprecated wasm-pack tooling. Three phases: (1) remove stale artifacts, (2) fix cfg guards so native tests use real Lua stdlib, (3) add wasm-bindgen-test integration tests for pampa's Lua filter code.
- Add explicit success criteria - Require phases 2+3 land together (no validation gap) - Add --no-default-features --features lua-filter to WASM test command - Add workspace.dependencies.wasm-qmd-parser to removal checklist - Pin wasm-bindgen-cli to exact version 0.2.108 - Specify test-suite.yml as target CI workflow - Note that .cargo/config.toml runner setting doesn't affect production build
CI uses dev-setup to install version-matched wasm-bindgen-cli from Cargo.lock instead of hardcoding the version. Fix xtask.md rule to reference wasm-bindgen-cli instead of stale wasm-pack, and document the version pinning convention.
- Tighten success criteria: "no active build/test/runtime references" - Resolve hub-client-e2e.yml: remove stale wasm-pack install step - Normalize path references to crates/pampa/tests/wasm_lua.rs - Add local developer workflow section - Note WASM tests not part of xtask verify (requires nightly toolchain)
mlua/lua-src compiles Lua from C source — targeting wasm32 requires Clang + CC/CFLAGS env vars pointing to wasm-sysroot. Same setup as existing ts-test-suite.yml WASM build. Note opportunity to share toolchain setup across CI workflows. Also flag ts-test-suite.yml hardcoded wasm-bindgen-cli version for migration to xtask dev-setup.
WASM build (build-wasm.js) doesn't support Windows — no Clang wasm32 target. WASM tests have the same constraint. Both run in Linux CI only. Consistent with existing behavior.
wasm-qmd-parser is fully superseded by wasm-quarto-hub-client. The build-wasm.yml workflow only built the stale crate. wasm-pack is not used by the active WASM build pipeline (build-wasm.js uses cargo build + wasm-bindgen CLI directly). - Delete crates/wasm-qmd-parser/ entirely - Delete .github/workflows/build-wasm.yml - Remove wasm-qmd-parser from workspace exclude and deps - Remove stale wasm-pack install from hub-client-e2e.yml - Update hub-client and wasm-quarto-hub-client READMEs - Update workspace CLAUDE.md and Cargo.toml comments
Remove `test` from `#[cfg(any(target_arch = "wasm32", test))]` guards in filter.rs and shortcode.rs so native tests use Lua::new() with real C stdlib on all platforms. This fixes the 8 filter traversal tests that failed on Windows because the synthetic io.open only handles POSIX VFS paths. Add wasm-bindgen-test smoke tests in crates/pampa/tests/wasm_lua.rs that run on the real wasm32 target, validating: - Restricted Lua VM creation - Filter execution through WASM code path - Shortcode engine on WASM - Error handling (panic_unwind works) - Synthetic io/os module registration Configure wasm-bindgen-test-runner in .cargo/config.toml. WASM tests require nightly + Clang + wasm-sysroot (Linux/macOS CI only).
Rewrite dev-docs/wasm.md as single source of truth for WASM in this project: architecture, build pipeline, testing strategy, and C toolchain requirements. Add WASM testing guidance to pampa/CLAUDE.md and .claude/rules/wasm.md to prevent regression of the cfg test proxy pattern. Update testing.md to reflect the new native vs WASM testing approach.
Add wasm-tests job to test-suite.yml that runs pampa WASM smoke tests on Linux with nightly Rust + Clang + wasm-sysroot. Uses cargo xtask dev-setup for version-matched wasm-bindgen-cli installation. Migrate ts-test-suite.yml from hardcoded wasm-bindgen-cli version to cargo xtask dev-setup for consistent version management.
- Gate wasm_lua.rs on both wasm32 and lua-filter feature to prevent compilation errors in minimal-feature test runs - Fix dev-docs/wasm.md: clarify that build-wasm.js does not set -fno-builtin (only needed for WASM tests, not production builds) - Narrow shortcode coverage claim to match actual test (init only)
Debug-mode builds emit __builtin_* intrinsic calls that don't exist in the stub wasm-sysroot. Release builds inline these away.
The cc crate runs clang from OUT_DIR, not the repo root, so relative
-isystem paths don't resolve. Use $PWD for local dev instructions and
${{ github.workspace }} in CI.
The test_dofile_script_dir_stack test depends on register_wasm_dofile which pushes/pops the script-dir stack around dofile calls. On native, the C Lua dofile doesn't interact with the stack. Neither Pandoc nor Quarto CLI provide this for raw dofile() — it's a WASM-only feature. Mark the test as ignored on non-wasm32 targets and document the finding in the design spec.
proptest pulls in getrandom 0.3.4 which doesn't compile for wasm32-unknown-unknown. Move proptest, insta, tempfile, and tokio to the cfg(not(wasm32)) dev-dependencies section. No effect on native builds — the condition is always true on x86_64/aarch64.
Add Phase 6a documenting: sysroot absolute paths, -fno-builtin rationale, wasm-incompatible dev-deps gating, and dofile behavioral difference (linking to #112).
cargo xtask dev-setup uses --locked which may produce a different wasm-bindgen-cli binary. Revert to the hardcoded install that main uses until the root cause of the externref type mismatch is understood.
cargo xtask dev-setup with --locked causes externref mismatch in TS Test Suite. Reverted to hardcoded install. Tracked as bd-jakt.
Two independent build errors in the WASM Tests CI job: 1. Duplicate core lang item (E0152): The toolchain setup installed the prebuilt wasm32-unknown-unknown target AND used -Zbuild-std, which rebuilds core from source. Two copies of core conflict. The production WASM build (ts-test-suite.yml) correctly uses only rust-src without the prebuilt target. Removed targets: wasm32-unknown-unknown. 2. Bin targets compiled for wasm32: cargo test builds bin targets alongside integration tests (rust-lang/cargo#12980). The pampa and ast-reconcile bins use native-only types (NativeRuntime, tokio) that don't exist on wasm32. Added required-features = ["terminal-support"] so bins are skipped when --no-default-features --features lua-filter is used.
rust-toolchain.toml installs the prebuilt wasm32-unknown-unknown target (needed for the production WASM build). But -Zbuild-std rebuilds core from source, and having both causes E0152 (duplicate lang item) when building within the workspace. The production build (wasm-quarto-hub-client) avoids this because it is excluded from the workspace and gets an isolated target/ directory. The WASM test runs pampa within the workspace, where the conflict manifests. Explicitly remove the prebuilt target before running WASM tests.
The wasm-tests job failed with E0152 (duplicate lang item in core) because rust-toolchain.toml declares targets = ["wasm32-unknown-unknown"], and the rustup proxy auto-reinstalls the prebuilt target on every cargo invocation — undoing the explicit rustup target remove step. Fix: set RUSTUP_TOOLCHAIN=nightly as a job-level env var, which tells rustup to bypass rust-toolchain.toml entirely. Replace dtolnay/rust-toolchain action with plain rustup toolchain install since we only need nightly + rust-src (no prebuilt wasm32 target). The target removal step is no longer needed because the target is never installed in the first place.
apply_lua_filters became async in e537fb8 (Apr 6) but the WASM tests written in 22f9a4f (Apr 9) called it without .await. The test binary also needs panic_abort in the -Zbuild-std list. Neither issue was caught because CI always failed earlier at E0152 (duplicate core sysroot). - Make filter/io/os test functions async, add .await on apply_lua_filters - Add panic_abort to -Zbuild-std in CI and docs
…cturing apply_lua_filters returns a FilterOutput struct (with pandoc, context, diagnostics fields), not a tuple. The tests were written with tuple destructuring that never compiled on wasm32.
The CI job no longer uses rustup target remove — it sets RUSTUP_TOOLCHAIN=nightly to bypass rust-toolchain.toml entirely, preventing the prebuilt target from being installed in the first place.
Document three rounds of CI fixes discovered after resolving E0152: - RUSTUP_TOOLCHAIN approach supersedes rustup target remove - dtolnay/rust-toolchain replaced with plain rustup - async/struct/panic_abort test compilation fixes Add wasm-c-shim extraction plan: shared crate for C stdlib stubs needed by both production WASM build and WASM integration tests.
Move the ~980-line c_shim.rs from wasm-quarto-hub-client into a new wasm-c-shim crate. This provides #[no_mangle] C stdlib stubs (malloc, fprintf, snprintf, abort, etc.) needed by tree-sitter and Lua when compiled for wasm32-unknown-unknown, which has no libc. The crate is a no-op on native targets (all exports gated on wasm32). Both wasm-quarto-hub-client (production) and pampa WASM tests now depend on wasm-c-shim for the shared symbols.
The shim code was written for edition 2021 (in wasm-quarto-hub-client).
Edition 2024 requires explicit unsafe {} blocks inside unsafe fn, which
would add wrappers to ~65 FFI call sites with no safety benefit. Use
edition 2021 for this crate and document the rationale.
The four `raw_module` extern blocks in quarto-system-runtime/src/wasm.rs
import absolute paths (/src/wasm-js-bridge/{template,sass,cache,fetch}.js)
that hub-client serves through Vite at runtime. wasm-bindgen generates
unconditional `require()` calls for these paths in the shim it produces,
so any wasm32 binary that links quarto-system-runtime — including the
new pampa wasm_lua tests — fails to load under Node.js with
`MODULE_NOT_FOUND` before any test body runs.
Add a `js-bridge` Cargo feature, default off. Gate the four extern blocks
behind it, and provide stubs that return `Err("js-bridge feature not
enabled")` / `false` when off so the SystemRuntime impl still compiles.
wasm-quarto-hub-client opts in via `features = ["js-bridge"]`; pampa's
wasm test build does not, so the require()s disappear from the shim.
Native builds are unaffected (wasm.rs is `#![cfg(target_arch = "wasm32")]`).
This clears the blocker documented in PR #109. A separate downstream
issue is now visible: rust_lua_throw panics propagate as wasm
RuntimeError instead of being caught by rust_lua_protected_call, so the
6 wasm tests still fail. That looks like a panic-strategy problem in
wasm-c-shim, unrelated to the JS bridge fix.
The pampa wasm_lua tests link against C-compiled Lua and rely on wasm-c-shim's catch_unwind-based replacement for Lua's setjmp/longjmp. That mechanism only works when the binary uses panic=unwind. The default panic strategy for wasm32-unknown-unknown is abort, which makes catch_unwind a no-op and turns any Lua throw during mlua initialization into an immediate wasm RuntimeError: unreachable. wasm-quarto-hub-client already sets these flags via its own .cargo/config.toml — but it lives in an isolated workspace, so that config doesn't reach builds invoked from the workspace root. Mirror the same flags here so the test build behaves like the production build: -C target-feature=+bulk-memory,+exception-handling -C panic=unwind -Zwasm-c-abi=spec The other wasm32 consumers in the repo (wasm-quarto-hub-client itself, test-unwind/) are isolated workspaces with their own configs and are unaffected. Native builds are unaffected (target-scoped flags). `-Zbuild-std=std,panic_unwind` is still required at the cargo invocation because the prebuilt wasm32 sysroot doesn't include the unwind runtime and `[unstable] build-std` can't be set workspace-wide without forcing build-std on native builds too. With this and the prior js-bridge feature gate, all 6 wasm_lua tests pass locally.
main's panic-suppression commit (675c22d) added a `LuaThrow` marker struct in wasm-quarto-hub-client/src/lib.rs and changed rust_lua_throw to `panic_any(crate::LuaThrow)`. This branch's earlier commit (01df02e) extracted the C shim into a separate `wasm-c-shim` crate, so after the rebase `crate::LuaThrow` no longer resolved. Move the LuaThrow definition into wasm-c-shim where the panic actually originates, and import it from the hub-client's panic hook. This preserves the panic-suppression behavior without introducing a circular dep — the shim owns the marker, the hub-client owns the hook.
Adds three findings discovered after Phase 4 CI started executing: 1. raw_module extern blocks in quarto-system-runtime/src/wasm.rs cause wasm-bindgen-test-runner to fail at module load under Node.js, because the JS bridge paths only resolve under Vite. Resolved by adding a default-off js-bridge Cargo feature, gated extern blocks, and stub fallbacks; hub-client opts in. 2. Workspace wasm32 builds inherit panic=abort, which makes wasm-c-shim's catch_unwind-based replacement for Lua's setjmp/longjmp a no-op. Resolved by mirroring hub-client's rustflags into the workspace-root .cargo/config.toml. [unstable] build-std deliberately stays on the command line, since it is not target-scoped. 3. main introduced a LuaThrow marker struct in wasm-quarto-hub-client while this branch was open. After rebase the extracted shim crate referenced crate::LuaThrow which no longer resolved; moved the definition into wasm-c-shim where the panic originates.
Extend dev-docs/wasm.md with three sections: - LuaThrow marker payload — the panic type wasm-c-shim uses to wrap Lua errors, downcast by hub-client's panic hook for noise filtering. - Wasm32 panic strategy and rustflags — why panic=unwind + exception-handling are required for wasm-c-shim's catch_unwind to work, and why the flags live in two .cargo/config.toml files (one per workspace) but [unstable] build-std deliberately stays on the command line. - JS bridge feature gate — what quarto-system-runtime's js-bridge feature does, why it is default off, and how hub-client opts in. Also update the local test command to drop panic_abort from -Zbuild-std (the binary now uses panic=unwind, picked up from the workspace-root .cargo/config.toml), and add a macOS note about Homebrew LLVM since Apple clang lacks wasm32 support.
Member
|
@cderv, thanks for writing these tests! I was trying to understand why these paths weren't tested, and either misunderstood or miscommunicated this to Claude, testing them the wrong way! This was the mistake you fixed in I've fixed the remaining issues with this PR and it runs clean here: I also rebased onto latest main. May I force push, or do you have other work on this branch? |
Member
Author
|
Oh thanks for chiming in ! I was going to continue with this this week. It needed to be rebased on main and it is also related to work I started in #125 My aim was that we have some wasm build test, but I was sorting out quarto-hub build from other wasm build. interested to see your change ! Yes you can force push here. And maybe we can discuss quickly all this together if you have time today. |
gordonwoodhull
force-pushed
the
feature/wasm-testing-and-cleanup
branch
from
ba0ec15 to
901f9f7
Compare
Member
|
Thanks! I've pushed. It's really just
(I'm going for a hike today since I stupidly worked over the weekend, glad to discuss later in the week.) |
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
2 participants
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.
Replace the lost cfg proxy coverage (removed in #116) with 6 real WASM smoke tests
in
crates/pampa/tests/wasm_lua.rsthat compile and run onwasm32-unknown-unknown:Lua::new_with())panic_unwindworks)ioandosmodule registrationThese run in a new
wasm-testsCI job (Linux, nightly + Clang).Infrastructure
crates/wasm-c-shim/— shared C stdlib shim crate for wasm32 (no libc on wasm32,C deps like tree-sitter and Lua need
malloc,fprintf, etc. at link time).cargo/config.toml— wasm-bindgen-test runner configproptest,insta,tempfile,tokiodev-deps onnot(wasm32)to avoidpulling
getrandom 0.3.4which doesn't compile for wasm32[[bin]]targets viarequired-featuresto prevent compilation duringWASM test runs (Add an option to skip building binaries when there are integration tests rust-lang/cargo#12980)
RUSTUP_TOOLCHAIN=nightlyin CI to bypassrust-toolchain.tomlsysroot conflictwith
-Zbuild-stdKnown blocker
The
wasm-testsCI job currently fails becausequarto-system-runtime/src/wasm.rshas 4
raw_moduleextern blocks (/src/wasm-js-bridge/{template,sass,cache,fetch}.js)that get baked into any wasm32 binary.
wasm-bindgen-test-runnergeneratesrequire()calls for these absolute paths, which fail in Node.js.
Recommended fix: feature-gate the JS bridge in
quarto-system-runtime— add ajs-bridgefeature, gate the 4 extern blocks, provide stubs when off.wasm-quarto-hub-clientenablesjs-bridge; pampa test builds don't.Depends on #116.