Skip to content

refactor(swift): replace build scripts with xtask - #455

Open
Dzejkop wants to merge 3 commits into
mainfrom
refactor/swift-xtask
Open

refactor(swift): replace build scripts with xtask#455
Dzejkop wants to merge 3 commits into
mainfrom
refactor/swift-xtask

Conversation

@Dzejkop

@Dzejkop Dzejkop commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • replace Swift shell scripts with cargo xtask swift commands
  • update CI, release workflows, local tooling, and docs

Testing

  • cargo clippy -p xtask --all-targets -- -D warnings
  • cargo test -p xtask
  • full iOS build and Swift foreign-binding test

Note

Medium Risk
Touches iOS XCFramework build and release packaging paths used in production Swift distribution, though behavior is largely a straight port from the removed scripts.

Overview
Moves Swift/iOS automation from deleted swift/*.sh scripts into cargo xtask swift (build, test, local, archive), aligned with the existing Kotlin xtask pattern.

CI and release now invoke cargo xtask swift build, cargo xtask swift test --skip-build, and cargo xtask swift archive instead of shell entrypoints; the Swift test matrix job also sets up Rust so xtask can run. Release workflow notes that WALLETKIT_CARGO_FEATURES applies to both Kotlin and Swift builds.

Docs and helpers point at the new commands (root README, swift/README.md, Nix docs); make_local_swift_package.sh calls cargo xtask swift local. Local SPM output stays under swift/local_build/walletkit-swift.

Reviewed by Cursor Bugbot for commit 147df78. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread xtask/src/swift/test.rs

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 516a8da. Configure here.

Comment thread xtask/src/swift/build.rs
@Dzejkop

Dzejkop commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

/otto --modle=opus review this PR

@agentotto

agentotto Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Kicked off a thorough review of PR #455 using Opus with a workspace to inspect the full diff and check behavior parity between the removed shell scripts and the new xtask swift commands. It'll post inline comments plus a summary verdict directly on the PR — I'll let you know once it's done.

Comment thread .github/workflows/ci.yml
- name: Set up Rust
uses: dtolnay/rust-toolchain@fa04a1451ff1842e2626ccb99004d0195b455a88 # master
with:
toolchain: 1.94.1

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.

Minor CI efficiency: unlike the sibling swift-build job, this swift-test matrix job doesn't add a Swatinem/rust-cache step, so cargo xtask swift test --skip-build recompiles xtask and all its deps from scratch on each of the 5 Xcode matrix entries (the uploaded artifact only contains WalletKit.xcframework + Sources, not target/). Adding a rust-cache step here would cut redundant compile time. Not blocking.

@agentotto

agentotto Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review summary — ✅ Approve (one minor, non-blocking inline note)

I checked out refactor/swift-xtask, diffed each new xtask module against the shell scripts it replaces, and ran cargo check -p xtask + cargo test -p xtask (all 8 tests pass).

Behavior parity verified — the ports are faithful:

  • build: same iOS targets, IPHONEOS_DEPLOYMENT_TARGET, AWS_LC_SYS_CFLAGS/RUSTFLAGS workaround, lipo universal-sim step, uniffi bindgen invocation, module → framework module modulemap rewrite, and Info.plist contents all match build_swift.sh/make_walletkit_framework.sh.
  • test: SDK check, generated-sources copy, DerivedData cleanup, simulator selection (iPhone 16 → any iPhone), and CI-only erase/boot/bootstatus hygiene all match run_swift_tests.sh. Result detection now relies on xcodebuild's exit code rather than grepping the log — this is more robust than the old string-matching and removes a false-positive/negative risk.
  • local/archive: output layout (swift/local_build/walletkit-swift) and Package.swift template substitution match local_swift.sh/archive_swift.sh, with added empty-arg validation.

Other checks:

  • Relative paths in the xtask are anchored via sh.change_dir(workspace_root()) in main.rs, so cargo xtask swift … works regardless of the invoking cwd — the cd "$PROJECT_ROOT" added to make_local_swift_package.sh is harmless but not strictly required.
  • No dangling references to any of the deleted swift/*.sh scripts remain anywhere in the repo.
  • CI/release YAML swaps are consistent; swift-test correctly adds a Rust setup step (needed now that the entrypoint is cargo xtask), and --skip-build avoids requiring nargo/iOS targets in that job.

One behavior change worth calling out (intentional & documented, not a bug): the old build_swift.sh hardcoded compress-zkeys,embed-zkeys,v3 and ignored WALLETKIT_CARGO_FEATURES. The xtask now reads WALLETKIT_CARGO_FEATURES (falling back to that same default), so the release workflow's env var now also drives the iOS build — which matches the updated comment ("Used by both the Kotlin and Swift xtasks"). Output is identical today since the values coincide; just be aware that changing that env var in the release workflow will now affect iOS too.

Nice cleanup overall — good alignment with the existing Kotlin xtask pattern, solid error messages, and useful unit tests around the pure logic (path resolution, manifest rendering, simulator-ID parsing).

@agentotto

agentotto Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Review complete on PR #455 (worldcoin/walletkit).

Verdict: ✅ Approve (one minor, non-blocking note)

The subagent checked out the branch, compared each new xtask module against the shell script it replaces, and ran cargo check/cargo test -p xtask (8/8 passing). Behavior parity across build, test, local, and archive looked solid, with no dangling references to the deleted scripts.

Posted to the PR:

  • Inline comment on ci.yml: the swift-test matrix job is missing a Swatinem/rust-cache step, so xtask recompiles from scratch across all 5 Xcode matrix runs — minor CI efficiency issue, not blocking.
  • Summary comment: overall approval, plus a flag that WALLETKIT_CARGO_FEATURES now also drives the iOS build (previously ignored) — output is currently identical but worth being aware of.

No correctness bugs or parity gaps found.

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.

1 participant