Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces comprehensive CI/CD improvements for a mixed R and Rust project, adding dedicated Rust testing workflows and enhancing existing R package workflows.
Key Changes:
- Adds dedicated Rust CI workflow with formatting, linting, testing, and building across multiple platforms
- Adds automated release workflow for building and publishing package binaries across platforms
- Enhances existing workflows with Rust toolchain support, improved caching, and expanded test matrices
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
.github/workflows/rust.yml |
New dedicated CI workflow for Rust code with fmt, clippy, tests, and builds across Ubuntu, macOS, and Windows |
.github/workflows/release.yml |
New release automation workflow with version validation and multi-platform binary building |
.github/workflows/pkgdown.yaml |
Updated with Rust toolchain support, system dependencies, concurrency control, and newer action version |
.github/workflows/build.yml |
Enhanced R CMD check with Rust support, expanded test matrix (release/devel/oldrel), caching, and system dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| inputs: | ||
| tag: | ||
| description: "Release tag (e.g., v3.0.6)" | ||
| required: false | ||
| type: string |
There was a problem hiding this comment.
The workflow has a workflow_dispatch input for tag but this input is never used in the workflow. If manual dispatch is intended to support custom tags, the workflow should use inputs.tag somewhere, or if not needed, the input should be removed to avoid confusion.
| inputs: | |
| tag: | |
| description: "Release tag (e.g., v3.0.6)" | |
| required: false | |
| type: string |
| - name: Validate tag matches version | ||
| if: github.event_name == 'release' | ||
| run: | | ||
| TAG="${{ github.ref_name }}" | ||
| VERSION="${{ steps.version.outputs.version }}" | ||
| if [[ "$TAG" != "v$VERSION" && "$TAG" != "$VERSION" ]]; then | ||
| echo "::error::Tag ($TAG) does not match package version (v$VERSION)" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
The tag validation only runs when github.event_name == 'release' but not for manual workflow_dispatch events. If a user manually triggers this workflow, the validation step will be skipped entirely. Consider whether validation should also run for manual dispatches, or if the manual dispatch should be removed if not fully supported.
| permissions: read-all | ||
|
|
||
| concurrency: | ||
| group: pkgdown-${{ github.event_name != 'pull_request' || github.run_id }} |
There was a problem hiding this comment.
The concurrency group logic pkgdown-${{ github.event_name != 'pull_request' || github.run_id }} will produce pkgdown-true for all non-pull_request events, which means all pushes to main, releases, and manual dispatches will share the same concurrency group. This could cause unintended cancellations. Consider using a more specific group identifier like pkgdown-${{ github.event_name == 'pull_request' && github.run_id || github.ref }} to properly separate different branches/events.
| group: pkgdown-${{ github.event_name != 'pull_request' || github.run_id }} | |
| group: pkgdown-${{ github.event_name == 'pull_request' && github.run_id || github.ref }} |
| include: | ||
| - os: ubuntu-latest | ||
| r: release | ||
| - os: macos-latest | ||
| r: release | ||
| - os: windows-latest | ||
| r: release | ||
| - os: ubuntu-latest | ||
| r: devel | ||
| - os: ubuntu-latest |
There was a problem hiding this comment.
The matrix includes three entries with r: release (ubuntu, macos, windows) which are redundant. The matrix configuration can be simplified by using a cross-product of os and r arrays with exclude rules if needed, rather than repeating r: release three times.
| include: | |
| - os: ubuntu-latest | |
| r: release | |
| - os: macos-latest | |
| r: release | |
| - os: windows-latest | |
| r: release | |
| - os: ubuntu-latest | |
| r: devel | |
| - os: ubuntu-latest | |
| os: [ubuntu-latest, macos-latest, windows-latest] | |
| r: [release, devel, oldrel-1] | |
| exclude: | |
| - os: macos-latest | |
| r: devel | |
| - os: macos-latest | |
| r: oldrel-1 | |
| - os: windows-latest | |
| r: devel | |
| - os: windows-latest |
| error-on: '"error"' | ||
|
|
||
| upload-snapshots: true | ||
| args: 'c("--no-manual", "--as-cran")' |
There was a problem hiding this comment.
The args parameter uses single quotes around the outer string and double quotes around inner values: 'c("--no-manual", "--as-cran")'. This will be interpreted as an R expression, which is correct, but the syntax could be clearer. Consider using c("--no-manual", "--as-cran") without outer quotes if the action supports it, or document why the outer quotes are necessary.
| args: 'c("--no-manual", "--as-cran")' | |
| args: --no-manual --as-cran |
| paths: | ||
| - "src/rust/**" | ||
| - "Cargo.toml" | ||
| - ".github/workflows/rust.yml" | ||
| pull_request: | ||
| branches: [main] | ||
| paths: | ||
| - "src/rust/**" | ||
| - "Cargo.toml" |
There was a problem hiding this comment.
The workflow paths filter includes the root Cargo.toml but not src/rust/Cargo.toml. While changes to the root workspace manifest should trigger the workflow, direct changes to src/rust/Cargo.toml (dependencies, package metadata, etc.) would not trigger this workflow. Consider adding "src/rust/Cargo.toml" to the paths list.
| - "src/rust/**" | ||
| - "Cargo.toml" | ||
| - ".github/workflows/rust.yml" | ||
| pull_request: | ||
| branches: [main] | ||
| paths: | ||
| - "src/rust/**" |
There was a problem hiding this comment.
The workflow paths filter should also include "src/rust/Cargo.lock" to trigger on dependency updates. Lock file changes indicate dependency version changes that should be validated by CI.
| - "src/rust/**" | |
| - "Cargo.toml" | |
| - ".github/workflows/rust.yml" | |
| pull_request: | |
| branches: [main] | |
| paths: | |
| - "src/rust/**" | |
| - "src/rust/**" | |
| - "src/rust/Cargo.lock" | |
| - "Cargo.toml" | |
| - ".github/workflows/rust.yml" | |
| pull_request: | |
| branches: [main] | |
| paths: | |
| - "src/rust/**" | |
| - "src/rust/Cargo.lock" |
| - uses: actions/checkout@v4 | ||
| - uses: dtolnay/rust-toolchain@stable | ||
| with: | ||
| components: rustfmt |
There was a problem hiding this comment.
The fmt job does not utilize caching like the clippy, test, and build jobs do. While formatting is typically fast, adding Rust cache here would improve consistency and could provide minor performance benefits, especially if rustfmt needs to compile any dependencies.
| components: rustfmt | |
| components: rustfmt | |
| - name: Cache Rust dependencies | |
| uses: Swatinem/rust-cache@v2 | |
| with: | |
| workspaces: "src/rust -> target" |
1 participant
No description provided.