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

dogfooding new remote types #2334

Closed
flisky opened this issue Nov 28, 2024 · 8 comments
Closed

dogfooding new remote types #2334

flisky opened this issue Nov 28, 2024 · 8 comments

Comments

@flisky
Copy link
Contributor

flisky commented Nov 28, 2024

Hello, I'm using the new merged #2317 to avoid some callback wrappings between uniffi binding and core crate. And it's served well, but I still find some glitches, and want to share here.

As a old project starting with uniffi v0.17, we have a mixed udl and procmacro for binding crate, and only procmacro for core crate.

udl: Remote for interface [Enum]

It seems the old interface enum cannot be annotated both with [Enum] and [Remote], because [Enum] is specified it must be the only attribute.

// this panics
[Enum, Remote]
interface Foo {};

And [External=...] is a workaround. It doesn't really matter to me.

procmacro: bindings for custom types

The custom types in core crate without referenced by other core uniffi types, are not visible in python bindings (maybe all other bindings, neither?), and it seems due to extract_from_library doesn't give a code for custom types?

I have to do a dummy uniffi::Record to export them. I can stay with this.

python: ImportError: attempted relative import with no known parent package

We use uniffi's bindgen-tests feature to run python test. However, it throws the above error message because core crate is already a root package.

I tried with bindings.python.external_packages to give core crate a namespace, and cargo test still lacks the structure. More importantly, the core binding should be the same directory as the dylib file, because it calls ctypes.cdll.LoadLibrary!

I have to manually create a directory beside the test file, move all generated assets under it, import them from that package, and run python test_binding.py by hand... It's painful, so I have to disable the test.


That's all my journey, and thank you all for the hard working!

@mhammond
Copy link
Member

Thanks for the report:

udl: Remote for interface [Enum]

We should look in to that!

procmacro: bindings for custom types
The custom types in core crate without referenced by other core uniffi types, are not visible in python bindings

If I understand correctly, that's intended - only types actually used are exposed to foreign bindings. I might be misunderstanding though, so an example would be appreciated.

python: ImportError: attempted relative import with no known parent package
I tried with bindings.python.external_packages to give core crate a namespace,

To be clear, the problem you are having is that uniffi_test doesn't really work in that scenario because there's no opportunity to put the generated files into the correct package structure? But for your "real" use of that code, it's fine because you can change that structure?

If so, that's a good and interesting point. We welcome ideas (or even better, PRs :) which tried to address this.

@flisky
Copy link
Contributor Author

flisky commented Nov 28, 2024

If I understand correctly, that's intended - only types actually used are exposed to foreign bindings.

Yes and no.

In core crate:

struct Foo { ... }

uniffi::custom_types!(Foo, String, { ... });

In binding crate:

#[uniffi::export]
fn use_foo(foo: core::Foo) { ... }

In python binding:

from .core import Foo

But core.py has no Foo


It could be wonderful if binding crate registers Foo to core crate, but for now, it seems the foreign code generation is dependent from each other?

@mhammond
Copy link
Member

We have some examples of this - eg, here's a custom type in a "core" crate and here is where it is used in a "binding" crate - and here's the Python test for that function

So I'm not sure what the difference is in what you are reporting versus what we have tests for.

@flisky
Copy link
Contributor Author

flisky commented Nov 28, 2024

So I'm not sure what the difference is in what you are reporting versus what we have tests for.

Because Ouid is referenced by get_ouid, so it's exported. And you can take a look at this commit to see the difference, and it also shows my third problem.

@mhammond
Copy link
Member

Sorry, but bouncing back to that first issue (#2352, thanks for that info!)

And [External=...] is a workaround. It doesn't really matter to me.

I don't see how [Remote] and [External] can be interchanged - do you mind explaining more about how you are using this?

I'll try and get more insight into the Ouid soon.

@flisky
Copy link
Contributor Author

flisky commented Dec 13, 2024

I don't see how [Remote] and [External] can be interchanged - do you mind explaining more about how you are using this?

Haha, because the binding and core crates are both owned by me, and I would use uniffi::export in core crate and [External] in binding udl. It would be only way to use [Remote] if core crate has no uniffi support.

@mhammond
Copy link
Member

mhammond commented Dec 20, 2024

So the only thing left here is Python and I agree that's a bit of a mess. We don't use python + uniffi, so I'd love to hear your thoughts on how this might work with multiple crates and packages. It seems wrong to insist the .so is next to the .py even without that consideration. Maybe we should open a new issue for improving the python package story?

Really happy to hear you've been using this for so long and it keeps working for you! I'd love your feedback on #2355 too!

@flisky
Copy link
Contributor Author

flisky commented Dec 21, 2024

The change of #2355 is welcomed because the more type info, the more good. 👍

Talking about python binding, actually, we're using python + uniffi only when testing.
We're detaching the uniffi bindgen-tests feature, instead, some glue codes, likeMakefile, are good to go:)

Thank you for response, I'm totally satisfied.

@flisky flisky closed this as completed Dec 21, 2024
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

No branches or pull requests

2 participants