Merge override config with `uniffi.toml` #1788

arg0d · 2023-10-11T11:09:12Z

Instead of replacing uniffi.toml with override config entirely, merge the override config into uniffi.toml. The merge recursively upserts TOML keys.

This strategy seems pretty standard everywhere when it comes to configuration. Merging allows the configuration to be selectively changed, without having to duplicate the entire default configuration.

The main use case for this would be to allow "global" configuration across multiple crates in a library. Some configuration options should be the same for all crates in a library, without having to duplicate these options across all crates. Right now I can think of a few use cases of such configuration:

In Swift generator, omit_argument_labels. I'm not entirely sure if anyone is actually using this, but it seems like an option that someone might potentially want to configure for all crates in a library.
In uniffi-bindgen-go, go_mod configuration key is used to control the prefix for external type imports. Multiple crates in a library using external types share a common prefix when it comes to importing the external types, e.g. com.example.uniffi.*. This should configured as a single property for all crates.
In uniffi-bindgen-cs, visibility_modifier configuration key is used to control the visibility modifier for "public" types. In C# visibility is applied to "assembly" level, not "file" level. So if bindings consumers wishes to package the bindings into his own "assembly" (library?), then "public" fields would be public for the consumers of the wrapper aswell Upgrade to uniffi @0.24 NordSecurity/uniffi-bindgen-go#13. This option would also benefit from a global config, since you would expect this value to remain the same for all crates in Rust library.
External binding generators can use the merge option to inject additional configuration into custom_types fixture. This one is a bit sketchy, because external binding generators might want to modify the configuration for specific fixture/example crates in upstream, instead of doing global configuration for all crates. Still, global configuration solves the immediate issue of configuring custom_types crate to including Url custom type for external bindings.

arg0d · 2023-10-11T11:12:04Z

After writing the motivation for this change, it looks to be quite niche and I'm not sure anymore if its worth supporting this, at least with the current configuration options. In fact, the remaining configuration options don't even make sense to override globally, e.g. package_name. Still, being able to selectively override configuration is pretty standard and it sounds like this could be useful in the future.

Instead of replacing `uniffi.toml` with override config entirely, merge the override config into `uniffi.toml`. The merge recursively upserts TOML keys. This strategy seems pretty standard everywhere when it comes to configuration. Merging allows the configuration to be selectively changed, without having to duplicate the entire default configuration. The main use case for this would be to allow "global" configuration across multiple crates in a library. Some configuration options should be the same for all crates in a library, without having to duplicate these options across all crates. Right now I can think of 3 use cases of such configuration: - In Swift generator, `omit_argument_labels`. I'm not entirely sure if anyone is actually using this, but it seems like an option that someone might potentially want to configure for all crates in a library. - In `uniffi-bindgen-go`, `go_mod` configuration key is used to control the prefix for external type imports. Multiple crates in a library using external types share a common prefix when it comes to importing the external types, e.g. `com.example.uniffi.*`. This should configured as a single property for all crates. - External binding generators can use the merge option to inject additional configuration into `custom_types` fixture. This one is a bit sketchy, because external binding generators might want to modify the configuration for specific fixture/example crates in upstream, instead of doing global configuration for all crates. Still, global configuration solves the immediate issue of configuring `custom_types` crate to including `Url` custom type for external bindings.

arg0d · 2023-10-12T12:01:15Z

I can't think of a better way to handle go_mod and visibility_modifier configuration, so I think merging configs is a sane way to go. I think in the future this could pave way for adding more nuanced configuration options. NordSecurity/uniffi-bindgen-go#13 (comment)

mhammond

I think Mozilla could take advantage of this too - eg, 3 random uniffi.toml files in our repo:

https://github.com/mozilla/application-services/blob/8a7493212e0beacd4849d4c43cde9a30ca312f36/components/nimbus/uniffi.toml
https://github.com/mozilla/application-services/blob/8a7493212e0beacd4849d4c43cde9a30ca312f36/components/push/uniffi.toml
https://github.com/mozilla/application-services/blob/8a7493212e0beacd4849d4c43cde9a30ca312f36/components/sync_manager/uniffi.toml

Shows a fair bit of duplication, including things that really don't make sense to have in each component - eg, it doesn't really make sense for the sync_manager component to know it is in a cdylib named megazord, because that's just a decision made by the consumer of the component, not the component itself.

So all in all I think I'm +1 on this, although I might try and see how it works in practice for us. In the meantime I've a couple of suggestions...

mhammond · 2023-10-12T15:42:25Z

uniffi_bindgen/src/lib.rs

