-
Notifications
You must be signed in to change notification settings - Fork 0
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
support native async calls introduced in uniffi 0.24 #5
Comments
I created a branch of the kotlin multiplatform bindings that supports both the new callback interface and the ForeignExecutor. It was developed before unfffi 0.24.0 was released and arbitrarily targets mozilla/uniffi-rs@22b8c21 It should cover at least I was going to contribute it to https://gitlab.com/trixnity/uniffi-kotlin-multiplatform-bindings. However, the changes are quite massive, @benkuly doesn't have a whole lot of time to spare for the project and I'm not quite ready to merge this without a second pair of eyeballs going over it. Maybe we could team up on this (after I clean up my branch a little bit)? |
Great! I started to look into this so I'm happy to pair up with you. |
Glad to hear that uniffi-kotlin-multiplatform-bindings is making progress! 🙂 I'm open to add code owners to the original project, if you like to. |
Here is what I currently have: https://gitlab.com/fweissberg/uniffi-kotlin-multiplatform-bindings/-/tree/target_22b8c_w_async?ref_type=heads I am going to port the futures test from upstream uniffi but it's not finished, yet. Also, the callbacks tests are currently in disrepair because I originally just put some async tests there. Then, I discovered an issue with the JVM version of either the callbacks or async implementation when used together (see the comments in the test file). |
Cool, I'll try to dedicate some time to review it tomorrow. |
I finished porting the futures tests. Good thing I did, because it uncovered some bugs. |
I fixed the bug preventing the callbacks test from passing on the JVM and pushed the commit to the branch mentioned above. |
Nice! Not only the test method you commented was failing for me though but also all the regular callback tests like I see you added many async methods to the callback test. Was that for debugging purposes and do you intend to remove them again? |
Yes, I think much of the code can be removed. However, the bug itself was present before any of my patches, it was just not triggered. I'll try building a minimal regression test maybe by manually triggering the GC. |
I wasn't able to quickly come up with something that triggers the issue without using the async callbacks. At least, I reduced the async stuff in the callbacks test to a call to a single function and removed the rest of the clutter. It should all be covered by the futures test. Feel free to give it a try, though. I was going to give you permissions on my Gitlab fork of the project. But it seems that doesn't work because you logged into that service using your Github account? If it's more convenient, I could also move my fork to Github? |
Hmm interesting, I ran the tests on a Mac.
I'm @sunkig at GitLabs (https://gitlab.com/sunkig), was that the user you were trying to add? It would helpful be able to comment on code in your changes, maybe open a (draft) PR? |
@fweissberg btw if you want to discuss in real-time we can do it here: |
https://github.com/mozilla/uniffi-rs/blob/main/CHANGELOG.md#v0240-backend-crates-v0240---2023-06-21
The text was updated successfully, but these errors were encountered: