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

Workspace broken | UniFFI generates invalid Swift & Python bindings #2153

Open
Sajjon opened this issue Jun 14, 2024 · 9 comments
Open

Workspace broken | UniFFI generates invalid Swift & Python bindings #2153

Sajjon opened this issue Jun 14, 2024 · 9 comments

Comments

@Sajjon
Copy link
Contributor

Sajjon commented Jun 14, 2024

(@bendk asked me to create an issue )

Following up on my comment in 2087, using UniFFI with a Workspace works well in the land of Rust - especially with the new system for remote / custom / external types laid out in #2087.

However, when one generates Swift bindings, no cigar, the Swift code in each file tries to reference (file)private symbols such as lift/lower` on types in the other file - which are not visible. The Kotlin bindings, however, does seem to work. The Kotlin Bindgen test in my small UniFFI Workspace demo project Lemon Meringue Pie passes. I have not tried doing an actual Kotlin bindgen generation besides the bindgen test.

The Python bindings seems broken, for the small Python Bindgen tests in Lemon Meringue Pie, I get error:

    from .common import Uuid
ImportError: attempted relative import with no known parent package

We cannot simply change the access modifier to internal in the generated Swift code since each generated Swift file contains the same types, such as fileprivate protocol FfiConverterRustBuffer.

In this issue I lay out a straightforward idea on how to fix the broken Swift bindings specifically. I do not know enough about Python to lay out a plan for it, but perhaps the say idea is viable also for Python?

Idea of solution for Swift bindings

My idea is to:

  1. All generated Converters will be placed in separate files, in lets call it generated/ folder (as specified by --out-dir), and only be generated once, i.e. the contents of template/StringHelper.swift will be put Generated/ScaffoldingUniFFI folder
  2. The actual type - fileprivate struct FfiConverterString: FfiConverter - gets a changed visibility to internal struct instead of fileprivate.
  3. All protocols, types, functions and methods in RustBufferTemplate (and similar templates) get changed visibility to internal too
  4. Bindgen code for a single crate only contain the author written UniFFI code in one file per crate, places in Generated/Foo, Generated/Bar, Generated/Buzz, where Foo, Bar, `Buzz are the names of each of the users UniFFI exported crates.

This is a straightforward solution which should not involve too much changes here in UniFFI.

The only downside of this solution I can think of is that some users of UniFFI might need to migrate their build scripts from mv $out/Foobar.swift to `mv $out/Generated (or similar).

@mhammond
Copy link
Member

(@bendk asked me to create an issue )

Following up on my comment in 2087, using UniFFI with a Workspace works well in the land of Rust - especially with the new system for remote / custom / external types laid out in #2087.

However, when one generates Swift bindings, no cigar,

I don't really understand the problem you are describing - how is this different from what we do in our tests and examples?

The Python bindings seems broken, for the small Python Bindgen tests in Lemon Meringue Pie, I get error:

    from .common import Uuid
ImportError: attempted relative import with no known parent package

This looks just like https://mozilla.github.io/uniffi-rs/latest/python/configuration.html#external-packages - uniffi doesn't attempt to generate the complete package structure and __init__.py etc files.

In this issue I lay out a straightforward idea on how to fix the broken Swift bindings specifically.

Can you please try and clarify exactly what is broken for you, ideally by using our examples and fixtures?

@Sajjon
Copy link
Contributor Author

Sajjon commented Jun 14, 2024

@mhammond UniFFI does not support Swift bindgen of Cargo Workspace where CrateX depend in CrateY - where both CrateX and CrateY are UniFFI crates.

UniFFI generates broken Swift code.

@mhammond
Copy link
Member

We have a number of examples where CrateX depends on CrateY (eg, https://github.com/mozilla/uniffi-rs/tree/main/fixtures/ext-types) - I'm trying to understand what's different about your use-case which is broken vs that which apparently is not.

@Sajjon
Copy link
Contributor Author

Sajjon commented Jun 14, 2024

Next week I can write a detailed report - with minimalistic code examples.

@Sajjon
Copy link
Contributor Author

Sajjon commented Jun 25, 2024

Next week I can write a detailed report - with minimalistic code examples.

I did not forget, just have to focus on completing some tasks at work this week before my vacation, will get back to this next week!

@Sajjon
Copy link
Contributor Author

Sajjon commented Jul 11, 2024

@mhammond @bendk OK so I've created three small demo projects - projects which uses UniFFI with many crates having UniFFI as a dependency in a workspaces setup:

Repo Works? SemVer No of crates Description
Alpha ☑️ 0.28.0 2 Crate "two" references proc-macro exported types from crate "one", "imported" in UDL. Swift & Kotlin bindings tests works, Python does not compile.
Beta 0.28.0 2 Newtype does not work
Gamma ☑️ 0.28.0 3 Crate "one" references proc-macro exported types from crate "zero", "imported" in UDL. Crate "two" references types from "zero" and "one". Swift & Kotlin bindings tests works, Python does not compile.

Insights:

  • Python seems to not work at all with Workspace setup
  • Seems that newtype cannot be "imported" at all (see: repo "beta"). hopefully I'm simply missing something?
  • Other finding: UniFFI's binding test macro is excellent, the combined.modulemap merges many crates together, why do we not support this for prod? for the generate CLI?

Do you want me to create some demo projects using @bendk s fork (source branch of PR #2087 )?

@mhammond
Copy link
Member

I would like to see this reproduced in our ext-types fixture against main. There we have many crates, all in the top-level workspace, and all of which depend on each other in various ways, including directly and indirectly. It's also worth noting that what you describe above is exactly how mozilla uses uniffi in production.

Assuming Python doesn't compile due to an invalid package import, I believe your problem is that you don't have an appropriate uniffi.toml for your project. If true, I believe that would explain all the symptoms described. If flase, by working against that set of fixtures it would help to better understand what is different about your setup than our setups and form a great starting point for having our fixtures exercise that.

@Sajjon
Copy link
Contributor Author

Sajjon commented Jul 13, 2024

@mhammond ok so here is an update:

  1. Workspace works with Python if one uses [bindings.python.external_packages]; crate_x = ""; crate_x = "" I had missed this completely, so my bad.
  2. newtypes can be imported across crates in workspace setup, by use of uniffi::ffi_converter_forward! macro call, however, please note that this macro is never mentioned in any documentation. I had used this macro many months ago but completely forgot about it. It seems to be the only way to get a proc-macro (custom_newtype!) newtype to be usable across crates in a workspace setup.
  3. What about my proposal of productifying the test macro's excellent combined.modulemap? It seems to be one missing piece in the Workspace puzzle.

@mhammond
Copy link
Member

newtypes can be imported across crates in workspace setup, by use of uniffi::ffi_converter_forward! macro call,

Oops - the docs should mention that, but I think that's going to go away with Ben's work.

What about my proposal of productifying the test macro's excellent combined.modulemap? It seems to be one missing piece in the Workspace puzzle.

While that seems fine, I'm not sure what problems you are actually seeing here. Or to put it another way, what are you trying to do that's different to our fixtures?

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