chore(ffi): use ruma's built-in uniffi support to eliminate some event type enums#6161
Conversation
e0e9256 to
5acec1d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6161 +/- ##
==========================================
+ Coverage 89.86% 89.89% +0.02%
==========================================
Files 396 396
Lines 110260 110260
Branches 110260 110260
==========================================
+ Hits 99088 99113 +25
+ Misses 7395 7378 -17
+ Partials 3777 3769 -8 ☔ View full report in Codecov by Harness. |
|
This is failing the complement-crypto tests now. These downgrade uniffi to 0.28.3. This no longer works because ruma's new uniffi support depends on changes from uniffi 0.31.0. In particular:
I can think of the following ways to resolve this:
I'm not sure which of these is worse but maybe I've missed a better option? |
|
The better option is to help move NordSecurity/uniffi-bindgen-go#86 forward, IMHO. |
That's still two minor versions behind what ruma needs but, yes, ideally the complement crypto tests wouldn't have to downgrade uniffi in the first place. |
|
Yeah, but it would unblock additional upgrades (which may be much easier than that one, if you're lucky ^^). |
|
This should be unblocked as matrix-org/complement-crypto#233 is now merged. |
3ae18f2 to
41aaf22
Compare
|
Looks like this still fails the complement-crypto tests. Will have to investigate what's wrong there. |
Hywan
left a comment
There was a problem hiding this comment.
Thank you for your contributions!
| complement-crypto: | ||
| name: "Run Complement Crypto tests" | ||
| uses: matrix-org/complement-crypto/.github/workflows/single_sdk_tests.yml@399a1deeab0d7e4fa9604cbe83b1df6058c40193 # main | ||
| uses: matrix-org/complement-crypto/.github/workflows/single_sdk_tests.yml@1b1f86348079608fbc5276fe77c54dd66638a622 # main |
There was a problem hiding this comment.
This is the commit that updated complement-crypto to uniffi 0.31.0 (which is the version the uniffi support in Ruma requires) and it is slightly ahead of the currently used SHA: https://github.com/matrix-org/complement-crypto/commits/main/
The error appears to refer to this method. But confusingly that does already return |
|
Sigh. I think |
f5efb05 to
c8e7f7a
Compare
|
The cargo-deny error seems to also exist on |
andybalaam
left a comment
There was a problem hiding this comment.
This looks reasonable to me, without much context on the complement-crypto changes etc. Waiting for Ivan to comment, and asked one question about what I think is a merge mistake.
andybalaam
left a comment
There was a problem hiding this comment.
Good with me but I think there are unresolved conversations with other reviewers.
|
Sorry, I'm now realizing the bindings tests have uncovered a few more name clashes such as:
I'm not sure why these didn't show up before. I think this will require renaming the exported FFI type names or trying to eliminate the FFI types in favour of the Ruma types. |
9353167 to
b95e1d4
Compare
|
Moving back to draft as this seems to require still further work. 😕 |
b95e1d4 to
6dd33bd
Compare
23d068d to
a496a7a
Compare
| #[derive(Clone, uniffi::Enum, PartialEq, Eq, Hash)] | ||
| #[uniffi::export(Eq, Hash)] | ||
| pub enum TimelineEventType { | ||
| pub enum FfiTimelineEventType { |
There was a problem hiding this comment.
This required renaming because it caused a name-clash with the identically named Ruma type which is now also exported. Ruma's type is flat. So this wouldn't have been a drop-in replacement.
I tried renaming just the exported type name but that broke auto-generating the Eq and Hash implementations.
There was a problem hiding this comment.
I tried renaming just the exported type name but that broke auto-generating the
EqandHashimplementations.
Not sure what this means exactly, but sounds like an upstream bug? Did you file an issue already?
There was a problem hiding this comment.
I first tried adding #[uniffi::export(name = "FfiTimelineEventType")] but that then failed when generating bindings claiming that it cannot locate TimelineEventType. I have a hunch this might be because of the exported Eq and Hash implementations but haven't verified that. Reporting upstream sounds sensible assuming we can reproduce it in a minimal example.
There was a problem hiding this comment.
Tried to reproduce it in a minimal example but couldn't. So I may have hit something else or maybe it's already been fixed.
89765ef to
74c3f60
Compare
74c3f60 to
521bfb0
Compare
|
It looks like this is finally ready. The remaining lint error also exists on |
poljar
left a comment
There was a problem hiding this comment.
Looks good to me. We should just mention the rename of the exported type in the changelog as well.
| #[derive(Clone, uniffi::Enum, PartialEq, Eq, Hash)] | ||
| #[uniffi::export(Eq, Hash)] | ||
| pub enum TimelineEventType { | ||
| pub enum FfiTimelineEventType { |
There was a problem hiding this comment.
That's a bit of an ugly name, hopefully the users of the bindings won't mind too much.
The more important bit here is that this is a breaking change though. We should mention this in the changelog.
There was a problem hiding this comment.
Yeah, it's not pretty. Eventually this should just be replaced with the ruma type. That one has a different structure though. So doing that would have been a bigger breaking change than the renaming.
Maybe CC @stefanceriu for visibility on this.
There was a problem hiding this comment.
And @jmartinesp as well.
Beware there's an annoying rename in this PR.
There was a problem hiding this comment.
Argh, sorry for forgetting the Android side. I'm lacking coffee. 🙈 ☕
…t type enums Signed-off-by: Johannes Marbach <n0-0ne+github@mailbox.org>
521bfb0 to
bdb87ce
Compare
|
I feel like I've said this quite a few times, but next time we do something breaking in the FFI side, could we have some time to test it before merging something 😅 ? This broke a few unit tests we have on Android, since now |
Oh, I thought about requesting a real life test for it but underestimated how much this changed things. Sorry. If it's problematic we can revert this and reaply later, as it's a single commit. |
|
No, I think it's fine, it's just a few unit tests we'll disable. |
|
Apologies. I probably also should have pinged you about this much earlier than today. Sorry for the extra hassle. |
This is the smallest possible way that I could find to start using ruma/ruma#2338 in the FFI bindings. It eliminates the types
StateEventType,MessageLikeEventTypeandRoomAccountDataEventTypefrom the FFI crate and instead uses the Ruma types directly .The only difference of the two types in the foreign language should be the use of
Customvs.Other.