Move clippyand cargo-shear to separated jobs#7741
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request refactors CI workflow configuration by extracting ChangesCI Job Reorganization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
ShaharNaveh
force-pushed
the
cargo-clippy-once
branch
from
517f450 to
7780e73
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 372-374: The new GitHub Actions job "clippy" was extracted from
the "rust_tests" job but lost the skip condition, so add the same guard to the
"clippy" job to preserve skip:ci behavior; update the "clippy" job definition
(job name: clippy) to include the if condition that checks the commit message
for "skip:ci" (the same boolean expression used on the original rust_tests job)
so the clippy job is skipped when that token is present.
- Around line 404-407: The ci workflow uses an invalid cargo clippy flag
`--all-target`; update the `cargo clippy` invocation to use the correct flag
`--all-targets` (replace `--all-target` with `--all-targets`) so the job runs
successfully; locate the block containing the `cargo clippy` command in the CI
YAML and make this single-word replacement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 133f184a-876c-4521-b409-aefdd2c36147
📒 Files selected for processing (1)
.github/workflows/ci.yaml
| clippy: | ||
| name: clippy | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Preserve skip:ci behavior on the new clippy job.
clippy was moved out of rust_tests (which had the skip:ci condition), but this new job has no equivalent guard, so it still runs when skip:ci is set.
Suggested fix
clippy:
+ if: ${{ !contains(github.event.pull_request.labels.*.name, 'skip:ci') }}
name: clippy
runs-on: ubuntu-latest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yaml around lines 372 - 374, The new GitHub Actions job
"clippy" was extracted from the "rust_tests" job but lost the skip condition, so
add the same guard to the "clippy" job to preserve skip:ci behavior; update the
"clippy" job definition (job name: clippy) to include the if condition that
checks the commit message for "skip:ci" (the same boolean expression used on the
original rust_tests job) so the clippy job is skipped when that token is
present.
There was a problem hiding this comment.
This is fine. we always want to run clippy if the rust code changed
There was a problem hiding this comment.
@ShaharNaveh, understood — the clippy job is intentionally always-on regardless of the skip:ci label. Apologies for the noise!
✏️ Learnings added
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 7741
File: .github/workflows/ci.yaml:372-374
Timestamp: 2026-04-30T10:24:04.038Z
Learning: In RustPython/.github/workflows/ci.yaml, the `clippy` job intentionally does NOT have the `skip:ci` guard (`if: ${{ !contains(github.event.pull_request.labels.*.name, 'skip:ci') }}`). Clippy (and the `lint` job) are always meant to run, even when the `skip:ci` label is applied to a PR. Do not flag the missing guard on these jobs.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-06T05:30:53.056Z
Learning: Applies to **/*.rs : Always run clippy to lint Rust code (`cargo clippy`) before completing tasks and fix any warnings or lints that are introduced by your changes
Learnt from: reactive-firewall
Repo: RustPython/RustPython PR: 0
File: :0-0
Timestamp: 2026-01-19T20:10:58.864Z
Learning: In the RustPython project's CI/CD workflows (PR `#6176`), bash scripts are preferred over Python or Rust for test orchestration and result aggregation to maintain hermetic testing—avoiding circular dependencies where Python tools would depend on the RustPython interpreter being tested, and avoiding visibility loss into Rust runtime issues when using Rust tooling.
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 5932
File: .github/workflows/comment-commands.yml:18-24
Timestamp: 2025-07-10T10:08:43.330Z
Learning: In GitHub Actions workflows for the RustPython project, the maintainer ShaharNaveh prefers to keep workflows simple and doesn't mind if steps fail when the desired state is already achieved (e.g., user already assigned to an issue). Avoid suggesting complex error handling for edge cases they don't consider problematic.
|
I'm wondering under what circumstances is it possible that Clippy would fail on non-Ubuntu but pass on Ubuntu. |
I don't think there's. All the big rust projects that I saw are using I still haven't seen clippy pass on one platform and fails on the other |
In that case, we might not need to run Clippy specifically on WASM. |
ShaharNaveh
force-pushed
the
cargo-clippy-once
branch
2 times, most recently
from
84e001d to
025f9c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 468-476: The Clippy step using a multi-line "cargo clippy \"
invocation will fail on windows-latest because PowerShell uses backtick for
continuation; update the "Clippy" job step to ensure it runs under Bash by
adding "shell: bash" to the step (or alternatively collapse the cargo clippy
flags onto a single line) so the "${{ env.CARGO_ARGS }} ... -- -Dwarnings"
continuation is interpreted correctly across runners.
- Around line 478-485: The cargo_shear job references
needs.determine_changes.outputs.rust_code in its if: conditional but does not
declare a needs dependency, so the job will never evaluate that output; update
the cargo_shear job (job name: cargo_shear) to include needs:
[determine_changes] so it can access determine_changes outputs (same fix applied
earlier to clippy): add the needs entry under the cargo_shear job definition to
ensure the conditional runs on PRs and main branch as intended.
- Around line 430-437: The clippy job is missing a needs declaration so its if
expression referencing needs.determine_changes.outputs.rust_code is always
empty; add needs: [determine_changes] to the clippy job (same as cargo_check) so
the needs context exists and the if condition
(needs.determine_changes.outputs.rust_code == 'true' || github.ref ==
'refs/heads/main') evaluates correctly for PRs and main pushes; update the job
block named clippy to include needs: [determine_changes].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d0eddefa-3618-4697-b758-4f1b2235eed5
📒 Files selected for processing (2)
.github/workflows/ci.yamlcrates/stdlib/src/openssl.rs
✅ Files skipped from review due to trivial changes (1)
- crates/stdlib/src/openssl.rs
ShaharNaveh
force-pushed
the
cargo-clippy-once
branch
from
025f9c9 to
731dc2a
Compare
ShaharNaveh
changed the title
clippyand cargo-shear to separated jobs
After messing with clippy recently, I've saw it happend multiple times. So, I've made it to a dedicated step that will run conditionally if any rust related code was changed |
ShaharNaveh
force-pushed
the
cargo-clippy-once
branch
from
7d6ebcd to
bde4b06
Compare
ShaharNaveh
force-pushed
the
cargo-clippy-once
branch
from
bde4b06 to
2024fd9
Compare
3 participants
ATM we are running clippy 3 times (on Linux, macOS and Windows), it's better to run it once on Linux and give it the--all-targets --all-featuresflagsMove to a separate step so we will trigger it when rust related code changes
Summary by CodeRabbit