Unify lint CI job - #7505
Unify lint CI job#7505
Conversation
| - name: check compilation without threading | ||
| run: cargo check ${{ env.CARGO_ARGS }} | ||
|
|
||
| - run: cargo doc --locked |
There was a problem hiding this comment.
Moved to here because cargo check and cargo doc uses the same profile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.pre-commit-config.yaml (2)
24-31: Consider addingpass_filenames: falsefor consistency.The script
scripts/check_redundant_patches.pyignores CLI arguments and always scans the entireLib/testdirectory (see lines 10-12 of the script). The currenttypes_or: [python]setting passes filenames that are never used.For clarity, consider:
♻️ Suggested improvement
- id: redundant-test-patches name: Ensure no redundant test patches entry: python scripts/check_redundant_patches.py language: system - types_or: [python] + files: '^Lib/test/.*\.py$' + pass_filenames: false priority: 0This makes the trigger more explicit (only run when test files change) and documents that filenames aren't used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml around lines 24 - 31, The hook redundant-test-patches uses scripts/check_redundant_patches.py which ignores passed filenames, so update the hook definition for id "redundant-test-patches" to explicitly set pass_filenames: false and keep the existing entry ("python scripts/check_redundant_patches.py") so pre-commit won't pass unused file args to that script; this makes the hook intent explicit and avoids misleading invocation semantics.
43-49: Add file filter to only trigger on relevant changes.This hook runs on every commit since there's no
filesortypesfilter. The script only needs to run whencrates/compiler-core/src/bytecode/instruction.rschanges.♻️ Suggested improvement
- id: generate-opcode-metadata name: Generate opcode metadata entry: python scripts/generate_opcode_metadata.py pass_filenames: false language: system + files: 'crates/compiler-core/src/bytecode/instruction\.rs$' require_serial: true # so rustfmt runs first priority: 1This avoids unnecessary script execution and potential git status changes when the bytecode file hasn't been modified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.pre-commit-config.yaml around lines 43 - 49, The pre-commit hook with id "generate-opcode-metadata" currently runs on every commit; update its configuration in .pre-commit-config.yaml to add a files filter that matches the bytecode source file (crates/compiler-core/src/bytecode/instruction.rs) so the hook only triggers when that file changes; locate the hook by the id/name ("generate-opcode-metadata" / "Generate opcode metadata") and add an appropriate files entry (or types entry) to restrict execution to that path..github/workflows/ci.yaml (1)
355-358: Consider pinning cargo-shear version for reproducibility.The
cargo binstall --no-confirm cargo-shearcommand installs the latest version, which could cause unexpected CI failures if a new version introduces breaking changes or new lints.♻️ Suggested improvement
- name: cargo shear run: | - cargo binstall --no-confirm cargo-shear + cargo binstall --no-confirm cargo-shear@1.1.3 cargo shearAlternatively, Dependabot could manage this if cargo-shear is added as a dev-dependency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 355 - 358, Pin the cargo-shear installation in the "cargo shear" workflow step instead of installing the latest: replace the current installer invocation (cargo binstall --no-confirm cargo-shear) with a pinned installer command that specifies a known-good version (e.g., cargo-shear@<version>), or add cargo-shear as a dev-dependency and let Dependabot manage updates; update the workflow step that runs "cargo shear" to rely on that pinned installation so CI remains reproducible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 355-358: Pin the cargo-shear installation in the "cargo shear"
workflow step instead of installing the latest: replace the current installer
invocation (cargo binstall --no-confirm cargo-shear) with a pinned installer
command that specifies a known-good version (e.g., cargo-shear@<version>), or
add cargo-shear as a dev-dependency and let Dependabot manage updates; update
the workflow step that runs "cargo shear" to rely on that pinned installation so
CI remains reproducible.
In @.pre-commit-config.yaml:
- Around line 24-31: The hook redundant-test-patches uses
scripts/check_redundant_patches.py which ignores passed filenames, so update the
hook definition for id "redundant-test-patches" to explicitly set
pass_filenames: false and keep the existing entry ("python
scripts/check_redundant_patches.py") so pre-commit won't pass unused file args
to that script; this makes the hook intent explicit and avoids misleading
invocation semantics.
- Around line 43-49: The pre-commit hook with id "generate-opcode-metadata"
currently runs on every commit; update its configuration in
.pre-commit-config.yaml to add a files filter that matches the bytecode source
file (crates/compiler-core/src/bytecode/instruction.rs) so the hook only
triggers when that file changes; locate the hook by the id/name
("generate-opcode-metadata" / "Generate opcode metadata") and add an appropriate
files entry (or types entry) to restrict execution to that path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 98cc673b-3373-4fa2-aea3-bdbbc11edfd3
📒 Files selected for processing (4)
.github/dependabot.yml.github/workflows/ci.yaml.github/workflows/pr-format.yaml.pre-commit-config.yaml
💤 Files with no reviewable changes (1)
- .github/workflows/pr-format.yaml
a8a9358 to
89d4a0b
Compare
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/operator.py dependencies:
dependent tests: (43 tests)
Legend:
|
41c8e3a to
1796ab9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/check_redundant_patches.py`:
- Line 16: The function build_argparser has an incorrect return type annotation
that calls the constructor; change the annotation from argparse.ArgumentParser()
to the class type argparse.ArgumentParser (no parentheses) and keep the function
body returning a new instance as before; update the signature for
build_argparser accordingly so the type hint refers to the class, not an
instantiated object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 54e4bb07-26cf-48f8-994e-80f69b4f2b7d
⛔ Files ignored due to path filters (1)
Lib/test/test_operator.pyis excluded by!Lib/**
📒 Files selected for processing (5)
.github/dependabot.yml.github/workflows/ci.yaml.github/workflows/pr-format.yaml.pre-commit-config.yamlscripts/check_redundant_patches.py
💤 Files with no reviewable changes (1)
- .github/workflows/pr-format.yaml
✅ Files skipped from review due to trivial changes (2)
- .github/dependabot.yml
- .pre-commit-config.yaml
a0a0172 to
14e04ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 355-358: The workflow currently installs the latest cargo-shear
which can break CI; update the step that runs "cargo binstall --no-confirm
cargo-shear" to pin cargo-shear to an exact version using the cargo-binstall
syntax (e.g., "cargo-shear@=x.y.z"); modify the step named "cargo shear" so the
binstall command references the pinned package name and leave the subsequent
"cargo shear" invocation unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c3be1671-bb45-4158-82ed-2ae52e09aaa3
⛔ Files ignored due to path filters (1)
Lib/test/test_operator.pyis excluded by!Lib/**
📒 Files selected for processing (3)
.github/workflows/ci.yaml.pre-commit-config.yamlscripts/check_redundant_patches.py
✅ Files skipped from review due to trivial changes (2)
- .pre-commit-config.yaml
- scripts/check_redundant_patches.py
| - name: cargo shear | ||
| run: | | ||
| python scripts/generate_opcode_metadata.py | ||
| if [ -n "$(git status --porcelain)" ]; then | ||
| exit 1 | ||
| fi | ||
| cargo binstall --no-confirm cargo-shear | ||
| cargo shear |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking current cargo-shear install command:"
rg -n 'cargo binstall --no-confirm .*cargo-shear' .github/workflows/ci.yamlRepository: RustPython/RustPython
Length of output: 164
🏁 Script executed:
# Check the context around the cargo-shear installation and other tool installations
head -360 .github/workflows/ci.yaml | tail -30Repository: RustPython/RustPython
Length of output: 919
🏁 Script executed:
# Look for other cargo binstall or tool installation commands to see if versions are pinned elsewhere
rg -n 'cargo binstall|curl.*install|wget' .github/workflows/ci.yaml -A 1 | head -50Repository: RustPython/RustPython
Length of output: 426
🏁 Script executed:
# Check if there's a Cargo.lock or similar that might indicate version management strategy
git ls-files | grep -E '(Cargo|package.*lock|requirements)'Repository: RustPython/RustPython
Length of output: 822
🏁 Script executed:
# Check if there are any examples of pinned versions with cargo binstall in the repo
rg -n 'cargo binstall.*@' .Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Look at cargo-binstall documentation - check if the repo has any docs or comments about it
rg -n 'cargo-binstall' .github/workflows/ci.yaml -B 2 -A 2Repository: RustPython/RustPython
Length of output: 233
🏁 Script executed:
# Search for any references to cargo-shear version pinning or issues
rg -n 'cargo-shear' .Repository: RustPython/RustPython
Length of output: 123
🌐 Web query:
cargo binstall version pinning syntax @
💡 Result:
The version pinning syntax in cargo-binstall uses @ followed by a Cargo version requirement string. For exact versions, use @=VERSION, such as cargo binstall radio-sx128x@=0.14.1-alpha.5. This resolves using Cargo's semver parser, where =0.14.1-alpha.5 pins to exactly that version, and underspecified forms like =1.2 are equivalent to >=1.2.0, <1.3.0. Examples: - Exact version: cargo binstall crate@=1.2.3 - Pre-release: cargo binstall crate@=1.0.0-alpha.1 - Range (compatible updates): cargo binstall crate@^1.2 Alternatively, use the --version VERSION flag: cargo binstall --version 1.2.3 crate. This syntax is shown in the official README and logs, where radio-sx128x@0.14.1-alpha.5 resolves as 'radio-sx128x@=0.14.1-alpha.5'. It leverages Cargo's version req parser (clap-cargo dependency) for flexibility matching Cargo.toml dependencies.
Citations:
- 1: https://github.com/cargo-bins/cargo-binstall
- 2: https://crates.io/crates/cargo-binstall/1.11.0
- 3: https://lib.rs/crates/cargo-binstall
- 4: https://linuxcommandlibrary.com/man/cargo-binstall
Pin cargo-shear to a fixed version for reproducible CI.
Line 357 installs the latest cargo-shear on every run, which can cause sudden CI breakages from upstream releases. The cargo-binstall tool itself is pinned to v1.17.8, so cargo-shear should be pinned as well.
♻️ Proposed fix
- name: cargo shear
run: |
- cargo binstall --no-confirm cargo-shear
+ cargo binstall --no-confirm cargo-shear@=<pinned-version>
cargo shearUse the @=<version> syntax to pin to an exact version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yaml around lines 355 - 358, The workflow currently
installs the latest cargo-shear which can break CI; update the step that runs
"cargo binstall --no-confirm cargo-shear" to pin cargo-shear to an exact version
using the cargo-binstall syntax (e.g., "cargo-shear@=x.y.z"); modify the step
named "cargo shear" so the binstall command references the pinned package name
and leave the subsequent "cargo shear" invocation unchanged.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/workflows/ci.yaml (1)
355-358:⚠️ Potential issue | 🟠 MajorLine 357 still leaves
cargo-shearfloating.Installing the current
cargo-shearrelease on every run makes the unified lint job vulnerable to unrelated upstream breakage.cargo-binstallsupports selecting an explicit package version, so this should be pinned too. (github.com)♻️ Suggested fix
- name: cargo shear run: | - cargo binstall --no-confirm cargo-shear + cargo binstall --no-confirm cargo-shear@<tested-version> cargo shear🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 355 - 358, The CI step named "cargo shear" currently installs the latest cargo-shear with "cargo binstall --no-confirm cargo-shear" which risks upstream breakage; change the install to pin a specific release by using the cargo-binstall version syntax (e.g., replace that line with "cargo binstall --no-confirm cargo-shear@<VERSION>") and keep invoking the tool via "cargo shear" so the job installs a fixed cargo-shear release instead of the floating latest.
🤖 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 332-338: The unified lint job ("name: Lint") is missing the
skip:ci short-circuit used elsewhere; update the Lint job to include the same
conditional guard used by other jobs so the workflow is skipped when the skip:ci
label is present on a PR (i.e., add the same "if" expression/guard that checks
for the skip:ci label to the Lint job), leaving the existing permissions block
intact.
- Around line 334-338: Add the missing permission and guard the reviewdog action
to PRs only: under the workflow permissions block add "issues: write" alongside
the existing "checks: write" and "pull-requests: write", and update the
reviewdog/action-suggester step's if condition so it only runs in a pull request
context (e.g. change the current if: failure() to an expression that also checks
the event is a pull_request, like github.event_name == 'pull_request' &&
failure()).
- Line 353: The workflow step that uses the action
"cargo-bins/cargo-binstall@113a77a4ce971c41332f2129c3d995df993cf746" should
explicitly pin the cargo-binstall binary by adding an inputs block (with:) and
setting the version field to a specific release (e.g., version: "v1.17.8" or the
exact binary tag you want), so update the step that contains the uses:
cargo-bins/cargo-binstall... line to include a with: version: "<pinned-version>"
entry to ensure reproducible installs.
---
Duplicate comments:
In @.github/workflows/ci.yaml:
- Around line 355-358: The CI step named "cargo shear" currently installs the
latest cargo-shear with "cargo binstall --no-confirm cargo-shear" which risks
upstream breakage; change the install to pin a specific release by using the
cargo-binstall version syntax (e.g., replace that line with "cargo binstall
--no-confirm cargo-shear@<VERSION>") and keep invoking the tool via "cargo
shear" so the job installs a fixed cargo-shear release instead of the floating
latest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 51feb7dc-e3f3-4f03-9aa0-ad86e0d5ea14
⛔ Files ignored due to path filters (1)
Lib/test/test_operator.pyis excluded by!Lib/**
📒 Files selected for processing (3)
.github/workflows/ci.yaml.pre-commit-config.yamlscripts/check_redundant_patches.py
✅ Files skipped from review due to trivial changes (1)
- .pre-commit-config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/check_redundant_patches.py
| name: Lint | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| checks: write | ||
| pull-requests: write | ||
| security-events: write # for zizmor |
There was a problem hiding this comment.
Restore the skip:ci guard on lint if that label is still meant to suppress the whole workflow.
Every other job in this workflow still short-circuits on skip:ci, but the unified lint job no longer does. Applying that label will therefore still run this path.
♻️ Suggested fix
lint:
+ if: ${{ !contains(github.event.pull_request.labels.*.name, 'skip:ci') }}
name: Lint🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yaml around lines 332 - 338, The unified lint job
("name: Lint") is missing the skip:ci short-circuit used elsewhere; update the
Lint job to include the same conditional guard used by other jobs so the
workflow is skipped when the skip:ci label is present on a PR (i.e., add the
same "if" expression/guard that checks for the skip:ci label to the Lint job),
leaving the existing permissions block intact.
| permissions: | ||
| contents: read | ||
| checks: write | ||
| pull-requests: write | ||
| security-events: write # for zizmor |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the workflow file
find . -name "ci.yaml" -o -name "ci.yml" | head -20Repository: RustPython/RustPython
Length of output: 92
🌐 Web query:
reviewdog/action-suggester permissions requirements documentation
💡 Result:
The reviewdog/action-suggester GitHub Action requires the following permissions in your workflow YAML: permissions: contents: read checks: write issues: write pull-requests: write These are specified in the official README to enable posting code suggestions via GitHub's multi-line suggestions feature on pull requests. See GitHub docs on Assigning permissions to jobs for details. The action supports pull_request events primarily.
Citations:
🏁 Script executed:
# Read the workflow file around the mentioned lines
fd "ci.yaml" --type f --exec wc -l {} \; 2>/dev/null | head -5Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
# Read the workflow file to check permissions and action usage
cat -n .github/workflows/ci.yaml | sed -n '320,400p'Repository: RustPython/RustPython
Length of output: 3245
🏁 Script executed:
# Also check the workflow triggers at the top
cat -n .github/workflows/ci.yaml | head -20Repository: RustPython/RustPython
Length of output: 915
Add missing issues: write permission and condition the reviewdog/action-suggester to pull requests only.
The action's documentation specifies it requires issues: write in addition to checks: write and pull-requests: write. Since the workflow runs on push, merge_group, and workflow_dispatch events, the current if: failure() condition can invoke the action outside pull request context where it cannot function. Add the required permission and limit execution to pull request events.
Suggested fix
permissions:
contents: read
checks: write
+ issues: write
pull-requests: write
security-events: write # for zizmor - name: reviewdog
- if: failure()
+ if: failure() && github.event_name == 'pull_request'
uses: reviewdog/action-suggester@aa38384ceb608d00f84b4690cacc83a5aba307ff # 1.24.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yaml around lines 334 - 338, Add the missing permission
and guard the reviewdog action to PRs only: under the workflow permissions block
add "issues: write" alongside the existing "checks: write" and "pull-requests:
write", and update the reviewdog/action-suggester step's if condition so it only
runs in a pull request context (e.g. change the current if: failure() to an
expression that also checks the event is a pull_request, like github.event_name
== 'pull_request' && failure()).
|
|
||
| - name: Ensure docs generate no warnings | ||
| run: cargo doc --locked | ||
| - uses: cargo-bins/cargo-binstall@113a77a4ce971c41332f2129c3d995df993cf746 # v1.17.8 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the content around line 353 in the workflow file
cat -n .github/workflows/ci.yaml | sed -n '350,360p'Repository: RustPython/RustPython
Length of output: 431
🌐 Web query:
cargo-binstall action version input default behavior
💡 Result:
The cargo-binstall GitHub Action is cargo-bins/cargo-binstall@main. It installs the cargo-binstall tool (a faster alternative to cargo install that downloads prebuilt binaries). The version input specifies the cargo-binstall version to install (e.g., version: "1.2.3"). If not provided, it defaults to the latest version available from the cargo-binstall GitHub releases. Default behavior: Downloads and installs the latest cargo-binstall binary for the current platform into ~/.cargo/bin, making cargo binstall available for subsequent steps. It supports unattended CI use and works across platforms (Linux, macOS, Windows). For specific versions, provide the version input; otherwise, latest is used automatically.
Citations:
- 1: https://github.com/actions-marketplace-validations/cargo-bins_cargo-binstall
- 2: https://github.com/marketplace/actions/install-cargo-binstall
- 3: https://github.com/cargo-bins/cargo-binstall
🏁 Script executed:
# Check if the specific action SHA mentioned has version input support
# We'll search for action.yml metadata in the cargo-binstall repo history
curl -s "https://raw.githubusercontent.com/cargo-bins/cargo-binstall/113a77a4ce971c41332f2129c3d995df993cf746/action.yml" 2>/dev/null | head -50Repository: RustPython/RustPython
Length of output: 852
Pin the cargo-binstall binary version explicitly in the action inputs.
The action SHA pins the action code but not the cargo-binstall binary version. The action defaults to installing the latest cargo-binstall release when the version input is not set, so this job will pick up whatever version is current when the workflow runs, affecting reproducibility.
♻️ Suggested fix
- - uses: cargo-bins/cargo-binstall@113a77a4ce971c41332f2129c3d995df993cf746 # v1.17.8
+ - uses: cargo-bins/cargo-binstall@113a77a4ce971c41332f2129c3d995df993cf746 # v1.17.8
+ with:
+ version: "1.17.8"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - uses: cargo-bins/cargo-binstall@113a77a4ce971c41332f2129c3d995df993cf746 # v1.17.8 | |
| - uses: cargo-bins/cargo-binstall@113a77a4ce971c41332f2129c3d995df993cf746 # v1.17.8 | |
| with: | |
| version: "1.17.8" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yaml at line 353, The workflow step that uses the
action "cargo-bins/cargo-binstall@113a77a4ce971c41332f2129c3d995df993cf746"
should explicitly pin the cargo-binstall binary by adding an inputs block
(with:) and setting the version field to a specific release (e.g., version:
"v1.17.8" or the exact binary tag you want), so update the step that contains
the uses: cargo-bins/cargo-binstall... line to include a with: version:
"<pinned-version>" entry to ensure reproducible installs.
562a999 to
15b06c2
Compare
|
|
||
| def test_a(): | ||
| return super().test_a() | ||
|
|
There was a problem hiding this comment.
🎉
Edit: nvm, it doesn't show the alert in the comment:/ (but it does work)
2 participants
ATM we have multiple places where we define lint steps, like running
ruffthis PR aims to streamline the process by condensing everything to a single jobSummary by CodeRabbit
Chores
Tooling