Add a static library crate with a C FFI #21
Conversation
hulthe
force-pushed
the
ffi
branch
2 times, most recently
from
fa7ab5e
to
2923671
Compare
hulthe
changed the title
|
@pylls hey, we talked about upstreaming our ffi wrapper. Here it is :) |
instead of a static &CStr, which is not FFI safe
Also remove cbindgen version pin, since version pinning in Cargo.toml has problems when dealing with multiple versions of the some package in the dependency tree.
There was a problem hiding this comment.
Some smaller notes, but they are not critical items, would potentially just be nice to have.
Also going to write here what I said in the office: Personally I prefer that we ditch cbindgen as a build time dependency. It pulls in 20+ dependencies in our dependency chain. Both a supply chain risk and maintenance burden as well as a compile time time cost. I'm positive towards automatically generated bindings if the source changes frequently, so manually doing it is a burden. But for this API it will likely change extremely seldom, so manually updating the header when needed has very little downsides.
crates/maybenot-ffi/src/ffi.rs
Outdated
| /// `actions_out` must have capacity for [maybenot_num_machines] items of size | ||
| /// `sizeof(MaybenotAction)` bytes. | ||
| /// | ||
| /// The number of actions will be written to `num_actions_out`. |
There was a problem hiding this comment.
More items for the safety docs, if they should be complete:
num_actions_outmust also be a valid pointer where a 64 bit int can be written- "
thismust be a valid pointer to a [MaybenotFramework] instance"
crates/maybenot-ffi/src/lib.rs
Outdated
| ) -> usize { | ||
| let num_actions = self | ||
| .framework | ||
| .trigger_events(&[convert_event(event)], Instant::now()) |
There was a problem hiding this comment.
Here the API is changed from "list of events" (Rust) to "single event" (C). We can leave it like this for now. But I suspect we might revisit this in the future to make the API as similar to Rust as possible, and also we might hit performance bottlenecks making us want to batch event processing (like the Rust code allows)
There was a problem hiding this comment.
Good points. I've recently become aware that certain languages (cough Go cough) make FFI calls significantly more expensive, which means we might as well consider doing it now. I can have a look and see if it would be an annoying change to make for our existing project that depends on this crate.
There was a problem hiding this comment.
Unfortunately there's now a bit more code to review... 😔
* Add maybenot-ffi * Add makefile and readme * Run cbindgen in build script * Embed crate version in static library * Correctly expose crate version using a function instead of a static &CStr, which is not FFI safe * Move mod statements below use * Make public Rust API be same as public FFI API * Pin cbindgen to avoid any format change surprises * Remove obsolete error type * Remove thiserror * Remove default cbindgen.toml options * Remove --locked * Respect CARGO_TARGET_DIR in makefile * Commit Cargo.lock Also remove cbindgen version pin, since version pinning in Cargo.toml has problems when dealing with multiple versions of the some package in the dependency tree. * Set ffi lib version to 1.0.0 * Add maybenot-ffi to /README.md * Tighten restriction on null bytes in version str * Remove machine string filtering & trimming in ffi * More safety docs * Change on_event ffi func to accept multiple events
|
@hulthe Great work, let's use this! 💪 @pylls I was happy with the FFI bindings so I merged them. This also means we merge the We will work on migrating this to |
This let's us compile maybenot to a static library and use it in non-rust programs with support for C FFIs