fix(pre-commit): rustfmt a virtual workspace root - #363
Conversation
2455992 moved the hook to per-manifest --manifest-path so a crate one level below the repo root gets formatted. That broke every repo whose root manifest is a virtual workspace: cargo-fmt finds no targets for a manifest with no [package] and exits 1 with "Failed to find targets". rain.metadata is red on rs-static for this reason, on PRs and on main, and its ci-gate refuses to publish while rainix-rs is failing. --all formats the workspace members instead of the root package's own targets, which is what the hook wants in both layouts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe ChangesWorkspace Rust formatting
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This localized change updates formatting for virtual workspaces while preserving single-package behavior; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
What is broken
2455992dchanged therustfmt-conditionalhook to format each manifest via--manifest-path, so a crate one level below the repo root gets formatted instead of cargo erroring on a manifest-less root.That broke the other layout. A virtual workspace root —
[workspace]with members and no[package]— has no targets of its own, socargo-fmt fmt --manifest-path Cargo.tomlexits 1 with:The
*/Cargo.tomlarm does not save it when the members live deeper, e.g.crates/*/Cargo.toml.Impact
rainlanguage/rain.metadata— rootCargo.tomlis[workspace]overcrates/bindings,crates/cli,crates/metaboard— is red onrainix-rs / static / rs-staticon every PR and onmain, with no formatting problem at all: clippy passes in that same job before the hook runs.That repo's
ci-gatereads the lane, sorelease / releaseon main fails withci-gate: refusing to publish <sha> — rainix-rs (.github/workflows/rainix-rs.yaml) concluded failure. The repo cannot publish until this hook is fixed.The fix
--allformats the workspace members rather than the root package's own targets. That is what the hook wants in both layouts, and it is a no-op for a single-package manifest.QA
git-hooks.nixhook body, so there is no test that could fail on base. The hook is verified by running its exact command line against a virtual-workspace repo, below. A hook-execution harness is a larger change than this one-flag fix and belongs in its own issue.cargo-fmt fmt --all --manifest-path "$manifest"-> drop--all(the base as of 2455992) -> exits 1 withFailed to find targetsagainst rain.metadata's virtual root; restored -> exits 0. Run in rain.metadata's dev shell at 45ca96c:cargo-fmt fmt --all --manifest-path Cargo.toml --checkgives rc=0,cargo-fmt fmt --manifest-path Cargo.toml --checkgives rc=1 andFailed to find targets. The same tree passescargo fmt --all --checkat rc=0 across five separate merge commits, so the lane's red X was never a formatting diff.--allformats all workspace packages; a manifest with no[package]has no targets to find), plus the CI job log forrainix-rs / static / rs-staticon rain.metadata main at 45ca96c (run 32862493917), which shows every other pre-commit hook passing andrustfmt-conditionalalone exiting 1 onFailed to find targets.--allon a member manifest formats that member.🤖 Generated with Claude Code
Summary by CodeRabbit