Skip to content

Only run cargo check when rust code is changed #7572

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 1 commit into from
Apr 9, 2026
+41 −1
Merged

Only run cargo check when rust code is changed #7572

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 41 additions & 1 deletion .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,42 @@ env:
CARGO_TERM_COLOR: always

jobs:
determine_changes:
name: Determine changes
runs-on: ubuntu-latest
outputs:
# Flag that is raised when any rust code is changed.
rust_code: ${{ steps.check_rust_code.outputs.changed }}
steps:
- uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
with:
fetch-depth: 0
persist-credentials: false

- name: Determine merge base
id: merge_base
run: |
sha=$(git merge-base HEAD "origin/${BASE_REF}")
echo "sha=${sha}" >> "$GITHUB_OUTPUT"
env:
BASE_REF: ${{ github.event.pull_request.base.ref || 'main' }}
Comment on lines +49 to +55
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Verifies the push-to-main behavior that makes merge-base equal HEAD.
main_head="$(git rev-parse origin/main)"
mb="$(git merge-base "$main_head" origin/main)"

echo "origin/main HEAD: $main_head"
echo "merge-base(origin/main, origin/main): $mb"

if [ "$main_head" = "$mb" ]; then
  echo "Confirmed: for push-to-main, merge-base equals HEAD, so diff HEAD...HEAD is empty."
fi

Repository: RustPython/RustPython

Length of output: 285

🏁 Script executed:

cat -n .github/workflows/ci.yaml | sed -n '49,72p'

Repository: RustPython/RustPython

Length of output: 1011

🏁 Script executed:

cat -n .github/workflows/ci.yaml | sed -n '37,75p'

Repository: RustPython/RustPython

Length of output: 1595

🏁 Script executed:

cat -n .github/workflows/ci.yaml | sed -n '152,160p'

Repository: RustPython/RustPython

Length of output: 377

Fix merge-base logic for push events and correct pathspec in diff filter; currently cargo_check never runs and change detection is broken.

Two issues prevent the change detection from working:

  1. Push to main always skips cargo_check: At line 52, git merge-base HEAD "origin/${BASE_REF}" resolves to HEAD on push to main, making the diff empty and rust_code always false. Use github.event.before for push events instead.

  2. Pathspec at line 64 is incorrect: ':.github/workflows/ci.yaml' includes only the workflow file (inverted logic). Should be ':!.github/workflows/ci.yaml' to exclude it like the other directories.

Proposed fixes

For the merge-base step (lines 49-55):

      - name: Determine merge base
        id: merge_base
        run: |
-         sha=$(git merge-base HEAD "origin/${BASE_REF}")
+         if [ "${GITHUB_EVENT_NAME}" = "push" ] && [ -n "${BEFORE_SHA}" ]; then
+           sha="${BEFORE_SHA}"
+         else
+           sha=$(git merge-base HEAD "origin/${BASE_REF}")
+         fi
          echo "sha=${sha}" >> "$GITHUB_OUTPUT"
        env:
          BASE_REF: ${{ github.event.pull_request.base.ref || 'main' }}
+         BEFORE_SHA: ${{ github.event.before }}

For the pathspec (line 64):

          if git diff --quiet "${MERGE_BASE}...HEAD" -- \
            ':!Lib/**' \
            ':!scripts/**' \
            ':!extra_tests/**' \
-           ':.github/workflows/ci.yaml' \
+           ':!.github/workflows/ci.yaml' \
📝 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.

Suggested change
- name: Determine merge base
id: merge_base
run: |
sha=$(git merge-base HEAD "origin/${BASE_REF}")
echo "sha=${sha}" >> "$GITHUB_OUTPUT"
env:
BASE_REF: ${{ github.event.pull_request.base.ref || 'main' }}
- name: Determine merge base
id: merge_base
run: |
if [ "${GITHUB_EVENT_NAME}" = "push" ] && [ -n "${BEFORE_SHA}" ]; then
sha="${BEFORE_SHA}"
else
sha=$(git merge-base HEAD "origin/${BASE_REF}")
fi
echo "sha=${sha}" >> "$GITHUB_OUTPUT"
env:
BASE_REF: ${{ github.event.pull_request.base.ref || 'main' }}
BEFORE_SHA: ${{ github.event.before }}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yaml around lines 49 - 55, The merge-base step (id:
merge_base) currently uses git merge-base HEAD "origin/${BASE_REF}" which yields
HEAD on push-to-main and makes diffs empty; update the logic to use
github.event.before as the base when github.event_name == 'push' (fall back to
origin/${BASE_REF} for pull_request) so the computed sha reflects the previous
commit for pushes; also fix the diff pathspec that currently reads
':.github/workflows/ci.yaml' to be an exclusion by changing it to
':!.github/workflows/ci.yaml' so the workflow file is excluded like the other
directories (adjust any step that evaluates rust_code or cargo_check to use the
corrected merge_base and pathspec).


- name: Check if there was any code related change
id: check_rust_code
run: |
if git diff --quiet "${MERGE_BASE}...HEAD" -- \
':!Lib/**' \
':!scripts/**' \
':!extra_tests/**' \
':.github/workflows/ci.yaml' \
; then
Comment on lines +60 to +65
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Current expression (with typo) matches:"
git ls-files -- ':!Lib/**' ':!scripts/**' ':!extra_tests/**' ':.github/workflows/ci.yaml' | sed -n '1,20p'

echo
echo "Fixed expression matches:"
git ls-files -- ':!Lib/**' ':!scripts/**' ':!extra_tests/**' ':!.github/workflows/ci.yaml' | sed -n '1,20p'

echo
echo "Expected: first output is narrowly scoped; second output includes repo files excluding the specified paths."

Repository: RustPython/RustPython

Length of output: 774

🏁 Script executed:

cat -n .github/workflows/ci.yaml | head -100

Repository: RustPython/RustPython

Length of output: 4096

🏁 Script executed:

grep -n "rust_code" .github/workflows/ci.yaml

Repository: RustPython/RustPython

Length of output: 222

🏁 Script executed:

sed -n '150,165p' .github/workflows/ci.yaml

Repository: RustPython/RustPython

Length of output: 530

Fix pathspec typo at line 64 (:! is required for exclusion).

':.github/workflows/ci.yaml' is a positive pathspec that only includes the ci.yaml file. This breaks the change detection logic—when Rust code changes but ci.yaml doesn't, the check incorrectly reports no changes, causing the cargo check job to be skipped. It should be ':!.github/workflows/ci.yaml' to exclude ci.yaml from the set of files being checked.

One-line fix 
-            ':.github/workflows/ci.yaml' \
+            ':!.github/workflows/ci.yaml' \
📝 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.

Suggested change
if git diff --quiet "${MERGE_BASE}...HEAD" -- \
':!Lib/**' \
':!scripts/**' \
':!extra_tests/**' \
':.github/workflows/ci.yaml' \
; then
if git diff --quiet "${MERGE_BASE}...HEAD" -- \
':!Lib/**' \
':!scripts/**' \
':!extra_tests/**' \
':!.github/workflows/ci.yaml' \
; then
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yaml around lines 60 - 65, The git diff pathspec list
in the conditional uses ':.github/workflows/ci.yaml' which incorrectly includes
ci.yaml instead of excluding it; update the pathspec in the git diff command
(the list passed to git diff --quiet "${MERGE_BASE}...HEAD") to use
':!.github/workflows/ci.yaml' so ci.yaml is excluded like the other patterns
(e.g., ':!Lib/**', ':!scripts/**').

Copy link
Copy Markdown
Contributor

@fanninpm fanninpm Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShaharNaveh I think you may have missed this.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

echo "changed=false" >> "$GITHUB_OUTPUT"
else
echo "changed=true" >> "$GITHUB_OUTPUT"
fi
env:
MERGE_BASE: ${{ steps.merge_base.outputs.sha }}

rust_tests:
if: ${{ !contains(github.event.pull_request.labels.*.name, 'skip:ci') }}
env:
Expand Down Expand Up @@ -111,9 +147,13 @@ jobs:
if: runner.os == 'Linux'

cargo_check:
if: ${{ !contains(github.event.pull_request.labels.*.name, 'skip:ci') }}
name: cargo check
runs-on: ${{ matrix.os }}
needs:
- determine_changes
if: |
!contains(github.event.pull_request.labels.*.name, 'skip:ci') &&
needs.determine_changes.outputs.rust_code == 'true'
strategy:
matrix:
include:
Expand Down
Loading