Skip to content
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

Merged
merged 71 commits into from
Nov 14, 2023
Merged

Conversation

dignifiedquire
Copy link
Contributor

@dignifiedquire dignifiedquire commented Nov 1, 2023

Upgrades to NordSecurtiy/[email protected]
Based on #13.

TODOs

@dignifiedquire dignifiedquire mentioned this pull request Nov 1, 2023
36 tasks
@dignifiedquire dignifiedquire marked this pull request as ready for review November 3, 2023 10:08
@dignifiedquire
Copy link
Contributor Author

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

.gitmodules Outdated Show resolved Hide resolved
Copy link
Collaborator

@arg0d arg0d left a 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?

bindgen/src/gen_go/mod.rs Outdated Show resolved Hide resolved
bindgen/src/gen_go/mod.rs Outdated Show resolved Hide resolved
bindgen/src/gen_go/mod.rs Outdated Show resolved Hide resolved
bindgen/src/gen_go/mod.rs Outdated Show resolved Hide resolved
bindgen/templates/Async.go Outdated Show resolved Hide resolved
bindgen/templates/BytesHelper.go Show resolved Hide resolved
bindgen/templates/CallbackHelpers.go Outdated Show resolved Hide resolved
bindgen/templates/ForeignExecutorTemplate.go Outdated Show resolved Hide resolved
bindgen/templates/ForeignExecutorTemplate.go Outdated Show resolved Hide resolved
bindgen/templates/ObjectRuntime.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@arg0d arg0d left a 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.

bindgen/templates/Async.go Outdated Show resolved Hide resolved
bindgen/templates/BytesHelper.go Show resolved Hide resolved
bindgen/src/gen_go/mod.rs Outdated Show resolved Hide resolved
@dignifiedquire
Copy link
Contributor Author

I don't see any changes.

should be removed now

@dignifiedquire
Copy link
Contributor Author

dignifiedquire commented Nov 9, 2023

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
Copy link
Collaborator

arg0d commented Nov 9, 2023

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 😅

@kegsay
Copy link
Contributor

kegsay commented Nov 13, 2023

This almost compiles for me, just missing #21 (comment) - 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
Copy link
Contributor

kegsay commented Nov 13, 2023

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 mozilla/uniffi-rs@25ba41d (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
Copy link
Collaborator

arg0d commented Nov 14, 2023

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

@dignifiedquire
Copy link
Contributor Author

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
Copy link
Collaborator

arg0d commented Nov 14, 2023

Hmm, this is not nice. mozilla/uniffi-rs@25ba41d is tagged with 0.25.1, so it should be compatible with 0.25.

@arg0d
Copy link
Collaborator

arg0d commented Nov 14, 2023

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 mozilla/uniffi-rs@eb97592 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
Copy link
Collaborator

arg0d commented Nov 14, 2023

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!!

@arg0d arg0d merged commit a3cf5d6 into NordSecurity:main Nov 14, 2023
3 checks passed
@arg0d arg0d mentioned this pull request Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants