Page MenuHomePhabricator

[CI] fix iOS build issue following D9470
ClosedPublic

Authored by varun on Oct 25 2023, 4:22 PM.
Tags
None
Referenced Files
F3361322: D9602.id32434.diff
Sun, Nov 24, 4:34 PM
Unknown Object (File)
Sat, Oct 26, 10:35 PM
Unknown Object (File)
Sat, Oct 26, 10:35 PM
Unknown Object (File)
Sat, Oct 26, 10:35 PM
Unknown Object (File)
Sat, Oct 26, 10:34 PM
Unknown Object (File)
Sat, Oct 26, 10:30 PM
Unknown Object (File)
Sep 26 2024, 5:24 PM
Unknown Object (File)
Sep 24 2024, 6:41 PM
Subscribers

Details

Summary

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

Test Plan

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

Repository
rCOMM Comm
Branch
debugci/varun-test (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun requested review of this revision.Oct 25 2023, 4:40 PM
scripts/build-rust-native-library.sh
15–22

Please match conventions in the rest of the file (and our bash codebase):

  1. Two space indents
  2. Prefer newer [[ to older [
18

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

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.

fix formatting and brackets

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

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?

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.

don’t we need to cross compile macos for simulator? we should make sure that continues to work?

In D9602#281351, @atul wrote:

don’t we need to cross compile macos for simulator? we should make sure that continues to work?

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

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?

This revision is now accepted and ready to land.Oct 26 2023, 9:16 AM

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?

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

This revision was automatically updated to reflect the committed changes.

cargo-lipo is still referenced in scripts/ensure_rustup_setup.sh – would be good to clean that up