Repository navigation
cargo-oxide: fmt must not build the codegen backend - #1188
Conversation
`cargo oxide fmt` resolved its context with `resolve_context`, which calls `backend::find_or_build_backend`: on a checkout with no cached `librustc_codegen_cuda.so` the command compiled the entire codegen backend -- and in standalone mode cloned it first -- before formatting a single file. Formatting compiles no device code. `format_all` reads only `workspace_root`, `codegen_crate` and `examples_dir`; neither it nor `run_cargo_fmt`, `run_cargo_fmt_manifest`, `run_fmt_command` nor `collect_example_manifests` takes a `Context` at all, so `backend_so` is unreachable from this path. `resolve_passive_context` fills those three fields identically in every discovery branch, so the eager resolver bought nothing. `doctor`, `clean`, `list` and `update` already resolve passively for the same reason; `fmt` was the one read-only command left resolving eagerly. Its doc comment now names it alongside them. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
6009af3 to
88d3190
Compare
|
Rebased onto Re-verified the claims against the refactored tree rather than assuming they survived: Gates green on the rebase: 185 tests, clippy clean, |
|
Reviewed and validated end to end; ready as-is. The one-line switch to Reproduced on a fresh checkout (no cached backend On the open question you flagged (invalid config now warns instead of aborting): we're accepting that posture.
Also agree with the no-scan-test call; we closed #1050 and #1133 for exactly the green-while-wrong problem. Gates on the merged tree: 189 tests, clippy |
Closes #1187.
cargo oxide fmtresolved its context withresolve_context(), which callsbackend::find_or_build_backend. On a checkout with no cachedlibrustc_codegen_cuda.sothat compiled the whole codegen backend before formatting a single file; in standalone mode it cloned the backend first. Formatting compiles no device code, so none of it was used.What changed
The
Fmtarm resolves passively, andresolve_passive_context's doc nameslistandfmtalongside the commands it already listed.Root cause across the full path
format_allreads exactly three fields ofContext—workspace_root,codegen_crate,examples_dir— and neverbackend_soorconfig. Its call graph is closed againstContext:run_cargo_fmt,run_cargo_fmt_manifest,run_fmt_commandandcollect_example_manifestsall take&Path/&mut Vec<PathBuf>, so the backend is not reachable from this path even indirectly.Both resolvers agree on those three fields in every discovery branch:
workspace_rootcodegen_crateexamples_dirfind_workspace_root()root/crates/rustc-codegen-cudacodegen_crate/examplescwdcwdcwdexit(1)in bothAll eleven
resolve_context()call sites were classified.run,sanitize,build,fuzz-schedule,test,emit-ltoir,pipeline,inspect,debugandsetupcompile device code and legitimately build.list,clean,doctorandupdatealready resolve passively.fmtwas the only read-only command still resolving eagerly.Before / after
Workspace checkout with the host-build backend moved aside:
Before
After
The 16.94s is with the rest of the workspace already compiled; from a cold
target/it is minutes, and standalone mode adds a clone. Both runs above were cut off during the examples walk — the point of the comparison is the eight lines before📦 Checking root workspace..., not total runtime.Controls
Negative — unformatted code is still caught. Added a badly formatted file to a scaffolded project:
then
cargo oxide fmt(write mode) →✅ All crates formatted, and--checkagain →✅ All files are properly formatted, exit=0.Unaffected — device-codegen commands still build. With the backend absent,
cargo oxide build array_indexstill printsBuilding rustc-codegen-cuda backend.../✓ Backend built. The fix is not "make everything passive".Unaffected — standalone mode still formats. A project from
cargo oxide newformats and checks correctly through the passive resolver.One behaviour change, deliberate
The passive resolver uses
load_oxide_config_lenient, so an invalid.cargo/cuda-oxide.tomlnow warns instead of exiting:format_allnever readsctx.config, so nothing about what gets formatted changes. This matches the posturedoctor,clean,listandupdatealready have, and it seems right that formatting should not be gated on backend config — but it is a real delta, so flagging it rather than burying it. Happy to keep the strict loader forfmtif you would rather the change be strictly the backend build.On not adding a dispatch guard test
The obvious guard — a test that reads
main.rsand asserts theFmtarm calls the passive resolver — is the shape you closed #1050 and #1133 for, and correctly: it passes on the text while the property can be false, since an arm could reach the backend through a helper or callfind_or_build_backenddirectly. A behavioural test is not available either, becauseformat_allshells out tocargo fmtand callsprocess::exit(1)on failure, so it would pass or fail on whether the host's default toolchain has rustfmt.So this follows the tree's existing convention for the same decision:
doctor,clean,listandupdateeach carry a comment on the arm and no guard, and the one-line choice is verified by reading it. If you want the invariant enforced, I think it needs one executable source for "which commands may build the backend" rather than a scan, and I would rather agree that design with you first than ship a check that can be green while wrong.Scope
cargo-oxideitself, which this does not change; it removes the larger cost stacked on top.Test plan
cargo test -p cargo-oxide— 185 passed, 0 failed.cargo clippy -p cargo-oxide --all-targets -- -D warnings— clean.cargo fmt --check— clean.RUSTDOCFLAGS="-D warnings" cargo doc --no-deps -p cargo-oxide— clean.check-cli-doc-coverage,check-crate-inventory,check-spdx-headers,check-toolchain-parity,check-test-matrix-coverage,check-error-example-status,check-example-license-policy,check-reserved-prefixes,check-book-api-names,check-book-catalog-stamp— all pass.