Conversation
Changeset ✓This PR includes a changeset covering all affected packages:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
🔍 Devin Review: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
279b297 to
84e0391
Compare
Generates NAPI TypeScript bindings from any compiled uniffi cdylib, so it sits alongside the uniffi-bindgen already here rather than being vendored into one crate's build. `default-features = false` is load-bearing, not tidiness: ubrn's `wasm` feature pulls wasm-bindgen-cli-support, which `=`-pins the wasm-bindgen family to its own version — unsatisfiable against the wasm-bindgen this workspace resolves through livekit -> libwebrtc. Pinned by commit rather than tag: this is the released line's main, which carries #453 — without it a non-wasm build silently generates JSI bindings instead of NAPI, exit 0 and no warning. Move to a tag when one ships past 0.31.0-5. A `rev` also keeps `cargo update` from sweeping the generator forward on its own, since what it emits has to stay in step with the @ubjs/* runtime it imports.
84e0391 to
645ddcc
Compare
…ative Replaces uniffi-bindgen-node and its download-at-install shim with ubrn's NAPI backend and the prebuilt-sibling layout the node ecosystem already uses: `@livekit/uniffi` carries the TypeScript and declares one optional dependency per platform, and `@livekit/uniffi-<triple>` carries just the cdylib. @ubjs/node resolves the right sibling at load time, so nothing downloads a library on install. The node flow owns more tasks than any other language here and reads badly inline, so it lives in support/node/Makefile.node.toml beside the files it drives. cargo-make resolves `extend` relative to the crate Makefile and runs everything from the crate root, so paths there are written as if they were here. Two things worth knowing about the tasks: - Bindgen builds unstripped and packaging builds stripped, each on its own wrapper task. ubrn reads uniffi metadata out of the library's symbols; what ships must match the profile uniffi-cdylib.yml builds with. cargo-make env persists process-wide, so a single run doing both needs each setting re-asserted at its own step. - livekit_uniffi's cdylib bundles several uniffi namespaces into one physical library, and ubrn emits one `*-ffi.ts` per namespace, each resolving by its own `crateName` — every one of them pointing at a dylib that doesn't exist. node-fixup-crate-name rewrites them to the umbrella crate, and fails loudly if it matches nothing, since silence there means bindings that resolve nowhere. Two tests, both under support/node. node-package-test does the full FFI round trip against the built workspace, in CJS. node-pack-test installs the packed tarballs — which is what npm uploads — so `files`, `exports`, and the platform-package lookup all run for real, in ESM; a workspace link goes through none of the three.
Builds the packages for the host triple and runs both tests: the FFI round trip against the built workspace, and the tarball install that stands in for a published consumer. Between them the exports map is covered both ways. Also asserts the generator and the @ubjs/* runtime agree. The bindings are emitted by the generator Cargo.lock pins but import @ubjs/core at a version the manifest template carries; if those drift the package builds fine and fails at load time in a consumer, so the check compares what actually ran against what it actually produced. Not published yet — uniffi-packages.yml says what to add when it is.
The linux cdylibs linked against whatever glibc the runner shipped, which is newer than Debian 12, Ubuntu 22.04, Amazon Linux 2023, or node:*-slim — so a library built here failed to load on the images most consumers deploy to. `--target <triple>.2.28` states the floor instead of inheriting it. zig is also the cross-linker, so aarch64 builds on the x64 runner and the separate arm runner goes away. .cargo/config.toml forces -fuse-ld=lld for aarch64; that reaches `zig cc`, which accepts it.
645ddcc to
554f001
Compare
There was a problem hiding this comment.
Devin Review found 2 new potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| PACKAGES_DIR = "./packages" | ||
| SUPPORT_DIR = "./support" | ||
| LIB_NAME = "lib${CARGO_MAKE_CRATE_FS_NAME}" | ||
| PACKAGES_DIR = "${CARGO_MAKE_CRATE_TARGET_DIRECTORY}/packages" |
There was a problem hiding this comment.
🔴 Non-Node package outputs disappear
Setting PACKAGES_DIR to the workspace target relocates every Swift, Android, and Dart output. Existing workflows and Gradle still read the old directory.
Prompt for agents
The global PACKAGES_DIR change in livekit-uniffi/Makefile.toml moves every language's generated packages from livekit-uniffi/packages to the workspace target/packages directory. Update all consumers consistently, including .github/workflows/uniffi-swift.yml OUTPUT_DIR, .github/workflows/uniffi-dart-test.yml working-directory, and support/android/build.gradle.kts generatedKotlinDir, or scope the target-directory output only to the Node tasks while preserving the existing location for Swift, Android, Kotlin, and Dart.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "optionalDependencies": { | ||
| "{{ NPM_NAME }}-darwin-arm64": "{{ CARGO_MAKE_CRATE_VERSION }}", | ||
| "{{ NPM_NAME }}-darwin-x64": "{{ CARGO_MAKE_CRATE_VERSION }}", | ||
| "{{ NPM_NAME }}-linux-x64-gnu": "{{ CARGO_MAKE_CRATE_VERSION }}", | ||
| "{{ NPM_NAME }}-linux-arm64-gnu": "{{ CARGO_MAKE_CRATE_VERSION }}", | ||
| "{{ NPM_NAME }}-win32-x64-msvc": "{{ CARGO_MAKE_CRATE_VERSION }}" |
There was a problem hiding this comment.
🟡 Windows ARM64 package disappears
On Windows ARM64, optionalDependencies has no matching package despite the built ARM64 library. Loading @livekit/uniffi therefore loses prior platform support.
Prompt for agents
Restore Windows ARM64 support in the new per-architecture package layout. Add the aarch64-pc-windows-msvc to win32-arm64-msvc mapping in livekit-uniffi/support/node/Makefile.node.toml and declare @livekit/uniffi-win32-arm64-msvc in livekit-uniffi/support/node/package.json.tera. Ensure the native package manifest derives os=win32 and cpu=arm64, and include this target when the package matrix is introduced.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
I think as I started to review this I realized that what you had done here (add uniffi bindings to livekit-uniffi) and what I had in mind (add uniffi bindings to livekit-ffi) were sufficiently far apart where I think it makes sense to put this on pause for a bit until after the upcoming livekit-ffi / livekit-uniffi merge, and then as part of rebasing this PR onto the updated main, address some of the comments I left at that point.
Also that dovetails nicely into when @lukasIO will be back from vacation and I would really like this to be reviewed by him before being merged as well.
| node ecosystem already uses: `@livekit/uniffi` carries the TypeScript and | ||
| declares one optional dependency per platform, and `@livekit/uniffi-<triple>` | ||
| carries just the native library. The right one is resolved at load time, so |
There was a problem hiding this comment.
question: I am curious why exactly you opted to publish this as a separate set of packages. Today @livekit/rtc-ffi-bindings and the corresponding @livekit/rtc-ffi-bindings-linux-arm64-gnu / @livekit/rtc-ffi-bindings-darwin-arm64 / etc packages are already being published, each containing the dylib/so/dll which is included in each of these packages.
Here's an example of what the existing platform specific packages look like:
IMO it would be good to combine these - ideally keep the @livekit/rtc-ffi-bindings- prefix on the package names, and name the library files the same as the existing files, but also include the node bindings alongside, so there is only one set of library containing packages which can be used for the legacy c abi fii and for uniffi.
| - name: Install cargo-zigbuild | ||
| if: ${{ matrix.platform == 'linux' }} | ||
| uses: taiki-e/install-action@682e7d9e49c5e653d371fc6adbda67653461378a # v2.82.4 | ||
| with: | ||
| tool: cargo-zigbuild | ||
|
|
||
| # `.2.28` pins the glibc the cdylib links against, so it loads on Debian | ||
| # 12, Ubuntu 22.04, Amazon Linux 2023 and node:*-slim rather than only on | ||
| # whatever the runner ships. zig is also the cross-linker, so aarch64 | ||
| # builds here too. .cargo/config.toml's -fuse-ld=lld for aarch64 reaches | ||
| # `zig cc`, which accepts it. | ||
| - name: Build (Linux) | ||
| if: ${{ matrix.platform == 'linux' }} | ||
| run: | | ||
| if [ "${{ matrix.target }}" = "aarch64-unknown-linux-gnu" ]; then | ||
| sudo apt-get update | ||
| sudo apt-get install -y gcc-aarch64-linux-gnu | ||
| export CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_LINKER=aarch64-linux-gnu-gcc | ||
| # .cargo/config.toml forces -fuse-ld=lld for this target to link | ||
| # libwebrtc; livekit-uniffi doesn't use libwebrtc. A non-empty global | ||
| # RUSTFLAGS overrides that per-target config, selecting the GNU bfd | ||
| # linker the cross binutils provides. | ||
| export RUSTFLAGS="-C link-arg=-fuse-ld=bfd" | ||
| fi | ||
| cargo build --release --target ${{ matrix.target }} -p livekit-uniffi | ||
| run: cargo zigbuild --release --target ${{ matrix.target }}.2.28 -p livekit-uniffi |
There was a problem hiding this comment.
question: Can you explain what cargo-zigbuild is and why it is needed here? I looked it up online and it seems to be related to cross compilation? IMO if what was here before worked I think it would be better to keep it as is.
| @@ -25,7 +26,7 @@ import { | |||
|
|
|||
| async function main() { | |||
| // Receive log messages from Rust | |||
| logForwardBootstrap("debug"); | |||
| logForwardBootstrap(LogForwardFilter.Debug); | |||
|
|
|||
| // Print FFI version | |||
| console.log(`FFI version: v${buildVersion()}`); | |||
| @@ -54,8 +55,28 @@ async function main() { | |||
| const decodedGrants = tokenVerify(jwt, credentials); | |||
| console.log("Verified generated JWT:", decodedGrants); | |||
There was a problem hiding this comment.
Ah, this is a fairly important point that I'm not sure we had the same shared understanding on - the old node implementation was in livekit-uniffi, not livekit-ffi. But for node to interact with the existing ffi interface over livekit-ffi the uniffi implementation needs to be in livekit-ffi package instead, and the old livekit-uniffi node implementation (what you ported on top of here) can be dropped.
I think seeing all the work you have done here, it's making me think the easiest path to get from the current state to the desired end state (ie, node bindings in existing livekit-ffi built node packages) might be to couple this PR with the imminent livekit-ffi / livekit-uniffi merge.
What comes to mind is something like:
- Let this sit for a few weeks (IMO probably has to happen anyway so @lukasIO can review once he is back from vacation, he's the subject matter expert on node sdk builds in this repo) while the
livekit-ffi/livekit-uniffimerge takes place - Once that is done rebase it on top of the merged
livekit-ffi - While doing this rebase, move over the changes made here to
livekit-ffiand merge with the existing packages (ie, publish these new packages under the old names, with the old dylib names, etc so they are fully backwards compatible)
Thoughts on this?
2 participants
This is largely derived from the AIC work, which publishes to npmjs.