NordSecurity / uniffi-bindgen-go

Uniffi bindings generator for Golang
Mozilla Public License 2.0
75 stars 21 forks source link

feat: upgrade to uniffi25 #26

Closed dignifiedquire closed 10 months ago

dignifiedquire commented 11 months ago

Upgrades to NordSecurtiy/uniffi-rs@0.25.0-1 Based on #13.

TODOs

dignifiedquire commented 11 months ago

this is ready for review, only the latest commit is new

dignifiedquire commented 11 months ago

I don't see any changes.

should be removed now

dignifiedquire commented 11 months ago

Will you change this to reuse the sequence template, or do you still see possible optimizations here?

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 include_once_check. So I would leave it for now.

arg0d commented 11 months ago

I tried to do it, but ran into a bunch of import issues, where templates where either missing or included twice, even though I used the include_once_check. So I would leave it for now.

You are right, looks like my "trick" is actually buggy. It doesn't work when UInt8 isn't used in definitions somewhere, and also clashes with Vec<u8> type. I will need to do a fix for C# aswell :sweat_smile:

kegsay commented 10 months ago

This almost compiles for me, just missing https://github.com/NordSecurity/uniffi-bindgen-go/issues/21#issuecomment-1774958892 - having a struct X with a field called Error, where X meets the error interface, causes names to collide as the error interface means there is a function called Error() and a field called Error which isn't valid.

I can also confirm this makes async functions work correctly (previously on the 0.24 PR they would block indefinitely).

kegsay commented 10 months ago

However, using bleeding edge rust code which uses a newer commit of uniffi-rs seems to fail to link:

ld: Undefined symbols:
  _ffi_matrix_sdk_ffi_rust_future_continuation_callback_set, referenced from:
      __cgo_8643f67aa596_Cfunc_ffi_matrix_sdk_ffi_rust_future_continuation_callback_set in 000001.o

I think it's because of https://github.com/mozilla/uniffi-rs/commit/25ba41db5fee67afccf4f8efd9a4e03f02f2e378 (after much git bisecting..). I think this is because that commit removes what was a global callback to be per-crate, and that is not being defined.

arg0d commented 10 months ago

About the Error variant issue, it sounds like a separate issue unrelated to 0.25 update.

dignifiedquire commented 10 months ago

However, using bleeding edge rust code which uses a newer commit of uniffi-rs seems to fail to link:

yeah bleeding edge won't work until we upgrade to the next released version, due to breaking changes under the hood

arg0d commented 10 months ago

Hmm, this is not nice. https://github.com/mozilla/uniffi-rs/commit/25ba41db5fee67afccf4f8efd9a4e03f02f2e378 is tagged with 0.25.1, so it should be compatible with 0.25.

arg0d commented 10 months ago

False alarm. The commit you specified is from 0.25.1 branch, and 0.25.1 lib interops fine with 0.25.0 Go bindings. I think you meant https://github.com/mozilla/uniffi-rs/commit/eb97592f8c48a7f5cf02a94662b8b7861a6544f3 from main, and I can confirm that indeed 0.25.0 Go bindings don't interop with that commit from main branch. But this is expected, Go bindings aren't supposed to interop correctly with an arbitrary commit in main branch. Users should only use release tags, and match compatible lib/binding uniffi versions.

arg0d commented 10 months ago

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 :sweat_smile:

Huge thanks @dignifiedquire for your efforts on this!!