UniFFI C++ Bindgen Initial Integration - #249
alan-george-lk wants to merge 10 commits into
Conversation
960aa52 to
46a4fab
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| namespace livekit { | ||
|
|
||
| std::optional<std::string> uniffiBindgenBuildVersion() { | ||
| auto version = livekit_ffi::build_version(); |
There was a problem hiding this comment.
is livekit_ffi::build_version() a rust impl thats getting called in cpp?
There was a problem hiding this comment.
i.e. is this the "Secret Sauce"
There was a problem hiding this comment.
This is currently the only-available uniffi exported function in livekit-ffi: https://github.com/livekit/rust-sdks/blob/main/livekit-ffi/src/build_info.rs#L20-L23
This is the secret sauce in the sense that livekit_ffi is the uniffi generated namespace, and build_version is rust impl interfaced by the bindgen, so yes.
I tested all of this infra against livekit-uniffi but that is not actually targeted for critical path (per @1egoman), but allowed me to test more things:
https://github.com/livekit/rust-sdks/blob/main/livekit-uniffi/src/access_token.rs#L185-L189
and
https://github.com/livekit/rust-sdks/blob/main/livekit-uniffi/src/log_forward.rs#L27-L32
(removed from this branch for PR but part of history)
There was a problem hiding this comment.
shouldn't we expoe the ffi version via the public livekit header instead ?
livekit_ffi.hpp seems internal to me.
| namespace livekit { | ||
|
|
||
| std::optional<std::string> uniffiBindgenBuildVersion() { | ||
| auto version = livekit_ffi::build_version(); |
There was a problem hiding this comment.
shouldn't we expoe the ffi version via the public livekit header instead ?
livekit_ffi.hpp seems internal to me.
| /// | ||
| /// @return The generated binding's build version, or no value if it is invalid | ||
| /// or the binding call fails. | ||
| LIVEKIT_INTERNAL_API std::optional<std::string> uniffiBindgenBuildVersion(); |
There was a problem hiding this comment.
same here, the uniffi_bindgen_adapter.h seems internal to me, can we wrap it via some livekit public interface rather than directly expose it to public ?
There was a problem hiding this comment.
Our build versions come from a compile-time generated file, with no actual linkage or implementation (just static values in a header): https://github.com/livekit/client-sdk-cpp/blob/main/build.h.in
I considered integrating this into the header, but that would significantly change how that file works and the build code around it. The goal of this integration was the infra around the bindgen fork, the Rust bindgen calls, and the CMake to wire it all up. This adapter is intentionally temporary and isn't actually useful, I planned on removing it later.
Hope this makes sense.
There was a problem hiding this comment.
There might be a way to compile the bindgen code into a simple binary that CMake could call to get the same file statically, but that didn't feel useful
There was a problem hiding this comment.
step back a bit, what is the purpose of exposing this uniffiBindgenBuildVersion() publicly ? is it for testing purpose ?
xianshijing-lk
left a comment
There was a problem hiding this comment.
Sorry, I missed read the macro, it is LIVEKIT_INTERNAL_API
Summary
This PR introduces the initial steps for
uniffiintegration. Depends on livekit/rust-sdks#1389 and release tag.For this initial integration, a private header/adapter translation unit is added to test the UniFFI build version signature offered by the current
livekit-ffi. This approach was taken to:Testing
Significant testing occurred locally across the bingden fork repository (testing C++ features), and in this repository through building/linking/etc. The new unit test is isolated and can be deleted later once other features move to UniFFI as mentioned above.