go: split moq-go-ffi bindings from the ergonomic moq-go wrapper, add the wrapper#1563
Conversation
WalkthroughThis PR separates raw UniFFI Go bindings (moq-go-ffi) from an ergonomic wrapper (moq-go), adds dedicated CI workflows for each, refactors staging/packaging/publish scripts, creates module metadata and READMEs, updates docs/site configuration, and implements the wrapper API (types, client/server, origin/publish/subscribe, iterator helpers) with tests. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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 |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
go/scripts/check.sh (1)
107-119: ⚡ Quick winValidate the published wrapper shape here, not a hand-assembled copy.
This block reimplements wrapper staging instead of reusing
scripts/package-wrapper.sh, sojust go checkcan pass while the actual publish packaging drifts. Please routecheck.shthrough the same wrapper packager, then add the localreplaceon the staged output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go/scripts/check.sh` around lines 107 - 119, The check script currently reimplements the wrapper staging logic instead of using the packager; update check.sh to invoke the existing packaging routine (scripts/package-wrapper.sh or the packageWrapper entrypoint) to produce the staged wrapper output into STAGE_WRAPPER, then apply the local module tweaks: run go mod edit -require="github.com/moq-dev/moq-go-ffi@v0.0.0-dev" and go mod edit -replace="github.com/moq-dev/moq-go-ffi=$FFI_PKG" against the staged output rather than copying files manually; remove the manual cp/mkdir steps and ensure the subsequent go check runs against the packager-produced staged output.go/ffi/README.md (1)
35-43: ⚡ Quick winDrop the em dash in the layout snippet.
Line 39 uses
— committed. Please switch that to a period or parentheses to match the repo's prose convention.Based on learnings "No em dashes (—) in code, comments, doc comments, commit messages, or any prose. Use a period and start a new sentence, or use a comma/parenthesis if the clauses are tightly bound."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go/ffi/README.md` around lines 35 - 43, Replace the em dash in the README snippet entry for "moq/cgo.go" — change "Per-platform cgo LDFLAGS only — committed" to use a period or parentheses (e.g., "Per-platform cgo LDFLAGS only. Committed" or "Per-platform cgo LDFLAGS only (committed)") so the line follows the repo's no-em-dash prose convention; update the go/ffi/README.md snippet accordingly.go/scripts/package-wrapper.sh (1)
148-155: ⚡ Quick winSet
GOWORK=offforgo mod tidyto avoid outer workspace interference.This repo has no
go.work, so workspace mode won’t come from within the checkout—butGOWORK=offensures the stagedgo.sumcan’t be influenced by a parent/ambient workspace if this script runs under one.Suggested hardening
if [[ "$TIDY" == true ]]; then ( cd "$PKG_STAGE" - GOFLAGS=-mod=mod GOPROXY="${GOPROXY:-direct}" go mod tidy + GOWORK=off GOFLAGS=-mod=mod GOPROXY="${GOPROXY:-direct}" go mod tidy ) else echo " skipping go mod tidy (no go.sum staged)" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@go/scripts/package-wrapper.sh` around lines 148 - 155, The go mod tidy invocation can be influenced by an outer go.work; set GOWORK=off when running go mod tidy to prevent ambient workspace interference. In the TIDY block around the subshell that cds into "$PKG_STAGE" and runs GOFLAGS=-mod=mod GOPROXY="${GOPROXY:-direct}" go mod tidy, add GOWORK=off to the environment for that command (or export it inside the subshell) so the tidy run is always in module mode and unaffected by any parent workspace.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/release-go-ffi.yml:
- Around line 88-89: The Rust cache steps named "Rust cache" are saving cache on
pull_request runs which lets untrusted PRs populate caches; modify the
rust-cache usages (the steps with name "Rust cache") to avoid saving on PRs by
gating the save behavior—either add an if: github.event_name != 'pull_request'
to the step(s) or configure the action inputs to conditionally set save: false
when github.event_name == 'pull_request'; apply the same change to the other
identical Rust cache step referenced in the workflow.
In `@doc/lib/go/index.md`:
- Line 14: The phrase "Pure Go" is misleading because the package depends
transitively on `moq-ffi` and requires CGO; update the sentence that currently
ends with "Pure Go; the native libraries come transitively from `moq-ffi`" to
something like "Go-first wrapper; native libraries are provided transitively via
`moq-ffi` and require CGO_ENABLED=1" (or "idiomatic Go wrapper") so readers
understand the CGO dependency while keeping the intent of the ergonomic wrapper
described earlier.
In `@go/scripts/package-wrapper.sh`:
- Around line 75-98: The script currently uses the CLI --line value (LINE) to
name the staged package but does not verify it matches the VERSION file copied
from SOURCE_DIR; add a validation step after copying VERSION into PKG_STAGE (or
immediately after setting PKG_STAGE) that reads the staged VERSION, strips any
leading "v", extracts its major.minor (MAJOR.MINOR) and compares it to LINE
(also normalized), and exit with an error if they differ so the staged package
name (PKG_NAME/PKG_STAGE) cannot diverge from the actual VERSION; reference
variables/functions: LINE, FFI_VERSION normalization, PKG_STAGE, PKG_NAME,
SOURCE_DIR and the VERSION file.
In `@go/wrapper/moq/client.go`:
- Around line 99-104: The build logic currently sets c.consumer from
c.publishOrigin as a fallback, causing publish-only clients to get a consumer;
change the logic in the client constructor (where c.consumer, c.consumeOrigin
and c.publishOrigin are referenced) to only assign c.consumer when
c.consumeOrigin != nil (remove the else-if branch that uses c.publishOrigin).
Ensure Dial remains responsible for wiring consumeOrigin into inner when
appropriate and that Announced / AnnouncedBroadcast continue to surface
ErrNoConsumeOrigin when no consume origin was provided.
In `@go/wrapper/moq/moq_test.go`:
- Line 58: Replace the unbounded context.Background() used for blocking stream
calls with a bounded context created via context.WithTimeout and ensure you
cancel it in test cleanup: change the ctx assignment (the variable named ctx in
moq_test.go) to use context.WithTimeout(...) with an appropriate timeout and
register cancel via t.Cleanup(cancel) (or defer cancel in the test helper), and
make the same replacement for the other occurrence of ctx at the second location
(around the other test at the later occurrence).
In `@go/wrapper/moq/origin.go`:
- Around line 27-29: OriginProducer.Publish currently dereferences
broadcast.inner unconditionally which panics if broadcast is nil; update the
method (OriginProducer.Publish) to check if broadcast == nil and return a
descriptive error (e.g., fmt.Errorf or errors.New) instead of calling
o.inner.Publish, otherwise proceed to call o.inner.Publish(path,
broadcast.inner). Ensure the error message clearly names the parameter
(broadcast) so callers can handle it.
In `@go/wrapper/moq/server.go`:
- Around line 184-185: Accept and related methods call runCancellable with
s.inner.Cancel (the server-wide stopper), so canceling the per-call context will
shut down the whole server; change Accept, Requests, Serve (and similar callers
of runCancellable) to use a per-call canceler instead of s.inner.Cancel: create
a derived cancel context from the provided ctx (e.g., via
context.WithCancel/WithTimeout) and pass that per-call cancel function or a
no-op canceler into runCancellable, leaving s.inner.Cancel reserved only for
Close; reference Server.Accept, runCancellable, s.inner.Cancel, s.inner.Accept,
Close, Requests and Serve when making these changes.
---
Nitpick comments:
In `@go/ffi/README.md`:
- Around line 35-43: Replace the em dash in the README snippet entry for
"moq/cgo.go" — change "Per-platform cgo LDFLAGS only — committed" to use a
period or parentheses (e.g., "Per-platform cgo LDFLAGS only. Committed" or
"Per-platform cgo LDFLAGS only (committed)") so the line follows the repo's
no-em-dash prose convention; update the go/ffi/README.md snippet accordingly.
In `@go/scripts/check.sh`:
- Around line 107-119: The check script currently reimplements the wrapper
staging logic instead of using the packager; update check.sh to invoke the
existing packaging routine (scripts/package-wrapper.sh or the packageWrapper
entrypoint) to produce the staged wrapper output into STAGE_WRAPPER, then apply
the local module tweaks: run go mod edit
-require="github.com/moq-dev/moq-go-ffi@v0.0.0-dev" and go mod edit
-replace="github.com/moq-dev/moq-go-ffi=$FFI_PKG" against the staged output
rather than copying files manually; remove the manual cp/mkdir steps and ensure
the subsequent go check runs against the packager-produced staged output.
In `@go/scripts/package-wrapper.sh`:
- Around line 148-155: The go mod tidy invocation can be influenced by an outer
go.work; set GOWORK=off when running go mod tidy to prevent ambient workspace
interference. In the TIDY block around the subshell that cds into "$PKG_STAGE"
and runs GOFLAGS=-mod=mod GOPROXY="${GOPROXY:-direct}" go mod tidy, add
GOWORK=off to the environment for that command (or export it inside the
subshell) so the tidy run is always in module mode and unaffected by any parent
workspace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8d4ac011-09a7-4001-adce-0c1934302e34
📒 Files selected for processing (33)
.github/workflows/release-go-ffi.yml.github/workflows/release-go.ymlCLAUDE.mddoc/.vitepress/config.tsdoc/lib/go/index.mddoc/lib/go/moq-ffi.mddoc/lib/go/moq.mddoc/lib/index.mdgo/.gitignorego/README.mdgo/ffi/README.mdgo/ffi/go.modgo/ffi/moq/cgo.gogo/go.modgo/justfilego/scripts/check.shgo/scripts/package-ffi.shgo/scripts/package-wrapper.shgo/scripts/publish-ffi.shgo/scripts/publish-wrapper.shgo/wrapper/README.mdgo/wrapper/VERSIONgo/wrapper/go.modgo/wrapper/moq/client.gogo/wrapper/moq/doc.gogo/wrapper/moq/errors.gogo/wrapper/moq/iter.gogo/wrapper/moq/moq_test.gogo/wrapper/moq/origin.gogo/wrapper/moq/publish.gogo/wrapper/moq/server.gogo/wrapper/moq/subscribe.gogo/wrapper/moq/types.go
💤 Files with no reviewable changes (2)
- go/README.md
- go/go.mod
…the wrapper Mirror the Python split (#1551) for Go. Today `go/` is a single module that is just the raw uniffi-bindgen-go output plus native libs, released lockstep with the moq-ffi crate, with no ergonomic layer. Split it into two co-located module skeletons: * go/ffi/ -> github.com/moq-dev/moq-go-ffi: the raw bindings + native libs (the old go/ content), released lockstep with the crate on each moq-ffi-v* tag (release-go-ffi.yml -> moq-dev/moq-go-ffi). * go/wrapper/ -> github.com/moq-dev/moq-go: a new ergonomic wrapper (package moq, import path unchanged), versioned independently and published by a new release-go.yml -> moq-dev/moq-go. The wrapper is a full re-wrap with idiomatic Go: context.Context cancellation (uniffi async is blocking in Go, so a goroutine + ctx.Done()->Cancel() helper bridges it), Go error returns with an IsShutdown helper, Go 1.23 iter.Seq2 stream iterators, and records/enums re-exported without the Moq prefix. Go MVS resolves to the max moq-go-ffi across the build graph, so a wrapper consumer always gets an ffi >= what the wrapper requires; since ffi is additive that is always new enough. release-go.yml auto-bumps the wrapper's require to the newest moq-go-ffi on every moq-ffi-v* tag and re-publishes, so moq-go@latest floats to the latest native core. MAJOR.MINOR lives in go/wrapper/VERSION; the patch is derived from the mirror's tags by publish-wrapper.sh, which skips publishing when the staged tree is unchanged so no-op triggers never mint empty releases. Also fixes the x86_64-apple-darwin matrix entry to use macos-15-intel (the old macos-latest is arm64 and mis-built the Intel .a). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ray) actionlint/shellcheck flagged the unquoted $TIDY_FLAG. Quoting it would pass an empty '' positional that the script rejects, so build the args as an array and append --skip-tidy conditionally instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- release-go-ffi.yml: gate Swatinem/rust-cache saves to non-PR events (save-if) so untrusted PR builds can't poison caches reused by trusted runs. - client.go: drop the publish-origin fallback when building the consumer, so a publish-only client surfaces ErrNoConsumeOrigin instead of silently reading the local publish origin. - server.go: Accept no longer passes the server-wide Cancel into runCancellable (a per-call ctx cancel was tearing down the whole listener). It now returns ctx.Err() and leaves the listener up; Serve calls Close on ctx-driven exit to stop and unblock the background accept. - origin.go: guard nil *BroadcastProducer in Publish instead of panicking. - package-wrapper.sh: validate --line against the source VERSION; GOWORK=off for go mod tidy. - check.sh: stage the wrapper via package-wrapper.sh so check exercises the real publish packaging instead of a hand-assembled copy. - moq_test.go: bound the blocking test contexts with a timeout. - docs/prose: stop calling the wrapper "Pure Go" (it needs CGO); remove em dashes per the repo convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebasing onto dev picked up the moq-ffi API where MoqOriginProducer.Publish was renamed to Announce (the announce-based origin reshaping in #1530/#1536). The wrapper's public OriginProducer.Publish stays; only the underlying ffi call changes. Everything else (announced, broadcast, producers/consumers) is unchanged, and just go check passes against dev's bindings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
713aa8b to
9941146
Compare
Summary
Mirrors the Python split (#1551) for Go, and writes the Go wrapper that didn't exist yet.
Today
go/is a single module that is just the rawuniffi-bindgen-gooutput plus prebuilt native libs, released lockstep with themoq-fficrate, with no ergonomic layer. This splits it into two co-located module skeletons:go/ffi/→github.com/moq-dev/moq-go-ffi— the raw bindings + native libs (the oldgo/content), released lockstep with the crate on eachmoq-ffi-v*tag viarelease-go-ffi.yml→moq-dev/moq-go-ffi.go/wrapper/→github.com/moq-dev/moq-go— a new ergonomic wrapper (packagemoq, import pathgithub.com/moq-dev/moq-go/moqunchanged), versioned independently and published by a newrelease-go.yml→moq-dev/moq-go. Starts at0.3(opening a line above the lockstep0.2.x, likepy/moq-rs).Ergonomic wrapper
Full re-wrap of every
Moq*type in idiomatic Go:context.Contextcancellation. uniffi-bindgen-go renders Rustasync fnas blocking Go calls, so arunCancellablehelper runs each on a goroutine and calls the object'sCancel()onctx.Done().errorreturns with anIsShutdownhelper (gracefulCancelled/Closed).iter.Seq2stream iterators (Frames/Groups/GroupsAsArrived/Updates/All) alongsideNext(ctx)-style methods.Moqprefix.Defeating Go's minimum-version-selection
Go MVS resolves to the max
moq-go-ffiacross the build graph, so a wrapper consumer always gets an ffi ≥ what the wrapper requires; since ffi is additive, that's always new enough.release-go.ymlauto-bumps the wrapper'srequireto the newestmoq-go-ffion everymoq-ffi-v*tag and re-publishes, somoq-go@latestfloats to the latest native core with no manual work.MAJOR.MINORlives ingo/wrapper/VERSION(human-owned API line); the patch is derived from the mirror's tags bypublish-wrapper.sh, which skips publishing when the staged tree is unchanged, so no-op triggers never mint empty patch releases.Drive-by fix
release-go-ffi.yml'sx86_64-apple-darwinmatrix entry now usesmacos-15-intel— the oldmacos-latestis arm64 and was mis-building the Intel.a.Test plan
just go check— builds hostmoq-ffi, regenerates bindings, stages both modules wired via a localreplace, runsgo vet/build/test. Passes, including local pub/sub + raw-track smoke tests.shellcheckclean on allgo/scripts/*.sh;gofmtclean; Biome clean onconfig.ts; both workflows parse as valid YAML.^{}refs, fresh-line.0) and thepackage-wrapper.shrequirerewrite +VERSIONstaging.release-go-ffi.yml/release-go.ymlPR jobs) exercise packaging +--dry-runpublish against the mirrors.Before merge (infra)
moq-dev/moq-go-ffimirror repo is created and seeded (initial README onmain, clone verified). It still needs the moq-bot GitHub App grantedcontents:write. The ffi publish job mints a token scoped tomoq-go-ffi; the wrapper keeps the existingmoq-gomirror.Reviewer notes
dev, rebased on top of the Swift (swift: split MoqFFI bindings from the Moq wrapper + Swift-native redesign #1561) and Kotlin (moq-ffi (Kotlin): split bindings from an independent ergonomic wrapper #1562) splits already there. The CLAUDE.md, docs, and sidebar edits were re-resolved against those (the/go/two-module block replaces the old single-skeleton entry).devpicked up the moq-ffi renameMoqOriginProducer.PublishtoAnnounce(the announce-based origin reshaping in REANNOUNCE via duplicate ANNOUNCE; announced() returns (path, Announced) #1530/moq-net: auto-create Origin on connect/accept, expose via Session #1536). The wrapper's publicOriginProducer.Publishis unchanged; only the underlying ffi call moved, andjust go checkpasses against dev's regenerated bindings.pushtrigger has nopathsfilter (GitHub silently drops tag pushes whenpathsis set, and we must react tomoq-ffi-v*tags); unrelated main pushes do a cheap idempotent no-op.MediaProducer.Used/Unusedhave no underlyingCancel, so a cancelledctxreturnsctx.Err()while the goroutine unwinds when the producer is finished (documented inline).🤖 Generated with Claude Code
(Written by Claude)