-
Notifications
You must be signed in to change notification settings - Fork 24
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
feat: upgrade to uniffi25 #26
Conversation
1572f7d
to
5a31cae
Compare
this is ready for review, only the latest commit is new |
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.
Looks good, I left a small nits, but one thing that stood out was the object.pointer == nil
check. Could you provide more context why it was added?
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.
Do you have time the make the remaining changes? It looks like the only major issue is error handling in async code. We have a requirement to have 0.25 ready in one weeks time, so I'm also invested to have this finished, and could offer some help.
should be removed now |
I tried to do it, but ran into a bunch of import issues, where templates were either missing or included twice, even though I used the |
You are right, looks like my "trick" is actually buggy. It doesn't work when |
This almost compiles for me, just missing #21 (comment) - having a struct X with a field called I can also confirm this makes async functions work correctly (previously on the 0.24 PR they would block indefinitely). |
However, using bleeding edge rust code which uses a newer commit of uniffi-rs seems to fail to link:
I think it's because of mozilla/uniffi-rs@25ba41d (after much |
About the |
yeah bleeding edge won't work until we upgrade to the next released version, due to breaking changes under the hood |
Hmm, this is not nice. mozilla/uniffi-rs@25ba41d is tagged with 0.25.1, so it should be compatible with 0.25. |
False alarm. The commit you specified is from |
I'm proceeding to merge this into main. This has been open for quite a while, and I think its time to merge this. I'm also under pressure to get this ready for our internal use 😅 Huge thanks @dignifiedquire for your efforts on this!! |
Upgrades to NordSecurtiy/[email protected]
Based on #13.
TODOs