Skip to content

Enforce rustfmt and clippy in CI - #238

Draft
tschm wants to merge 3 commits into
oxfordcontrol:mainfrom
tschm:fix/ci-lint-enforcement
Draft

Enforce rustfmt and clippy in CI#238
tschm wants to merge 3 commits into
oxfordcontrol:mainfrom
tschm:fix/ci-lint-enforcement

Conversation

@tschm

@tschm tschm commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Closes #236.

rustfmt.toml declared a formatting standard for this crate, but no CI job enforced it, so the tree had drifted to 62 cargo fmt --check diffs across 9 files in src/algebra/dense/. Clippy was likewise never run in CI, and 4 warnings were shipping.

Changes

1. A lint job in ci.yml running cargo fmt --all -- --check and cargo clippy --all-targets -- -D warnings. It runs on macos-latest to match the existing build job, since sdp-accelerate is macOS-only.

2. The 4 warnings, fixed at source:

File Lint Fix
datamaps.rs:26 mismatched_lifetime_syntaxes SparseExpansionCone<T>SparseExpansionCone<'_, T>
supportedcone.rs:78 useless_borrows_in_formatting dropped the redundant & in write!
reverse_compact.rs:48 unnecessary_unwrap is_none()/unwrap()if let Some((tree_index, clique_index))
augment_compact.rs:563 explicit_counter_loop manual counter → .step_by(2).enumerate()

3. cargo fmt --all across the 9 drifted files.

One thing reviewers should know

cargo fmt --all alone does not make --check pass here. rustfmt reformats the tree but then exits 1 with error[internal]: left behind trailing whitespace on 6 lines inside #[rustfmt::skip] blocks — regions it refuses to rewrite yet still reports on. Those are stripped by hand in symv.rs (32, 33, 44, 45) and syr2k.rs (54, 55). Without that step the new lint job fails on a tree cargo fmt has just formatted.

Safety of the reflow

The src/algebra/dense/ changes are formatting only. Comparing each file against main with whitespace and commas stripped, 7 of 9 are byte-identical; the two that aren't (dense2x2/svd.rs, dense3x3/svd.rs) differ solely by rustfmt's default reorder_imports moving use crate::algebra::*;. No numeric literal is altered.

All four source fixes are MSRV-safe — '_ in path position has been stable since 1.31 — so the 1.70.0 leg of msrv.yml is unaffected.

Verification

cargo fmt --all -- --check                                    exit 0
cargo clippy --all-targets --features sdp-... -- -D warnings  exit 0
cargo clippy --all-targets -- -D warnings   (default feats)   exit 0
cargo test --features sdp-accelerate,faer-sparse,serde        235 passed, 0 failed, 0 ignored

Possible follow-ups (not in this PR)

  • The lint job uses macOS runners to reach sdp-accelerate; ubuntu with sdp-netlib would be cheaper.
  • Clippy covers the CI feature set but not python/julia, which would need a PyO3 toolchain in that job.

🤖 Generated with Claude Code

Closes oxfordcontrol#236.

`rustfmt.toml` declared a formatting standard that no CI job enforced,
so the tree had drifted to 62 `cargo fmt --check` diffs across 9 files
in src/algebra/dense/. Clippy was likewise never run, and 4 warnings
were shipping.

Add a `lint` job to ci.yml running `cargo fmt --all -- --check` and
`cargo clippy --all-targets -- -D warnings`. It runs on macos-latest to
match the existing build job, since sdp-accelerate is macOS-only.

Fix the 4 warnings at source:

  * datamaps.rs        mismatched_lifetime_syntaxes: SparseExpansionCone<'_, T>
  * supportedcone.rs   useless_borrows_in_formatting: drop the & in write!
  * reverse_compact.rs unnecessary_unwrap: is_none()/unwrap() -> if let Some(..)
  * augment_compact.rs explicit_counter_loop: .step_by(2).enumerate()

Then run `cargo fmt --all`. Note that this alone does not make --check
pass: rustfmt reformats the tree but still exits 1 with
"left behind trailing whitespace" on 6 lines inside #[rustfmt::skip]
blocks, which it will not rewrite. Those are stripped by hand in
symv.rs and syr2k.rs.

All source fixes are MSRV-safe ('_ in path position is stable since
1.31), so the 1.70.0 leg of msrv.yml is unaffected. The algebra changes
are pure reflow plus rustfmt's default import reordering; no numeric
literal is altered.

