M1 fixes (#309) #316
Merged
M1 fixes (#309) #316
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We no longer want to exclude arm64 for simulator SDKs as this excludes simulators running on M1 machines
Previously, the script would build for the `x86_64-apple-ios` whenever building for the `iphonesimulator` platform, however, M1 macs introduce a new target architecture for the iOS Simulator, `aarch64-apple-ios-sim`. Luckily the (now three) targets can be uniquely determined by the combination of the $PLATFORM_NAME + the architecture of the host computer. Unfortunately This new architecture is currently only supported by the nightly toolchain, so we must update the build command accordingly. Since a build only requires one target architecture for `librustzcash` we can drop the `cargo-lipo` dependency, which makes the resulting build command easier. Using nightly broke some `num-bigint` dependencies, so we update these to a compatible patch version.
For some reason `orchard` builds on revision `d0baa18f` were hanging indefinitely _only_ when building in release mode. Did a git bisect to find a more recent revision that still builds, (but does not fail due to other upstream requirements) and found `624a834` to be the first working one!
str4d
reviewed
Oct 4, 2021
|
Travis CI Build is failing with |
|
Bitrise CI is failing with this error |
|
[@pacu]
I'll get onto this now 👍 Need to update the rust environment setup. [@pacu]
I might need your help to update some pre build steps for setting up the rust environment, should be basically the same as the update to the travis script. |
This adds the nightly toolchain as well as the required targets
The upstream revision for `librustzcash` no longer exists! (It's pointing to a branch that is still being worked on, though should be merged soon) This change updates the reference in `Cargo.toml` to a compatible one and updates: - `rust/src/lib.rs` - `ZcashLightClientKit/zcashlc/zcashlc.h` - `ZcashLightClientKit/Rust/ZcashRustBackend.swift` ... to account for the updates in the new upstream reference.
nuttycom
approved these changes
Oct 5, 2021
There was a problem hiding this comment.
This looks good! nuttycom/librustzcash@cc58a21 is the most recent commit on the librustzcash branch and I hope the commit that will actually be merged soon, but the commit you're at here should be fine.
nuttycom
reviewed
Oct 5, 2021
nuttycom
reviewed
Oct 5, 2021
pacu
suggested changes
Oct 5, 2021
Scripts/build_librustzcash_xcode.sh
Outdated
| ZCASH_ACTIVE_ARCHITECTURE="aarch64-apple-ios" | ||
| fi | ||
|
|
||
| echo "fix 'permission denied issue'" | ||
| chmod -R +w ${PODS_TARGET_SRCROOT} | ||
|
|
||
| echo "cargo lipo --manifest-path ${PODS_TARGET_SRCROOT}/Cargo.toml --targets $ZCASH_ACTIVE_ARCHITECTURE --release" | ||
| echo "cargo +nightly build --manifest-path ${PODS_TARGET_SRCROOT}/Cargo.toml --target $ZCASH_ACTIVE_ARCHITECTURE --release" |
There was a problem hiding this comment.
@str4d suggested that we don't build nightly when the Xcode target is release. This would make every build, to run in nightly.
nightly should be used (for now) only for targets that require so. otherwise do it as always.
NOTE: This could be kind of confusing. The --release argument is for rust, which is fine and should be built that way, otherwise the rust compiler will run without optimizations.
There was a problem hiding this comment.
yeh this sounds like a good idea 👍 I'll add this change.
We now explicitely pin a nightly toolchain version for building for arm64 iOS simulator and use stable toolchain for any other architecture
|
Bitrise is failing with |
dh-ecc
force-pushed
the
m1-fixes
branch
2 times, most recently
from
3b59336 to
6161e30
Compare
Reintroduce `cargo-lipo` as a dependency since bitrise is building for multiple architectures.
|
@pacu Bitrise is failing because it is building in To sort this I reintroduced Would you be able to add a |
pacu
approved these changes
Oct 11, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[Addressing #309]
This is the absolute bare minimum diff to get the pod integration working for M1 machines, which can be demonstrated in this repo by
pod installing in theExample/ZcashLightClientSample/directory and running on simulator.In doing so we remove some rust build dependencies, but (at the time of writing) can only build on the
rust nightlytoolchain, so we add this as a dependency.Caveats / Known issues
We currently only build the rust library for a single architecture at a time, so this prevents some types of archive builds. (This is addressed with xcframeworks in the assessment of [Assessment/Research Spike] Simplifying Cocoapods build and Distributing over SPM #311)I've added back incargo-lipofor the time being so we can build universal builds, but this will still be superseded when moving to xcframeworks.Cargo.lockupdates are sufficient to make the rust lib build correctly on the nightly toolchain, however I don't know if this is the most idiomatic way to declare this, I feel like there could be a better alternative by declaring something in theCargo.tomlinstead.The following commands should be sufficient to test this working and should work on both intel and m1 macs.
It assumes you have working
rustupandpodcommands, try running it in a/tmp/folder 😄The build command is a very fancy way of saying "build it for an iOS Simulator on my machine" without having to assume an OS version or device that the host might have 😅, Some inspiration from https://nshipster.com/simctl/
. Would be much nicer if it was possibe to something like
-destination "generic/platform=iOS Simulator"but as far as I know this is not possible... please let me know if there is!Author
Automated tests: Did you add appropriate automated tests for any code changes?N/ACode coverage: Did you check the code coverage report for the automated tests? While we are not looking for perfect coverage, the tool can point out potential cases that have been missed.N/ADocumentation: Did you update Docs as appropiate? (E.g README.md, etc.)Explicitly deferredDid you provide Screenshots of what the App looks like before and after your changes as part of the description of this PR? (only applicable to UI Changes)N/AReviewer