Skip to content

Add pkg-config and CMake C smoke cells#9

Merged
kixelated merged 2 commits into
mainfrom
claude/quirky-cori-bcbf20
Jun 16, 2026
Merged

Add pkg-config and CMake C smoke cells#9
kixelated merged 2 commits into
mainfrom
claude/quirky-cori-bcbf20

Conversation

@kixelated

Copy link
Copy Markdown
Contributor

What

Adds two new C subscriber cells to the smoke matrix — c-pkgconfig and c-cmake — alongside the existing c. All three build the same subscribe-only clients/c/subscribe.c against the same prebuilt libmoq tarball; they differ only in how the build discovers the library:

Cell How it links Catches
c hand-rolled -I/-L/-l (unchanged) does the static archive link at all
c-pkgconfig pkg-config --cflags --static --libs moq wrong includedir/libdir or a missing framework in moq.pc
c-cmake find_package(moq)moq::moq broken lib/cmake/moq package config

This mirrors the existing js-vite/js-esbuild/js-jsdelivr "same client, different delivery" pattern.

Why

The C client hand-links libmoq with its own -I/-L/-l flags and hardcoded system libs, so it never reads the lib/pkgconfig/moq.pc or lib/cmake/moq/ files that the release tarball also ships. That leaves a whole class of packaging breakage invisible to the smoke test — e.g. a wrong includedir/libdir or a missing -framework CoreServices in moq.pc, the kind of thing moq#1758 fixed. Those are the FFmpeg --enable-libmoq / CMake-consumer paths the harness had no equivalent of.

The --static flag on the pkg-config build is the key bit: libmoq is a static archive, so --static pulls in Libs.private (the system frameworks/libs), meaning a missing framework genuinely fails the build.

Notes for reviewers

  • c_prepare is split into a shared, idempotent libmoq_fetch (one download for all three variants) plus c_build_direct / c_build_pkgconfig / c_build_cmake. Each variant fails only its own cell; a failed download marks all three broken (same as the js group).
  • New clients/c/CMakeLists.txt is driven by c_build_cmake (CMAKE_PREFIX_PATH → tarball root); it's not meant to be built standalone.
  • CI: added pkg-config + cmake to the Linux and macOS dep installs, and c-pkgconfig,c-cmake to the matrix + the just full recipe.
  • Still tests the nix-relocated tarball layout (what releases ship), not the raw cargo build target tree where moq#1758's path bug actually lived. Closing that gap would mean building libmoq from source in the harness — a bigger change, arguably outside this repo's "test the public packages" charter. Flagging as a possible follow-up.

Verification

  • bash -n smoke.sh clean.
  • Live run on macOS (libmoq-v0.3.5, aarch64): rust → c,c-pkgconfig,c-cmake all PASS.
  • Couldn't run shellcheck/shfmt locally (not installed); CI's just check will lint.

The C smoke client hand-links the prebuilt libmoq with its own -I/-L/-l and
hardcoded system libs, so it never reads the moq.pc or lib/cmake/moq package
that the release tarball also ships. That left a whole class of packaging
breakage invisible: a wrong includedir/libdir or a missing framework in moq.pc
(the FFmpeg --enable-libmoq consumer path) passes the smoke test today.

Add two more subscriber cells alongside `c`, mirroring the
js-vite/js-esbuild/js-jsdelivr "same client, different delivery" pattern:

  - c-pkgconfig: resolves cflags/libs via `pkg-config --cflags --static --libs
    moq`. --static pulls Libs.private (the system frameworks/libs), so a broken
    moq.pc fails the build.
  - c-cmake: builds via find_package(moq) against the shipped CMake package,
    importing the moq::moq target.

All three produce the same subscribe-only binary from the same tarball and run
identically. Split c_prepare into a shared, idempotent libmoq_fetch plus three
build helpers so the download happens once and each variant fails only its own
cell. Wire pkg-config + cmake into the CI deps and add the cells to the matrix
and the `full` recipe.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@kixelated, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 49 minutes and 6 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a6f19e42-d223-423e-ad48-68e7489e0c18

📥 Commits

Reviewing files that changed from the base of the PR and between d735bc0 and a688f20.

📒 Files selected for processing (4)
  • .editorconfig
  • freshness.sh
  • justfile
  • smoke.sh

Walkthrough

The PR expands C smoke-test coverage from one build variant to three. A new libmoq_fetch function replaces the old c_prepare, downloading the prebuilt libmoq release once and exposing LIBMOQ_ROOT and LIBMOQ_OS_LIBS. Three independent build functions (c_build_direct, c_build_pkgconfig, c_build_cmake) each compile subscribe.c using a different linkage method, producing separate binaries (C_SMOKE, C_PC_SMOKE, C_CMAKE_SMOKE). A new clients/c/CMakeLists.txt supports the CMake variant via find_package(moq). The subscriber dispatch and CI matrix are updated to include all three C variants, and the justfile full recipe gains js-native-node and js-native-bun subscribers.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding pkg-config and CMake C smoke cells to the test matrix.
Description check ✅ Passed The description comprehensively documents the what, why, and how of the changes, including motivation for catching packaging breakage and detailed implementation notes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch claude/quirky-cori-bcbf20

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

`just check` hardcoded `smoke.sh freshness.sh token.sh`, missing the two
extensionless bash wrappers under clients/docker/, and drove shfmt with CLI
flags. Switch to moq's setup: a committed .editorconfig carries the shfmt style
(indent 4, switch_case_indent, ignore vendored trees), and `just check`
discovers every shell file with `shfmt -f .` for both shfmt and shellcheck.

This surfaced pre-existing drift the local-only `just check` never caught (CI
doesn't run it): under the dev shell's shfmt 3.13.1 the scripts weren't clean.
Reformat them to be shfmt-clean (the compact `cmd || { ...; }` guards expand to
multi-line blocks) and silence freshness.sh's shellcheck findings -- SC2001 via
a parameter expansion, two intentional literal-$ greps via inline SC2016
disables.

The .editorconfig adds a clients/docker/* glob so the shebang-only wrappers get
the same style as the *.sh scripts.

Verified: shfmt --diff, shellcheck, and actionlint all clean over the discovered
set; rust -> c,c-pkgconfig,c-cmake still PASS after the reformat.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@kixelated kixelated merged commit 6fecbeb into main Jun 16, 2026
0 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant