ci: complete W50 — Windows install-tools.ps1 + restore extras (W50 PR-D)#83
Merged
ci: complete W50 — Windows install-tools.ps1 + restore extras (W50 PR-D)#83
Conversation
Final step in W50 / Plan B sub-3 (CI Nix-ify). Two changes that
together close the per-tool-install path on every CI runner:
1. Windows test job now uses scripts/windows/install-tools.ps1 +
scripts/gate-commit.sh, mirroring the local-Windows developer
experience. Drops ~272 lines of bespoke install/test steps in
ci.yml.
2. test-nix (Linux+Mac, PR-B + PR-C) regains the extras that the
pre-Nix `test` job ran but gate-commit.sh intentionally omits:
c-test, static-lib + static-link, Rust example, memory check.
These are CI-specific quality gates, not Commit Gate items, so
they live as ci.yml steps after the gate-commit step rather
than inside gate-commit.sh.
## Changes
- `scripts/windows/install-tools.ps1`: gained `Append-GithubPath`
and `Append-GithubEnv` helpers. When `$GITHUB_PATH` /
`$GITHUB_ENV` are set (CI mode), the script appends entries to
those files in addition to the User-scope environment. Local
Windows installs are unaffected (the env vars aren't set).
- `.github/workflows/ci.yml`:
- `test-nix` job: appended five "extras" steps after the gate
(c-test / static-lib / static-link / Rust example / memory).
All run inside `nix develop --command` for tool consistency.
- `test` job: replaced the multi-step Windows install path with
`pwsh install-tools.ps1` + `bash gate-commit.sh` + same five
extras. Memory check stays as pwsh (Windows lacks
/usr/bin/time). Binary size check dropped because size-matrix
already covers Windows. Single-element matrix simplified to a
plain runs-on.
- nightly.yml left untouched (mirroring it to nix-installer is a
natural follow-up but out of scope here — its sanitizer + fuzz
jobs are Linux-only and don't surface on PR CI).
Net diff: -184 lines, but the size-matrix and benchmark coverage
is unchanged, and all five extras now run on all three OSes.
CI is the test plan — no local pre-flight possible for ci.yml
changes.
Windows CI surfaced an existing bug in install-tools.ps1's rust install path — `rustup target add wasm32-wasip1` (run after rustup-init) errored partway through downloading rust-std, with a PowerShell "Cannot bind argument to parameter 'Path' because it is an empty string" trace. The local-Windows path that has been exercised by W52 / PR #74 does not hit it, but the GitHub-hosted Windows runner does (different LOCALAPPDATA layout / pre-existing rustup state). Workaround: skip the install-tools.ps1 rust install on CI and use the runner's pre-installed rustup directly, matching the Linux / macOS test-nix pattern. The deeper fix is left as a follow-up (needs a local Windows repro) — local Windows installs that want a self-contained rustup tree under %LOCALAPPDATA%\zwasm-tools\rust-* still work as before; CI just opts out via -SkipRust. - install-tools.ps1: new `-SkipRust` switch. When set, the rust install block is bypassed (PATH wiring for rust still skipped because $paths['rust'] never gets populated). - ci.yml `test (windows-latest)`: invoke install-tools.ps1 -SkipRust; add a separate `Setup Rust` step that uses the runner's pre-installed rustup. Same shape as test-nix's Setup Rust step; no behaviour difference from a developer perspective.
Second CI failure on PR-D surfaced TinyGo's wasm-opt dependency:
FAIL: tinygo_fib - error: could not find wasm-opt, set the WASMOPT
environment variable to override
On Linux/macOS the Nix `tinygo` derivation is wrapped to prepend
`binaryen-125`'s bin/ to PATH automatically (verified via
`realpath $(which tinygo)` showing the wrapper script). On the
GitHub-hosted Windows runner there's no such wrapper, so wasm-opt
isn't found and the four `tinygo_*` realworld programs fail to
build.
- versions.lock: added BINARYEN_VERSION=125 (matches the binaryen
version that Nix's tinygo 0.40.1 wrapper uses).
- install-tools.ps1:
- new `binaryen` install case (`OnlyTool -in 'all' / 'binaryen' /
'tinygo'`) — the tinygo trigger ensures `-OnlyTool tinygo`
automatically pulls binaryen too.
- `binaryen` added to ValidateSet of -OnlyTool plus param doc.
- `realworldKeys` now includes binaryen with the special-case
that `-OnlyTool tinygo` also requires BINARYEN_VERSION.
- PATH wiring: bin/ subdir of the binaryen install added to
User PATH and (in CI) GITHUB_PATH.
No CI YAML change in this commit — the Windows test job already
calls `install-tools.ps1 -SkipRust` which now also installs
binaryen as part of `-OnlyTool all`.
Third CI failure on PR #83 surfaced a Windows-specific filename collision that pre-existed but was masked by the old `test` job order: - `zig build shared-lib` writes both `zwasm.dll` and an MSVC- compatible import library `zwasm.lib` to `zig-out/lib/`. - `zig build static-lib -Dpic=true -Dcompiler-rt=true` writes a static archive *also named* `zwasm.lib` to the same directory, *overwriting* the import library. - A subsequent `cargo run` on the rust example then tries to link the static archive via MSVC `link.exe`, which fails with `LNK1143: invalid or corrupt file: no symbol for COMDAT section` (Zig's static archive uses MinGW-style COMDAT that MSVC link.exe doesn't accept). Linux / macOS don't have this collision because their archive extensions are distinct (`libzwasm.a` vs `libzwasm.so` / `libzwasm.dylib`). Fix: in both `test-nix` and `test (windows-latest)` extras blocks, move the `Run Rust FFI example (dynamic)` step BEFORE `Build static library`. The pre-PR-D test job had this order naturally; PR-D's first cut grouped the two zig-build steps together which broke Windows. Comment in the workflow explains the constraint. CI is the test plan.
chaploud
added a commit
that referenced
this pull request
Apr 29, 2026
…estore extras (W50 PR-D) (#83)
chaploud
added a commit
that referenced
this pull request
Apr 29, 2026
* docs: post-W50 cleanup — drop resume-guide, refocus memo on W53/C-g/W47 Plan B sub-3 (W50) and Plan C (W49) shipped via PRs #80..#83 in the 2026-04-29 PM autonomous session, plus the W47 investigation note in #84. The .dev/resume-guide.md handover doc is now stale: its "Plan B sub-3 is the next big lift" framing no longer matches reality, and the per-PR plan items are all marked complete. - Delete .dev/resume-guide.md; .dev/memo.md `## Current Task` is the single handover surface going forward. - memo.md: refresh the active-work section to W53 → C-g → W47, with the per-item plan inlined (was previously split between memo and resume-guide). - checklist.md: mark W50 done with the four-PR breakdown, refocus W49 on the C-g residual (3-platform bench baseline reset), open W53. - environment.md: explain that the only Windows-skipped CI step left is `benchmark` and link to C-g for the rationale. - roadmap.md: Windows CI guard removal flipped Done; W53 surfaced as the next active item. * fix(w53): route rustup-init stdout through Out-Host to keep return scalar PowerShell folds every native command's stdout into the enclosing function's pipeline output. Inside `Install-Rustup`, that meant rustup-init's `info: downloading component rust-std` (and the similar lines from `rustup target add wasm32-wasip1`) were piling up alongside the trailing `return $stampedDir`, so the caller's `$rustRoot = Install-Rustup ...` was a string array rather than a single path. The downstream $pathsToAdd += (Join-Path (Join-Path $paths['rust'] 'cargo') 'bin') then exploded on the empty leading element with Cannot bind argument to parameter 'Path' because it is an empty string. — matching the W53 symptom on a fresh GitHub-hosted Windows runner. Local Windows mini-PC was unaffected because rustup's "already installed" path is silent on stdout, so nothing leaked into the function's return value there. Fix: route both native command invocations through `2>&1 | Out-Host`, which keeps the lines visible in the CI log but pulls them out of the function's pipeline output. Also added a defensive check in the caller so any future regression of this shape fails loud rather than silently producing a malformed PATH. ci.yml: drop `-SkipRust` and the separate `Setup Rust` step on the Windows test job. The runner now goes through a single `install-tools.ps1` path with a self-contained `%LOCALAPPDATA%\zwasm-tools\rust-stable\` toolchain, the same as local Windows users get. * docs(w53): mark resolved with root-cause + fix summary Updates the four docs that were tracking W53 as open work to reflect the rustup-init stdout pollution diagnosis and the `Out-Host` redirect fix in the previous commit.
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
1 participant
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.
Final step in W50 / Plan B sub-3 (CI Nix-ify). Two changes that together close the per-tool-install path on every CI runner:
Windows test job now uses `scripts/windows/install-tools.ps1` + `scripts/gate-commit.sh`, mirroring the local-Windows developer experience. Drops ~272 lines of bespoke install/test steps in ci.yml.
test-nix (Linux+Mac, from PR-B ci: add test-nix Linux job using Nix devshell (W50 PR-B) #81 + PR-C ci: extend test-nix to macos-latest (W50 PR-C) #82) regains the extras that the pre-Nix `test` job ran but `gate-commit.sh` intentionally omits: `zig build c-test`, `zig build static-lib + run_static_link_test.sh`, Rust example `cargo run`, peak-RSS memory check. These are CI-specific quality gates, not Commit Gate items, so they live as ci.yml steps after the gate-commit step rather than inside gate-commit.sh.
Changes
Net diff: -184 lines, but the size-matrix and benchmark coverage is unchanged, and all five extras now run on all three OSes (PR-B/C had inadvertently dropped them on Linux/Mac).
Test plan
CI is the test: