Cargo check features on all os#7733
Conversation
📝 WalkthroughWalkthroughCI Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/ci.yaml (2)
151-152: Prefer a pinned Windows runner image for reproducible CI.This new matrix entry uses
windows-latest, while other jobs in this workflow are pinned towindows-2025. Aligning towindows-2025avoids surprise breakage whenlatestrolls over.Suggested change
- - os: windows-latest + - os: windows-2025 target: x86_64-pc-windows-msvc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 151 - 152, Update the CI matrix entry that currently sets "os: windows-latest" to the pinned runner "windows-2025" so it matches other jobs; locate the matrix definition containing the Windows entry (the line with os: windows-latest and target: x86_64-pc-windows-msvc) and replace the os value with windows-2025 to ensure reproducible builds.
211-213: Rename the second log group to match what this run is actually testing.
no threadingisn’t the clearest differentiator here;with sslwould better reflect the second command.Suggested change
- echo "::group::no threading" + echo "::group::with ssl" cargo check --target "${{ matrix.target }}" ${{ env.CARGO_ARGS }} echo "::endgroup::"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 211 - 213, Rename the GitHub Actions log group label around the cargo check that runs with SSL: replace the echo "::group::no threading" string with echo "::group::with ssl" that encloses the cargo check --target "${{ matrix.target }}" ${{ env.CARGO_ARGS }} line (keep the echo "::endgroup::" as-is) so the group name accurately reflects the test being run.
🤖 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 151-152: Update the CI matrix entry that currently sets "os:
windows-latest" to the pinned runner "windows-2025" so it matches other jobs;
locate the matrix definition containing the Windows entry (the line with os:
windows-latest and target: x86_64-pc-windows-msvc) and replace the os value with
windows-2025 to ensure reproducible builds.
- Around line 211-213: Rename the GitHub Actions log group label around the
cargo check that runs with SSL: replace the echo "::group::no threading" string
with echo "::group::with ssl" that encloses the cargo check --target "${{
matrix.target }}" ${{ env.CARGO_ARGS }} line (keep the echo "::endgroup::"
as-is) so the group name accurately reflects the test being run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8f75cf97-18ba-4ace-a4e5-0ca31787fac5
📒 Files selected for processing (1)
.github/workflows/ci.yaml
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| - os: macos-latest | ||
| target: x86_64-apple-darwin | ||
| - os: windows-latest | ||
| target: x86_64-pc-windows-msvc |
There was a problem hiding this comment.
isn't this duplication of default windows machine? did you mean x86 32bit?
There was a problem hiding this comment.
Yes it is. See my table below.
There was a problem hiding this comment.
I just moved this
RustPython/.github/workflows/ci.yaml
Lines 82 to 83 in 88be7bb
to the
cargo_check job. but if that was already duplicated then sure, we can remove it
There was a problem hiding this comment.
we don't need to run cargo check when we actually build the target
|
Here's what we're currently compiling for (on the
Here's the list of targets listed as Tier 1 with Host Tools (which is everything in Tier 1):
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)
197-210: Consider adding a guard to prevent duplicate[env]sections when appending to.cargo/config.toml.The current
.cargo/config.tomldoes not contain an[env]section, so the first append succeeds. However, if the step runs multiple times (e.g., via job retry or matrix re-execution with artifact caching), appending a duplicate[env]header will violate TOML syntax. Check if[env]exists before adding it.Suggested improvement
- name: Append env conf to cargo if: ${{ matrix.target == 'aarch64-linux-android' }} env: NDK_PATH: ${{ steps.setup-ndk.outputs.ndk-path }} run: | + mkdir -p .cargo + touch .cargo/config.toml + if ! grep -q '^\[env\]$' .cargo/config.toml; then + echo "[env]" >> .cargo/config.toml + fi { echo "CC_aarch64_linux_android = \"${NDK_PATH}/toolchains/llvm/prebuilt/linux-x86_64/bin/aarch64-linux-android24-clang\"" echo "AR_aarch64_linux_android = \"${NDK_PATH}/toolchains/llvm/prebuilt/linux-x86_64/bin/llvm-ar\"" echo "CARGO_TARGET_AARCH64_LINUX_ANDROID_LINKER = \"${NDK_PATH}/toolchains/llvm/prebuilt/linux-x86_64/bin/aarch64-linux-android24-clang\"" } >> .cargo/config.toml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 197 - 210, The append step that writes the "[env]" block to .cargo/config.toml can create duplicate headers; modify the run block that uses NDK_PATH (the "Append env conf to cargo" step) to first check whether a line exactly matching "[env]" already exists in .cargo/config.toml and only append the "[env]" header if missing, then append or update the three entries (CC_aarch64_linux_android, AR_aarch64_linux_android, CARGO_TARGET_AARCH64_LINUX_ANDROID_LINKER); implement this using a simple shell conditional (e.g., grep/awk test) so repeated executions won't add a second "[env]" section.
🤖 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 197-210: The append step that writes the "[env]" block to
.cargo/config.toml can create duplicate headers; modify the run block that uses
NDK_PATH (the "Append env conf to cargo" step) to first check whether a line
exactly matching "[env]" already exists in .cargo/config.toml and only append
the "[env]" header if missing, then append or update the three entries
(CC_aarch64_linux_android, AR_aarch64_linux_android,
CARGO_TARGET_AARCH64_LINUX_ANDROID_LINKER); implement this using a simple shell
conditional (e.g., grep/awk test) so repeated executions won't add a second
"[env]" section.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8fa56ebe-a634-4661-b2f4-99e0d0dd3724
📒 Files selected for processing (1)
.github/workflows/ci.yaml
4 participants
Summary by CodeRabbit