Verified: fmt, clippy (default and CI feature sets) all exit 0;
cargo test --features sdp-accelerate,faer-sparse,serde passes 235/235.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.13%. Comparing base (b6aa042) to head (aa369cb).

Files with missing lines Patch % Lines
src/solver/core/cones/supportedcone.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #238      +/-   ##
==========================================
+ Coverage   92.85%   93.13%   +0.28%     
==========================================
  Files          90       72      -18     
  Lines        7278     5743    -1535     
==========================================
- Hits         6758     5349    -1409     
+ Misses        520      394     -126     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The MSRV job has been failing on main since before this branch:

    error: package `serde_json v1.0.151` cannot be built because it
    requires rustc 1.71 or newer, while the currently active rustc
    version is 1.70.0

Cargo.lock is gitignored, so every CI run resolves dependencies afresh
and picks up releases that have raised their own MSRV past the 1.70.0
declared in Cargo.toml. It is not just serde_json: proc-macro2 >=1.0.107,
unicode-ident >=1.0.23 and syn 3.x have all moved to 1.71, so pinning
crates one at a time produces a long list that rots on the next release.

Instead, resolve the 1.70.0 leg with a current cargo under MSRV-aware
resolution (CARGO_RESOLVER_INCOMPATIBLE_RUST_VERSIONS=fallback), which
honours rust-version from Cargo.toml, then build and test with the pinned
toolchain. One explicit pin remains: serde requires serde_derive with an
exact `=` version, and serde_derive 1.0.229 depends on syn 3.x, which the
fallback resolver cannot see through.

The stable and beta legs are untouched and still resolve freely.

Verified locally against a real 1.70.0 toolchain: cargo build succeeds and
cargo test passes 182/182 on default features.

This is an unrelated pre-existing failure that blocks every PR to this
repo; it is included here so the branch can go green, and can be split
into its own PR if preferred.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tschm

tschm commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Pushed a second commit (96f56dd) fixing an unrelated, pre-existing CI failure that was blocking this branch — and every other PR to this repo.

The MSRV job was failing before this branch existed (the last run on main also concluded failure):

error: package `serde_json v1.0.151` cannot be built because it requires
rustc 1.71 or newer, while the currently active rustc version is 1.70.0

Cargo.lock is gitignored, so each CI run resolves dependencies afresh and picks up releases that have raised their own MSRV past the rust-version = "1.70.0" declared in Cargo.toml. It is not only serde_json — I reproduced it against a real 1.70.0 toolchain locally and walked the chain: serde_json >=1.0.150, proc-macro2 >=1.0.107, unicode-ident >=1.0.23 and syn 3.x have all moved to 1.71. Pinning crate-by-crate would produce a long list that rots on the next release.

Instead the 1.70.0 leg now resolves with a current cargo under MSRV-aware resolution (CARGO_RESOLVER_INCOMPATIBLE_RUST_VERSIONS=fallback), which honours rust-version from Cargo.toml, then builds and tests with the pinned toolchain. One explicit pin remains and is commented in the workflow: serde requires serde_derive with an exact = version, and serde_derive 1.0.229 depends on syn 3.x, which the fallback resolver cannot see through.

Verified locally against an installed 1.70.0 toolchain: cargo build succeeds and cargo test passes 182/182 on default features. CI now confirms it — the 1.70.0, stable and beta legs all pass.

The stable and beta legs are untouched and still resolve freely.

Happy to split this into its own PR if you would rather keep this one to the lint change — it is a self-contained commit. The long-term fix is arguably to raise the declared MSRV to 1.71, but that is a semver-relevant policy call for maintainers, so I have not made it here.

The macos-13 runner image was retired on 2025-12-04. Both wheel
workflows still request that label, so those jobs are never assigned a
runner: they sit queued with an empty runner_name until the six-hour
timeout, and no PR gated on that check can go green.

The last macos-13 job in this repo that actually completed ran on
2025-11-24, days before the shutdown. Everything since is cancelled or
stuck queued.

macos-15-intel is GitHub's replacement x86_64 image, available until
August 2027, after which Actions drops x86_64 macOS entirely and the
leg will have to go.

This matters beyond CI going green: the release job depends on
build-wheels, so the next tagged release would hang on the Intel wheel.
Version 0.11.1 ships clarabel-0.11.1-cp39-abi3-macosx_10_12_x86_64.whl,
uploaded 2025-06-11 -- before the retirement -- so no release has hit
this yet, and dropping the leg instead would silently stop publishing a
wheel that Intel-mac users currently install.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

Enforce rustfmt and clippy in CI (62 format diffs, 4 warnings currently shipping)

1 participant