uniffi-bindgen-swift #2248

bendk · 2024-09-27T14:05:42Z

This is a uniffi-bindgen variant dedicated to Swift code, with options specialized for Swift. I'm currently thinking that we should consider doing this for all languages. In addition to allowing specialized options, it also makes the in-tree bindings less special-cased compared to external bindings.

Reverted #2235, which was an attempt to do something similar. I like this approach better because it supports more features, like xcframework compatible module maps. It's also a non-breaking change, so we can release it in a 0.28.2.

This reverts commit fe71008.

gruberb

Just a couple of comments. I really like the Swift focused options, I think this makes it very clear what we can (and have to do) in each scenario!

gruberb · 2024-10-01T18:35:06Z

uniffi_bindgen/src/bindings/swift/mod.rs

@@ -66,15 +67,10 @@ impl BindingGenerator for SwiftBindingGenerator {

 fn update_component_configs(
 &self,
- settings: &GenerationSettings,
+ _settings: &GenerationSettings,


I personally would rather see changing the trait to Option<&GenerationSettings> instead.

This is implemented for all the bindings though. Some bindings, like Python, currently need the GenerationSettings to be passed.

Yeah, we would need to change a lot of files.

gruberb · 2024-10-01T18:39:55Z

uniffi/src/cli/swift.rs

+
+#[derive(Debug, Args)]
+#[group(required = true, multiple = true)]
+struct Kinds {


Would it be helpful to have a default? Like "building .Xcframework" could be the default. So we can set the bool values accordingly.

A default sounds good, but I'm not sure what default you're suggesting. Is it --xcframework is the default and --no-xcframework would disable it? Or is it that generating one these kinds of sources is the default?

I would say we could add the default to always be xcframework, which I think will be most of the time the case. We need Swift specific settings at the moment just for the documentation. So we could shorten the command when building for xcframework and don't have to pass any parameters.

After thinking about this a bit more, I'm not so sure. --no-xcframework seems a bit awkward as a flag. It's harder to describe since it's a negative, and it also sort-of implies that if you don't have it then you're building an xcframework somehow. Something like --not-for-xcframework is more precise and easier to describe, but much harder to type out.

How about we leave it as-is for now, we can always change the flag later.

gruberb · 2024-10-01T18:43:31Z

docs/manual/src/swift/configuration.md

 | `module_name` | `{namespace}`[^1] | The name of the Swift module containing the high-level foreign-language bindings. |
 | `ffi_module_name` | `{module_name}FFI` | The name of the lower-level C module containing the FFI declarations. |
 | `ffi_module_filename` | `{ffi_module_name}` | The filename stem for the lower-level C module containing the FFI declarations. |
-| `generate_module_map` | `true` | Whether to generate a `.modulemap` file for the lower-level C module with FFI declarations.  Not valid when using `--library`, in library-mode the modulemap is always generated.
+| `generate_module_map` | `true` | Whether to generate a `.modulemap` file for the lower-level C module with FFI declarations. (ignored by `uniffi-bindgen-swift`) | 


Can we add an example command for both a Swift and a .Xcframework?

I added a bunch to docs/manual/src/swift/uniffi-bindgen-swift.md.

This is a `uniffi-bindgen` variant dedicated to Swift code, with options specialized for Swift. I'm currently thinking that we should consider doing this for all languages. In addition to allowing specialized options, it also makes the in-tree bindings less special-cased compared to external bindings.

mhammond · 2024-10-03T01:43:38Z

I'm currently thinking that we should consider doing this for all languages

I think later we should try and push the implementation down to the bindings. If we treat ourselves more like external bindings, instead of top-level uniffi::uniffi_bindgen_main variants, we require an explicit dep on, say, uniffi_bindgen::swift::uniffi_bindgen_main and a uniffi-bindgen-swift binary?

ie, let uniffi_bindgen::swift, uniffi_bindgen::python etc all use clap and have their own cli driver. uniffi::cli becomes helper functions and b/w compat - but the "new" way to generate python bindings is uniffi-bindgen-python.

This means uniffi_bindgen takes the dep for clap - but in practice, the dep is already there. And later, if we move the bindings to their own crate, (ie,uniffi_bindgen_swift::uniffi_bindgen_main) - uniffi_bindgen then no longer needs clap and becomes more of an "internal" crate.

WDYT? Does that make any sense for gecko @bendk?

bendk · 2024-10-03T15:29:56Z

I'm currently thinking that we should consider doing this for all languages

I think later we should try and push the implementation down to the bindings. If we treat ourselves more like external bindings, instead of top-level uniffi::uniffi_bindgen_main variants, we require an explicit dep on, say, uniffi_bindgen::swift::uniffi_bindgen_main and a uniffi-bindgen-swift binary?

ie, let uniffi_bindgen::swift, uniffi_bindgen::python etc all use clap and have their own cli driver. uniffi::cli becomes helper functions and b/w compat - but the "new" way to generate python bindings is uniffi-bindgen-python.

This means uniffi_bindgen takes the dep for clap - but in practice, the dep is already there. And later, if we move the bindings to their own crate, (ie,uniffi_bindgen_swift::uniffi_bindgen_main) - uniffi_bindgen then no longer needs clap and becomes more of an "internal" crate.

WDYT? Does that make any sense for gecko @bendk?

That makes a lot of sense to me. The one thing I like about this PR is that there's both a programmatic API and a CLI. That way users can either just use the CLI or they can make their own tools. I think we're going to want to do the latter for app-services.

The way it's currently split up, only uniffi needs a clap dependency. That seems a kind of nice to me, but I would also be fine with moving the CLI code into uniffi_bindgen and adding the clap dependency there.

Gecko is very similar since we write our own CLI there. I definitely would like it if uniffi-bindgen-python and uniffi-bindgen-gecko-js were on similar footing.

mhammond · 2024-10-04T01:14:45Z

Gecko is very similar since we write our own CLI there. I definitely would like it if uniffi-bindgen-python and uniffi-bindgen-gecko-js were on similar footing.

Yeah exactly - which implies uniffi:: can't know about python etc - and the only way I can see for that to work is to have no cli in uniffi?

bendk · 2024-10-07T13:41:46Z

Yeah exactly - which implies uniffi:: can't know about python etc - and the only way I can see for that to work is to have no cli in uniffi?

Good point. You've convinced me that we should move the clap code into uniffi_bindgen.

Revert "Generate a single modulemap in library mode"

e73347c

This reverts commit fe71008.

bendk requested a review from a team as a code owner September 27, 2024 14:05

bendk requested review from gruberb and a team and removed request for a team September 27, 2024 14:05

gruberb approved these changes Oct 1, 2024

View reviewed changes

bendk force-pushed the push-uunuklpxuxzo branch from 15e796b to 5c4409a Compare October 1, 2024 20:47

bendk merged commit 3662c84 into mozilla:main Oct 3, 2024
5 checks passed

bendk deleted the push-uunuklpxuxzo branch October 3, 2024 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uniffi-bindgen-swift #2248

uniffi-bindgen-swift #2248

bendk commented Sep 27, 2024

gruberb left a comment

gruberb Oct 1, 2024

bendk Oct 1, 2024

gruberb Oct 2, 2024

gruberb Oct 1, 2024

bendk Oct 1, 2024

gruberb Oct 2, 2024

bendk Oct 2, 2024

gruberb Oct 1, 2024

bendk Oct 1, 2024

mhammond commented Oct 3, 2024

bendk commented Oct 3, 2024

mhammond commented Oct 4, 2024

bendk commented Oct 7, 2024

uniffi-bindgen-swift #2248

uniffi-bindgen-swift #2248

Conversation

bendk commented Sep 27, 2024

gruberb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhammond commented Oct 3, 2024

bendk commented Oct 3, 2024

mhammond commented Oct 4, 2024

bendk commented Oct 7, 2024