chore: CI speedups #89
Conversation
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
|
Warning Rate limit exceeded@jeluard has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe changes update the testing command and the CI workflow configuration. In the Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CI as CI Pipeline
participant Sanity as Sanity Job
participant Build as Build Job
participant BuildCrates as Build-Crates Job
Dev->>CI: Push commit
CI->>Sanity: Execute sanity job\n(check formatting & linting)
Sanity-->>CI: Report success/failure
CI->>Build: Run build job (if sanity successful)
CI->>BuildCrates: Run build-crates job (if sanity successful)
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
Compilation failure of the |
Signed-off-by: jeluard <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/CI.yml (2)
20-32: Spiffing new ‘sanity’ job for speedy formatting and linting checks
Adding a dedicated job for checking code format and running clippy is a clever move! This should help cut down on redundancy and speed up the CI process. It's also spot on with a 30-minute timeout, which seems plenty for these tasks.
45-48: Noted Disabled Musl Target – A word to the wise
The musl target remains commented out. While it's fine to keep temporarily, consider either a brief comment outlining when to re-enable it or a cleanup if it's redundant. It helps avoid later confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/CI.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: sanity
🔇 Additional comments (2)
.github/workflows/CI.yml (2)
34-34: Chaining dependencies with ‘needs: sanity’ – Cheers for that
Requiring the ‘sanity’ job before kicking off the main build is a top-notch strategy to ensure only well-formatted and linter-passed code makes it to further stages. This dependency chaining is as sound as a pound, mate!
68-100: Brilliant addition of the ‘build-crates’ job for wasm builds
This new job focusing on building crates for thewasm32-unknown-unknowntarget is an ace addition. The optional test execution and the warning mechanism on failure look well-thought-out—ensuring that a hiccup in these niche tests doesn't derail the whole build. Just double-check that the use ofcargo test(instead of the modified alias used elsewhere) is intentional for the selected package.
|
|
.github/workflows/CI.yml
Outdated
| build-crates: | ||
| name: Build crates on ${{ matrix.environments.runner }} with target ${{ matrix.environments.target }} | ||
| needs: sanity | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| environments: | ||
| - runner: ubuntu-latest | ||
| target: wasm32-unknown-unknown | ||
| package: amaru-ledger | ||
| optional: true | ||
| timeout-minutes: 30 | ||
| runs-on: ${{ matrix.environments.runner }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: Swatinem/rust-cache@v2 | ||
| with: | ||
| prefix-key: ${{ runner.os }}-${{ matrix.environments.target }} | ||
| - run: rustup target add ${{ matrix.environments.target }} | ||
| - name: Run tests | ||
| run: | | ||
| set +e | ||
| cargo test --locked --all-features -p ${{ matrix.environments.package }} --target ${{ matrix.environments.target }} |
There was a problem hiding this comment.
I am not sure I follow here? Why build the ledger separately from the rest (and has a separate job altogether)? Couldn't the same be achieved by adding packages to the matrix in the previous job? And more importantly, would the previous job already build and test the ledger crate?
There was a problem hiding this comment.
ledger would indeed be tested by the previous job but some target (e.g. wasm) won't realistically compile the full amaru, failing the whole job. So the idea is to restrict to some identified packages those specific targets.
I can see if there is some way to combine both.
Signed-off-by: jeluard <[email protected]>
Signed-off-by: jeluard <[email protected]>
Some CI improvements:
targetspecifics per target (i.e. runfmtonly once)wasm) do not run on finalamaruCLI (onlyamaru-ledger)Summary by CodeRabbit