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

Add callback interfaces for Swift #1066

Merged
merged 11 commits into from
Oct 7, 2021
Merged

Conversation

jhugman
Copy link
Contributor

@jhugman jhugman commented Oct 4, 2021

Fixes #353. It brings Swift parity with Kotlin, from #344 .

This is a rebase of #1045 by @pagr, following landing of #1042 and #1061.

Contributors @pagr and @patrick-fitzgerald will be interested in this.

Also in this PR:

  • deleted the examples/callback which seemed to be a duplicate to the fixture/callbacks directory
  • a cosmetic touch up to the kotlin implmentation of callbacks
    • changing names to match the ffi_converter language @bdk pioneered.

@jhugman jhugman requested a review from a team October 4, 2021 23:02
jhugman and others added 2 commits October 5, 2021 14:56
Kotlin relies on Handle instead of Long (or ULong)

Align kotlin implementation with swift
@jhugman jhugman force-pushed the jhugman/swift-callbacks-pagr branch from c5bbc25 to 0c66759 Compare October 5, 2021 13:57
@jhugman jhugman force-pushed the jhugman/swift-callbacks-pagr branch from 0c66759 to f17be20 Compare October 5, 2021 13:59
Copy link
Member

@travis79 travis79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had just a couple of suggestions, overall this is looking awesome!

fixtures/callbacks/tests/bindings/test_callbacks.swift Outdated Show resolved Hide resolved
fixtures/callbacks/tests/bindings/test_callbacks.swift Outdated Show resolved Hide resolved
fixtures/callbacks/tests/bindings/test_callbacks.swift Outdated Show resolved Hide resolved
fixtures/callbacks/tests/bindings/test_callbacks.swift Outdated Show resolved Hide resolved
fixtures/callbacks/tests/test_generated_bindings.rs Outdated Show resolved Hide resolved
@jhugman jhugman merged commit d2b8d56 into main Oct 7, 2021
@jhugman jhugman deleted the jhugman/swift-callbacks-pagr branch October 7, 2021 12:04
@jhugman jhugman mentioned this pull request Oct 7, 2021
@patrick-fitzgerald
Copy link

Looks great thanks @jhugman!

@DAOCUONG
Copy link

DAOCUONG commented Oct 7, 2021

I have experience error when tried to add main uniffi after this commit

Compiling uniffi v0.14.0 (https://github.com/mozilla/uniffi-rs.git?branch=main#d2b8d56a)
error[E0308]: mismatched types
--> /Users/dao/.cargo/git/checkouts/uniffi-rs-6f89edd2a1ffa4bd/d2b8d56/uniffi/src/testing.rs:144:34
|
144 | .args(&["test", out_dir, udl_files, test_file])
| ^^^^^^^^^
| |
| expected &str, found struct String
| help: consider borrowing here: &udl_files

For more information about this error, try rustc --explain E0308.
error: could not compile

saks pushed a commit to saks/uniffi-rs that referenced this pull request Oct 8, 2021
* Added swift callback support

* Temporary fix for running callbacks in another thread

* Generate all the required swift code

* Make run_uniffi_bindgen_test build

* [wip] add tests for callbacks (mozilla#1)

Co-authored-by: Paul Griffin <[email protected]>

* Fix nits

* Fit new Swift callbacks into the new Unit of Code structure

Re-work to fit in to the new structure

Change to FfiConverter language

cargo fmt

Add docs to include swift

* Backfill Kotlin callbacks to closer match the Swift implementation

Kotlin relies on Handle instead of Long (or ULong)

Align kotlin implementation with swift

* Fixup markdown weirdness

* Address self-review

* Address reviewer comments

Co-authored-by: Paul Griffin <[email protected]>
Co-authored-by: Paul Griffin <[email protected]>
@bendk
Copy link
Contributor

bendk commented Oct 11, 2021

@DAOCUONG I'm seeing the same issue. If you don't need to use the absolute latest code, you could try using uniffi from crates.io until it's fixed.

@jhugman I think it's just a one-liner, but it's hard to test because it only runs when builtin-bindgen is not set, which I don't think happens when the tests are run from the uniffi crate. Can you take a look at the above PR and see if it seems okay?

@artfuldev
Copy link

I'm seeing this message but wasn't sure if I should be, since I thought we implemented callback interfaces for swift in this PR:
image

I'm using 0.14 - is there another version I should be using?

@badboy
Copy link
Member

badboy commented Oct 27, 2021

I'm seeing this message but wasn't sure if I should be, since I thought we implemented callback interfaces for swift in this PR: image

I'm using 0.14 - is there another version I should be using?

There was no release since we merged this.

@artfuldev
Copy link

There was no release since we merged this.

Thank you so much! When can we expect the next release?

@bendk
Copy link
Contributor

bendk commented Oct 27, 2021

Just pushed it out, 0.14.1 should have what you need.

@artfuldev
Copy link

artfuldev commented Oct 27, 2021

@bendk
Copy link
Contributor

bendk commented Oct 27, 2021

I'm not sure what's happening, but it looks like you have version of the scaffolding built with the older unify. I would try cargo clean and see what happens.

That CI failure is related, but I don't think it's the cause of your issue.

@DAOCUONG
Copy link

Reinstall uniffi_bindgen will help

@artfuldev
Copy link

artfuldev commented Oct 28, 2021

Sorry to pollute this PR thread, but it doesn't work after cargo clean. Fails with the exact same error. I can share my Cargo.toml:
image

I've updated all my dependencies so I'm not sure what else I can do.

@bendk
Copy link
Contributor

bendk commented Oct 28, 2021

I think DAOCUONG might be right, it seems to me that uniffi and uniffi_bindgen are out of sync. Maybe try cargo install uniffi-bindgen or switching the dependency to:

uniffi_build = { version = "0.14.1", features=["builtin-bindgen"] }

If that doesn't work, feel free to reach out on matrix or email me directly ([email protected]).

@artfuldev
Copy link

Thanks @bendk , @DAOCUONG
I don't know how, but somehow the versions did get mismatched even though I updated my uniffi_bindgen.

I did

uniffi-bindgen --version

and got 0.14.1 as expected, so I couldn't understand why it failed cargo build.

After your responses, I did

cargo clean
cargo uninstall uniffi_bindgen
cargo install uniffi_bindgen
cargo build

and it worked.

}
```

Note: in Swift, this must be a `class`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't it be a struct

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.

Implement callback interfaces for Swift
9 participants