Release: fix linux-arm64 build via cross; pin cross; add permissions#20
Release: fix linux-arm64 build via cross; pin cross; add permissions#20rowan-stein merged 1 commit intomainfrom
Conversation
…; add release permissions
|
Requesting review to stabilize Release workflow. Summary:
Focus:
Context: tracked in Issue #21 (Release workflow failing on main). Thanks! |
noa-lucent
left a comment
There was a problem hiding this comment.
Review checklist and findings:
- Checked existing PR comments and goals; focused on linux-arm64 via cross, permissions, artifact naming, and other targets.
- Verified matrix entries for linux-amd64 (native cargo), linux-arm64 (cross), macOS amd64/arm64, and Windows amd64.
- Inspected build, strip, package, upload, and release steps for each target.
- Confirmed permissions: contents: write on release job.
Notes and suggestions:
-
Linux strip tool install: the step "Install strip tools (Linux)" installs "lvm" instead of "llvm":
sudo apt-get install -y lvm binutils
Later steps check for "llvm-strip" on Linux and Windows. To align, recommend changing the package to "llvm" (and optionally "llvm-14" or a pinned version) to ensure llvm-strip is available and avoid installing the unrelated LVM package. -
Pinning cross:
The install uses: cargo install cross --version 0.2.5. Consider adding --locked for reproducible dependency resolution: cargo install cross --version 0.2.5 --locked. -
Docker availability for cross:
cross defaults to Docker-backed builds. Ubuntu runners typically have Docker available, but it may be good to assert or document this (e.g., a quickdocker --versioncheck with a helpful error) to surface failures earlier if environments change. -
Artifact names and release assets:
- Non-Windows archives: pexels-cli-${{ matrix.runner_os }}-${{ matrix.arch }}.tar.gz
- Windows archive: pexels-cli-windows-amd64.zip
The release step publishes exactly these five assets, matching the build matrix. This looks consistent.
-
Build logic:
- linux-arm64 uses cross exclusively (no native fallback), which aligns with the stated goal and simplifies maintenance.
- Other targets continue to use native cargo builds and are unaffected.
-
Permissions and triggers:
- contents: write is set on the release job as needed.
- Triggers: push to main and workflow_dispatch are present, matching the intent to stabilize releases from main.
If you�d like, I can follow up with a small patch to correct the llvm package name and add --locked to the cross install.
noa-lucent
left a comment
There was a problem hiding this comment.
Looks good overall for stabilizing the release process: linux-arm64 via cross, pinned version, explicit permissions, and consistent artifact names across targets. Left a couple of non-blocking suggestions to refine the Linux strip tools and the cross install reproducibility. Thanks!
Simplify linux-arm64 release build to use cross only; removes unnecessary OpenSSL native fallback and pins cross to 0.2.5. Adds contents: write permissions for release.