this looks like it fixes the CI. added my changes from D9470 and this fix and the CI passed:
https://github.com/CommE2E/comm/actions/runs/6646956459/job/18061426837
Details
Details
- Reviewers
atul ashoat - Commits
- rCOMMedcd02905c56: [CI] fix iOS build issue following D9470
see the summary. also built for both aarch64 and x86_64 on my local machine. also ran an archive (Product -> Archive in Xcode) and it succeeded
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| scripts/build-rust-native-library.sh | ||
|---|---|---|
| 15–22 ↗ | (On Diff #32415) | Please match conventions in the rest of the file (and our bash codebase):
|
| 18 ↗ | (On Diff #32415) | Are you surethese are the only archs we target? Is there a risk this breaks the iOS release GitHub Action? I'm curious where you're getting the list from |
| scripts/build-rust-native-library.sh | ||
|---|---|---|
| 18 ↗ | (On Diff #32415) | these are the two targets we had to add for cargo lipo. the first is for real devices and the second is for the simulator. |
Comment Actions
Can you link to where you got the list from? I don't see them getting "added" to cargo-lipo in the old version of this file, perhaps it's somewhere else? To be honest I would've expected to see some other architectures on the list (doesn't Apple consider the iOS processor architecture and macOS processor architecture to be different?), but I'll be convinced if you can just link the code where you got the list from
Comment Actions
we add the targets in ensure_rustup_setup.sh, which we call on line 34 of build-rust-native-library.sh. See here for further confirmation that these are the only targets we need to build for.
To be honest I would've expected to see some other architectures on the list (doesn't Apple consider the iOS processor architecture and macOS processor architecture to be different?), but I'll be convinced if you can just link the code where you got the list from
i'm not sure why we'd need to target a macOS arch since we're only cross-compiling the native_rust_library for iOS. if a user wants to simply build the cargo project on their local machine (i.e. not cross-compile the library for iOS), they can just run cargo build.
Comment Actions
don’t we need to cross compile macos for simulator? we should make sure that continues to work?
Comment Actions
i think x86_64-apple-ios is the arch for the simulator. i was able to build the app for both simulator and real devices on my local machine
Comment Actions
Thanks for linking – I guess I was probably thinking of the 32-bit archs, which don't need to be supported.
Would be good to get an answer to @atul's question. Separately though, I'm a bit concerned that it looks like we can't simultaneously build both x64 and arm64 here... I thought that was a necessary step to the "archive" workflow for publishing a new iOS release.
Before landing, can you amend the test plan to cover testing the "archive" workflow?
Comment Actions
answered atul's q above. archive succeeded as well. i assume if the archive workflow builds for two arches then the native_rust_library is built twice during the workflow, once for each arch
Comment Actions
cargo-lipo is still referenced in scripts/ensure_rustup_setup.sh – would be good to clean that up