-
Notifications
You must be signed in to change notification settings - Fork 260
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
ffi: implement our own export
macro that always sets up the correct async runtime
#4094
Conversation
Including one that will always warn if used with async functions, and the other one always setting the tokio runtime if used for async stuff.
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 think we can wait a bit for the second commit, otherwise I would prefer to merge only the first commit. It's unlikely that anybody will use the wrong macro, people just copy paste things.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4094 +/- ##
==========================================
- Coverage 84.67% 84.67% -0.01%
==========================================
Files 269 269
Lines 28753 28753
==========================================
- Hits 24348 24347 -1
- Misses 4405 4406 +1 ☔ 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 like the first commit, and I think it's fine, we may not need the second if it's too much work. Well done, thanks!
description = "Helper macros to write FFI bindings" | ||
edition = "2021" | ||
homepage = "https://github.com/matrix-org/matrix-rust-sdk" | ||
keywords = ["matrix", "chat", "messaging", "ruma"] | ||
license = "Apache-2.0" | ||
name = "matrix-sdk-ffi-macros" | ||
readme = "README.md" | ||
repository = "https://github.com/matrix-org/matrix-rust-sdk" | ||
rust-version = { workspace = true } | ||
version = "0.7.0" |
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.
authot is missing :-).
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.
We don't usually set it, or we set it to Damir. Would need to do something more general for all the crates we have :)
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.
like setting all to "The Rust Matrix SDK developers" or something like this
145910c
to
327a46f
Compare
Dropped the second commit (since it's a bug in Clippy) and enabled auto-merge. Thanks for the quick reviews! |
@@ -0,0 +1,24 @@ | |||
[package] | |||
description = "Helper macros to write FFI bindings" | |||
edition = "2021" |
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.
2024?
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.
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.
@bmarty You can learn what a Rust edition is by reading https://doc.rust-lang.org/edition-guide/editions/index.html :-).
The first commit implements new macros:
matrix_sdk_ffi_macros::export
must be used for all sync functions and impl blocks. It will complain if anything is async (the exported function or any function in the exported impl block).matrix_sdk_ffi_macros::export_async
must be used for async functions and impl blocks. Currently, uniffi already complains if the exported item doesn't involve async, so no code had to be added for that.The second commit prevents use of the
uniffi::export
macro by making it into a clippy unauthorized macro, but then the local#[allow()]
statement doesn't seem to be taken into account when using theuniffi::export
macro internally :( I've opened a thread on the Rust Zulip, to ask if I've missed something by chance or not.Ways to move forward