Skip to content

fix: reuse target-specific sidecar binaries#396

Closed
webees wants to merge 1 commit into
zhom:mainfrom
webees:fix/sidecar-target-artifacts
Closed

fix: reuse target-specific sidecar binaries#396
webees wants to merge 1 commit into
zhom:mainfrom
webees:fix/sidecar-target-artifacts

Conversation

@webees
Copy link
Copy Markdown
Contributor

@webees webees commented May 29, 2026

Which issue does this PR fix?

No linked issue.

This fixes a sidecar lookup edge case in copy-proxy-binary.mjs: when TARGET is explicitly set and equals the host Rust target, the script previously still used src-tauri/target/{debug,release}. Release workflows often build sidecars with an explicit --target, which writes them to src-tauri/target/${TARGET}/{debug,release} instead.

On Apple Silicon runners this matters for TARGET=aarch64-apple-darwin, because the explicit target can be the same as the host target. Without this change, a prebuilt sidecar in target/aarch64-apple-darwin/release can be missed, and beforeBuildCommand may rebuild donut-proxy / donut-daemon even though the target-specific artifacts already exist.

Benefits:

  • Reuses the sidecar artifacts produced by the explicit Rust target build.
  • Avoids unnecessary sidecar rebuilds in Tauri beforeBuildCommand.
  • Reduces release latency on Apple Silicon self-hosted runners.
  • Keeps default host builds unchanged when TARGET is not explicitly set.

How to test

  • node --check src-tauri/copy-proxy-binary.mjs
  • Verified with a temporary fixture that PROFILE=release TARGET=aarch64-apple-darwin node src-tauri/copy-proxy-binary.mjs copies from target/aarch64-apple-darwin/release when those sidecar binaries already exist.
  • Verified with fake rustc / cargo fixture that a missing explicit host-target sidecar is built with --target aarch64-apple-darwin and then copied from target/aarch64-apple-darwin/release.

Checklist

  • Read CONTRIBUTING.md
  • Ran pnpm format && pnpm lint && pnpm test locally and it passes
  • I tested the changes myself by running the app locally
  • Updated translations in all locale files (if UI text changed; no UI text changed)

AI usage

  • I used AI to help write this PR

AI was used to identify the release-build sidecar lookup issue and draft the focused fix.

@github-actions
Copy link
Copy Markdown
Contributor

This PR fixes a sidecar binary lookup edge case where explicitly-set TARGET equal to the host target would miss target/${TARGET}/{debug,release}, and the approach is sound.

Code review:

  • The sourceDirs() logic and findSource() fallback chain are correct. The unique() call is necessary: when TARGET_FROM_ENV=true and TARGET !== HOST_TARGET, both branches push the same target/${TARGET}/${profileDir} path.
  • The build-args change (TARGET_FROM_ENV || TARGET !== HOST_TARGET) is the right fix — when TARGET is explicitly set to the host triple, cargo build --target must still be passed so output lands in the target-specific subdirectory that sourceDirs() checks first. Without this, the build would write to target/${profileDir} but findSource checks target/${TARGET}/${profileDir} first, potentially missing it on a second invocation.
  • One minor concern: when TARGET_FROM_ENV=true and TARGET === HOST_TARGET, sourceDirs() returns two directories (target/${TARGET}/${profileDir} then target/${profileDir}). If a stale binary exists in target/${profileDir} but not in the target-specific dir, it will be found and copied without rebuilding, even though subsequent builds would emit to the target-specific dir. This matches the old behavior and is likely fine in practice.
  • The warning message improvement (listing all searched dirs) is a nice touch.

Suggestions: None — the change is focused, correct, and well-scoped.

@webees webees force-pushed the fix/sidecar-target-artifacts branch 2 times, most recently from 6b09111 to 74b3b2d Compare May 29, 2026 16:58
@webees webees force-pushed the fix/sidecar-target-artifacts branch from 74b3b2d to 429c7d1 Compare May 29, 2026 17:03
@zhom
Copy link
Copy Markdown
Owner

zhom commented May 29, 2026

Hi there :) This script is not used in the CI. No need for this change

@zhom zhom closed this May 29, 2026
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.

2 participants