Skip to content

fix(compile): Windows staticlib without MSVC, actionable dylib error, .dll default, /STACK comment - #6643

Merged
proggeramlug merged 1 commit into
mainfrom
fix/win-toolchain-holes
Jul 19, 2026
Merged

proggeramlug merged 1 commit into
mainfrom
fix/win-toolchain-holes

Conversation

@proggeramlug

Copy link
Copy Markdown
Contributor

Fixes the four Windows toolchain holes from the 2026-07-18 audit (main @ 58e555e). All changes in crates/perry/src/commands/compile/.

(a) --output-type staticlib no longer hard-requires MSVC lib.exe

run_pipeline.rs unconditionally spawned Command::new("lib.exe"), so a lightweight-toolchain user (LLVM + xwin via perry setup windows, no Visual Studio) got a raw spawn error.

Now the archiver is selected with the established lookup precedence (library_search.rs):

  1. MSVC lib.exe — vswhere-located MSVC bin dir (the same dir find_msvc_link_exe picks), then PATH (covers vcvars prompts);
  2. llvm-lib — drop-in lib.exe replacement, ships with winget install LLVM.LLVM, resolved via find_llvm_tool (so PERRY_LLVM_LIB, rustup sysroot, and PATH all work);
  3. llvm-ar --format=coff crs — last resort (rustup's llvm-tools component carries llvm-ar but not llvm-lib).

If none resolve, the compile fails with the same two-toolchain install hint style as the executable linker path (LLVM winget line + VS Build Tools workload line), instead of an opaque spawn error.

(b) --output-type dylib on Windows: actionable failure + the link.exe /DLL investigation

Investigation result (required by the audit)

The code comment claimed MSVC link.exe "returns 0 without writing the DLL" under /DLL /FORCE:UNRESOLVED, which is why it was deliberately excluded. This no longer reproduces. Tested on this box with MSVC 14.50.35717 (VS 2026 Build Tools) against a clang-built COFF object with an unresolved js_* symbol, using Perry's exact dylib link line (/NOLOGO /DLL /FORCE:UNRESOLVED /DEF:... /OUT:... /defaultlib:libcmt):

  • MSVC link.exe: exit 0, DLL written (LNK2019 report + LNK4088 "image may not run" warning); loads via LoadLibrary; both .def exports resolve via GetProcAddress; perry_plugin_abi_version() callable and returns the right value.
  • lld-link (LLVM 22.1.3): exit 0, DLL written, identical load/export behavior.

Caveat: verified on MSVC 14.50 only — older toolsets may still exhibit the silent-drop behavior, which shaped the fix below.

Fix

Per the audit's guidance ("if it actually works now, wiring MSVC link.exe as fallback is the better fix"):

  • lld-link stays preferred (find_lld_link(), plus find_llvm_tool("lld-link") so Unix cross-hosts get a which probe too — same reach as the old PathBuf::from("lld-link") PATH-at-spawn fallback, but resolved up front).
  • MSVC link.exe (find_msvc_link_exe) is wired as the fallback when lld-link is absent.
  • If neither exists, the compile errors before spawning with an actionable message (winget LLVM line, PERRY_LLD_LINK, VS Build Tools alternative) instead of the raw spawn failure.
  • A post-link existence check guards the legacy failure mode: if an older toolset exits 0 without writing the DLL, the user gets "linker reported success but did not write X — install lld-link" instead of a mystery downstream failure.
  • The dylib link now also sets LIB (xwin sysroot first, then vswhere — mirroring select_linker_command) when the user hasn't, so /defaultlib:libcmt resolves from a plain shell for both linkers.

(c) /STACK comment corrected

link/platform_cmd.rs said "Reserve 8MB" next to /STACK:67108864, which is 64 MiB. The comment now says 64 MiB and why it's that large (deep recursion in compiled TS + game workloads; bakes in the old manual editbin /STACK post-link step this replaced).

(d) Extension-less --output-type dylib defaults to .dll on Windows

output_path.rs picked .dylib (host-macOS cfg) else .so — no Windows branch, so a no--o dylib build on Windows produced app.so. The dylib arm is now target-keyed like the rest of the file (host cfg only as the no---target fallback): windows/windows-winui → .dll, Apple family → .dylib, else .so. Matches windows_default_output_extension, which already handled the -o NAME path. Finishes #4771.

Verification

  • cargo check -p perry passes (0 errors; the only warnings are pre-existing ones in untouched files). Note: origin/main's perry-runtime doesn't compile on Windows (pre-existing ExitProcess divergence, fix in PR fix(runtime): declare ExitProcess as never-returning on Windows (build break) #6609); verified with that one-line local overlay applied — the overlay is not part of this PR.
  • New unit tests in windows_link_tests.rs (archiver precedence lib.exe → llvm-lib → llvm-ar → None; /OUT: vs --format=coff crs command shapes) and output_path.rs (target-keyed dylib extension incl. .dll, host-fallback matrix): all 29 matching tests pass (cargo test -p perry --bin perry -- output_path windows_link). Running them on Windows needed a local, uncommitted link shim for a second pre-existing Windows hole: the perry (test) binary fails to link with LNK2019: js_crypto_ed25519_verify — a perry-updater extern defined in perry-stdlib, which the perry CLI bin does not depend on (release-profile CGU partitioning is why release builds don't hit it). Not introduced by this PR; may be worth its own issue.
  • End-to-end staticlib smoke: done — perry-dev build, then perry hello.ts --output-type staticlib -o hello.lib → exit 0; the MSVC "Library Manager 14.50" banner confirms the new selector picked lib.exe via the vswhere rung; lib.exe /LIST hello.lib shows the member; hello.linkdeps.json sidecar written. The other two rungs were validated manually against the same object: both the llvm-lib-built and the llvm-ar --format=coff crs-built archives are readable by MSVC lib.exe (exit 0). (The perry-dev bin build itself required the same local shim for the pre-existing LNK2019 above.)
  • cargo fmt -p perry clean; diff touches only the intended files.

No version bump / changelog per maintainer instruction (folded in at merge time).

@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@proggeramlug, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 33 seconds

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 57e915c7-44d9-4bdd-9a9f-96fab2ecdb0d

📥 Commits

Reviewing files that changed from the base of the PR and between cca6e54 and 799c956.

📒 Files selected for processing (6)
  • crates/perry/src/commands/compile.rs
  • crates/perry/src/commands/compile/library_search.rs
  • crates/perry/src/commands/compile/link/platform_cmd.rs
  • crates/perry/src/commands/compile/output_path.rs
  • crates/perry/src/commands/compile/run_pipeline.rs
  • crates/perry/src/commands/compile/windows_link_tests.rs
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/win-toolchain-holes

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@proggeramlug
proggeramlug merged commit d01ec0b into main Jul 19, 2026
23 of 25 checks passed
@proggeramlug
proggeramlug deleted the fix/win-toolchain-holes branch July 19, 2026 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant