Skip to content

Eliminate redundant SwiftPM builds in sign-and-notarize-cli#75

Closed
Copilot wants to merge 2 commits into
ainfra-2351-sign-and-notarize-imessage-cli-in-ci-localfrom
copilot/sub-pr-70
Closed

Eliminate redundant SwiftPM builds in sign-and-notarize-cli#75
Copilot wants to merge 2 commits into
ainfra-2351-sign-and-notarize-imessage-cli-in-ci-localfrom
copilot/sub-pr-70

Conversation

Copy link
Copy Markdown

Copilot AI commented May 8, 2026

The implementation attempt in #75 showed a minor SLOC decrease at the cost of less clear code. Closing for the time being. —@mokagio


This stacked PR addresses review feedback on the local signing/notarization script: the previous flow invoked swift build more times than necessary when resolving per-arch binary paths.
The update ensures each architecture triggers a single SwiftPM invocation and reuses its resolved bin path.

  • Build path resolution

    • build_arch now runs swift build ... --show-bin-path and returns the path for that architecture.
    • Progress logging is sent to stderr so command substitution captures only the bin path.
  • Redundant invocation removal

    • Removed the separate bin_for_arch function that re-invoked swift build.
    • Universal flow now derives both arch binaries directly from build_arch output.
    • Single-arch flow now sets binary from the same build_arch output.
build_arch() {
  local target_arch="$1"
  printf >&2 "==> building %s\n" "$target_arch"
  swift build -c release --triple "${target_arch}-apple-macosx11" --product imessage-cli --show-bin-path
}

arm64_bin="$(build_arch arm64)/imessage-cli"
x86_bin="$(build_arch x86_64)/imessage-cli"

@indent
Copy link
Copy Markdown

indent Bot commented May 8, 2026

PR Summary

Refactors scripts/sign-and-notarize-cli to collapse the prior two-step "build, then query bin path" pattern into a single build_arch invocation per architecture, with progress logs sent to stderr so command substitution captures only the bin path. The intent is to eliminate a redundant SwiftPM process per arch on the local sign/notarize flow. ⚠️ As implemented this is broken — see the reported critical issue.

  • build_arch rewritten to invoke swift build ... --show-bin-path and emit the path on stdout
  • printf "==> building" redirected to stderr to keep stdout clean for command substitution
  • Removed the now-unused bin_for_arch helper
  • Universal and single-arch branches both derive binary directly from build_arch output

Issues

1 potential issue found:

  • swift build --show-bin-path only prints the bin path and returns without compiling, so this change makes the script never build imessage-cli — on a clean checkout lipo/strip/codesign operate on non-existent files; on a warm checkout the script silently signs and notarizes whatever stale binary already lived at that path. → Autofix

CI Checks

Waiting for CI checks...


⚡ Autofix All Issues

Copilot AI changed the title [WIP] [WIP] Address feedback on local script for signing and notarizing imessage-cli Eliminate redundant SwiftPM builds in sign-and-notarize-cli May 8, 2026
Copilot AI requested a review from mokagio May 8, 2026 04:00
Copilot finished work on behalf of mokagio May 8, 2026 04:00
bin_for_arch() {
swift build -c release --triple "${1}-apple-macosx11" --product imessage-cli --show-bin-path
printf >&2 "==> building %s\n" "$target_arch"
swift build -c release --triple "${target_arch}-apple-macosx11" --product imessage-cli --show-bin-path
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Critical: --show-bin-path skips the build. Per SwiftPM's SwiftBuildCommand.swift, when --show-bin-path is passed the command early-returns after printing the path:

if options.shouldPrintBinPath {
    return try print(swiftCommandState.productsBuildParameters.buildPath.description)
}

No compilation happens. The previous code worked because build_arch (without --show-bin-path) actually compiled, and bin_for_arch re-invoked SwiftPM only to query the cached path. After this change:

  • Clean checkout: arm64_bin/x86_bin point at non-existent files; lipo -create (or strip in the single-arch branch) fails with no-such-file.
  • Warm checkout: whatever stale imessage-cli happens to sit at the printed path is what gets stripped, signed, and notarized — silently shipping outdated bits.

Minimal fix that keeps the single-callsite ergonomics:

build_arch() {
  local target_arch="$1"
  printf >&2 "==> building %s\n" "$target_arch"
  swift build -c release --triple "${target_arch}-apple-macosx11" --product imessage-cli >&2
  swift build -c release --triple "${target_arch}-apple-macosx11" --product imessage-cli --show-bin-path
}

This still costs one extra SwiftPM process per arch for the path lookup (same as the original bin_for_arch pattern), but it actually builds.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wow, good catch. On a clean cache, this does

✦ ➜ ./scripts/sign-and-notarize-cli
==> building arm64
==> building x86_64
==> creating universal binary
fatal error: /Applications/Xcode-26.3.0.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/lipo: can't open input file: /Users/gio/Developer/a8c/platform-imessage/.build/arm64-apple-macosx/release/imessage-cli (No such file or directory)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for confirming on a clean cache — that lipo: can't open input file is exactly the failure mode. Want me to push the two-invocation fix (real build to stderr, then --show-bin-path for the captured path), or are you planning to take it from here?

@mokagio
Copy link
Copy Markdown
Contributor

mokagio commented May 8, 2026

The implementation attempt in #75 showed a minor SLOC decrease at the cost of less clear code. Closing for the time being.

@mokagio mokagio closed this May 8, 2026
@mokagio mokagio deleted the copilot/sub-pr-70 branch May 8, 2026 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants