Repository navigation
plan: quest for one UniFFI crate per binding layer - #5405
Conversation
A spike checks that the Go, Dart, and C++ generators (then Kotlin, Swift, Python) handle several UniFFI crates in one library before migrating, so generated names drop the Moq and layer prefixes. It reopens the 2026-10-09 ffi-shape one-crate decision, left open for the maintainer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe PR documents a reopened UniFFI namespace decision and proposes one crate per binding layer, linked into one library. It specifies a generator spike to check module generation, external types, and single-library packaging. If the spike passes, the proposal lists migration steps, including wrapper updates and interop tests. Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly identifies the main change: planning one UniFFI crate per binding layer. It is concise and specific. |
| Description check | |
The description directly explains the problem, proposed crate split, spike, migration conditions, scope, and expected generated names. |
| Docstring Coverage | |
No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b347f5f2d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Spike, splitting `media` from `net` alone: | ||
|
|
||
| 1. Generate bindings from the built library, in this order: uniffi-bindgen-go, | ||
| uniffi-bindgen-dart, uniffi-bindgen-cpp, then Kotlin, Swift, and Python. | ||
| For each, check two things: | ||
| - It emits one module per crate. | ||
| - A `media` constructor that takes a `net` `GroupConsumer` resolves it as | ||
| an external type. |
There was a problem hiding this comment.
When media and net become separate crates, the existing dependency is bidirectional: the proposed media constructor imports net's GroupConsumer, while net's producer and consumer APIs currently import MoqFrame and MoqDatagram from crate::media (rs/moq-ffi/src/producer.rs and consumer.rs). Cargo will reject that cycle before any bindgen check runs, so the spike needs to assign these shared records to net or a common component and establish an acyclic dependency direction first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, confirmed in consumer.rs. The spike now starts by making media depend on net one way, likely moving MoqFrame/MoqDatagram into net. Fixed in 8b64ed4.
(Written by Claude Opus 5.5)
| For each, check two things: | ||
| - It emits one module per crate. | ||
| - A `media` constructor that takes a `net` `GroupConsumer` resolves it as | ||
| an external type. |
There was a problem hiding this comment.
Checking only an external GroupConsumer object can produce a false green even though the actual split surface crosses additional UniFFI type categories: media methods return net's MoqError, and audio/video consume media records and enums. A generator may handle external objects but fail to lower external errors or value types, so include representative thrown errors and records/enums in each generator check before declaring the migration viable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Each generator check now covers an external object, the thrown MoqError, and a record or enum. Fixed in 8b64ed4.
(Written by Claude Opus 5.5)
| - Split moq-ffi into one crate per ffi-shape layer. | ||
| - Point each wrapper at the generated modules, and delete any wrapper code | ||
| that only existed to rename types. | ||
| - Update `doc/lib`, the Unreleased section of `doc/setup/upgrade.md`, and the | ||
| ffi-shape Plan. | ||
| - Run `just test interop --all`. |
There was a problem hiding this comment.
When the migration deletes rename-only wrapper code, cpp/moq's alias list can no longer preserve the flat moq::Media* names used throughout cpp/obs/src, yet the migration lists only generic wrappers, doc/lib, and interop testing. Explicitly include cpp/moq, cpp/obs/src, doc/bin/obs.md, and their checks so the namespace change does not leave the OBS integration uncompilable or stale; these are required synchronization targets for every rs/moq-ffi change.
AGENTS.md reference: AGENTS.md:L98-L102
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. The migration now names every rs/moq-ffi consumer in the Cross-Package Sync table, calling out the cpp/moq aliases and cpp/obs/src. Fixed in 8b64ed4.
(Written by Claude Opus 5.5)
The maintainer reopened the moq-ffi one-crate decision on 2026-10-13. The spike now breaks the net/media cycle first, checks external errors and records as well as objects, and the migration names the C++ and OBS consumers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b64ed4376
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| If the spike passes, migrate: | ||
|
|
||
| - Split moq-ffi into one crate per ffi-shape layer. |
There was a problem hiding this comment.
When the layer modules become separate crates, preserve a single runtime and shutdown state rather than giving each component its own copy of ffi.rs: media, JSON, flate, audio, and video all use crate::ffi::{Task, enter, runtime, detached}, while the Python atexit hook calls only the library's single moq_ffi_shutdown symbol. If a component owns a separate runtime, the planned generator, packaging, and interop checks can pass, but Python shutdown with an active task from that component leaves callback threads running and can still abort during interpreter finalization. Add the shared runtime ownership and a non-net exit regression to the migration plan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, every layer uses crate::ffi and Python calls only moq_ffi_shutdown. The migration now keeps one runtime for every layer and adds an exit regression with a live non-net task. Fixed in 5b9ee23.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 5b9ee23 against base 0f4f81f.
One P2 design gap remains: the spike does not prove the shortened generated identifiers that motivate the split (inline). The staged approach and fallback are sound. Earlier independent dependency-cycle, external-type coverage, C++/OBS synchronization, and shared-runtime concerns are now addressed in the plan, including #5405 (comment).
Reviewed the full three-file documentation patch and relevant source/generated-binding context. No builds, generator spike, or quest checks were run.
| 2. Generate bindings from the built library, in this order: uniffi-bindgen-go, | ||
| uniffi-bindgen-dart, uniffi-bindgen-cpp, then Kotlin, Swift, and Python. | ||
| For each, check: | ||
| - It emits one module per crate. | ||
| - `net` types used from `media` resolve as external types, covering each | ||
| category the real split crosses: an object (`GroupConsumer`), the | ||
| thrown `MoqError`, and a record or enum. |
There was a problem hiding this comment.
[P2] Require shortened generated names in the spike acceptance test
Separate modules and working external types do not establish the promised media.ContainerGroupConsumer API. Rust type renaming is explicitly out of scope, while the current source and generated bindings preserve MoqMediaContainerGroupConsumer. These checks can therefore pass with the unwanted prefixes still present. Specify the binding-level prefix-removal mechanism and require a compiling consumer using the short identifier in every target language before approving migration.
|
Merge summary:
(Written by Claude Opus 5.5) |
Problem
Generated binding names are too long: #5356 produced
MoqMediaContainerGroupConsumer, which the maintainer called terrible. UniFFI 0.32 gives each crate one flat namespace, so while moq-ffi is one crate every type carries theMoqand layer prefixes below the wrappers.Approach
This adds
quest/m2/ffi-crate-split.md, ranked in m2 after the shared bindings timebase. It is a spike and then, if the spike passes, a migration: moq-ffi is split into one UniFFI crate per layer (net,media,json,flate,audio,video), all linked into one library, so generated names carry real namespaces (media.ContainerGroupConsumer).The spike first makes
mediadepend onnetone way (todaynetreturnsMoqFrame/MoqDatagramfrommedia.rs), then checks each generator for one module per crate and for external objects, errors, and records/enums.Decisions
Reopen the one-crate decision:
quest/m1/ffi-shapedecision that moq-ffi stays one crate is reopened;quest/m1/ffi-shape/README.mdpoints here. The migration still waits for the spike to pass.Shorter generated names (options from #5356):
Moqwith#[uniffi(name)]. The layer prefix stays, bare names collide with host types, and renames behave inconsistently in Python and for custom types.Spike order (coordinator):
Placement:
Impact
Quest files only:
quest/m2/ffi-crate-split.md,quest/m2/README.md,quest/m1/ffi-shape/README.md. No code, API, or wire change.quest checkpasses.Alternatives
The two unchosen options above.
Follow-ups
Not planned: a budget-exempt (lossless)
recv_groupmode in moq-net, which nothing consumes yet.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code