Seperate bindings mvp #1177
Seperate bindings mvp #1177
Conversation
bendk
force-pushed
the
seperate-bindings-mvp
branch
4 times, most recently
from
515da92 to
db2f6e4
Compare
uniffi/src/lib.rs
Outdated
| // | ||
| // Basically, if another library adds uniffi as an external crate, then builds into a dylib, that | ||
| // library normally won't have the symbols from uniffi. However, if that library calls an extern | ||
| // function from function, then it *will* have the symbols. So this is a dummy function that the |
There was a problem hiding this comment.
nit: an extern function from function seems off?
| @@ -242,18 +246,19 @@ pub trait BindingGenerator: Sized { | |||
| /// - `udl_file`: The path to the UDL file | |||
There was a problem hiding this comment.
nit: we should change the rustdocs to reflect the new arguments
| out_dir_override: Option<P>, | ||
| pub fn generate_external_bindings( | ||
| binding_generator: impl BindingGenerator, | ||
| udl_file: impl AsRef<Path>, |
There was a problem hiding this comment.
just out of curiosity, should we change this to be &Path like we did in write_bindings? (I know it was a generic parameter before this change, so no need for any changes here 😄)
There was a problem hiding this comment.
We could, but to me this feels like the right function to put as_ref() call. I don't really have any strong reason for it though, so I could be easily convinced otherwise.
| } | ||
| } | ||
|
|
||
| impl<'de> Deserialize<'de> for EmptyBindingGeneratorConfig { |
There was a problem hiding this comment.
Can we replace this with a derive on the EmptyBindingGeneratorConfig? or does serde complain?
There was a problem hiding this comment.
It's a unit struct, but we want to deserialize from an empty map, so the derive(Deserialize) doesn't work. I'll add a comment about this.
| @@ -113,7 +113,7 @@ pub mod interface; | |||
| pub mod scaffolding; | |||
|
|
|||
| use bindings::TargetLanguage; | |||
| use interface::ComponentInterface; | |||
| pub use interface::ComponentInterface; | |||
There was a problem hiding this comment.
Do we need this? I have no objections, but it looks like all of interface is pub on line 112 - not that it matters much, there is convenience in getting access to the ComponentInterface with one less module.. maybe interface should be a private module and only expose ComponentInterface 🤷
There was a problem hiding this comment.
I'm never completely sure how to handle this. In the bindings code we use a several other components from interface. Maybe we could make that module private and add pub use interface::*. Now that these are all separate crates, it seems more natural to import from the uniffi_bindgen rather uniffi_bindgen::interface.
- Updated `BindingsGeneratorConfig` and added an empty implementation for bindings that don't have config values. - Instead of having `generate_external_bindings` construct the `BindingsGenerator` it now inputs one instead. The advantage here is that bindings generators can have whatever fields they want. For the desktop JS project I want to make a generator store an enum that controls if it's generating the .webidl, .cpp, .h, or .jsm file. - Have `write_bindings()` input the config since we're no longer passing that to the `BindingsGenerator` constructor. - Removed the `compile_bindings()` and `run_script()` methods. These are going to be replaced with some other system to run tests. - Separated the trait bounds for the `AsRef<Path>` arguments to `write_bindings()`. I think the previous code required all of them to be the same type. Now one can be a `PathBuf` and another can be a `Path`. - Made ComponentInterface public, since it's used in `BindingsGenerator` - Added `ComponentInterface` method to get all FFI functions, except the `RustBuffer` ones - Pass in an owned ComponentInterface rather than a reference - Don't return an error when `uniffi.toml` is missing - Added some anyhow context calls for better error tracking
bendk
force-pushed
the
seperate-bindings-mvp
branch
from
db2f6e4 to
fd0cf3b
Compare
No description provided.