Repository navigation
quest(m1): binding namespace spike is a no-go - #5460
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
UniFFI 0.32 keys a crate's types by bare name, so a layer crate cannot reuse the net noun it wraps (media.GroupConsumer over GroupConsumer). Every generator refuses it except Python, which drops the type silently. Keep the wrapper namespaces and delete the crate-split migration. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Spike outcome: no-go. Codecs ( Open decision for the maintainer: do we accept the no-go as final?
Spike harness (local only, not committed): a two-crate (Written by Claude Opus 5.5) |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 26 minutes. View limit details
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 |
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. |
![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: ef103c60c2
ℹ️ 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".
| - Still open from #5356: whether the import-side | ||
| `MoqMediaContainerStreamProducer` (`media.ContainerStreamProducer`) is | ||
| named by role. "Container" there means an importer that recovers framing | ||
| from a raw byte stream and may publish several tracks, unlike the | ||
| per-track `media.TrackConsumer` and `media.GroupConsumer`. |
There was a problem hiding this comment.
Track this decision in its own quest or assign it to one of this quest's required children. This commit deletes ffi-crate-split.md, the only task that owned reviewing this import-side name, but replaces it with an unlinked “Still open” note outside the Required list. When Codecs and Layers guide finish, FFI shape can therefore be retired and the binding release can proceed without settling the public name, even though naming decisions must be put to the maintainer.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. d32c9c6 moves it into quest/m1/ffi-shape/stream-producer-name.md, listed under the FFI shape epic's Required, so the epic cannot retire and the binding release cannot ship without the maintainer settling the name.
(Written by Claude Opus 5.5)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: ef103c6.
No actionable bugs found in these quest-only changes. Keeping wrapper namespaces follows the accepted no-go, removes the rejected split prerequisites, and preserves the unresolved import-side naming question. The separate task-ownership concern is already recorded at #5460 (comment); I found no runtime or packaging defect to add.
Verification: GitHub-only inspection of the changed plans, remaining references and current FFI configuration. The spike harness and generator logs are local-only, so I could not independently reproduce the reported experiments or size measurements. No tests or builds run.
The deleted crate split owned this naming review; an unlinked note in the epic's settled shape could be skipped when the epic retires. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merging on the maintainer's decision: ✅ accept the no-go as final. The wrapper namespaces stay, the crate split is dropped, and Codecs ( Since the last review: merged main (no conflicts, (Written by Claude Opus 5.5) |
|
Follow-up after merge: the maintainer asked for two more checks. The verdict is still no-go until mozilla/uniffi-rs#3026 is resolved.
The evidence table and the quest update linking the issue are in #5473. (Written by Claude Opus 5.5) |
Spike for
quest/m1/ffi-crate-spike.md. Verdict: no-go. The wrappers keep supplying the binding namespaces, and the migration quest (quest/m1/ffi-crate-split.md) is deleted, as the spike's plan specified for a measured no-go.Why
The split exists so a layer can reuse the net noun it wraps:
media.GroupConsumer(group: GroupConsumer). UniFFI 0.32.2 cannot generate that.uniffi_bindgen'sTypeUniversekeys every type a crate references by bare name, so a media crate that definesGroupConsumerand also takes the netGroupConsumerfails withconflicting typesin every generator built onComponentInterface. Python, which uses the newer pipeline, generates without an error but silently dropsmedia.GroupConsumerandmedia.Frame. The check is unchanged on uniffimain, and the Go, Dart, and C++ forks inherit it from upstreamuniffi_bindgen, so a fork patch cannot fix it.Distinct names do work across crates, but that only turns
MoqMediaGroupConsumerintomedia.MediaGroupConsumer. The wrappers already shipmedia.GroupConsumer.Evidence
The spike was a standalone two-crate workspace:
net(namespacemoq) andmedia, linked into onelibmoq_fficdylib/staticlib.netexports an object (GroupConsumer, sync and async methods), an enum error with fields and an exportedDisplay(MoqError), a record (Frame), and an enum (ErrorScope).mediawraps the object, returns a media record that holds the net record and enum, throws the net error from sync and async calls, and nests the net object in a media enum variant. Each generator ran against the built library, and each consumer was compiled and run against it.mediauniffi-bindgen-gofork)moq/andmedia/packagesconflicting typesgo_modconfig);go runpassesuniffi-dartfork)moq.dartandmedia.dart, one native asset iddart runpassesuniffi-bindgen-cppfork)moq::andmedia::, shared guarded runtime--lang c)uniffi.moqanduniffi.mediaimport moqadded tomedia.swift, which the generator does not emitmoq.pyandmedia.py; maturin builds one wheel with one.soPackaging stays one native library on every platform: Go links one staticlib from both cgo packages, Dart shares one asset id, Kotlin and Python load one
.so, and C++ compiles both namespaces against one library.Size
Each extra UniFFI crate adds its own 53
ffi_<crate>_*scaffolding exports (RustBuffer and rust_future entry points), plus the code behind them. The same items as one crate vs. two crates, release profile (fat LTO,strip = "debuginfo"): 460,088 B vs. 507,800 B for the cdylib, so about +47.7 KB per crate. Six layer crates would add about 240 KB to today's moq-ffi release cdylib (29,177,272 B on x86_64 Linux with default features, so about +0.8%). The real moq-ffi was not split, because the generator verdict already decided the question.Quest changes
quest/m1/ffi-crate-spike.md(done) andquest/m1/ffi-crate-split.md(abandoned on this no-go).quest/m1/ffi-shape/README.md: records the rejection and when to revisit.quest/m1/ffi-shape/stream-producer-name.md, required by the FFI shape epic: the openmedia.ContainerStreamProducernaming question from feat(ffi): decode a raw group as media, for arrival-order reads #5356, which the deleted migration had absorbed.quest/m1/cpp-generated-shape.mdandquest/m1/c/README.mdno longer wait on the split.Decisions
Does the no-go stand as final?
TypeUniverseto namespace external types, then revisit the split.uniffi_bindgen: patch the bare-name check ourselves and carry it in every generator fork.Public API and wire impact
None. Quest files only.
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code