UniFFI trial #750
UniFFI trial #750
Conversation
ladvoc
force-pushed
the
ladvoc/uniffi-trial
branch
from
48449a0 to
41bd64c
Compare
| pub struct Claims { | ||
| pub exp: u64, | ||
| pub iss: String, | ||
| pub nbf: u64, |
There was a problem hiding this comment.
[nitpick] can we use SystemTime instead?
Makes clear what comes from uniffi
Allows client to access the version specified in Cargo.toml
* Unverified token * CR: Names * CR: Test token
ladvoc
force-pushed
the
ladvoc/uniffi-trial
branch
from
a028a03 to
3faab50
Compare
|
The I'd also commit (local) cc @1egoman |
# Conflicts: # Cargo.lock
|
I'll try to complete the Swift distribution path in the meantime. |
|
Here's more or less everything we need for local distribution, the tricky part is: [tasks.swift-generate-manifest.env]
SPM_URL = { value = "https://dummy.com" }
SPM_CHECKSUM = { value = "1234" }Let's assume the following workflow:
optionally:
Alternatively, we can skip |
|
I made a manual pre-release here: https://github.com/livekit/livekit-uniffi-xcframework @ladvoc if you're able to finish the release pipeline here, that would be cool (artifact is OK as mentioned above). |
livekit-uniffi/Makefile.toml
Outdated
| --platforms maccatalyst \ | ||
| --platforms visionos \ | ||
| --platforms tvos \ | ||
| --lib-type dynamic \ |
There was a problem hiding this comment.
And one more thing @ladvoc static release is still ~40MB whereas dylib ~1.8MB.
Dynamic one won't pass AppStore validation (we'd need to wrap it in a Framework).
90426: Invalid Swift Support. The SwiftSupport folder is missing. Rebuild your app using the current public (GM) version of Xcode and resubmit it.
I'd rather go for stripping the static one further, any ideas?
There was a problem hiding this comment.
iOS in --release profile:
Ar Archive - 47 MB
There was a problem hiding this comment.
Okay, looks like DEAD_CODE_STRIPPING does the job and the bundle size increase is negligible (0.1 MB), moved back to static 729bc10
It also passes 🍏 validation 👍
I'm still open to optimization ideas 😄
livekit-uniffi/Makefile.toml
Outdated
| script = """ | ||
| # Rename modules inside the xcframework to match the module name (fix for Swift >6.2) | ||
| # Workaroud for cargo-swift v0.10.0 not supporting ffi_module_name | ||
| find "${SPM_NAME}/Rust${SPM_NAME}.xcframework" -depth -type d -name "Rust${SPM_NAME}" -exec bash -c 'mv "$1" "${1%Rust'${SPM_NAME}'}livekit_uniffiFFI"' _ {} \; || true |
There was a problem hiding this comment.
Trying to fix it here antoniusnaumann/cargo-swift#87
Applies a patch to fix newer Swift versions 6.2+ failing when the module name does not match folder name inside xcframework. `cargo-swift` fix in progress, livekit/rust-sdks#750 (comment) tests https://github.com/livekit/client-sdk-swift/actions/runs/20334177943/job/58416504807
📝 WalkthroughWalkthroughAdds a new workspace member Changes
Sequence DiagramssequenceDiagram
participant Client as Client (Node/Python/Swift)
participant UniFFI as UniFFI Binding Layer
participant Rust as livekit-uniffi (Rust)
participant LiveKitAPI as livekit-api
Client->>UniFFI: token_generate(options, credentials)
UniFFI->>Rust: call token_generate
Rust->>LiveKitAPI: construct & sign token
LiveKitAPI-->>Rust: return JWT
Rust-->>UniFFI: return JWT
UniFFI-->>Client: return JWT
sequenceDiagram
participant Client as Client (Node/Python/Swift)
participant UniFFI as UniFFI Binding Layer
participant RustLogger as Rust Logger (log_forward)
participant Channel as MPSC Channel
Client->>UniFFI: log_forward_bootstrap(level)
UniFFI->>RustLogger: init global logger
RustLogger->>Channel: create sender/receiver
Note over RustLogger,Channel: Rust emits logs -> sent to Channel
Client->>UniFFI: log_forward_receive()
UniFFI->>Channel: poll receiver
Channel-->>UniFFI: Option<LogForwardEntry>
UniFFI-->>Client: deliver entry or None
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@livekit-uniffi/python_test/main.py`:
- Around line 44-52: The receive_log_messages loop using log_forward_receive can
block forever because the Rust channel may never close; update
receive_log_messages (or the caller) to avoid hanging by adding a timeout around
await log_forward_receive (e.g., with asyncio.wait_for) or by running the
receiver concurrently with the main token operations and cancelling it
afterwards; specifically modify the async function receive_log_messages and the
asyncio.run invocation so that log_forward_receive calls time out (handle
asyncio.TimeoutError and treat it as "no more logs") or run receive_log_messages
as a background task and cancel/join it after the token operations complete.
In `@livekit-uniffi/support/node/download-lib.ts.tera`:
- Around line 45-54: generateLibraryName currently switches on
'windows'/'macos'/'linux' but targetPlatform() returns Rust triple fragments
(e.g. 'pc-windows-msvc', 'apple-darwin', 'unknown-linux-gnu'), so no case
matches; update generateLibraryName to normalize or pattern-match the triples
returned by targetPlatform() (e.g. use startsWith/includes checks like
startsWith('pc-windows') or includes('darwin')/includes('linux') or map the
known triples to the three targets) and return the correct filename for each,
and add a default branch that throws/returns a clear error; reference
generateLibraryName and targetPlatform to locate and change the logic.
🧹 Nitpick comments (9)
livekit-uniffi/README.md (1)
19-21: Add language specifier to fenced code blocks.The shell command blocks should specify a language for proper syntax highlighting and to satisfy markdown linting.
📝 Suggested fix
-``` +```bash cargo make swift-package```diff -``` +```bash cargo make node-package</details> Also applies to: 26-28 </blockquote></details> <details> <summary>livekit-uniffi/support/node/download-lib.ts.tera (1)</summary><blockquote> `119-126`: **Consider adding type assertions for CLI arguments.** The `parseArgs` values are typed as `string | boolean | undefined`, but `downloadLib` expects specific platform/arch types. Consider adding runtime validation or type assertions to catch invalid CLI inputs early. <details> <summary>♻️ Suggested improvement</summary> ```diff +function assertValidPlatform(value: unknown): asserts value is TargetPlatform { + const valid = ['pc-windows-msvc', 'apple-darwin', 'unknown-linux-gnu']; + if (typeof value !== 'string' || !valid.includes(value)) { + throw new Error(`Invalid platform: ${value}`); + } +} + +function assertValidArch(value: unknown): asserts value is TargetArch { + const valid = ['aarch64', 'x86_64']; + if (typeof value !== 'string' || !valid.includes(value)) { + throw new Error(`Invalid arch: ${value}`); + } +} if (require.main === module) { // ... const { values } = parseArgs({ args: process.argv.slice(2), options }); + if (values.platform) assertValidPlatform(values.platform); + if (values.arch) assertValidArch(values.arch); downloadLib( values.platform, // ...livekit-uniffi/src/log_forward.rs (1)
102-102: Consider adding context to theunwrap()for future debugging.While the
unwrap()is currently safe (the receiver can't be dropped while theLoggerexists inOnceCell), adding anexpect()with a message would help if the design ever changes.♻️ Suggested improvement
- self.tx.send(record).unwrap(); + self.tx.send(record).expect("Log receiver unexpectedly dropped");livekit-uniffi/node_test/index.ts (1)
67-68: Handle async errors frommain()to avoid silent failures.Top-level promise rejections won’t be surfaced clearly; add a catch handler.
🛠️ Proposed fix
if (require.main === module) { - main(); + main().catch((err) => { + console.error(err); + process.exit(1); + }); }livekit-uniffi/node_test/package.json (1)
5-7: Consider cleaning up the placeholder test script.The
npm testscript in this package is not currently invoked in CI or any automation (tests are run directly vianpx tsx index.tsper the README). However, if someone manually runsnpm testin this directory, it will unconditionally fail. Either replace it with"test": "tsx index.ts"or remove it entirely to avoid confusion.livekit-uniffi/python_test/main.py (1)
21-21: Consider explicit imports instead of star import.While star imports are acceptable for test files, using explicit imports would silence the static analysis warnings and make dependencies clearer:
from livekit_uniffi import ( log_forward_bootstrap, log_forward_receive, LogForwardFilter, build_version, ApiCredentials, token_generate, token_verify, TokenOptions, )livekit-uniffi/Cargo.toml (1)
1-11: Consider reordering sections for conventional layout.Placing
[lib]before[package]is valid but unconventional. The typical Cargo.toml order is[package]first, followed by[lib],[[bin]], dependencies, and profiles. This is a minor style preference.livekit-uniffi/support/node/package.json.tera (1)
12-17: Consider settingmainto CJS for broader compatibility.The
mainfield points to the ESM entry (dist/index.mjs), but conventionallymainshould point to CommonJS for tools that don't supportexports. Consider:"main": "dist/index.js", "module": "dist/index.mjs",However, with
engines.node >= 18and properexportsfield, this may not be necessary for your target audience.livekit-uniffi/src/access_token.rs (1)
184-185: Address TODO: Remove test logging or the TODO comment.The TODO indicates this debug log was added for testing log forwarding. Either remove the log statement if it's not needed in production, or remove the TODO comment if the logging should be kept.
Do you want me to help clean up these debug log statements across
token_generateandtoken_verify?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locklivekit-uniffi/node_test/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
Cargo.tomllicenserc.tomllivekit-api/src/access_token.rslivekit-api/src/test_token.txtlivekit-uniffi/.gitignorelivekit-uniffi/Cargo.tomllivekit-uniffi/Makefile.tomllivekit-uniffi/README.mdlivekit-uniffi/bindgen.rslivekit-uniffi/node_test/.gitignorelivekit-uniffi/node_test/index.tslivekit-uniffi/node_test/package.jsonlivekit-uniffi/python_test/main.pylivekit-uniffi/src/access_token.rslivekit-uniffi/src/build_info.rslivekit-uniffi/src/lib.rslivekit-uniffi/src/log_forward.rslivekit-uniffi/support/node/download-lib.ts.teralivekit-uniffi/support/node/package.json.teralivekit-uniffi/support/node/tsconfig.jsonlivekit-uniffi/support/node/tsup.config.tslivekit-uniffi/support/swift/Package.swift.teralivekit-uniffi/support/swift/[email protected]
🧰 Additional context used
🧬 Code graph analysis (1)
livekit-uniffi/python_test/main.py (3)
livekit-uniffi/src/log_forward.rs (2)
log_forward_bootstrap(28-32)log_forward_receive(41-44)livekit-uniffi/src/build_info.rs (1)
build_version(17-19)livekit-uniffi/src/access_token.rs (2)
token_generate(180-230)token_verify(238-250)
🪛 Gitleaks (8.30.0)
livekit-api/src/test_token.txt
[high] 1-1: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🪛 markdownlint-cli2 (0.18.1)
livekit-uniffi/README.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.13)
livekit-uniffi/python_test/main.py
21-21: from livekit_uniffi import * used; unable to detect undefined names
(F403)
25-25: log_forward_bootstrap may be undefined, or defined from star imports
(F405)
25-25: LogForwardFilter may be undefined, or defined from star imports
(F405)
28-28: build_version may be undefined, or defined from star imports
(F405)
30-30: ApiCredentials may be undefined, or defined from star imports
(F405)
30-30: Possible hardcoded password assigned to argument: "secret"
(S106)
32-32: token_generate may be undefined, or defined from star imports
(F405)
33-33: TokenOptions may be undefined, or defined from star imports
(F405)
38-38: token_verify may be undefined, or defined from star imports
(F405)
46-46: log_forward_receive may be undefined, or defined from star imports
(F405)
🔇 Additional comments (25)
livekit-uniffi/node_test/.gitignore (1)
1-1: LGTM!Standard
.gitignoreentry for Node.js projects.livekit-uniffi/.gitignore (1)
1-2: LGTM!Correctly excludes generated artifacts and packaged bindings from version control.
livekit-api/src/test_token.txt (1)
1-1: Test token is appropriate for unit testing.The Gitleaks warning is a false positive—this is a test fixture with dummy credentials (
myapikeyas issuer,identityas subject) and a far-future expiration date (year 2100). The token is used for testingClaims::from_unverifiedand related token parsing functionality.licenserc.toml (1)
16-16: LGTM!The negation pattern correctly ensures
Makefile.tomlis checked for license headers despite the*.tomlexclusion. Excludinglivekit-uniffi/packagesappropriately skips generated artifacts.Also applies to: 21-21
livekit-uniffi/src/build_info.rs (1)
15-19: LGTM!Clean implementation using compile-time
env!macro to expose the crate version through UniFFI. The#[uniffi::export]attribute correctly generates cross-language bindings.Cargo.toml (1)
8-8: LGTM!The new
livekit-unifficrate is properly added to the workspace members.livekit-uniffi/Makefile.toml (2)
24-26: Placeholder download URL needs to be updated before release.The
CDYLIB_DOWNLOAD_BASE_URLpoints to an example URL. Ensure this is updated to the actual release artifact location before merging or enabling release builds.
293-299:sed -i ''syntax is acceptable for this macOS-only manual task.The
swift-packagetask is a manual developer workflow documented in the README for local Swift package generation, not part of CI automation. Since XCFramework/Swift package building is inherently macOS-focused, the BSD sed syntaxsed -i ''is appropriate and doesn't require portability fixes.If this task were automated in CI on multiple platforms, portability would be a concern. As-is, no action needed.
livekit-uniffi/support/swift/[email protected] (1)
1-41: LGTM!The Swift 6.0 manifest template properly handles development vs release profiles with appropriate binaryTarget configurations. The visionOS platform support (.v2) is correctly included.
livekit-uniffi/support/swift/Package.swift.tera (1)
8-13: visionOS is missing from platform list, unlike the Swift 6.0 template.The
[email protected]includes.visionOS(.v2)but this template omits it. Since visionOS support was added in Swift 5.9, this may be intentional (to support older Xcode versions) or an oversight.livekit-uniffi/src/log_forward.rs (2)
41-44:try_lock()may silently returnNoneunder lock contention.If multiple consumers call
log_forward_receive()concurrently and one holds the lock, others will returnNoneimmediately (interpreted as "forwarding ended"). This could cause premature termination of log polling loops in client code.Consider whether this is the intended behavior, or if you want to use
.lock().awaitto wait for the mutex (though this changes semantics for single-consumer scenarios).
27-32: LGTM!The bootstrap function correctly handles idempotent initialization while allowing subsequent level changes, as discussed in prior review comments.
livekit-uniffi/bindgen.rs (1)
15-19: LGTM for the bindgen entrypoint.livekit-uniffi/support/node/tsconfig.json (1)
2-13: LGTM for the TypeScript compiler settings.livekit-uniffi/support/node/tsup.config.ts (1)
3-15: LGTM for the tsup build configuration.livekit-uniffi/src/lib.rs (1)
15-26: LGTM for the module wiring.livekit-uniffi/node_test/index.ts (1)
57-63: The review comment is incorrect. The concern about empty strings terminating the loop does not apply here.The function
logForwardReceive()returnsOption<LogForwardEntry>(TypeScript:LogForwardEntry | null), not a string. The checkif (!message)correctly tests whether the entire object isnullto detect loop termination. Empty content in the message field insideLogForwardEntryhas no effect on the loop condition, so the current code handles the intended termination signal correctly.livekit-api/src/access_token.rs (2)
154-167: LGTM!The
from_unverifiedimplementation correctly disables signature validation while retainingexpandnbftime validation, which is appropriate for inspecting token contents without the secret. The method name clearly communicates the security implications.
398-441: LGTM!Comprehensive test coverage for
from_unverified:
- Parses pre-existing token with room configuration
- Parses freshly generated token
- Correctly handles malformed token with altered signature
Good validation of the intended behavior.
livekit-uniffi/Cargo.toml (1)
28-34: LGTM!The release profile is well-configured for FFI distribution:
panic = "abort"prevents undefined behavior from unwinding across FFI boundaries- Size optimizations (
opt-level = "z",lto,strip) are appropriate for distributable binarieslivekit-uniffi/support/node/package.json.tera (1)
34-44: Potential trailing comma issue in generated JSON.The template iterates over
devDependenciesand adds a trailing comma after each entry (line 37), which is followed by hardcoded entries. However, ifdevDependenciesis empty, the first hardcoded entry ("tsup") won't have a preceding comma separator from the loop, and the JSON will still be valid.The current pattern works but is fragile. If
devDependencieshas entries, the trailing comma from line 37 correctly precedes"tsup". Consider explicitly handling the comma separator for clarity:💡 Alternative approach
"devDependencies": { {% for package_name, package_version in devDependencies %} "{{package_name | addslashes}}": "{{package_version | addslashes}}", {%- endfor %} "tsup": "^8.4.0", ...The current approach works because of the trailing comma in the loop, but ensure the template is tested with both empty and populated
devDependencies.livekit-uniffi/src/access_token.rs (4)
21-29: LGTM!Correct use of
#[uniffi::remote(Error)]to expose the existingAccessTokenErrorfromlivekit_api. Theflat_errorattribute is appropriate for cross-language error handling.
107-139: LGTM!Using
u64for timestamps (exp,nbf) is appropriate for FFI - it's more portable across target languages thanSystemTime. Theusize as u64conversion at lines 126 and 128 is safe on both 32-bit (widening) and 64-bit (no-op) platforms.
237-250: LGTM!The verification logic correctly handles optional credentials and converts the result to the UniFFI-compatible
Claimstype. Note: the TODO on line 242-243 should also be addressed (same as noted fortoken_generate).
252-262: LGTM!Excellent documentation with a clear security warning. The implementation correctly delegates to the API layer's
from_unverifiedand converts to the UniFFI-compatible type.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| async def receive_log_messages(): | ||
| while True: | ||
| message = await log_forward_receive() | ||
| if message is None: | ||
| print("Log forwarding ended") | ||
| break | ||
| print(f"Log from Rust: {message}") | ||
|
|
||
| asyncio.run(receive_log_messages()) |
There was a problem hiding this comment.
Log receive loop may block indefinitely.
The async loop waits for log messages but has no mechanism to exit after the token operations complete. Unless the Rust log channel is explicitly closed, log_forward_receive() will block forever waiting for more messages, causing the test script to hang.
Consider adding a timeout or running the log receiver concurrently with the main operations:
💡 Suggested approach using asyncio.wait_for or concurrent execution
async def receive_log_messages():
import asyncio
try:
while True:
message = await asyncio.wait_for(log_forward_receive(), timeout=1.0)
if message is None:
print("Log forwarding ended")
break
print(f"Log from Rust: {message}")
except asyncio.TimeoutError:
print("No more log messages (timeout)")🧰 Tools
🪛 Ruff (0.14.13)
46-46: log_forward_receive may be undefined, or defined from star imports
(F405)
🤖 Prompt for AI Agents
In `@livekit-uniffi/python_test/main.py` around lines 44 - 52, The
receive_log_messages loop using log_forward_receive can block forever because
the Rust channel may never close; update receive_log_messages (or the caller) to
avoid hanging by adding a timeout around await log_forward_receive (e.g., with
asyncio.wait_for) or by running the receiver concurrently with the main token
operations and cancelling it afterwards; specifically modify the async function
receive_log_messages and the asyncio.run invocation so that log_forward_receive
calls time out (handle asyncio.TimeoutError and treat it as "no more logs") or
run receive_log_messages as a background task and cancel/join it after the token
operations complete.
| function generateLibraryName(platform: TargetPlatform) { | ||
| switch (platform) { | ||
| case "windows": | ||
| return `${PACKAGE_NAME}.dll`; | ||
| case "macos": | ||
| return `lib${PACKAGE_NAME}.dylib`; | ||
| case "linux": | ||
| return `lib${PACKAGE_NAME}.so`; | ||
| } | ||
| } |
There was a problem hiding this comment.
Platform string mismatch will cause generateLibraryName to never match.
The targetPlatform() function returns Rust target triple fragments ('pc-windows-msvc', 'apple-darwin', 'unknown-linux-gnu'), but generateLibraryName() switches on different strings ('windows', 'macos', 'linux'). This will cause all cases to fall through and return undefined.
🐛 Proposed fix
function generateLibraryName(platform: TargetPlatform) {
switch (platform) {
- case "windows":
+ case "pc-windows-msvc":
return `${PACKAGE_NAME}.dll`;
- case "macos":
+ case "apple-darwin":
return `lib${PACKAGE_NAME}.dylib`;
- case "linux":
+ case "unknown-linux-gnu":
return `lib${PACKAGE_NAME}.so`;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function generateLibraryName(platform: TargetPlatform) { | |
| switch (platform) { | |
| case "windows": | |
| return `${PACKAGE_NAME}.dll`; | |
| case "macos": | |
| return `lib${PACKAGE_NAME}.dylib`; | |
| case "linux": | |
| return `lib${PACKAGE_NAME}.so`; | |
| } | |
| } | |
| function generateLibraryName(platform: TargetPlatform) { | |
| switch (platform) { | |
| case "pc-windows-msvc": | |
| return `${PACKAGE_NAME}.dll`; | |
| case "apple-darwin": | |
| return `lib${PACKAGE_NAME}.dylib`; | |
| case "unknown-linux-gnu": | |
| return `lib${PACKAGE_NAME}.so`; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@livekit-uniffi/support/node/download-lib.ts.tera` around lines 45 - 54,
generateLibraryName currently switches on 'windows'/'macos'/'linux' but
targetPlatform() returns Rust triple fragments (e.g. 'pc-windows-msvc',
'apple-darwin', 'unknown-linux-gnu'), so no case matches; update
generateLibraryName to normalize or pattern-match the triples returned by
targetPlatform() (e.g. use startsWith/includes checks like
startsWith('pc-windows') or includes('darwin')/includes('linux') or map the
known triples to the three targets) and return the correct filename for each,
and add a default branch that throws/returns a clear error; reference
generateLibraryName and targetPlatform to locate and change the logic.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.