+    Ok(toml::Value::from(config).try_into()?)
+}
+
+fn merge_toml(a: &mut toml::value::Table, b: toml::value::Table) {


I think we should have some tests here.

Added test_merge_toml

mhammond · 2023-10-12T15:44:56Z

uniffi/src/cli.rs

@@ -35,7 +35,9 @@ enum Commands {
        #[clap(long, short)]
        no_format: bool,

-        /// Path to the optional uniffi config file. If not provided, uniffi-bindgen will try to guess it from the UDL's file location.
+        /// Path to optional uniffi config file. This config will be merged on top of default


This docstring could probably be improved - it describes the implementation but not really the UI - ie, maybe it should be described in terms of being the default config or similar? It's also worth adding some docs to https://github.com/mozilla/uniffi-rs/blob/main/docs/manual/src/bindings.md.

This docstring could probably be improved - it describes the implementation but not really the UI - ie, maybe it should be described in terms of being the default config or similar? It's also worth adding some docs to https://github.com/mozilla/uniffi-rs/blob/main/docs/manual/src/bindings.md.

I will update the markdown fil, but I don't agree about the description. I would agree that its quite verbose, but its not wrong. This config lets you override default config values in crate config. I'm not really sure how else to describe this config.

Reading your comment again, I see your point. Describing it in terms of being the default config sounds very terse. But this does not correctly map to the behaviour. It does not work like the default config. It works as an override to the config in each crate.

Now I'm thinking if its worth changing the implementation to make this the "default" config for all crates, instead of overriding.

If the config acts as a default, then what you see in a crate's config config is what you get in the bindings. This sounds convenient when thinking from individual crates perspective. Setting foo: foo in crate config, I can be sure that this is what I will always get.

If the config acts as an override, then what you see in the override config is what you get in all crate's bindings. This sounds convenient when thinking from bindings generation perspective. Setting foo: bar in override config, I can be sure that all crates will share the same value, regardless of their config.

I'm still leaning towards the override option. Thinking about a theoretical option force_snake_case for Kotlin (not sure why anyone would want that, but hey), I want a way to ensure that all my crates use the same configuration value, regardless of their crate configuration. I don't want to have to go and grep all crate config files, to ensure that none of the crates using this configuration key.

I've reworded the docstring, and updated documentation file. Let me know what you think.

bendk · 2023-10-12T16:38:36Z

The main use case for this would be to allow "global" configuration across multiple crates in a library. Some configuration options should be the same for all crates in a library, without having to duplicate these options across all crates.

I think this would be great, but there's one thing I've never been able to figure out: How would this interact with external crates? Suppose UniFFI popularity continues to increase and libraries like serde, anyhow, log, etc. add a feature to export UniFFI bindings. Would the global configuration apply to them? If so, is this a safe thing to do? If not, how do you distinguish between the crates your project is bundling together and external crates?

mhammond · 2023-10-12T20:12:41Z

Suppose UniFFI popularity continues to increase and libraries like serde, anyhow, log, etc. add a feature to export UniFFI bindings. Would the global configuration apply to them? If so, is this a safe thing to do? If not, how do you distinguish between the crates your project is bundling together and external crates?

That's an interesting question! TBH, if that happened, I'd imagine it would only be for the Rust scaffolding - eg, I could imagine (say) them offering an FfiConverter, but can't picture them offering much in the way of Kotlin binding support. Thus, I doubt they'd actually rely on anything in uniffi.toml. But ultimately, even if they did offer binding support, I'd think that yes, we would want the global uniffi.toml to apply at least in some cases - eg, there's no possible way they could specify a meaningful cdylib_name and without a facility like offered in this PR, there'd be no reasonable way for us to supply it when building local bindings for that crate?

mhammond

This looks great to me, thanks.

arg0d requested a review from a team as a code owner October 11, 2023 11:09

arg0d requested review from jhugman and removed request for a team October 11, 2023 11:09

This was referenced Oct 11, 2023

Upgrade to uniffi @0.24 NordSecurity/uniffi-bindgen-go#13

Closed

Append custom types example to include C# and Go configuration NordSecurity/uniffi-rs#8

Merged

arg0d force-pushed the kristupas/merge-override-config branch from 1f48562 to 047ce3a Compare October 11, 2023 12:25

mhammond reviewed Oct 12, 2023

View reviewed changes

Add unit test for merge_toml

b4d0cc7

arg0d force-pushed the kristupas/merge-override-config branch from 1f43c7c to b4d0cc7 Compare October 13, 2023 10:53

Update documentation for --config

18f8b52

mhammond approved these changes Oct 31, 2023

View reviewed changes

mhammond merged commit e671882 into mozilla:main Oct 31, 2023

arg0d deleted the kristupas/merge-override-config branch September 13, 2024 12:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge override config with `uniffi.toml` #1788

Merge override config with `uniffi.toml` #1788

arg0d commented Oct 11, 2023 •

edited

Loading

arg0d commented Oct 11, 2023 •

edited

Loading

arg0d commented Oct 12, 2023

mhammond left a comment •

edited

Loading

mhammond Oct 12, 2023

arg0d Oct 31, 2023

mhammond Oct 12, 2023

arg0d Oct 13, 2023

arg0d Oct 31, 2023

bendk commented Oct 12, 2023 •

edited

Loading

mhammond commented Oct 12, 2023

mhammond left a comment

Merge override config with uniffi.toml #1788

Merge override config with uniffi.toml #1788

Conversation

arg0d commented Oct 11, 2023 • edited Loading

arg0d commented Oct 11, 2023 • edited Loading

arg0d commented Oct 12, 2023

mhammond left a comment • edited Loading

Choose a reason for hiding this comment

mhammond Oct 12, 2023

Choose a reason for hiding this comment

arg0d Oct 31, 2023

Choose a reason for hiding this comment

mhammond Oct 12, 2023

Choose a reason for hiding this comment

arg0d Oct 13, 2023

Choose a reason for hiding this comment

arg0d Oct 31, 2023

Choose a reason for hiding this comment

bendk commented Oct 12, 2023 • edited Loading

mhammond commented Oct 12, 2023

mhammond left a comment

Choose a reason for hiding this comment

Merge override config with `uniffi.toml` #1788

Merge override config with `uniffi.toml` #1788

arg0d commented Oct 11, 2023 •

edited

Loading

arg0d commented Oct 11, 2023 •

edited

Loading

mhammond left a comment •

edited

Loading

bendk commented Oct 12, 2023 •

edited

Loading