build: make the logs option reach Rust - #38913
Conversation
The build's `logs` option (on by default in debug, set by the release-assertions profile and `bun run build:logs`, overridable with --logs=on|off) did nothing for Rust: buildOptionsRs.ts emitted `ENABLE_LOGS = cfg!(bun_debug)` and rust.ts only passes --cfg=bun_debug for Debug builds, so a release build configured with logs compiled every scoped logger out and a debug build configured without still logged. scoped_log!, syslog!, mark_binding! and the two mark_binding fns also gated on IS_DEBUG directly, so fixing the constant alone would not have been enough. rust.ts now passes --cfg=bun_logs exactly when cfg.logs is set (plus the matching --check-cfg, and the cfg is registered in Cargo.toml for bare cargo), build_options.rs reads ENABLE_LOGS from it, and the loggers gate on ENABLE_LOGS. Debug, release and release-asan resolve to the same values as before; release-assertions and --logs=on|off now take effect. A cfg rather than a literal in build_options.rs keeps a bare `cargo check` / `cargo miri test` (which read build/debug's generated file without any RUSTFLAGS) on logs-off semantics, as with bun_debug.
|
Warning Review limit reached
Next review available in: 34 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 (12)
Comment |
|
Status: ready for review. Reproduced on main with configure only (no build needed): Test: Builds: debug rebuilt with the new rustflags, CI on the first push: GitHub checks green (Source lints runs the new test; cargo clippy and cargo miri cover bare cargo against the generated |
There was a problem hiding this comment.
LGTM — mechanical wiring of a new --cfg=bun_logs flag following the existing bun_debug/bun_asan pattern, plus swapping six IS_DEBUG guards to ENABLE_LOGS.
What was reviewed:
- Confirmed
cfg.logsdefaults todebug(config.ts:889) andrelease-assertionssets it (profiles.ts:173), so debug/release/release-asan resolve to the sameENABLE_LOGSas before; onlyrelease-assertionsand explicit--logs=on|offchange. - Checked
ScopedLogger::is_visible()already gates onENABLE_LOGS, so the#[cfg(bun_debug)]self-check inscoped_writer()remains unreachable underdebug --logs=off. - Verified no remaining
IS_DEBUG && ….is_visible()sites insrc/**/*.rs; the new test'smockToolchain/linuxConfigpattern matches siblingbuild-debug-info-flags.test.ts.
Extended reasoning...
Overview
This PR makes the build system's logs option actually reach Rust code. It adds a --cfg=bun_logs RUSTFLAGS entry in scripts/build/rust.ts (gated on cfg.logs), changes buildOptionsRs.ts to emit ENABLE_LOGS = cfg!(bun_logs) instead of cfg!(bun_debug), registers the cfg in Cargo.toml's unexpected_cfgs list, and swaps six logger guard sites from env::IS_DEBUG to env::ENABLE_LOGS (in scoped_log!, syslog!, mark_binding!, two mark_binding fns, and one bundler dump). The remaining edits are comment/doc updates. A new source-lint test pins the config→rustflags→constant chain for six profiles and scans src/**/*.rs for regressions.
Security risks
None. This affects only compile-time gating of debug-log macros; no user input handling, no auth/crypto, no runtime API surface. Release builds are byte-for-byte unchanged (cfg!(bun_logs) without the flag folds to the same false as cfg!(bun_debug) did).
Level of scrutiny
Low-to-moderate. The change is mechanical and follows the exact pattern already used for bun_asan/bun_debug/socket_fault_injection in rust.ts and Cargo.toml. It touches build plumbing and debug-only logging infrastructure, not shipped code paths. I verified against config.ts and profiles.ts that every CI-built profile (debug, release, release+asan) resolves ENABLE_LOGS identically before and after — only release-assertions and explicit --logs= overrides change, which is the stated intent.
Other factors
- The new test follows the established
mockToolchain()/linuxConfig()pattern frombuild-debug-info-flags.test.tsandwindows-cross-config.test.ts(same fake-toolchain, samelinuxSysroot: buildDirtrick). It usestempDir, is hermetic, and the PR reports 0.3s runtime. - I confirmed
ScopedLogger::is_visible()(output.rs:1460) already returns false when!ENABLE_LOGS, solog()early-returns before reachingscoped_writer()— the#[cfg(bun_debug)] unreachable!self-check cannot fire underdebug --logs=off. The comment update there (droppingrelease-assertionsfrom the list of profiles withdebug_assertions && !ENABLE_LOGS) is correct post-change. - Grep confirms no remaining
IS_DEBUG … is_visible()guards in the tree, matching the test's source scan. - No prior human or bot review comments to address (CodeRabbit was rate-limited).
-Zlocation-detail=none was passed for every non-assertions release build, which now includes `release --logs=on` (bun run build:logs). The mark_binding() and test-runner group::begin() loggers that build turns on print Location::caller(), so they logged `<redacted>:0`. Gate the flag on !cfg.logs as well; the shipped profiles have logs off and are unchanged.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches build infrastructure across several layers (rust.ts RUSTFLAGS, workspace Cargo.toml, and the scoped_log!/syslog!/mark_binding! macro gates), a human look would still be worthwhile.
What was reviewed:
- The new
bun_logscfg follows the exactbun_asan/bun_debugpattern in rust.ts andunexpected_cfgs;cfg.logsdefaults todebug(config.ts:889), so debug/release/release-asan resolve to the sameENABLE_LOGSas before. - The six
IS_DEBUG→ENABLE_LOGSswaps are the complete set — greppedsrc/**/*.rsfor remainingIS_DEBUG … is_visible()guards, none left; the new source-lint test enforces this. - The added
&& !cfg.logson-Zlocation-detail=noneonly affectsrelease --logs=on(release-assertions was already excluded by!cfg.assertions), so shipped release binaries are unchanged.
Extended reasoning...
Overview
This PR wires the build's logs option through to Rust. It adds a --cfg=bun_logs RUSTFLAG (set when cfg.logs), changes the generated build_options::ENABLE_LOGS to read cfg!(bun_logs) instead of cfg!(bun_debug), registers the cfg in Cargo.toml's unexpected_cfgs, and updates six logger gates (scoped_log!, syslog!, mark_binding!, mark_binding(), mark_member_binding(), and the ReachableFiles dump) to check env::ENABLE_LOGS instead of env::IS_DEBUG. It also gates -Zlocation-detail=none on !cfg.logs so #[track_caller]-based loggers keep their file:line in a logs-enabled release build. A new 223-line source-lint test pins every piece: it resolves six configs, generates build_options.rs for each into a temp dir, checks the emitted rustflags, verifies the Cargo.toml registration, and scans src/**/*.rs for the old guard pattern.
Security risks
None. This affects only debug-logging plumbing and build-time RUSTFLAGS. No user-facing API, no untrusted-input handling, no auth/crypto surface.
Level of scrutiny
Moderate. Each individual change is mechanical — the new cfg mirrors the existing bun_asan/bun_debug/socket_fault_injection wiring exactly, and the Rust edits are one-token swaps in compile-time-const conditions plus comment rewording. Shipped-binary behavior is unchanged: cfg.logs defaults to debug, so release (false), release-asan (false), and debug (true) all produce the same ENABLE_LOGS value as before. Only release-assertions (which explicitly sets logs: true and is documented as "Release + assertions + logs") and manual --logs=on|off overrides change — that is the bug being fixed. However, the change spans build scripts, the workspace Cargo.toml, and macros that expand at every scoped_log! call site, and it changes RUSTFLAGS (forcing a one-time full Rust recompile after merge). That cross-cutting nature makes it worth a maintainer's confirmation rather than an auto-approve.
Other factors
The comment-cop bot flagged over-long comments in an earlier revision; the author shortened them in 63181fb and all threads are resolved. CI is green on GitHub checks (including cargo clippy, cargo miri, and the new source-lint test); Buildkite build 97515 passed 177/179 with the remainder being an unrelated darwin agent-capacity stall. No CODEOWNERS cover these paths, and there are no outstanding human review comments. The scoped_writer() self-check under #[cfg(bun_debug)] remains sound: in a debug build cfg.logs defaults to true, so the unreachable! only fires if someone runs bun bd --logs=off and then reaches a logger — which is exactly what the check is meant to catch.
Problem
logsoption does nothing for Rust.release-assertions(documented as "Release + assertions + logs", used bybun run build:assert) andbun run build:logs(--profile=release --logs=on) produce binaries in which everyscoped_log!is compiled out, soBUN_DEBUG_<scope>=1prints nothing; converselybun bd --logs=offstill logs. Configure even reports the option as live:bun scripts/build.ts --profile=release-assertions --configure-onlyprintsfeatures: assertions, logs, baseline.scripts/build/buildOptionsRs.ts:67emitspub const ENABLE_LOGS: bool = cfg!(bun_debug);andscripts/build/rust.ts:467passes--cfg=bun_debugonly whencfg.debug.cfg.logs(config.ts:889, defaultdebug) is read nowhere except the configure-timefeatures:line (config.ts:1576); C++ never consumed it, so Rust is the only consumer that matters.ENABLE_LOGS.scoped_log!(src/bun_core/output.rs:1550),syslog!(src/sys/lib.rs:4930),mark_binding!(src/bun_core/Global.rs:442),mark_binding()/mark_member_binding()(src/jsc/lib.rs:1384,:1393) and theReachableFilesdump (src/bundler/bundle_v2.rs:1981) gate onenv::IS_DEBUG, with comments asserting it equalsENABLE_LOGS. Fixing the generated constant alone would therefore still log nothing in a non-debug build.release-assertionsgetsENABLE_LOGS = cfg!(bun_debug)and no--cfg=bun_debugin the cargo edge;debug --logs=offgets--cfg=bun_debug.Fix
rust.ts: pass--check-cfg=cfg(bun_logs)always and--cfg=bun_logsexactly whencfg.logs, next tobun_debug/bun_asan.buildOptionsRs.tsemitsENABLE_LOGS = cfg!(bun_logs).Cargo.tomlregisterscfg(bun_logs)inunexpected_cfgslike the other RUSTFLAGS cfgs, so a barecargo checkdoes not warn about the generated file.env::ENABLE_LOGSinstead ofIS_DEBUG;ScopedLogger::is_visible()/log()already did.ENABLE_LOGSgets a doc comment; the comments claiming the two constants are equal are updated.ENABLE_LOGSis the Rust spelling of thelogsoption (the Zig build passed it as-Denable_logs=${cfg.logs}and gatedOutput.Scopedon it), and the option was only meant to default todebug, not to bedebug. The two constants became one value in build: enable Rust debug-assertions for asan/assertions builds; decouple IS_DEBUG #32520, which moved the loggers ontoIS_DEBUGso that release-asan would not pick up logs; they are unchanged by this PR becausecfg.logsis false there. Every profile CI builds (ci-build, with and without--asan=on) and the default debug build resolve to the sameENABLE_LOGSas before (the test pins debug, release and release+asan); onlyrelease-assertionsand explicit--logs=on|offchange, and those are the cases that asked for it.build_options.rs: a barecargo check,cargo clippyand thecargo miri testCI job readbuild/debug/codegen/build_options.rswith no RUSTFLAGS. With a literaltruefrom a debug configure,scoped_log!bodies would go live there andScopedLogger::evaluate_is_visible()scans the environment throughbun_core::strings(Highway FFI), which Miri cannot call;bun_ptr's ref-count paths andbun_shell_parser's brace expansion, both in the Miri crate list, callscoped_log!. A cfg keeps bare cargo on logs-off semantics exactly asbun_debugdoes today.cfg!(bun_logs)without the flag is the samefalseconstant the oldcfg!(bun_debug)was, so theifstill folds away. The new--check-cfgflag changes RUSTFLAGS, so the first build after this lands recompiles the Rust side once.rust.tsalso stops passing-Zlocation-detail=nonewhencfg.logsis set (second commit, found in self-review). That flag was keyed onrelease && !assertions, whichrelease --logs=onsatisfies, and the loggers this PR turns on there includemark_binding()and the test runner'sgroup::begin(), which printLocation::caller(): with the flag they logged[jsc] (<redacted>:0).release-assertionsalready kept the flag off; shipped profiles have logs off and are unchanged.--tinycchas the same kind of bug (ENABLE_TINYCCandtcc_externs!hard-code the target default instead of followingcfg.tinycc) and can use the same--cfgmechanism; it touches the FFI crate and overlaps feat(ffi): enable bun:ffi on FreeBSD #31528, so it is left to a separate change.test/internal/source-lints/build-logs-option.test.ts(new file; this directory keeps one file per build-script topic and nothing existing covers rustflags orbuild_options.rs): resolves release-assertions, release--logs=on, debug--logs=off, debug, release and release+asan configs, generatesbuild_options.rsfor each into a temp dir, reads the cfg it names and checks it against the rustflagscargoBuildInvocation()emits; checks the cfg is registered inCargo.toml; checks the three macros gate onENABLE_LOGS; scanssrc/**/*.rsforIS_DEBUG ... .is_visible()guards; and checks-Zlocation-detail=noneis passed for release but not for release--logs=onor release-assertions (fails with the second commit'srust.tshunk reverted). Withsrc/stashed the two source tests fail (listing the six sites); withscripts/stashed the three changed configs fail (bun_debugis named but not set for release-assertions and--logs=on, set for debug--logs=off); with onlyCargo.tomlstashed the registration test fails; the debug / release / release+asan cases pass in every state, which is the "nothing else moves" claim. 0.3s on a release bun, ~6s underbun bd.bun bd(debug,--cfg=bun_logsnow in the cargo edge):BUN_DEBUG_fs=1 bun bd -e 1,BUN_DEBUG_SYS=1,BUN_DEBUG_JSC=1(mark_binding) andBUN_DEBUG_ReachableFiles=1 bun bd build(thebundle_v2site) all still print.bun scripts/build.ts --profile=release --logs=on(whatbun run build:logsruns) on this branch, on top of areleasetree built from main the day before: withBUN_DEBUG_fs=1the main-builtbun-profileprints nothing and the rebuilt one prints[fs] open(/)...;BUN_DEBUG_JSC=1prints[jsc] (src/jsc/VirtualMachine.rs:2382)... (mark_binding; it was[jsc] (<redacted>:0)before the second commit, rebuilt to confirm);BUN_DEBUG_SYS=1 bun-profile build --compileprints[sys] ioctl_ficlone(6, 5) = -1/[sys] copy_file_range(...)(syslog!, frombun_sys::copy_file). Without anyBUN_DEBUG_*variable the binary stays quiet (BUN_DEBUG_QUIET_LOGS=1in this environment;visiblescopes otherwise print by default, as in debug builds). The strippedbunbehaves the same. Note that some individual call sites are additionally wrapped incfg!(debug_assertions)(Fd::close's[sys] close(...),posix_spawn's), so those still needrelease-assertions; that gating predates this PR.--configure-onlyfor release-assertions, release, release--logs=on, debug, debug--logs=offand release-asan:--cfg=bun_logsappears in exactly the first, third and fourth;--check-cfg=cfg(bun_logs)in all six.rustfmt --checkon the touched.rsfiles;tsc -p scripts/buildreports the same 11 pre-existing diagnostics as main;windows-cross-config,macos-cross-configandbuild-debug-info-flagstests still pass.Background
logsoption: a boolean in the flatConfig(scripts/build/config.ts), defaulting todebug; settable per profile (profiles.ts, onlyrelease-assertionssets it) or with--logs=on|off. It is the TypeScript build's version of the old CMakeENABLE_LOGS/ Zig-Denable_logs.declare_scope!(name, hidden|visible)defines aScopedLogger;scoped_log!writes through it when the scope is enabled at runtime (BUN_DEBUG_<name>=1,BUN_DEBUG_ALL, orvisibleby default). The macro wraps the whole body, including argument evaluation, inif <const> && scope.is_visible()so that builds with the constant false compile the call to nothing; that constant is what this PR changes.build_options.rs: generated at configure time bybuildOptionsRs.tsfromConfigandinclude!d asbun_core::build_options;env.rsre-exports its values asEnvironment::*. A bare cargo invocation (noBUN_CODEGEN_DIR) reads the copy inbuild/debug/codegen/, which is why constants that must follow RUSTFLAGS rather than whichever profile last configured that directory are written ascfg!(...).--cfg/--check-cfg: rustc takes arbitrary cfg names from RUSTFLAGS;--check-cfg=cfg(name)(or theunexpected_cfgsentry inCargo.toml) declares the name so theunexpected_cfgslint does not fire oncfg!(name)in builds that do not set it.rust.tsalready wiresbun_asan,bun_debug,bun_codegen_embedandsocket_fault_injectionthis way.IS_DEBUG(cfg!(bun_debug)): true only for thedevcargo profile; it gates Debug-build conveniences (debug_warn!,DUMP_SOURCE, thebun-debugname). It stays as is; logging is the one thing moved off it.Configure probes on main vs this branch
This branch (
ENABLE_LOGS = cfg!(bun_logs)in every build_options.rs;--check-cfg=cfg(bun_logs)in every build.ninja):The source scan's output with
src/at main: