chore(ci): simplify the CI and image building #1015
Conversation
WalkthroughThe pull request introduces modifications to GitHub Actions workflows and the Changes
Sequence DiagramsequenceDiagram
participant PR as Pull Request
participant Workflow as GitHub Actions
participant Build as Build Job
participant Test as Test Job
participant Lint as Linting Job
PR->>Workflow: Trigger
Workflow->>Build: Compile Rust
Build-->>Workflow: Build Complete
Workflow->>Test: Run Tests
Test-->>Workflow: Tests Passed
Workflow->>Lint: Check Formatting
Lint-->>Workflow: Linting Complete
Workflow->>PR: Status Report
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.gitignore (1)
22-22: Consider removing redundant venv entryThe new
**/.venv/pattern effectively covers all.venvdirectories at any depth. Consider removing the redundantvenv/entry since it's now covered by this more comprehensive pattern.-venv/ **/.venv/.github/workflows/wren-core-base.yml (1)
3-6: Consider adding push events for main branchThe workflow currently only triggers on pull requests affecting the
wren-core-basedirectory. Consider also triggering on push events to the main branch to ensure continuous validation of the main branch state.on: pull_request: paths: - 'wren-core-base/**' + push: + branches: + - main + paths: + - 'wren-core-base/**'.github/workflows/build-image.yml (1)
Line range hint
156-183: Consider simplifying merge job for single platformSince we're now building for a single platform, the merge job could be simplified. Consider whether this complex manifest creation is still necessary for a single platform build.
You could potentially replace this with a simpler tag-based approach since there's no need to merge multiple platform manifests anymore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build-image.yml(1 hunks).github/workflows/wren-core-base.yml(1 hunks).github/workflows/wren-core.yml(0 hunks).gitignore(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/wren-core.yml
🔇 Additional comments (3)
.github/workflows/wren-core-base.yml (1)
1-129: Well-structured workflow that aligns with PR objectives
The workflow is well-organized with appropriate job dependencies and comprehensive checks. It correctly implements the PR's platform simplification goals by focusing on Linux (amd64) and macOS (arm64) only.
.github/workflows/build-image.yml (2)
81-81: Platform specification aligns with PR objectives
The removal of linux/arm64 platform support successfully simplifies the image building process, aligning with the PR's goals.
Line range hint 89-95: Matrix configuration properly simplified
The matrix configuration has been correctly updated to remove arm64 support, maintaining consistency with the platform changes.
.github/workflows/wren-core-base.yml
Outdated
| - name: Cache Cargo | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: | | ||
| ~/.cargo/bin/ | ||
| ~/.cargo/registry/index/ | ||
| ~/.cargo/registry/cache/ | ||
| ~/.cargo/git/db/ | ||
| ./wren-core/target/ | ||
| # this key equals the ones on `linux-build-lib` for re-use | ||
| key: cargo-cache-benchmark-${{ hashFiles('wren-core/Cargo.toml') }} | ||
|
|
There was a problem hiding this comment.
Fix cache key to match working directory
The cache key references wren-core but the working directory is set to wren-core-base. This mismatch could lead to cache misses.
- key: cargo-cache-benchmark-${{ hashFiles('wren-core/Cargo.toml') }}
+ key: cargo-cache-benchmark-${{ hashFiles('wren-core-base/Cargo.toml') }}📝 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.
| - name: Cache Cargo | |
| uses: actions/cache@v4 | |
| with: | |
| path: | | |
| ~/.cargo/bin/ | |
| ~/.cargo/registry/index/ | |
| ~/.cargo/registry/cache/ | |
| ~/.cargo/git/db/ | |
| ./wren-core/target/ | |
| # this key equals the ones on `linux-build-lib` for re-use | |
| key: cargo-cache-benchmark-${{ hashFiles('wren-core/Cargo.toml') }} | |
| - name: Cache Cargo | |
| uses: actions/cache@v4 | |
| with: | |
| path: | | |
| ~/.cargo/bin/ | |
| ~/.cargo/registry/index/ | |
| ~/.cargo/registry/cache/ | |
| ~/.cargo/git/db/ | |
| ./wren-core/target/ | |
| # this key equals the ones on `linux-build-lib` for re-use | |
| key: cargo-cache-benchmark-${{ hashFiles('wren-core-base/Cargo.toml') }} |
There was a problem hiding this comment.
addressed in 9cc2356
There was a problem hiding this comment.
@goldmedal Thanks for addressing this in commit 9cc2356! Everything looks good now; no further changes needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/wren-core-base.yml (2)
125-127: Add caching for taplo binary.Consider caching the taplo binary to speed up workflow runs. The binary is currently installed fresh in each run.
+ - name: Cache taplo + uses: actions/cache@v4 + with: + path: ~/.cargo/bin/taplo + key: ${{ runner.os }}-taplo-0.9 - name: Install taplo + if: steps.cache-taplo.outputs.cache-hit != 'true' run: cargo +stable install taplo-cli --version ^0.9 --locked
94-94: Optimize job dependencies.Consider running the
clippyandcargo-toml-formatting-checksjobs in parallel with tests instead of depending onlinux-build-lib. Since these are independent checks, they could run earlier and provide faster feedback.clippy: name: clippy - needs: [ linux-build-lib ] + needs: [ ] cargo-toml-formatting-checks: name: check Cargo.toml formatting - needs: [ linux-build-lib ] + needs: [ ]Also applies to: 113-113
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/wren-core-base.yml(1 hunks)
🔇 Additional comments (2)
.github/workflows/wren-core-base.yml (2)
1-15: LGTM! Well-structured workflow configuration.
The workflow configuration is properly set up with:
- Appropriate path filters for PR triggers
- Concurrency controls to prevent redundant runs
- Correct working directory configuration
61-61: Verify intentional exclusion of doctests.
Both Linux and macOS test jobs exclude doctests (--lib --tests --bins). Please confirm if this is intentional, as doctests can be valuable for documentation accuracy.
rust" wren-core-base/src
Also applies to: 76-76
<details>
<summary>✅ Verification successful</summary>
rust' wren-core-base/src/
# Also check for #[doc = "..."] attributes that might contain code examples
rg -l '#\[doc.*```rust' wren-core-base/src/
# Check if there are any doc comments with code blocks
rg -l '//!.*```rust|///.*```rust' wren-core-base/src/
" wren-core-base/src/mdl/cls.rs wren-core-base/src/mdl/builder.rs wren-core-base/src/mdl/manifest.rs
---
' wren-core-base/src/mdl/cls.rs wren-core-base/src/mdl/builder.rs wren-core-base/src/mdl/manifest.rs
Excluding doctests appears appropriate for this codebase
Based on the analysis of the documentation in the codebase, while there are documented functions and methods, there don't appear to be any actual Rust code examples (```rust blocks) in the documentation that would be executed as doctests. The documentation consists mainly of descriptive comments and method/field explanations without executable examples. Therefore, excluding doctests in the test configuration is appropriate for this codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any doctests in the codebase that would be skipped
rg -l "Length of output: 116
Script:
#!/bin/bash
# Check if there are any doctests in the codebase that would be skipped
rg -l 'Length of output: 116
Script:
#!/bin/bash
# Search for Rust documentation tests in the codebase
rg --type rust -l "(//!|///|#\[doc)" wren-core-base/src/
Length of output: 157
Script:
#!/bin/bash
# Check the content of doc comments to see if they contain actual code examples
rg "(//!|///|#\[doc).*Length of output: 116
Script:
#!/bin/bash
# Check the content of doc comments to see if they contain actual code examples
rg -U '(//!|///|#\[doc =)(.|\n)*Length of output: 116
Script:
#!/bin/bash
# Check for documentation content in the files
cat wren-core-base/src/mdl/cls.rs wren-core-base/src/mdl/builder.rs wren-core-base/src/mdl/manifest.rs | grep -A 2 "//\|#\[doc"
Length of output: 2743
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/wren-core.yml (1)
Line range hint
1-124: Workflow structure aligns with PR objectivesThe workflow has been successfully streamlined by:
- Removing Windows testing
- Removing Mac (Intel) testing
- Maintaining Linux (amd64) and macOS-aarch64 support
- Keeping essential checks (fmt, clippy, Cargo.toml formatting)
The job dependencies and concurrency controls are properly configured.
Consider these potential optimizations:
- Add cargo cache for macOS-aarch64 job to improve build times
- Consider matrix strategy for Rust versions if backward compatibility is needed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/wren-core-base.yml(1 hunks).github/workflows/wren-core.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/wren-core-base.yml
🔇 Additional comments (1)
.github/workflows/wren-core.yml (1)
42-42: Verify the impact of checking all targets
The step has been changed from checking specific features to checking all targets. While this provides better coverage, it might increase the build time.
Run the following script to analyze the available targets and their impact:
✅ Verification successful
Checking all targets is consistent with the project's CI practices
After analyzing the workflow files, I can see that checking all targets is already being used in the wren-core-base.yml workflow, and this change aligns the wren-core.yml workflow with that practice. The change from checking specific features to checking all targets is part of a consistent CI strategy across the project's Rust components.
Key observations:
- Both
wren-coreandwren-core-baseworkflows usecargo check --all-targets - The change maintains consistency between related components
- The workflow has proper caching configured to optimize build times
- The step is part of the initial compilation check before running more intensive tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze cargo targets and their dependencies
# Note: This helps verify if all targets are necessary
# Check available targets
cd wren-core && cargo metadata --format-version=1 | jq -r '.packages[] | select(.name == "wren-core") | .targets[] | {name: .name, kind: .kind}'
# Check build time impact by searching for relevant discussions
rg -l 'cargo.*check.*all-targets' .github/
Length of output: 335
Script:
#!/bin/bash
# Let's try a different approach to analyze the workflow file and check targets
# First, let's check if the workflow file exists and examine its content
fd -t f "wren-core.yml" .github/workflows/ --exec cat {}
# Let's also search for any other workflow files that might provide context
fd -t f "\.ya?ml$" .github/workflows/ --exec echo "=== {} ===" \; --exec cat {}
# Search for cargo check commands in all workflow files
rg "cargo.+check" .github/workflows/
Length of output: 31110
Description
Summary by CodeRabbit
New Features
Improvements
.gitignorefile to prevent tracking of.venvdirectories.Bug Fixes