windows_sys: gate #[link] behind cfg(windows) so non-Windows cargo test binaries link - #35084
Conversation
bun_errno and bun_spawn_sys depend on bun_windows_sys unconditionally so
that Win32 POD types (IO_COUNTERS, Win32Error, NTSTATUS) resolve on every
target. The unconditional #[link(name = "ntdll"/"kernel32"/...)]
attributes on the extern blocks were therefore baked into the rlib on
non-Windows hosts too, and any standalone test/bench binary that
transitively pulled in bun_windows_sys failed to link:
ld.lld: error: unable to find library -lntdll
ld.lld: error: unable to find library -lkernel32
...
bun bd was unaffected because it builds a staticlib (no final link step
driven by cargo).
Wrap every #[link(name = ...)] in #[cfg_attr(windows, ...)] so the -l
flags are only emitted for Windows targets. Typedefs, consts and the
extern fn declarations themselves remain available on all targets.
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 3:12 AM PT - Jul 22nd, 2026
✅ @robobun, your commit 06d79c3d9848c1a355e8e6ce4dff84976281b854 passed in 🧪 To try this PR locally: bunx bun-pr 35084That installs a local version of the PR into your bun-35084 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the workspaceResolvable guard from my earlier comment is now in place (bdd901b) and matches linear-fifo.test.ts exactly.
What was reviewed:
- All 15
#[link(name = ...)]sites insrc/windows_sys/externs.rsare wrapped; grep confirms none remain unwrapped. On Windowscfg_attr(windows, link(...))expands to the identical attribute, so the import-library set is unchanged. - The extern blocks with no
#[link](GetUserNameW,SetStdHandle/GetConsoleCP/GetConsoleOutputCP,windows_enable_stdio_inheritance) were correctly left alone — they never emitted-lflags. - Guard test drains stdout/stderr concurrently, asserts the combined
{stdout, stderr, exitCode}object, and skips on Windows / missing cargo / unresolvable workspace.
Extended reasoning...
Overview
Mechanical build-tooling fix in src/windows_sys/externs.rs: each #[link(name = "ntdll"/"kernel32"/"ws2_32"/"shell32"/"advapi32")] attribute (15 sites) is wrapped in #[cfg_attr(windows, ...)]. A new guard test in test/internal/rust-windows-sys-link.test.ts spawns cargo test -p bun_windows_sys --no-run on non-Windows hosts to prove the crate now links standalone. A doc comment at the top of externs.rs explains why the gating exists.
Security risks
None. This touches linker attribute metadata only — no user-facing surface, no input handling, no runtime code paths change on any platform.
Level of scrutiny
Low. On Windows, #[cfg_attr(windows, link(name = "X"))] expands to the byte-identical #[link(name = "X")], so bun.exe/bun_shim_impl.exe see no change. On non-Windows, the extern fn declarations remain (so type re-exports for bun_errno/bun_spawn_sys still resolve) but the -l flags stop being baked into the rlib — which only ever mattered when cargo drove a final link for a standalone test binary; bun bd produces a staticlib and was never affected. This is a standard Rust idiom for cross-platform FFI crates.
Other factors
- I verified via grep that no un-gated
#[link(name = ...)]remains anywhere undersrc/windows_sys/. - My earlier inline finding (missing
workspaceResolvableskip guard for test-only CI lanes) was addressed in bdd901b; the guard now checks bothvendor/lolhtml/Cargo.tomlandbuild/debug/codegen/build_options.rs, identical to the siblingtest/internal/linear-fifo.test.ts. That thread is resolved. - The test follows harness conventions:
await usingon the spawned process, concurrent pipe draining viaPromise.all, and the combined{stdout, stderr, exitCode}assertion pattern so failure output includes cargo's stderr. - PR description documents fail-before/pass-after verification and cross-checks against both Windows MSVC targets.
The cargo-link test skips on test-only lanes (no vendor/lolhtml), so add a grep-based lint that runs unconditionally via the source-lints workflow. Also reword the externs.rs doc comment to state the invariant rather than an incomplete list of dependents.
There was a problem hiding this comment.
LGTM — the earlier workspaceResolvable guard concern is addressed in bdd901b, and the remaining --locked nit below is non-blocking (CI checkouts always have a synced lockfile).
Extended reasoning...
Overview
This PR wraps every #[link(name = "...")] attribute in src/windows_sys/externs.rs with #[cfg_attr(windows, ...)] (15 sites: kernel32 ×8, ntdll ×2, ws2_32 ×2, shell32, advapi32), so that non-Windows builds of the bun_windows_sys rlib stop emitting -lntdll/-lkernel32/... to the linker. Two guard tests are added: a source-lint that greps externs.rs for any bare #[link(name = ...)], and an end-to-end cargo test -p bun_windows_sys --no-run link check that skips on Windows / no cargo / unresolvable workspace.
Security risks
None. This is a build-attribute change with no runtime code path affected. On Windows, #[cfg_attr(windows, link(name = "X"))] expands to the identical #[link(name = "X")], so the import-library set for bun.exe is byte-for-byte unchanged. On non-Windows, the extern declarations remain for type-checking; only the -l linker flags are dropped, which were unresolvable there anyway.
Level of scrutiny
Low-to-medium. The Rust change is purely mechanical and uses the canonical idiom for this problem. bun bd was already unaffected (staticlib, no cargo-driven final link), so this only widens what cargo test -p ... can link on developer machines. The PR description shows fail-before/pass-after and cross-checks on both Windows MSVC targets.
Other factors
- My previous review flagged the missing
workspaceResolvableskip guard; the author addressed it in bdd901b by mirroringlinear-fifo.test.tsexactly (vendor/lolhtml + build/debug/codegen/build_options.rs), and the thread is resolved. - The source-lint regex correctly distinguishes
#[link(name = ...)]from#[link_name = ...](symbol rename), strips line comments, and is scoped toexterns.rsonly (bare#[link]is fine in modules already gated by#[cfg(windows)]). - The cargo test spreads
process.envrather thanbunEnv, matchinglinear-fifo.test.ts(it spawns cargo, not Bun); pipes are drained concurrently;await usinghandles cleanup. - The one remaining finding is a nit:
--lockedshould be added to match the sibling test's invocation for hermeticity on developer machines with a driftedCargo.lock. It cannot flake CI and is a one-token fix, so it does not block approval.
There was a problem hiding this comment.
LGTM — both earlier findings (the workspaceResolvable skip guard and --locked) are now in place.
What was reviewed:
externs.rs: confirmed all 15#[link(name = ...)]sites are now#[cfg_attr(windows, ...)]-wrapped and none remain bare;#[link_name = ...](symbol rename) is correctly left alone. On Windows this expands to the identical attribute, so the import-library set is unchanged.rust-windows-sys-link.test.ts: matcheslinear-fifo.test.tsconventions (workspaceResolvable gate on vendor/lolhtml + build/debug/codegen,--locked,await using, concurrent stdout/stderr/exited drain).windows-sys-link-cfg.test.ts: same shape as the other regex-based source-lints (e.g.shim-stdint-includes.test.ts); the regex correctly excludes#[link_name = ...]and comment-stripped lines.
Extended reasoning...
Overview
This PR wraps every #[link(name = "...")] attribute in src/windows_sys/externs.rs with #[cfg_attr(windows, ...)] (15 sites across ntdll/kernel32/ws2_32/shell32/advapi32 extern blocks), adds a module-level doc comment explaining why, and ships two guard tests: an end-to-end cargo test -p bun_windows_sys --no-run link check and a source-lint that greps for any future bare #[link(name = ...)] in this file.
Security risks
None. This is purely build-time link-attribute metadata; no runtime code paths, inputs, or trust boundaries are touched.
Level of scrutiny
Low. The Rust change is a mechanical attribute rewrite: on Windows #[cfg_attr(windows, link(name = X))] expands to exactly #[link(name = X)], so bun.exe's import-library set is unchanged; on non-Windows the attribute is simply dropped, which removes the spurious -lntdll/-lkernel32/... flags that were breaking standalone cargo test/bench binaries. bun bd was already unaffected (staticlib, no cargo-driven final link), so there is no shipped-binary behavior change on any platform. I verified against the full file that no #[link(name = ...)] remains unwrapped and that the unrelated #[link_name = "WaitForSingleObject"] is untouched.
Other factors
Two prior review rounds raised findings that have both been addressed and resolved: (1) the workspaceResolvable guard on vendor/lolhtml/Cargo.toml + build/debug/codegen/build_options.rs so test-only CI lanes skip cleanly, and (2) --locked on the cargo invocation for hermeticity. The resulting test now mirrors test/internal/linear-fifo.test.ts line-for-line on those conventions (same guard expression, same spawn shape, concurrent pipe drain, await using). The source-lint test follows the established pattern in test/internal/source-lints/ (e.g. shim-stdint-includes.test.ts): read source, regex-scan, expect(violations).toEqual([]). Its regex (#\[\s*link\s*\(\s*name\s*=) correctly distinguishes the library-link attribute from the #[link_name = ...] symbol-rename attribute and strips line comments first.
…n block The windows-sys-link-cfg source lint from #35084 requires every #[link] in bun_windows_sys to use the cfg_attr form. The block is already #[cfg(windows)] (CONTEXT only exists there), so the cfg_attr is redundant but matches the enforced pattern.
2 participants
What does this PR do?
bun_sys,bun_errnoandbun_spawn_sysdepend onbun_windows_sysunconditionally so that Win32 POD types (IO_COUNTERS,Win32Error,NTSTATUS) resolve on every target. The#[link(name = "ntdll"/"kernel32"/"ws2_32"/"shell32"/"advapi32")]attributes on the extern blocks were therefore being baked into the rlib on non-Windows hosts too, and any standalone test/bench binary that transitively pulled inbun_windows_sysfailed to link:Same for
cargo test -p bun_spawn_sys, and (further downstream)cargo test -p bun_resolver/bun_routerper the note in #35082.bun bdwas unaffected because it produces a staticlib and never asks cargo to drive a final link.Fix: wrap every
#[link(name = "...")]in#[cfg_attr(windows, ...)]. On Windows this expands to the identical#[link(name = "...")], so the import-library set forbun.exeandbun_shim_impl.exeis unchanged. On every other target the-lflags stop being emitted. Typedefs, consts and theextern "system"declarations themselves stay available on all targets, so the unconditional dependents keep resolving their type re-exports without any downstream#[cfg]churn.Guard tests:
test/internal/source-lints/windows-sys-link-cfg.test.ts: grepssrc/windows_sys/externs.rsfor any bare#[link(name = ...)]not wrapped incfg_attr(windows, ...). Runs on every PR that touchessrc/**/*.rsvia the source-lints workflow, with no cargo/workspace prerequisite.test/internal/rust-windows-sys-link.test.ts: end-to-end check that spawnscargo test -p bun_windows_sys --no-runon non-Windows hosts and asserts it links.cargo check --testsonly type-checks, so this is the one that actually drives the linker. Skips on test-only CI lanes where the cargo workspace is not resolvable (sameworkspaceResolvablegate aslinear-fifo.test.ts).cargo test -p bun_resolverstill fails to link on Linux after this change becausebun_core/bun_simdutf_sysreference native C symbols (highway_index_of_char,simdutf__*) that only the full CMake build provides. That is a separate pre-existing issue and out of scope here; the-lntdll/-lkernel32/... flags are gone from its link line.How did you verify your code works?
Fail-before / pass-after verified with
git checkout origin/main -- src/windows_sys/externs.rsaround both tests.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/internal/rust-windows-sys-link.test.ts