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

Seperate bindings mvp #1177

Merged
merged 2 commits into from
Feb 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions uniffi/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -639,6 +639,16 @@ where
}
}

// This function is not intended to be called, but can be used as for the to workaround
// https://github.com/rust-lang/rust/issues/50007
//
// 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 a uniffi
// function, then it *will* have the symbols. So as a workaround to 50007, we create a dummy
// function here, and call it from a dummy function in the library,.
#[no_mangle]
pub extern "C" fn extern_symbol_hack() {}

#[cfg(test)]
mod test {
use super::*;
Expand Down
14 changes: 13 additions & 1 deletion uniffi_bindgen/src/interface/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,18 @@ impl<'ci> ComponentInterface {
/// The set of FFI functions is derived automatically from the set of higher-level types
/// along with the builtin FFI helper functions.
pub fn iter_ffi_function_definitions(&self) -> Vec<FFIFunction> {
let mut functions = self.iter_user_ffi_function_definitions();
functions.append(&mut self.iter_rust_buffer_ffi_function_definitions());
functions
}

/// List all FFI functions definitions for user-defined interfaces
///
/// This includes FFI functions for:
/// - Top-level functions
/// - Object methods
/// - Callback interfaces
pub fn iter_user_ffi_function_definitions(&self) -> Vec<FFIFunction> {
vec![]
.into_iter()
.chain(
Expand All @@ -416,10 +428,10 @@ impl<'ci> ComponentInterface {
.flat_map(|cb| cb.iter_ffi_function_definitions()),
)
.chain(self.functions.iter().map(|f| f.ffi_func.clone()))
.chain(self.iter_rust_buffer_ffi_function_definitions())
.collect()
}

/// List all FFI functions definitions for RustBuffer functionality
pub fn iter_rust_buffer_ffi_function_definitions(&self) -> Vec<FFIFunction> {
vec![
self.ffi_rustbuffer_alloc(),
Expand Down
93 changes: 51 additions & 42 deletions uniffi_bindgen/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ pub mod interface;
pub mod scaffolding;

use bindings::TargetLanguage;
use interface::ComponentInterface;
pub use interface::ComponentInterface;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

use scaffolding::RustScaffolding;

/// A trait representing a Binding Generator Configuration
Expand All @@ -122,8 +122,8 @@ use scaffolding::RustScaffolding;
/// the `BindingGenerator.config` associated type. `generate_external_bindings()` then uses it to
/// generate the config that's passed to `BindingGenerator.write_bindings()`
pub trait BindingGeneratorConfig: for<'de> Deserialize<'de> {
/// Key that specifies this bindings config in the `bindings` table from `uniffi.toml`.
fn language_key() -> String;
/// Get the entry for this config from the `bindings` table.
fn get_entry_from_bindings_table(bindings: &toml::Value) -> Option<toml::Value>;

/// Get default config values from the `ComponentInterface`
///
Expand Down Expand Up @@ -151,7 +151,35 @@ fn load_bindings_config<BC: BindingGeneratorConfig>(
}

// Leverage serde to convert toml::Value into the config type
Ok(toml::Value::from(config_map).try_into()?)
toml::Value::from(config_map)
.try_into()
.context("Generating bindings config from toml::Value")
}

/// Binding generator config with no members
#[derive(Clone, Debug, Hash, PartialEq, PartialOrd, Ord, Eq)]
pub struct EmptyBindingGeneratorConfig;

impl BindingGeneratorConfig for EmptyBindingGeneratorConfig {
fn get_entry_from_bindings_table(_bindings: &toml::Value) -> Option<toml::Value> {
None
}

fn get_config_defaults(_ci: &ComponentInterface) -> Vec<(String, toml::Value)> {
Vec::new()
}
}

// EmptyBindingGeneratorConfig is a unit struct, so the `derive(Deserialize)` implementation
// expects a null value rather than the empty map that we pass it. So we need to implement
// `Deserialize` ourselves.
impl<'de> Deserialize<'de> for EmptyBindingGeneratorConfig {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we replace this with a derive on the EmptyBindingGeneratorConfig? or does serde complain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

fn deserialize<D>(_deserializer: D) -> Result<Self, D::Error>
where
D: serde::Deserializer<'de>,
{
Ok(EmptyBindingGeneratorConfig)
}
}

// Load the binding-specific config
Expand All @@ -167,9 +195,7 @@ fn load_bindings_config_toml<BC: BindingGeneratorConfig>(
) -> Result<Option<toml::Value>> {
let config_path = match config_file_override {
Some(cfg) => cfg.to_owned(),
None => guess_crate_root(udl_file)?
.join("uniffi.toml")
.canonicalize()?,
None => guess_crate_root(udl_file)?.join("uniffi.toml"),
};

if !config_path.exists() {
Expand All @@ -183,9 +209,8 @@ fn load_bindings_config_toml<BC: BindingGeneratorConfig>(

Ok(full_config
.get("bindings")
.map(|c| c.get(&BC::language_key()))
.flatten()
.cloned())
.map(BC::get_entry_from_bindings_table)
.flatten())
}

/// A trait representing a UniFFI Binding Generator
Expand All @@ -197,36 +222,18 @@ pub trait BindingGenerator: Sized {
/// uniffi.toml
type Config: BindingGeneratorConfig;

/// A Constructor, allows passing configuration that was
/// parsed from the `uniffi.toml` to the type that implements
/// BindingGenerator
fn new(config: Self::Config) -> Self;

/// Writes the bindings to the output directory
///
/// # Arguments
/// - `ci`: A reference to a [`ComponentInterface`] representing the interface
/// - `out_dir`: The path to where the binding generator should write the output bindings
fn write_bindings(&self, ci: &ComponentInterface, out_dir: &Path) -> anyhow::Result<()>;

/// Compiles the bindings that are written by `write_bindings`, this is only relevant to run tests
/// and for languages that are compiled
///
/// # Arguments
/// - `ci`: A reference to a [`ComponentInterface`] representing the interface
/// - `ci`: A [`ComponentInterface`] representing the interface
/// - `config`: A instance of the BindingGeneratorConfig associated with this type
/// - `out_dir`: The path to where the binding generator should write the output bindings
fn compile_bindings(&self, _ci: &ComponentInterface, _out_dir: &Path) -> anyhow::Result<()> {
Ok(())
}

/// Runs a script against the written, and compiled bindings. This is only relevant to run tests
///
/// # Arguments
/// - `out_dir`: The directory where the bindings are located
/// - `script_file`: The script file to run against the bindings
fn run_script(&self, _out_dir: &Path, _script_file: &Path) -> anyhow::Result<()> {
unimplemented!()
}
fn write_bindings(
&self,
ci: ComponentInterface,
config: Self::Config,
out_dir: &Path,
) -> anyhow::Result<()>;
}

/// Generate bindings for an external binding generator
Expand All @@ -239,21 +246,23 @@ pub trait BindingGenerator: Sized {
/// - Creates an instance of [`BindingGenerator`], based on type argument `B`, and run [`BindingGenerator::write_bindings`] on it
///
/// # Arguments
/// - `binding_generator`: Type that implements BindingGenerator
/// - `udl_file`: The path to the UDL file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we should change the rustdocs to reflect the new arguments

/// - `config_file_override`: The path to the configuration toml file, most likely called `uniffi.toml`. If [`None`], the function will try to guess based on the crate's root.
/// - `out_dir_override`: The path to write the bindings to. If [`None`], it will be the path to the parent directory of the `udl_file`
pub fn generate_external_bindings<B: BindingGenerator, P: AsRef<Path>>(
udl_file: P,
config_file_override: Option<P>,
out_dir_override: Option<P>,
pub fn generate_external_bindings(
binding_generator: impl BindingGenerator,
udl_file: impl AsRef<Path>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

config_file_override: Option<impl AsRef<Path>>,
out_dir_override: Option<impl AsRef<Path>>,
) -> Result<()> {
let out_dir_override = out_dir_override.as_ref().map(|p| p.as_ref());
let config_file_override = config_file_override.as_ref().map(|p| p.as_ref());
let out_dir = get_out_dir(udl_file.as_ref(), out_dir_override)?;
let component = parse_udl(udl_file.as_ref())?;
let component = parse_udl(udl_file.as_ref()).context("Error parsing UDL")?;
let bindings_config =
load_bindings_config(&component, udl_file.as_ref(), config_file_override)?;
B::new(bindings_config).write_bindings(&component, out_dir.as_path())
binding_generator.write_bindings(component, bindings_config, out_dir.as_path())
}

// Generate the infrastructural Rust code for implementing the UDL interface,
Expand Down