Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds automated binary release capabilities by introducing a GitHub Actions workflow that integrates with release-plz for creating releases and building distributable binaries. The Cargo.toml is updated with cargo-binstall metadata to enable easy installation of pre-built binaries.
Changes:
- Added a new GitHub Actions workflow (
.github/workflows/release.yml) that triggers on successful Quality Check workflow completion or manual dispatch - Updated
ostool/Cargo.tomlwith binstall metadata configuration specifying binary distribution format and download URLs - Alphabetically reordered dependencies in
ostool/Cargo.toml(movedfitimageandfutures)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| .github/workflows/release.yml | New workflow that orchestrates release creation via release-plz and builds/packages binaries for x86_64-unknown-linux-gnu target |
| ostool/Cargo.toml | Added binstall metadata for binary distribution and corrected dependency ordering to be alphabetical |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
When the workflow is triggered by workflow_run, the code is checked out at the ref that triggered the workflow_run event, not necessarily the latest code in main. If release-plz creates commits or tags during its execution, the build-binaries job won't have access to those changes because it checked out before those changes were made. Consider fetching the latest state or using the tag created by release-plz to checkout the correct version.
| uses: actions/checkout@v4 | |
| uses: actions/checkout@v4 | |
| with: | |
| ref: main | |
| fetch-depth: 0 |
| run: | | ||
| cargo build --release -p ostool --bin ostool --target ${{ steps.meta.outputs.target }} | ||
| cargo build --release -p ostool --bin cargo-osrun --target ${{ steps.meta.outputs.target }} |
There was a problem hiding this comment.
The shell script doesn't have error handling enabled. If any command in the multi-line script fails (e.g., cargo build fails), the subsequent commands will still execute. Consider adding 'set -e' at the beginning of the script to exit on any error, or check the exit status of the cargo build commands before proceeding.
| run: | | ||
| version="${{ steps.meta.outputs.version }}" | ||
| target="${{ steps.meta.outputs.target }}" | ||
| dist="dist/ostool-${target}-v${version}" | ||
| mkdir -p "$dist" | ||
| cp "target/${target}/release/ostool" "$dist/" | ||
| cp "target/${target}/release/cargo-osrun" "$dist/" | ||
| tar -czf "ostool-${target}-v${version}.tar.gz" -C dist "ostool-${target}-v${version}" | ||
| sha256sum "ostool-${target}-v${version}.tar.gz" > "ostool-${target}-v${version}.tar.gz.sha256" |
There was a problem hiding this comment.
The shell script lacks error handling. If mkdir or cp commands fail, or if the tar command fails, the subsequent sha256sum command will still execute. Consider adding 'set -e' at the beginning to ensure the script exits on any error.
|
|
||
| jobs: | ||
| release-plz: | ||
| if: ${{ github.repository_owner == 'drivercraft' && (github.event_name == 'workflow_dispatch' || github.event.workflow_run.conclusion == 'success') }} |
There was a problem hiding this comment.
The repository owner check 'drivercraft' does not match the repository URL in ostool/Cargo.toml which points to 'github.com/ZR233/ostool'. This inconsistency will prevent the workflow from running when triggered. Either update this check to match 'ZR233' or update the repository field in Cargo.toml to match 'drivercraft'.
| - name: Install Rust toolchain | ||
| uses: dtolnay/rust-toolchain@stable | ||
| with: | ||
| targets: x86_64-unknown-linux-gnu |
There was a problem hiding this comment.
The workflow only builds for x86_64-unknown-linux-gnu target, but cargo-binstall users may need binaries for other common platforms like macOS (aarch64-apple-darwin, x86_64-apple-darwin) and Windows (x86_64-pc-windows-msvc). Consider adding a matrix strategy to build for multiple platforms to improve the utility of the binstall integration.
| - name: Compute version and tag | ||
| id: meta | ||
| run: | | ||
| version=$(grep -m1 '^version' ostool/Cargo.toml | sed -E 's/version = "([^"]+)"/\1/') |
There was a problem hiding this comment.
The version extraction uses 'grep -m1' which will match the first occurrence of 'version' in the file. This could incorrectly match a version string in a comment or different section. Consider using a more specific pattern like 'grep -m1 '^version =' ostool/Cargo.toml' to ensure it only matches the package version field.
| version=$(grep -m1 '^version' ostool/Cargo.toml | sed -E 's/version = "([^"]+)"/\1/') | |
| version=$(grep -m1 '^version =' ostool/Cargo.toml | sed -E 's/version = "([^"]+)"/\1/') |
| - name: Build binaries | ||
| if: steps.release_check.outputs.exists == 'true' |
There was a problem hiding this comment.
The build and packaging steps only execute if the release already exists (exists == 'true'), which seems counterintuitive. If the release doesn't exist, no binaries will be built or uploaded. This logic appears reversed - binaries should be built when the release exists to upload to it, but the workflow won't create binaries for new releases. Consider reviewing this conditional logic to ensure it matches the intended behavior.
No description provided.