-
Notifications
You must be signed in to change notification settings - Fork 246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
crypto: Apple Crypto Bindings | Fix crypto xcframework script #3334
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3334 +/- ##
=======================================
Coverage 83.22% 83.23%
=======================================
Files 247 247
Lines 25051 25051
=======================================
+ Hits 20848 20850 +2
+ Misses 4203 4201 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not confident enough to review this change, since I have no clues about the uniffi build step; maybe @poljar or Ivan (when back) could chime in, please?
Also: would it make sense to add a CI step to make sure this always runs and is never broken again?
# Rename and move modulemap to the right place | ||
mv ${GENERATED_DIR}/*.modulemap ${HEADERS_DIR}/module.modulemap | ||
# Rename and merge the modulemap files into a single file to the right place | ||
for f in ${GENERATED_DIR}/*.modulemap; do cat $f; echo; done > ${HEADERS_DIR}/module.modulemap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we split this statement into multiple lines here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
It looks OK to me. Please address the feedback from @bnjbvr and rebase your commits. Once it's done, I can approve this PR :-).
It's also surprising that it's not detected by the CI. Do you know if the CI is tested this script? I was sure we were testing it previously, but running a git rev-list --all | xargs git grep …
returns nothing valuable. Maybe let's add this check on the CI? I can take a while though… cc @poljar
Can you update |
Yeah, if possible we should ensure that this works using CI. |
I added a new job in bindings_ci |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new CI step takes 38 minutes, which is longer than the whole pipeline, so we agreed to try to make it shorter first.
1d09b33
to
40e3d40
Compare
Now |
1f92955
to
ee040ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tiny change and we are good! You don't need to add all these targets for Rust with rustup add target …
because you run build_crypto_xcframework.sh
with -i
, so only for iOS.
save-if: ${{ github.ref == 'refs/heads/main' }} | ||
|
||
- name: Run the Build Framework script | ||
run: ./bindings/apple/build_crypto_xcframework.sh -i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The -i
option describes the fact you want to test for iOS only. Then, this is not necessary to add the following targets with rustup
hereinabove (in the “Add rust targets” step):
aarch64-apple-darwin
x86_64-apple-darwin
aarch64-apple-ios-sim
x86_64-apple-ios
.
Only the aarch64-apple-ios
target can be kept, according to https://github.com/matrix-org/matrix-rust-sdk/pull/3334/files#diff-3bbe5c410acf8c549fb356c29229df7b45a37e8db0932b773ce6294cc3935262R44.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done b430d95
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, kudos!
Fixes #3310
The script to build the crypto bindings was not working anymore since the move from UDL to procedural macro.
Modified as per the following recommendations https://mozilla.github.io/uniffi-rs/tutorial/foreign_language_bindings.html#running-uniffi-bindgen-using-a-library-file-recommended
There is some changes in the generated framework, there are now two swift files and two modulemap files that have to be merged together in the final xcframework.
Previous structure was
The new one is
Signed-off-by: