Refactor uniffi structure #205

eoger · 2020-08-05T23:42:52Z

Fixes #27.
So this is something I wanted to do for a while, I believe it reduces the mental load trying to understand how the pieces fit together (especially the testing parts!).

I've split up the project in a 3 crates:

uniffi is the Rust runtime dependency (think ViaFFI traits and friends`). Target crates depend only on this.
uniffi_bindgen contains the Rust scaffolding and Kotlin/Swift bindings generators. It is not meant to be depended upon, but installed through uniffi-bindgen and run at build time.
uniffi_build is a convenience crate that will call uniffi-bindgen scaffolding for you.

I'll go and add review comments to make it easier to review this giant patch, initially I made multiple commits but before I knew it it proved too hard to amend the previous commits when making touchups.

eoger · 2020-08-05T23:43:43Z

examples/arithmetic/Cargo.toml

+uniffi = {path = "../../uniffi"}

 [build-dependencies]
-uniffi = {path = "../../uniffi"}
+uniffi_build = {path = "../../uniffi_build"}


See how we depend on 1 crate for the bytebuffer traits/methods + tests helpers, and 1 for build time.

eoger · 2020-08-05T23:44:22Z

examples/arithmetic/src/main.rs

- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
-
-fn main() {
-    uniffi::run_bindgen_for_component("arithmetic").unwrap();


The test helpers already generate the bindings for you.

examples/arithmetic/tests/test_generated_bindings.rs

eoger · 2020-08-05T23:45:24Z

uniffi/Cargo.toml

+anyhow = "1.0"
 bytes = "0.5"
 ffi-support = "0.4"
-anyhow = "1.0"
-askama = "0.9"
-heck = "0.3"
-clap = "2.33"
-object = "0.20"
-serde = "1.0"
-bincode = "1.3"
-cargo_metadata = "0.10.0"
 lazy_static = "1.4"
+log = "0.4"


These dependencies are re-exported for convenience.

Big +1 for the re-exporting, saves the consumer a lotta awkward errors if they don't have the right dependencies.

Yeah, +1 I like this a lot.

eoger · 2020-08-05T23:46:24Z

uniffi/src/testing.rs

-    bindings::compile_bindings(&ci, &out_dir, language)?;
-    generated_bindings.insert((cdylib_file, out_dir, language));
+
+    let status = Command::new("uniffi-bindgen")


We don't depend on uniffi-bindgen, but that's OK we call it on the command line :)

Hmmm This is the price we pay for having the bindings separate I guess 😆

I wonder if there is a way to ensure that it exists, maybe we can run a

cargo install --path ../../uniffi-bindgen

Before running the actual command.
(not sure about the path or the name, but something like that might help us with CI until the crate goes on crates.io)

I'm a little hesitant about this, since we don't have any guarantees that the uniffi-bindgen found on the system is the same version as used by the crate under test here. It seems like a footgun and potentially confusing experience for developers.

We have the pkg_dir here in order to compile the crate under test, I wonder if we should try to do something like:

Inspect the cargo metadata to find the version of uniffi on which the crate depends.

Shell out to cargo to build the matching version of uniffi_bindgen into our target dir, caching it similar to how we cache the compilation of the crate itself.

Use that built version of the uniffi-bindgen command the run the tests.

I think this would resolve the pending CI failures, and also produce a more consistent/reliable developer experience.

(A simpler version of this might be to just build and use the uniffi-bindgen in the current workspace, which would suffice to get our CI passing).

eoger · 2020-08-05T23:48:15Z

uniffi_bindgen/src/bindings/swift/mod.rs

@@ -0,0 +1,73 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


Nothing new, git is confused.

eoger · 2020-08-05T23:48:31Z

uniffi_bindgen/src/interface/mod.rs

@@ -100,23 +100,6 @@ impl<'ci> ComponentInterface {
        Ok(ci)
    }

-    pub fn from_bincode(data: &[u8]) -> Result<Self> {


The .idl file is the source of truth now.

😢 I really liked this neat hack, but yeah, using the .idl as the source of truth makes more sense for our setup, at least in the short term.

eoger · 2020-08-05T23:49:12Z

uniffi_bindgen/src/lib.rs

@@ -0,0 +1,173 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public


I'd encourage the reviewer to read the updated module doc :)

eoger · 2020-08-05T23:49:59Z

uniffi_bindgen/src/lib.rs

+pub fn generate_component_scaffolding(
+    idl_file: &OsStr,
+    out_dir_override: Option<&OsStr>,
+    format_code: bool,
+) -> Result<()> {
+    let component = parse_idl(idl_file)?;
+    let mut filename = Path::new(idl_file)
+        .file_stem()
+        .ok_or_else(|| anyhow!("not a file"))?
+        .to_os_string();
+    filename.push(".uniffi.rs");
+    let out_dir = get_out_dir(idl_file, out_dir_override)?;
+    let mut out_file = PathBuf::from(out_dir);
+    out_file.push(filename);
+    let mut f =
+        File::create(&out_file).map_err(|e| anyhow!("Failed to create output file: {:?}", e))?;
+    write!(f, "{}", RustScaffolding::new(&component))
+        .map_err(|e| anyhow!("Failed to write output file: {:?}", e))?;
+    if format_code {
+        Command::new("rustfmt").arg(&out_file).status()?;
+    }
+    Ok(())
+}
+
+// Generate the bindings in the target languages that call the scaffolding
+// Rust code.
+pub fn generate_bindings(
+    idl_file: &OsStr,
+    target_languages: Vec<&str>,
+    out_dir_override: Option<&OsStr>,
+) -> Result<()> {
+    let component = parse_idl(idl_file)?;
+    let out_dir = get_out_dir(idl_file, out_dir_override)?;
+    for language in target_languages {
+        bindings::write_bindings(&component, &out_dir, language.try_into()?)?;
+    }
+    Ok(())
+}


The 2 important functions we expose through the CLI (see main.rs).

eoger · 2020-08-05T23:50:49Z

uniffi_build/src/lib.rs

+use anyhow::{bail, Result};
+use std::{env, process::Command};
+
+pub fn generate_scaffolding(idl_file: &str) -> Result<()> {


As I said this is just a helper crate.

tarikeshaq

+1 For bobozord!

I did a quick first pass over it (haven't actually tried it yet) and wrote some thoughts I had while going over it, I'll do a second pass tomorrow and give it a shot locally!

Overall this looks good to me, and the ability to generate bindings separately draws us a step closer to nimbus 😄 Thanks a lot for the refactoring ❤️

examples/arithmetic/tests/test_generated_bindings.rs

tarikeshaq · 2020-08-06T00:28:40Z

uniffi/Cargo.toml

+anyhow = "1.0"
 bytes = "0.5"
 ffi-support = "0.4"
-anyhow = "1.0"
-askama = "0.9"
-heck = "0.3"
-clap = "2.33"
-object = "0.20"
-serde = "1.0"
-bincode = "1.3"
-cargo_metadata = "0.10.0"
 lazy_static = "1.4"
+log = "0.4"


Big +1 for the re-exporting, saves the consumer a lotta awkward errors if they don't have the right dependencies.

uniffi/src/lib.rs

tarikeshaq · 2020-08-06T00:40:17Z

uniffi/src/testing.rs

-    bindings::compile_bindings(&ci, &out_dir, language)?;
-    generated_bindings.insert((cdylib_file, out_dir, language));
+
+    let status = Command::new("uniffi-bindgen")


Hmmm This is the price we pay for having the bindings separate I guess 😆

I wonder if there is a way to ensure that it exists, maybe we can run a

cargo install --path ../../uniffi-bindgen

Before running the actual command.
(not sure about the path or the name, but something like that might help us with CI until the crate goes on crates.io)

uniffi_bindgen/src/lib.rs

uniffi_bindgen/src/main.rs

uniffi_build/src/lib.rs

rfk · 2020-08-06T01:58:22Z

Note that the CI failures is a bootstrapping problem: uniffi-bindgen is not available on crates.io, so it can't execute the tests

Are we feeling ready to publish this thing to crate.io..? :-)

rfk

Overall, I really like this, and I agree that the separation helps to form a more clear and correct mental model of the project as a whole. I have one concern that made me not want to r+ this quite yet, and one broad suggestion.

I'm pretty hesitant about requiring uniffi-bindgen to be installed separately on the system, and the way it seems to be making it hard to run CI on this PR. It's true that there are other system-level dependencies that you need to have installed to run the tests (swift, python, kotlinc, JNA...) but those are once-and-done affairs that we can handle nicely for CI by building a docker image. For the bindgen executable itself, IIUC you need to have a matching version for what's being used in the crate under test. Will we have this CI problem any time we want to make a breaking change in uniffi, because the tests need a new version of uniffi-bindgen installed but we can't install it yet because it hasn't been published?

I'm not entirely sure how to resolve that, but trying to discover and build an appropriate uniffi-bindgen based on metadata for the crate under test seems worth exploring (if you haven't already tried it and hated it, that is!).

I also wonder about the test support code, and I'm curious about the considerations that informed its current setup. I can see an argument that having all that "this is how you compile a .jar, this is how you run a kotlin script" logic mixed in next to the bindings generation stuff, makes it harder to understand the bindings generation stuff. But it also feels a bit awkward shoe-horned in to the runtime support crate, and I think that shows up in the way you had to take a few shortcuts to e.g. guess the namespace from the name of the IDL file. My suggestion is to consider keeping some of this functionality in a uniff-bindgen subcommand (maybe uniffi-bindgen compile and uniffi-bindgen exec or similar?) but to split out the code for that subcommand so that it's separate from the bindings generation stuff, so that it can be ignored unless you're specifically working with the test harness.

That's not a blocker for landing though, just something to consider.

examples/arithmetic/tests/test_generated_bindings.rs

rfk · 2020-08-06T02:01:00Z

uniffi/Cargo.toml

-path = "src/main.rs"
-
+# The dependency versions here have to agree with the corresponding ones in
+# uniffi_bindgen.


What will happen if they don't?

rfk · 2020-08-06T02:01:43Z

uniffi/Cargo.toml

+anyhow = "1.0"
 bytes = "0.5"
 ffi-support = "0.4"
-anyhow = "1.0"
-askama = "0.9"
-heck = "0.3"
-clap = "2.33"
-object = "0.20"
-serde = "1.0"
-bincode = "1.3"
-cargo_metadata = "0.10.0"
 lazy_static = "1.4"
+log = "0.4"


Yeah, +1 I like this a lot.

uniffi/src/lib.rs

rfk · 2020-08-06T02:03:30Z

uniffi/src/lib.rs

+
+// I'd be nice this module was behind a cfg(test) guard, but it doesn't
+// work between crates so let's hope LLVM tree-shaking works well.
+pub mod testing;


I guess we could consider moving this into a fourth "dev-dependencies" crate, but I think this will be fine as is, and I have confidence in the tree-shaking stuff 🤞

uniffi_bindgen/src/lib.rs

uniffi_bindgen/src/main.rs

uniffi_bindgen/src/templates/RustBuffer.rs

uniffi_build/src/lib.rs

uniffi_macros/src/lib.rs

rfk · 2020-08-06T02:47:45Z

Also, the backtrace from failing CI is a good indicator that we should take the time to provide a clearer error message when uniffi-bindgen cannot be found, it's currently pretty opaque:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: No such file or directory (os error 2)', examples/rondpoint/build.rs:6:5

eoger

@tarikeshaq @rfk I think I've addressed your concerns, let me know if this makes sense!

eoger · 2020-08-06T19:17:54Z

.circleci/config.yml

@@ -2,6 +2,12 @@

 version: 2.1

+commands:


This should allow CI to run properly with the current bindgen version.

eoger · 2020-08-06T19:20:17Z

uniffi/src/testing.rs

+        .to_str()
+        .unwrap();
+
+    let _lock = UNIFFI_BINDGEN.lock();


I put all the compile and run_script machinery back in uniffi_ bindgen, leaving a single call to uniffi-bindgen. As a bonus we can now run tests outside of Rust.

eoger · 2020-08-06T19:21:38Z

uniffi_bindgen/src/lib.rs

+// If the crate for which we are generating bindings for depends on
+// a `uniffi` runtime version that doesn't agree with our own version,
+// the developer of that said crate will be in a world of pain.
+fn ensure_versions_compatibility(


Added this to ensure the crates compatibility between the bindgen and the runtime crates. This basically compares the major versions and fails if they are different.

Great, thank you!

One nit: I think things are changing fast enough (and we're pre-1.0-enough) that we should just do a full version number check here rather than just the major version, at least until we start being deliberate about releasing versions with semver. Put another way: the commit that prepares our 1.0 release should be the one that makes this do a major-version-only comparison.

eoger · 2020-08-06T19:22:01Z

uniffi_bindgen/src/lib.rs

+
+// Run tests against the foreign language bindings (generated and compiled at the same time).
+// Note that the cdylib we're testing against must be built already.
+pub fn run_tests(cdylib_dir: &OsStr, idl_file: &OsStr, test_scripts: Vec<&str>) -> Result<()> {


The new test machinery living in uniffi_bindgen is here!

From an API-consumer point of view, I'm lightly surprised to see this take the directory of the cdylib rather than the path to the cdylib itself. Not a big deal, just noting my surprise.

eoger · 2020-08-06T19:22:14Z

uniffi_bindgen/src/main.rs

+                )
+                .arg(clap::Arg::with_name("idl_file").required(true)),
+        )
+        .subcommand(


And here is the subcommand to use it.

eoger · 2020-08-06T19:22:51Z

uniffi_macros/src/lib.rs

+        let idl_file: LitStr = input.parse()?;
+        let _comma: Token![,] = input.parse()?;
+        let array_contents;
+        bracketed!(array_contents in input);
+        let test_scripts = Punctuated::<LitStr, Token![,]>::parse_terminated(&array_contents)?
+            .iter()
+            .map(|s| s.value())
+            .collect();
+        Ok(FilePaths {
+            idl_file: idl_file.value(),
+            test_scripts,
+        })


There might be simpler way of doing this 😅

We might be able to come up with something more declarative as we learn more, but this LGTM!

rfk

This looks great! Let's get this landed ASAP to avoid merge conflicts (and also, please give all the other folks a heads-up in e.g. slack for rebasing/merging purposes).

rfk · 2020-08-06T21:09:51Z

uniffi_bindgen/src/bindings/mod.rs

-    let out_dir: Option<&Path> = out_dir.as_ref().map(|v| v.as_ref());
-    let script_file: Option<&Path> = script_file.as_ref().map(|v| v.as_ref());
+    let out_dir = out_dir.as_ref();
+    let script_file = script_file.as_ref();


These are a lot simpler without all the Option stuff, I like it. (I'll probably find myself missing the ability to launch into a shell for a bit of ad-hoc debugging, but we can bring that back later if we need to).

rfk · 2020-08-06T21:11:42Z

uniffi_bindgen/src/lib.rs

+    write!(f, "{}", RustScaffolding::new(&component))
+        .map_err(|e| anyhow!("Failed to write output file: {:?}", e))?;
+    if format_code {
+        Command::new("rustfmt").arg(&out_dir).status()?;


uniffi_bindgen/src/lib.rs

rfk · 2020-08-06T21:14:35Z

uniffi_bindgen/src/lib.rs

+// If the crate for which we are generating bindings for depends on
+// a `uniffi` runtime version that doesn't agree with our own version,
+// the developer of that said crate will be in a world of pain.
+fn ensure_versions_compatibility(


Great, thank you!

One nit: I think things are changing fast enough (and we're pre-1.0-enough) that we should just do a full version number check here rather than just the major version, at least until we start being deliberate about releasing versions with semver. Put another way: the commit that prepares our 1.0 release should be the one that makes this do a major-version-only comparison.

rfk · 2020-08-06T21:15:45Z

uniffi_bindgen/src/lib.rs

+
+// Run tests against the foreign language bindings (generated and compiled at the same time).
+// Note that the cdylib we're testing against must be built already.
+pub fn run_tests(cdylib_dir: &OsStr, idl_file: &OsStr, test_scripts: Vec<&str>) -> Result<()> {


From an API-consumer point of view, I'm lightly surprised to see this take the directory of the cdylib rather than the path to the cdylib itself. Not a big deal, just noting my surprise.

rfk · 2020-08-06T21:20:55Z

uniffi_bindgen/src/lib.rs

+            if !cargo_toml.exists() {
+                bail!("Could not find Cargo.toml file")
+            }
+            cargo_toml


If you didn't want to assume a particular location of the IDL file, I think you could cd into its parent directory and run cargo metadata from there. From local testing it would give you the metadata for the containing workspace as long as you are somewhere in a subdirectory of it, which IIUC is all you need here.

rfk · 2020-08-06T21:22:39Z

uniffi_macros/src/lib.rs

+        let idl_file: LitStr = input.parse()?;
+        let _comma: Token![,] = input.parse()?;
+        let array_contents;
+        bracketed!(array_contents in input);
+        let test_scripts = Punctuated::<LitStr, Token![,]>::parse_terminated(&array_contents)?
+            .iter()
+            .map(|s| s.value())
+            .collect();
+        Ok(FilePaths {
+            idl_file: idl_file.value(),
+            test_scripts,
+        })


We might be able to come up with something more declarative as we learn more, but this LGTM!

tarikeshaq

Gave this a shot locally as well, generation works nicely!

LGTM! Let's get this in and iterate if we need to 🚀

eoger requested review from rfk and tarikeshaq August 5, 2020 23:42

eoger commented Aug 5, 2020

View reviewed changes

tarikeshaq reviewed Aug 6, 2020

View reviewed changes

rfk reviewed Aug 6, 2020

View reviewed changes

eoger force-pushed the bobozord branch from 6104151 to ca49404 Compare August 6, 2020 19:17

eoger commented Aug 6, 2020

View reviewed changes

eoger force-pushed the bobozord branch 2 times, most recently from 8c3a8f0 to 0492693 Compare August 6, 2020 20:07

rfk approved these changes Aug 6, 2020

View reviewed changes

tarikeshaq approved these changes Aug 6, 2020

View reviewed changes

tarikeshaq mentioned this pull request Aug 6, 2020

Add error handling for python #202

Closed

Refactor uniffi structure

2267545

eoger force-pushed the bobozord branch from 0492693 to 2267545 Compare August 7, 2020 01:45

eoger merged commit 71092af into main Aug 7, 2020

eoger deleted the bobozord branch August 7, 2020 01:49

This was referenced Aug 7, 2020

Make cargo test in this repo Just Work (tm) #208

Closed

Code generation failing in Python #206

Closed

Clarify type conversions by differentiating between API-level and FFI-level types. #192

Merged

		@@ -0,0 +1,73 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

		@@ -0,0 +1,173 @@
		/* This Source Code Form is subject to the terms of the Mozilla Public

Refactor uniffi structure #205

Refactor uniffi structure #205

Conversation

eoger commented Aug 5, 2020 • edited Loading

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

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

tarikeshaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfk commented Aug 6, 2020

rfk 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

rfk commented Aug 6, 2020

eoger 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

rfk 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

tarikeshaq left a comment

Choose a reason for hiding this comment

eoger commented Aug 5, 2020 •

edited

Loading