-
Notifications
You must be signed in to change notification settings - Fork 255
Workspace broken | UniFFI generates invalid Swift & Python bindings #2153
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
Comments
I don't really understand the problem you are describing - how is this different from what we do in our tests and examples?
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
Can you please try and clarify exactly what is broken for you, ideally by using our examples and fixtures? |
@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. |
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. |
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! |
@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:
Insights:
Do you want me to create some demo projects using @bendk s fork (source branch of PR #2087 )? |
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 |
@mhammond ok so here is an update:
|
Oops - the docs should mention that, but I think that's going to go away with Ben's work.
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? |
I believe I've encountered this issue as well when trying to construct a Swift Package from a Rust Workspace. Basically IMO types need to be namespace/package aware, e.g., ModuleA::TypeA1's dependency on ModuleB::TypeB1 needs to refer to it as ModuleB.TypeB1 not assume it can be referred to as TypeB1 in Swift code. Remove the assumption that types from different crates can be smashed into a single Swift module -- that may also alleviate some need for fixing the renaming types issue. See code/situation in #2382 (comment). The final I'm using these versions at the moment: [dependencies]
uniffi = { git = "http://github.com/mozilla/uniffi-rs", rev = "e6cb4a37cfd03ab053ab11324349feb280df3a22", features = [
"cli",
"default",
] }
uniffi_bindgen = { git = "http://github.com/mozilla/uniffi-rs", rev = "e6cb4a37cfd03ab053ab11324349feb280df3a22", features = [
"default",
] } |
It would be very helpful if you could explain how your use-case is different from our fixtures and/or demonstrate your problem in our fixtures. |
I created pull request #2396 that is a simplified bash version of the Rust code I have to build a Swift framework. It demonstrates what I believe to be the core issue that dependencies are not namespace aware, at least as far as I can tell in Swift, so one cannot put put the Swift code in different targets to avoid the other outstanding issue of naming clashes since can't name / prefix things. Of course I could be using the library wrong and I'm missing supplying some option. My preference for how things should work (without causing spam to linking to all the other issues) is:
However even with that there still may be problems, maybe like this one if still relevant: https://forums.swift.org/t/ambiguous-use-of-a-same-name-var-or-func-of-extension-from-different-modules/58306. If so perhaps the unappealing, but working, solution would be to generate Swift type names based on the package/mod path. Due to autocomplete, I don't really care if my type names are long, just want to not have to think how to adjust my Rust code to accommodate a FFI library too much. |
I'm still a little confused though - are we maybe conflating 2 different problems here:
Is that accurate? |
Yes, that is accurate.
And also:
Finally for distribution which isn't necessarily Uniffi-related:
|
When I last tried using a multi-UniFII-crate setup last summer it worked beautifully for Kotlin. Worked well for Swift with UniFFI bindgen tests but not for production. I later on realized it was because Uniffis bindgen test generation code merged all modulemaps into a combined, why it worked. I think later last year a new Swift bindgen cli tool was released that does this (merged the modulemap of each rust crate into one combined). But I havent tried it yet. |
How is that different from the name collision I described? Re the names: We've a few issues around swift naming which should be considered as a whole (so maybe we need one new meta issue?). I can't see why an option to rename swift items based on some kind of "pattern" wouldn't work (ie, being able to rename multiple items with a single config entry), which could probably deal with this. But ultimately I think it all will come down to "config" options, and having the swift bindings use them. We'd be happy to help someone having a go at this! Re the new example, I'll need time to look in detail but conceptually I love it, thanks! |
It probably is not different. What I was getting at, in probably a naive Swift way, was that perhaps qualifying the names could work like instead of code importing
But seeing threads like https://forums.swift.org/t/fixing-modules-that-contain-a-type-with-the-same-name/3025/26, makes me think renaming/prefixing solutions may be the only thing that will easily/reliably work. I wouldn't mind just specifying a Swift name for a type in the macro, or even better an Objective-C style prefix for a given scope of types (crate/mod levels) since that would require less boilerplate and decision making. |
Uh oh!
There was an error while loading. Please reload this page.
(@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 aslift
/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:
We cannot simply change the access modifier to
internal
in the generated Swift code since each generated Swift file contains the same types, such asfileprivate 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:
generated/
folder (as specified by--out-dir
), and only be generated once, i.e. the contents oftemplate/StringHelper.swift
will be putGenerated/ScaffoldingUniFFI
folderfileprivate struct FfiConverterString: FfiConverter
- gets a changed visibility tointernal struct
instead offileprivate
.RustBufferTemplate
(and similar templates) get changed visibility tointernal
tooGenerated/Foo
,Generated/Bar
,Generated/Buzz
, whereFoo
,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).The text was updated successfully, but these errors were encountered: