Portable ffmpeg linkage + build.sh for releases#43
Conversation
Make the plugin link the ffmpeg the obs-deps bundle ships (the same one OBS uses at runtime) on macOS and Windows, so release binaries are portable: - CMakeLists: use find_package(FFmpeg) on WIN32 OR APPLE via a vendored FindFFmpeg.cmake that searches the obs-deps prefix (obs-deps ship ffmpeg with no pkg-config .pc files). Linux keeps pkg-config. Previously macOS used pkg-config (reached for Homebrew ffmpeg at an absolute path) and Windows couldn't find ffmpeg at all. - cmake/common/FindFFmpeg.cmake: vendored finder (no pkg-config dependency). - bootstrap.cmake: honor -DPLUGIN_VERSION_OVERRIDE so release CI can version the plugin to match the libmoq release it links. - cmake/macos/buildspec.cmake: make the xattr quarantine strip non-fatal (it errors on build outputs that carry no quarantine attribute). - build.sh: drive per-platform configure/build/package. --libmoq-release fetches a published libmoq release (no local cargo build) and versions the plugin to match; otherwise the buildspec MOQ_VERSION is used. Verified on macOS arm64: links @rpath/libavcodec.dylib (obs-deps ffmpeg 7), not Homebrew. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 52 minutes and 43 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 To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThe PR adds 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmake/common/bootstrap.cmake (1)
58-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate override version shape before deriving
PLUGIN_VERSION_*.The override path assumes at least three dot-separated parts; malformed input crashes at
list(GET ...)with an opaque error instead of a clear message.Suggested fix
string(REPLACE "." ";" _version_canonical "${_version}") +list(LENGTH _version_canonical _version_parts_len) +if(_version_parts_len LESS 3) + message(FATAL_ERROR "Invalid plugin version '${_version}'. Expected at least MAJOR.MINOR.PATCH.") +endif() list(GET _version_canonical 0 PLUGIN_VERSION_MAJOR) list(GET _version_canonical 1 PLUGIN_VERSION_MINOR) list(GET _version_canonical 2 PLUGIN_VERSION_PATCH) +unset(_version_parts_len) unset(_version_canonical)🤖 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 `@cmake/common/bootstrap.cmake` around lines 58 - 71, Add validation of the PLUGIN_VERSION_OVERRIDE value before attempting to parse it into version components. After setting _version from PLUGIN_VERSION_OVERRIDE, verify that the string contains at least two dots (to ensure three dot-separated parts for major.minor.patch format). If validation fails, output a clear error message indicating the expected format and halt execution using message(FATAL_ERROR...), preventing the subsequent list(GET...) calls from failing with opaque errors when attempting to extract PLUGIN_VERSION_MAJOR, PLUGIN_VERSION_MINOR, and PLUGIN_VERSION_PATCH.
🤖 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 `@build.sh`:
- Around line 76-95: The macOS pattern matching in the case statement checking
against TARGET is too restrictive. The pattern `*-apple-darwin` does not match
triples with Darwin version suffixes like `arm64-apple-darwin23.x.y`, causing
valid macOS targets to fall through to the unsupported case. Update the pattern
for the macOS branch to use `*-apple-darwin*` to match triples with optional
Darwin version suffixes following the word "darwin".
- Around line 25-45: The flag option parser for --target, --version, --output,
and --libmoq-release does not validate that their required argument values exist
before accessing $2, which causes unhelpful shell errors when set -u is enabled.
Add validation before each variable assignment (TARGET, VERSION, OUTPUT_DIR,
MOQ_RELEASE) to check that $2 is not empty and is not another flag, and provide
clear error messages indicating which flag is missing its required value.
In `@cmake/common/FindFFmpeg.cmake`:
- Around line 166-169: The undefined variable `_library_var` in the assignment
to `FFmpeg_${component}_LIBRARIES` causes it to resolve to empty, breaking
downstream consumers that depend on proper library linking information. Replace
the reference to the undefined `_library_var` with the correct library variable
`FFmpeg_${component}_LIBRARY` in the set statement at line 166 where
`FFmpeg_${component}_LIBRARIES` is assigned. Apply the same fix to the duplicate
issue mentioned at lines 282-284 where the same undefined variable pattern
occurs.
---
Outside diff comments:
In `@cmake/common/bootstrap.cmake`:
- Around line 58-71: Add validation of the PLUGIN_VERSION_OVERRIDE value before
attempting to parse it into version components. After setting _version from
PLUGIN_VERSION_OVERRIDE, verify that the string contains at least two dots (to
ensure three dot-separated parts for major.minor.patch format). If validation
fails, output a clear error message indicating the expected format and halt
execution using message(FATAL_ERROR...), preventing the subsequent list(GET...)
calls from failing with opaque errors when attempting to extract
PLUGIN_VERSION_MAJOR, PLUGIN_VERSION_MINOR, and PLUGIN_VERSION_PATCH.
🪄 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: 74da4167-29be-4e3c-8d3c-2a53fc381d33
📒 Files selected for processing (5)
CMakeLists.txtbuild.shcmake/common/FindFFmpeg.cmakecmake/common/bootstrap.cmakecmake/macos/buildspec.cmake
| while [[ $# -gt 0 ]]; do | ||
| case $1 in | ||
| --target) | ||
| TARGET="$2" | ||
| shift 2 | ||
| ;; | ||
| --version) | ||
| VERSION="$2" | ||
| shift 2 | ||
| ;; | ||
| --output) | ||
| OUTPUT_DIR="$2" | ||
| shift 2 | ||
| ;; | ||
| --libmoq-release) | ||
| # Link a published libmoq release of this version instead of | ||
| # building rs/libmoq from source. CMake fetches the matching | ||
| # moq-<version>-<target> archive from the GitHub release and the | ||
| # plugin is versioned to match. Used by CI on a libmoq-v* tag. | ||
| MOQ_RELEASE="$2" | ||
| shift 2 |
There was a problem hiding this comment.
Flag options that are missing a required value.
For --target/--version/--output/--libmoq-release, the parser reads $2 without checking it exists. With set -u, this exits abruptly with an unhelpful shell error.
Suggested fix
+require_value() {
+ if [[ $# -lt 2 || "$2" == --* ]]; then
+ echo "Missing value for $1" >&2
+ exit 1
+ fi
+}
+
while [[ $# -gt 0 ]]; do
case $1 in
--target)
+ require_value "$@"
TARGET="$2"
shift 2
;;
--version)
+ require_value "$@"
VERSION="$2"
shift 2
;;
--output)
+ require_value "$@"
OUTPUT_DIR="$2"
shift 2
;;
--libmoq-release)
+ require_value "$@"
# Link a published libmoq release of this version instead of🤖 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 `@build.sh` around lines 25 - 45, The flag option parser for --target,
--version, --output, and --libmoq-release does not validate that their required
argument values exist before accessing $2, which causes unhelpful shell errors
when set -u is enabled. Add validation before each variable assignment (TARGET,
VERSION, OUTPUT_DIR, MOQ_RELEASE) to check that $2 is not empty and is not
another flag, and provide clear error messages indicating which flag is missing
its required value.
| case "$TARGET" in | ||
| *-linux-*) | ||
| PRESET="ubuntu-x86_64" | ||
| BUILD_DIR="$SCRIPT_DIR/build_x86_64" | ||
| KIND="unix" | ||
| ;; | ||
| *-apple-darwin) | ||
| PRESET="macos" | ||
| BUILD_DIR="$SCRIPT_DIR/build_macos" | ||
| KIND="macos" | ||
| ;; | ||
| *-windows-*) | ||
| PRESET="windows-x64" | ||
| BUILD_DIR="$SCRIPT_DIR/build_x64" | ||
| KIND="windows" | ||
| ;; | ||
| *) | ||
| echo "Unsupported target: $TARGET" >&2 | ||
| exit 1 | ||
| ;; |
There was a problem hiding this comment.
macOS auto-detection pattern is too strict and can reject valid triples.
cc -dumpmachine on macOS commonly includes a Darwin version suffix (for example, arm64-apple-darwin23.x.y), which does not match *-apple-darwin and falls into “Unsupported target”.
Suggested fix
-*-apple-darwin)
+*-apple-darwin*)
PRESET="macos"
BUILD_DIR="$SCRIPT_DIR/build_macos"
KIND="macos"
;;📝 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.
| case "$TARGET" in | |
| *-linux-*) | |
| PRESET="ubuntu-x86_64" | |
| BUILD_DIR="$SCRIPT_DIR/build_x86_64" | |
| KIND="unix" | |
| ;; | |
| *-apple-darwin) | |
| PRESET="macos" | |
| BUILD_DIR="$SCRIPT_DIR/build_macos" | |
| KIND="macos" | |
| ;; | |
| *-windows-*) | |
| PRESET="windows-x64" | |
| BUILD_DIR="$SCRIPT_DIR/build_x64" | |
| KIND="windows" | |
| ;; | |
| *) | |
| echo "Unsupported target: $TARGET" >&2 | |
| exit 1 | |
| ;; | |
| case "$TARGET" in | |
| *-linux-*) | |
| PRESET="ubuntu-x86_64" | |
| BUILD_DIR="$SCRIPT_DIR/build_x86_64" | |
| KIND="unix" | |
| ;; | |
| *-apple-darwin*) | |
| PRESET="macos" | |
| BUILD_DIR="$SCRIPT_DIR/build_macos" | |
| KIND="macos" | |
| ;; | |
| *-windows-*) | |
| PRESET="windows-x64" | |
| BUILD_DIR="$SCRIPT_DIR/build_x64" | |
| KIND="windows" | |
| ;; | |
| *) | |
| echo "Unsupported target: $TARGET" >&2 | |
| exit 1 | |
| ;; |
🤖 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 `@build.sh` around lines 76 - 95, The macOS pattern matching in the case
statement checking against TARGET is too restrictive. The pattern
`*-apple-darwin` does not match triples with Darwin version suffixes like
`arm64-apple-darwin23.x.y`, causing valid macOS targets to fall through to the
unsupported case. Update the pattern for the macOS branch to use
`*-apple-darwin*` to match triples with optional Darwin version suffixes
following the word "darwin".
| if(FFmpeg_${component}_LIBRARY AND FFmpeg_${component}_INCLUDE_DIR) | ||
| set(FFmpeg_${component}_FOUND TRUE) | ||
| set(FFmpeg_${component}_LIBRARIES ${${_library_var}}) | ||
| set(FFmpeg_${component}_INCLUDE_DIRS ${FFmpeg_${component}_INCLUDE_DIR}) |
There was a problem hiding this comment.
Undefined _library_var empties component library outputs and breaks variable-based consumers.
FFmpeg_${component}_LIBRARIES is assigned from an undefined _library_var, so it resolves empty. Downstream, FFmpeg_LIBRARIES is built from FFmpeg_${component}_LIBRARY (DLL path on Windows), which is not the right link artifact for variable-based linking.
Suggested fix
@@
if(FFmpeg_${component}_LIBRARY AND FFmpeg_${component}_INCLUDE_DIR)
set(FFmpeg_${component}_FOUND TRUE)
- set(FFmpeg_${component}_LIBRARIES ${${_library_var}})
+ if(CMAKE_HOST_SYSTEM_NAME MATCHES "Windows" AND FFmpeg_${component}_IMPLIB)
+ set(FFmpeg_${component}_LIBRARIES "${FFmpeg_${component}_IMPLIB}")
+ else()
+ set(FFmpeg_${component}_LIBRARIES "${FFmpeg_${component}_LIBRARY}")
+ endif()
set(FFmpeg_${component}_INCLUDE_DIRS ${FFmpeg_${component}_INCLUDE_DIR})
@@
if(FFmpeg_${component}_FOUND)
- list(APPEND FFmpeg_LIBRARIES ${FFmpeg_${component}_LIBRARY})
+ list(APPEND FFmpeg_LIBRARIES ${FFmpeg_${component}_LIBRARIES})
list(APPEND FFmpeg_DEFINITIONS ${FFmpeg_${component}_DEFINITIONS})
list(APPEND FFmpeg_INCLUDE_DIRS ${FFmpeg_${component}_INCLUDE_DIR})
endif()Also applies to: 282-284
🤖 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 `@cmake/common/FindFFmpeg.cmake` around lines 166 - 169, The undefined variable
`_library_var` in the assignment to `FFmpeg_${component}_LIBRARIES` causes it to
resolve to empty, breaking downstream consumers that depend on proper library
linking information. Replace the reference to the undefined `_library_var` with
the correct library variable `FFmpeg_${component}_LIBRARY` in the set statement
at line 166 where `FFmpeg_${component}_LIBRARIES` is assigned. Apply the same
fix to the duplicate issue mentioned at lines 282-284 where the same undefined
variable pattern occurs.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two pre-existing CI gaps that blocked the plugin build:
- Ubuntu: the .Aptfile installed no ffmpeg dev libraries, so the Linux
pkg_check_modules(FFMPEG ...) failed at configure. Add libav{codec,util}-dev
and libsw{scale,resample}-dev.
- macOS: build-macos forced a universal (arm64 + x86_64) build, but the plugin
links a published libmoq release, which ships per-architecture (no universal
archive). The x86_64 slice couldn't link the arm64 libmoq. Build arm64 only.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Local nix gersemi (0.23.2) formats differently than CI's pinned 0.21.0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Makes the plugin link the ffmpeg that the obs-deps bundle ships (the same one OBS uses at runtime) on macOS and Windows, so release binaries are portable, and adds a
build.shfor release/CI.Why
On macOS the build used pkg-config, which reached for Homebrew ffmpeg at an absolute
/opt/homebrewpath (ffmpeg 8 /libavcodec.62) — the artifact would not load without that exact brew install. On Windows, the obs-deps ship ffmpeg with no pkg-config.pcfiles, sopkg_check_modulescould not find it at all. obs-deps actually bundles ffmpeg (libavcodec.61= ffmpeg 7, what OBS ships).Changes
CMakeLists.txt: usefind_package(FFmpeg)onWIN32 OR APPLEvia a vendoredFindFFmpeg.cmakethat searches the obs-deps prefix directly. Linux keeps pkg-config (system ffmpeg).cmake/common/FindFFmpeg.cmake: vendored OBS-style finder, no pkg-config dependency.cmake/common/bootstrap.cmake: honor-DPLUGIN_VERSION_OVERRIDEso release CI can version the plugin to match the libmoq release it links.cmake/macos/buildspec.cmake: make thexattrquarantine strip non-fatal (it errors on build outputs with no quarantine attribute).build.sh: per-platform configure/build/package.--libmoq-release <ver>fetches a published libmoq release (no local cargo build) and versions the plugin to match; otherwise the buildspecMOQ_VERSIONis used. macOS ships the.plugin(zip); Linux/Windows ship the portable layout (obs-moq/bin/64bit/<lib>+ data), archived withcmake -E tar.Context
This repo is canonical for the OBS plugin (an attempt to vendor it into the moq monorepo, moq-dev/moq#1787, was reverted to keep that repo permissively licensed). These fixes were developed and verified there and are ported here.
Test plan
./build.sh --target aarch64-apple-darwinbuilds against obs-deps and links@rpath/libavcodec.dylib(ffmpeg 7), not/opt/homebrew. Producesobs-moq-<version>-aarch64-apple-darwin.zip.find_package(FFmpeg)path mirrors the verified macOS path; needs a Windows build to confirm.Follow-up (not in this PR)
A release workflow that builds against each published libmoq release and a cross-repo trigger from moq-dev/moq (
repository_dispatch, needs a PAT) — so the plugin ships with every libmoq release.(Written by Claude)