Add MSRV testing to CI and Justfile for dev tasks#113
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
WalkthroughBumps workspace MSRV to 1.83, converts several crates to workspace-pinned dependencies, restructures CI into separate fmt/clippy/build/test jobs, adds a Justfile with developer tasks, and updates README to reflect MSRV and new dev workflow. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (1)
401-416: Consider mentioningjust --listfor discoverability.The documented tasks cover the essentials, but the Justfile also includes
bench,fuzz, andmsrv-check. A note aboutjust --listwould help developers discover all available tasks.📝 Suggested addition
just ci # Run full CI checks locally
+Run
just --listto see all available tasks.</details> </blockquote></details> <details> <summary>Justfile (1)</summary><blockquote> `14-16`: **Consider adding `--all-targets` to clippy.** This would also lint tests, examples, and benches, catching more issues during local development. <details> <summary>📝 Suggested improvement</summary> ```diff lint: cargo fmt -- --check - cargo clippy -- -D warnings + cargo clippy --all-targets -- -D warnings
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.ymlCargo.tomlJustfileREADME.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (nightly)
- GitHub Check: test (stable)
- GitHub Check: build
🔇 Additional comments (13)
Cargo.toml (1)
21-21: LGTM!The MSRV declaration is correctly placed under
[workspace.package]and is consistent with the CI matrix (1.81.0), Justfile (msrv := "1.81"), and README documentation.README.md (3)
28-28: LGTM!MSRV requirement is clearly stated and consistent with
Cargo.tomland CI configuration.
418-422: LGTM!Clear and practical debug logging example.
424-427: LGTM!Clear MSRV policy with appropriate breaking-change communication commitment.
Justfile (4)
1-6: LGTM!Shell configuration with
-ucflags (errexit undefined variable checking + pipefail) and the default task for discoverability are good practices.
32-33: LGTM!Good use of
--all-targetsfor comprehensive MSRV checking. Note that this requires the 1.81 toolchain to be installed (rustup toolchain install 1.81).
8-12: LGTM!Standard build and test targets.
27-30: LGTM!The fuzz target correctly uses nightly toolchain. The
ciaggregation has a sensible order: lint catches formatting/style issues first, then tests verify correctness, and finally build confirms release compilation..github/workflows/ci.yml (5)
9-10: LGTM!
CARGO_TERM_COLOR: alwaysensures readable colored output in CI logs.
13-20: LGTM!The fmt job is correctly minimal—no build dependencies needed for formatting checks.
22-30: LGTM!Clippy job correctly uses
-D warningsto fail on warnings. Consistent with the Justfile lint target.
32-38: LGTM!Standard release build job with required system dependencies.
40-53: LGTM!Well-structured test matrix:
- MSRV (1.81.0) matches
Cargo.tomland Justfilefail-fast: falseensures all toolchains are testedcontinue-on-errorfor nightly prevents unstable compiler issues from blocking merges- Using
@masterwith explicittoolchainparameter is the correct pattern for dynamic toolchain selection
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
keep-core/Cargo.toml (1)
25-25: Consider adding a comment for consistency.The other workspace pins (lines 11-13) have
# Pin for MSRV compatibilitycomments. Adding a similar comment here would maintain consistency and clarify whyredbis workspace-managed.Suggested change
-redb.workspace = true +redb.workspace = true # Pin for MSRV compatibility.github/workflows/ci.yml (3)
30-30: Consider adding--all-targetsto also lint tests and examples.The current command only lints the library/binary targets. Adding
--all-targetsensures clippy also checks test code, examples, and benchmarks for issues.♻️ Suggested improvement
- - run: cargo clippy -- -D warnings + - run: cargo clippy --all-targets -- -D warnings
32-53: Consider adding Rust caching to speed up CI.Rust builds can be slow without caching. Adding
Swatinem/rust-cachebefore the build/test steps can significantly reduce CI times, especially for the test matrix which runs three separate builds.♻️ Suggested improvement for build job
- uses: dtolnay/rust-toolchain@stable + - uses: Swatinem/rust-cache@v2 - run: cargo build --release♻️ Suggested improvement for test job
- uses: dtolnay/rust-toolchain@master with: toolchain: ${{ matrix.rust }} + - uses: Swatinem/rust-cache@v2 - run: cargo test
22-53: Consider adding job dependencies to fail fast on lint errors.Currently all jobs run in parallel. Adding
needs: [fmt, clippy]tobuildandtestjobs would prevent running expensive builds when formatting or linting fails, saving CI minutes.♻️ Suggested improvement
build: runs-on: ubuntu-latest + needs: [fmt, clippy] steps: ... test: runs-on: ubuntu-latest + needs: [fmt, clippy] strategy: ...
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/ci.ymlCargo.tomlJustfilekeep-core/Cargo.tomlkeep-enclave/host/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- Cargo.toml
- Justfile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (nightly)
- GitHub Check: build
- GitHub Check: test (stable)
🔇 Additional comments (7)
keep-enclave/host/Cargo.toml (1)
32-32: LGTM!Moving
redbto a workspace-managed dependency ensures consistent versioning across the workspace and aligns with the MSRV compatibility strategy. This is a clean change.keep-core/Cargo.toml (2)
5-5: LGTM!MSRV bump to 1.83 aligns with the PR objectives and ensures compatibility with the edition 2024-related dependencies.
11-13: LGTM!The workspace pins for
base64ct,blake3, andconstant_time_eqwith inline comments explaining the MSRV compatibility reasoning is good practice. This makes it clear to future maintainers why these are pinned at the workspace level..github/workflows/ci.yml (4)
9-11: LGTM!Good addition for improved CI log readability.
13-20: LGTM!Clean separation of the fmt job. Correctly omits system dependencies since formatting doesn't require compilation.
32-38: LGTM!Release build verification with stable is appropriate. The test matrix covers build compatibility for MSRV and nightly.
40-53: Test matrix looks good!The matrix correctly covers MSRV (1.83.0), stable, and nightly. Using
fail-fast: falseandcontinue-on-errorfor nightly follows best practices for Rust CI.
kwsantiago
force-pushed
the
MSRV-and-Justfile
branch
from
993cd8a to
fe84173
Compare
2 participants
Summary by CodeRabbit
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.