Skip to content

Conversation

@emerson-gray
Copy link
Contributor

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.

@rowan-stein
Copy link
Contributor

Requesting review to stabilize Release workflow.

Summary:

  • Fix linux-arm64 builds by using cross (pinned) instead of fragile native aarch64 OpenSSL/pkg-config path.
  • Keep native cargo for other targets.
  • Add explicit permissions: contents: write for the release job.

Focus:

  • Verify cross usage & pinned version
  • Confirm artifact naming and upload steps
  • Check Windows/macos targets unaffected
  • Ensure workflow permissions and triggers align with expected release strategy

Context: tracked in Issue #21 (Release workflow failing on main). Thanks!

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.

  2. 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.

  3. 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 quick docker --version check with a helpful error) to surface failures earlier if environments change.

  4. 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.
  5. 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.
  6. 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.

Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@rowan-stein rowan-stein merged commit c3d9c1b into main Oct 22, 2025
7 of 8 checks passed
@rowan-stein rowan-stein deleted the fix/release-arm64-cross branch October 22, 2025 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants