Conversation
Add versioned R2 bucket uploads (obs.moq.dev) to macOS release builds
and create a Windows x64 build script using CMake + CPack. Builds are
uploaded to moqbs/{platform}/{arch}/{version}/ when R2_BUCKET is set.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe PR introduces a comprehensive release infrastructure for cross-platform builds. New files include release automation scripts for macOS (with signing and notarization), Windows, and Linux (via Docker). Configuration files include environment variables for code signing credentials, Docker and Wrangler configurations, and a justfile for coordinating build tasks. Supporting utilities for keychain management are added. The .gitignore file is updated to handle Wrangler cache and other artifacts. These additions enable automated, signed releases across macOS, Windows, and Linux, with optional cloud artifact uploads. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (2)
bin/release-mac (2)
49-51: Quote CMake variable expansions to prevent word splitting.While the current constrained values (
arm64/x86_64,Release/RelWithDebInfo/Debug) won't cause issues, unquoted expansions in aset -euo pipefailscript are fragile.Proposed fix
cmake -S . --preset macos \ - -DCMAKE_OSX_ARCHITECTURES=$ARCH \ - -DCMAKE_BUILD_TYPE=$CONFIG + -DCMAKE_OSX_ARCHITECTURES="$ARCH" \ + -DCMAKE_BUILD_TYPE="$CONFIG"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/release-mac` around lines 49 - 51, The cmake invocation uses unquoted shell variable expansions (ARCH and CONFIG) which can cause word-splitting; update the cmake command that sets -DCMAKE_OSX_ARCHITECTURES and -DCMAKE_BUILD_TYPE to use quoted expansions (e.g. -DCMAKE_OSX_ARCHITECTURES="$ARCH" and -DCMAKE_BUILD_TYPE="$CONFIG") so the cmake line that invokes cmake --preset macos safely handles values.
104-125: Notarization depends on env vars that are commented out by default.
APPLE_IDandAPPLE_PASSWORDare commented out in.env(lines 9-10). If someone runsjust release-notarized,set -uwill abort with an unhelpful "unbound variable" error. Consider adding an explicit check with a descriptive message before the notarization block.Proposed improvement
if [ "$NOTARIZE" = "true" ]; then + if [ -z "${APPLE_ID:-}" ] || [ -z "${APPLE_PASSWORD:-}" ]; then + echo "Error: APPLE_ID and APPLE_PASSWORD must be set for notarization" + echo "Uncomment them in .env and set the correct 1Password references" + exit 1 + fi + echo "" echo "Submitting for notarization..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/release-mac` around lines 104 - 125, The notarization block can fail with "unbound variable" when set -u is enabled because APPLE_ID/APPLE_PASSWORD (and possibly APPLE_TEAM_ID) are optional and commented out; add an explicit pre-check before the if [ "$NOTARIZE" = "true" ] block that verifies NOTARIZE is set to "true" only when APPLE_ID, APPLE_PASSWORD and APPLE_TEAM_ID are non-empty, and if any are missing print a clear descriptive error referencing APPLE_ID, APPLE_PASSWORD, APPLE_TEAM_ID and xcrun notarytool submit (or the notarization step) and exit 1; alternatively use a safe parameter expansion test (e.g. ${VAR:+}) to avoid unbound variable errors when checking NOTARIZE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/keychain.sh`:
- Around line 45-48: The script writes an error about a missing Developer ID
Application identity to stdout (the branch checking the variable "identity"),
which gets swallowed by the caller capturing stdout (setup_signing_keychain).
Change the echo in that branch to write to stderr instead of stdout so callers
still receive the exit code and the error is visible (i.e., redirect the message
from the echo that outputs "Error: Could not find Developer ID Application
identity" to stderr while keeping the return 1).
- Line 34: The command substitution in the security invocation is unquoted and
will word-split keychain paths containing spaces; update the code around the
security list-keychain -s invocation to build an array of arguments instead of
inlining unquoted substitution: capture the output of `security list-keychains
-d user` into a shell array (splitting on newlines), prepend the target
`$keychain_path`, and call `security list-keychain -d user -s` with the array
expanded as quoted parameters so each path (including those with spaces) is
passed intact; ensure all uses of the variable and the array elements are
properly quoted when expanded.
In `@bin/release-mac`:
- Around line 38-39: The command substitution assigning CODESIGN_IDENT captures
stdout from setup_signing_keychain and therefore swallows any error messages;
update the setup_signing_keychain implementation in keychain.sh so it writes
error messages to stderr (e.g., use echo ... >&2 or printf ... >&2) instead of
stdout and only emit the valid identity on stdout on success, ensuring the
function still returns a non-zero status on failure so the caller
(bin/release-mac using CODESIGN_IDENT=$(setup_signing_keychain "$TEMP_DIR"))
will both fail and display the error to the user.
---
Nitpick comments:
In `@bin/release-mac`:
- Around line 49-51: The cmake invocation uses unquoted shell variable
expansions (ARCH and CONFIG) which can cause word-splitting; update the cmake
command that sets -DCMAKE_OSX_ARCHITECTURES and -DCMAKE_BUILD_TYPE to use quoted
expansions (e.g. -DCMAKE_OSX_ARCHITECTURES="$ARCH" and
-DCMAKE_BUILD_TYPE="$CONFIG") so the cmake line that invokes cmake --preset
macos safely handles values.
- Around line 104-125: The notarization block can fail with "unbound variable"
when set -u is enabled because APPLE_ID/APPLE_PASSWORD (and possibly
APPLE_TEAM_ID) are optional and commented out; add an explicit pre-check before
the if [ "$NOTARIZE" = "true" ] block that verifies NOTARIZE is set to "true"
only when APPLE_ID, APPLE_PASSWORD and APPLE_TEAM_ID are non-empty, and if any
are missing print a clear descriptive error referencing APPLE_ID,
APPLE_PASSWORD, APPLE_TEAM_ID and xcrun notarytool submit (or the notarization
step) and exit 1; alternatively use a safe parameter expansion test (e.g.
${VAR:+}) to avoid unbound variable errors when checking NOTARIZE.
| "$keychain_path" &> /dev/null | ||
|
|
||
| # Add to search list | ||
| security list-keychain -d user -s "$keychain_path" $(security list-keychains -d user | tr -d '"') |
There was a problem hiding this comment.
Unquoted command substitution can break with spaces in keychain paths.
If any existing keychain path contains spaces, the unquoted $(security list-keychains ...) will word-split and corrupt the keychain search list. ShellCheck SC2046 flags this correctly.
Proposed fix
- security list-keychain -d user -s "$keychain_path" $(security list-keychains -d user | tr -d '"')
+ local existing_keychains
+ existing_keychains=$(security list-keychains -d user | tr -d '"' | tr '\n' ' ')
+ eval security list-keychain -d user -s "\"$keychain_path\"" $existing_keychainsActually, this is tricky to fix cleanly due to how security list-keychain -s expects its arguments. A safer approach using an array:
Safer fix using array
- security list-keychain -d user -s "$keychain_path" $(security list-keychains -d user | tr -d '"')
+ local -a existing_keychains
+ while IFS= read -r line; do
+ line=$(echo "$line" | xargs) # trim whitespace
+ [ -n "$line" ] && existing_keychains+=("$line")
+ done < <(security list-keychains -d user | tr -d '"')
+ security list-keychain -d user -s "$keychain_path" "${existing_keychains[@]}"📝 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.
Suggested change
| security list-keychain -d user -s "$keychain_path" $(security list-keychains -d user | tr -d '"') | |
| local -a existing_keychains | |
| while IFS= read -r line; do | |
| line=$(echo "$line" | xargs) # trim whitespace | |
| [ -n "$line" ] && existing_keychains+=("$line") | |
| done < <(security list-keychains -d user | tr -d '"') | |
| security list-keychain -d user -s "$keychain_path" "${existing_keychains[@]}" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 34-34: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/lib/keychain.sh` at line 34, The command substitution in the security
invocation is unquoted and will word-split keychain paths containing spaces;
update the code around the security list-keychain -s invocation to build an
array of arguments instead of inlining unquoted substitution: capture the output
of `security list-keychains -d user` into a shell array (splitting on newlines),
prepend the target `$keychain_path`, and call `security list-keychain -d user
-s` with the array expanded as quoted parameters so each path (including those
with spaces) is passed intact; ensure all uses of the variable and the array
elements are properly quoted when expanded.
Comment on lines
+38
to
+39
| CODESIGN_IDENT=$(setup_signing_keychain "$TEMP_DIR") | ||
| echo "Using identity: $CODESIGN_IDENT" |
There was a problem hiding this comment.
Error messages from setup_signing_keychain are swallowed by command substitution.
Since CODESIGN_IDENT captures stdout, the error message on line 46 of keychain.sh (echo "Error: Could not find Developer ID Application identity") will be silently captured into the variable instead of being displayed to the user. The script will then exit (due to set -e + return 1) with no visible explanation.
The fix is in keychain.sh — redirect the error message to stderr:
Proposed fix in bin/lib/keychain.sh
if [ -z "$identity" ]; then
- echo "Error: Could not find Developer ID Application identity"
+ echo "Error: Could not find Developer ID Application identity" >&2
return 1
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/release-mac` around lines 38 - 39, The command substitution assigning
CODESIGN_IDENT captures stdout from setup_signing_keychain and therefore
swallows any error messages; update the setup_signing_keychain implementation in
keychain.sh so it writes error messages to stderr (e.g., use echo ... >&2 or
printf ... >&2) instead of stdout and only emit the valid identity on stdout on
success, ensuring the function still returns a non-zero status on failure so the
caller (bin/release-mac using CODESIGN_IDENT=$(setup_signing_keychain
"$TEMP_DIR")) will both fail and display the error to the user.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Dockerfile (3)
28-29: Docker layer cache is invalidated on every source change for the CEF download.
COPY . .before the CEF download step means any source file change busts the cache for the expensive CEF fetch. Copy onlyCMakePresets.jsonfirst, download CEF, then copy the rest.♻️ Proposed fix for layer caching
WORKDIR /build -COPY . . +COPY CMakePresets.json . # Download CEF (parse version from CMakePresets.json) RUN CEF_VERSION=$(jq -r '...' CMakePresets.json) && \ ... + +COPY . . # Configure + Build + Package🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 28 - 29, The Dockerfile currently does COPY . . right after WORKDIR /build which invalidates the layer cache for the expensive CEF download; change the layer order so you first COPY only CMakePresets.json (e.g., "COPY CMakePresets.json ."), then perform the CEF download step (the RUN that fetches/extracts CEF), and only after that COPY the rest of the source (restore the broader "COPY . ." later) so source edits won't bust the cached CEF download layer.
32-40: CEF version is parsed twice fromCMakePresets.json.
CEF_VERSIONis extracted with the samejqexpression in both the download and configureRUNsteps. If you consolidate them into a singleRUNlayer (or use anARG/ENV), you eliminate the duplication and reduce fragility.♻️ Example: single RUN layer or ARG approach
+# Parse CEF details once +RUN CEF_VERSION=$(jq -r '.configurePresets[] | select(.name=="dependencies") | .vendor["obsproject.com/obs-studio"].dependencies.cef.version' CMakePresets.json) && \ + echo "$CEF_VERSION" > /tmp/cef_version + # Download CEF -RUN CEF_VERSION=$(jq -r '...' CMakePresets.json) && \ +RUN CEF_VERSION=$(cat /tmp/cef_version) && \ ... # Configure + Build + Package -RUN CEF_VERSION=$(jq -r '...' CMakePresets.json) && \ +RUN CEF_VERSION=$(cat /tmp/cef_version) && \ ...Alternatively, merge both
RUNsteps into one to keep it simple.Also applies to: 42-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 32 - 40, The Dockerfile duplicates the jq extraction of CEF_VERSION/CEF_REVISION/CEF_HASH across RUN layers; fix by parsing those values once and reusing them—either (A) combine the two RUN steps into a single RUN so the variables CEF_VERSION, CEF_REVISION, CEF_HASH and computed FILENAME are defined once and then used for download, sha256 check and extraction, or (B) extract them into Docker build-time variables using ARG/ENV (define ARG CEF_VERSION/CEF_REVISION/CEF_HASH, set them from jq in an initial RUN and export to ENV) and then reference those symbols (CEF_VERSION, CEF_REVISION, CEF_HASH, FILENAME) in subsequent RUNs to remove duplication and reduce fragility.
48-49: No final packaging stage — image will be very large.The image retains all build dependencies (~GBs). Since it's only used for
docker cpartifact extraction, this works but a finalFROM scratchor minimal stage copying just the.deboutput would shrink the image and speed updocker create.♻️ Example: minimal output stage
cd build_ubuntu && cpack -C Release + +FROM scratch AS output +COPY --from=build /build/build_ubuntu/*.deb /Then use
docker build --target output --output type=local,dest=./build_linux .to extract artifacts directly, eliminating thedocker create/docker cp/docker rmdance inrelease-linux.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 48 - 49, The Dockerfile currently builds artifacts with "cmake --build build_ubuntu --config Release --parallel" and runs "cpack -C Release" but leaves all build dependencies in the final image, making it very large; add a final lightweight output stage (e.g., "FROM scratch" or a minimal base) named "output" that copies only the produced .deb/.tar artifacts from the build_ubuntu stage into the final image (using COPY --from=build_ubuntu <path-to-artifacts> /output), and update CI/build instructions to use "docker build --target output --output type=local,dest=./build_linux ." (or build the output stage so docker create/docker cp are unnecessary) so the image contains only the packaging artifacts and not build toolchains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/release-linux`:
- Around line 19-32: The created container (CONTAINER_ID) can leak if any
command after docker create fails; add a trap that always removes the container
(docker rm "$CONTAINER_ID") on EXIT to guarantee cleanup, and simplify the .deb
discovery by querying the actual created container instead of spawning a
separate docker run --rm (e.g., inspect or export/tar the created container to
locate the .deb path) so you use the same container for docker cp; update the
logic around CONTAINER_ID, the .deb path lookup, and the docker cp usage to
reference the single created container and ensure removal via trap.
---
Nitpick comments:
In `@Dockerfile`:
- Around line 28-29: The Dockerfile currently does COPY . . right after WORKDIR
/build which invalidates the layer cache for the expensive CEF download; change
the layer order so you first COPY only CMakePresets.json (e.g., "COPY
CMakePresets.json ."), then perform the CEF download step (the RUN that
fetches/extracts CEF), and only after that COPY the rest of the source (restore
the broader "COPY . ." later) so source edits won't bust the cached CEF download
layer.
- Around line 32-40: The Dockerfile duplicates the jq extraction of
CEF_VERSION/CEF_REVISION/CEF_HASH across RUN layers; fix by parsing those values
once and reusing them—either (A) combine the two RUN steps into a single RUN so
the variables CEF_VERSION, CEF_REVISION, CEF_HASH and computed FILENAME are
defined once and then used for download, sha256 check and extraction, or (B)
extract them into Docker build-time variables using ARG/ENV (define ARG
CEF_VERSION/CEF_REVISION/CEF_HASH, set them from jq in an initial RUN and export
to ENV) and then reference those symbols (CEF_VERSION, CEF_REVISION, CEF_HASH,
FILENAME) in subsequent RUNs to remove duplication and reduce fragility.
- Around line 48-49: The Dockerfile currently builds artifacts with "cmake
--build build_ubuntu --config Release --parallel" and runs "cpack -C Release"
but leaves all build dependencies in the final image, making it very large; add
a final lightweight output stage (e.g., "FROM scratch" or a minimal base) named
"output" that copies only the produced .deb/.tar artifacts from the build_ubuntu
stage into the final image (using COPY --from=build_ubuntu <path-to-artifacts>
/output), and update CI/build instructions to use "docker build --target output
--output type=local,dest=./build_linux ." (or build the output stage so docker
create/docker cp are unnecessary) so the image contains only the packaging
artifacts and not build toolchains.
Comment on lines
+19
to
+32
| CONTAINER_ID=$(docker create --platform linux/amd64 moqbs-linux) | ||
| mkdir -p "$PROJECT_ROOT/build_linux" | ||
|
|
||
| # Find the .deb filename inside the container (exclude debug .ddeb) | ||
| DEB_PATH=$(docker run --rm --platform linux/amd64 moqbs-linux sh -c 'ls /build/build_ubuntu/*.deb 2>/dev/null | grep -v dbgsym | head -1') | ||
| if [ -z "$DEB_PATH" ]; then | ||
| docker rm "$CONTAINER_ID" | ||
| echo "Error: No .deb found in build output" | ||
| exit 1 | ||
| fi | ||
|
|
||
| OUTPUT_NAME="moqbs-linux-x86_64.deb" | ||
| docker cp "$CONTAINER_ID:$DEB_PATH" "$PROJECT_ROOT/build_linux/$OUTPUT_NAME" | ||
| docker rm "$CONTAINER_ID" |
There was a problem hiding this comment.
Container leak on failure — CONTAINER_ID is not cleaned up if commands between docker create and docker rm fail.
With set -e, if docker cp on line 31 fails, the script exits immediately and the container created on line 19 is never removed. Use a trap to ensure cleanup.
Also, line 23 uses docker run --rm (a separate ephemeral container) to discover the .deb path, then line 31 uses docker cp against the docker create container. This works because both derive from the same image, but it's unnecessary indirection — you could discover and copy in a single step.
🛡️ Proposed fix: add a trap for container cleanup and simplify extraction
CONTAINER_ID=$(docker create --platform linux/amd64 moqbs-linux)
+trap "docker rm -f $CONTAINER_ID 2>/dev/null" EXIT
mkdir -p "$PROJECT_ROOT/build_linux"
-# Find the .deb filename inside the container (exclude debug .ddeb)
-DEB_PATH=$(docker run --rm --platform linux/amd64 moqbs-linux sh -c 'ls /build/build_ubuntu/*.deb 2>/dev/null | grep -v dbgsym | head -1')
+# Find the .deb filename inside the container's filesystem (exclude debug .ddeb)
+DEB_PATH=$(docker export "$CONTAINER_ID" | tar -t 2>/dev/null | grep '\.deb$' | grep -v dbgsym | head -1)
if [ -z "$DEB_PATH" ]; then
- docker rm "$CONTAINER_ID"
echo "Error: No .deb found in build output"
exit 1
fi
OUTPUT_NAME="moqbs-linux-x86_64.deb"
-docker cp "$CONTAINER_ID:$DEB_PATH" "$PROJECT_ROOT/build_linux/$OUTPUT_NAME"
-docker rm "$CONTAINER_ID"
+docker cp "$CONTAINER_ID:/$DEB_PATH" "$PROJECT_ROOT/build_linux/$OUTPUT_NAME"
echo "Created: $PROJECT_ROOT/build_linux/$OUTPUT_NAME"The trap ensures the container is removed on any exit (success or failure). Using docker export | tar -t queries the actual container's filesystem instead of creating a second container.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/release-linux` around lines 19 - 32, The created container (CONTAINER_ID)
can leak if any command after docker create fails; add a trap that always
removes the container (docker rm "$CONTAINER_ID") on EXIT to guarantee cleanup,
and simplify the .deb discovery by querying the actual created container instead
of spawning a separate docker run --rm (e.g., inspect or export/tar the created
container to locate the .deb path) so you use the same container for docker cp;
update the logic around CONTAINER_ID, the .deb path lookup, and the docker cp
usage to reference the single created container and ensure removal via trap.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
release/release-mac (1)
49-51: Unquoted CMake variable expansions.While
$ARCHis validated and$CONFIGhas a safe default, quoting these prevents issues if the validation is ever relaxed or the script is refactored.Proposed fix
cmake -S . --preset macos \ - -DCMAKE_OSX_ARCHITECTURES=$ARCH \ - -DCMAKE_BUILD_TYPE=$CONFIG + -DCMAKE_OSX_ARCHITECTURES="$ARCH" \ + -DCMAKE_BUILD_TYPE="$CONFIG"Also on line 56:
-cmake --build "$BUILD_DIR" --config $CONFIG -j +cmake --build "$BUILD_DIR" --config "$CONFIG" -j🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@release/release-mac` around lines 49 - 51, The cmake invocation uses unquoted variable expansions (the command shown with "cmake -S . --preset macos -DCMAKE_OSX_ARCHITECTURES=$ARCH -DCMAKE_BUILD_TYPE=$CONFIG"); update this to quote the expansions so they become "-DCMAKE_OSX_ARCHITECTURES=\"$ARCH\"" and "-DCMAKE_BUILD_TYPE=\"$CONFIG\"" (and apply the same quoting to the other cmake call referenced on line 56) to prevent word-splitting if values contain spaces or are changed later.release/Dockerfile (1)
32-40: CEF version is extracted twice across separateRUNlayers.The same
jqextraction ofCEF_VERSIONfromCMakePresets.jsonis repeated on lines 32 and 43 because eachRUNis a new shell. Consider combining the download and build into a singleRUNto avoid the duplication and reduce the number of layers.Proposed consolidation
-RUN CEF_VERSION=$(jq -r '...' CMakePresets.json) && \ - ... download and extract ... - -# Configure + Build + Package -RUN CEF_VERSION=$(jq -r '...' CMakePresets.json) && \ - cmake -S . --preset ubuntu \ +RUN CEF_VERSION=$(jq -r '.configurePresets[] | select(.name=="dependencies") | .vendor["obsproject.com/obs-studio"].dependencies.cef.version' CMakePresets.json) && \ + CEF_REVISION=$(jq -r '.configurePresets[] | select(.name=="dependencies") | .vendor["obsproject.com/obs-studio"].dependencies.cef.revision["ubuntu-x86_64"] // empty' CMakePresets.json) && \ + CEF_HASH=$(jq -r '.configurePresets[] | select(.name=="dependencies") | .vendor["obsproject.com/obs-studio"].dependencies.cef.hashes["ubuntu-x86_64"]' CMakePresets.json) && \ + mkdir -p .deps && cd .deps && \ + FILENAME="cef_binary_${CEF_VERSION}_linux_x86_64${CEF_REVISION:+_v${CEF_REVISION}}.tar.xz" && \ + curl -fSLO "https://cdn-fastly.obsproject.com/downloads/${FILENAME}" && \ + echo "${CEF_HASH} ${FILENAME}" | sha256sum -c - && \ + mkdir -p "cef_binary_${CEF_VERSION}_linux_x86_64" && \ + tar --strip-components=1 -xJf "${FILENAME}" -C "cef_binary_${CEF_VERSION}_linux_x86_64" && \ + cd /build && \ + cmake -S . --preset ubuntu \ -DCMAKE_BUILD_TYPE=Release \ -DENABLE_BROWSER=ON \ - -DCEF_ROOT_DIR="/build/.deps/cef_binary_${CEF_VERSION}_linux_x86_64" && \ + -DCEF_ROOT_DIR="/build/.deps/cef_binary_${CEF_VERSION}_linux_x86_64" && \ cmake --build build_ubuntu --config Release --parallel && \ cd build_ubuntu && cpack -C ReleaseThat said, keeping them separate has a Docker caching benefit — the CEF download layer is cached if only source code changes. So the current split is a valid tradeoff. Up to you.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@release/Dockerfile` around lines 32 - 40, The CEF version extraction is duplicated because each RUN starts a new shell; fix by consolidating the jq extractions and subsequent download/extract steps into a single RUN so CEF_VERSION, CEF_REVISION and CEF_HASH (and FILENAME) are computed once and reused, or alternatively export them via ENV/ARG before the separate RUNs; update the Dockerfile to compute CEF_VERSION/CEF_REVISION/CEF_HASH once and then perform curl, sha256sum and tar using that single set of variables (referencing the CEF_VERSION, CEF_REVISION, CEF_HASH and FILENAME symbols).release/release-windows.ps1 (1)
9-9: Version extraction may produce unexpected results ifgit describeincludes extra hyphens.
git describe --tags --alwayscan produce output like32.0.4-moqbs-3-g1234567where-replace '-moqbs.*', ''correctly strips to32.0.4. However, if the tag itself doesn't contain-moqbs(e.g., during early development), the entire describe output (including commit-distance suffixes like-3-g1234567) passes through unmodified, producing a different version format than expected. The same pattern exists in the bash scripts (sed 's/-moqbs.*//').This is likely acceptable given the team's tagging convention, but worth being aware of.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@release/release-windows.ps1` at line 9, The current OBSVersion assignment using git describe and -replace '-moqbs.*' can leave commit-distance suffixes when tags lack the -moqbs marker; update the OBSVersion extraction to run git -C $ProjectRoot describe --tags --always and then apply a regex that captures only the leading semantic version (e.g., match digits.digits.digits) and replace the whole string with that capture so OBSVersion contains just the X.Y.Z part (change the assignment that defines OBSVersion to use this regex-based extraction).release/lib/keychain.sh (1)
11-11:localmasks the return value ofopenssl rand(SC2155).If
openssl randfails, the exit code is swallowed by thelocaldeclaration. Same pattern applies on lines 40 and 57. Low risk here, but good hygiene to declare and assign separately.Proposed fix (lines 11, 40, 57)
- local keychain_password="$(openssl rand -base64 32)" + local keychain_password + keychain_password="$(openssl rand -base64 32)"- local identity=$(security find-identity -v -p codesigning "$keychain_path" | \ + local identity + identity=$(security find-identity -v -p codesigning "$keychain_path" | \- local keychain_path=$(cat "$temp_dir/keychain_path") + local keychain_path + keychain_path=$(cat "$temp_dir/keychain_path")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@release/lib/keychain.sh` at line 11, The shell snippet uses "local var=$(openssl ...)" which masks openssl's exit status (SC2155); for each occurrence (e.g., the keychain_password assignment to keychain_password and the similar assignments later) generate the random value first into a temporary/unnamed variable (e.g., tmp_password="$(openssl rand -base64 32)") and check its exit code, then declare the variable local and assign it (local keychain_password="$tmp_password"); if openssl fails, handle the error (exit or return) before creating the local. Ensure you apply the same pattern to the other two similar assignments referenced in the diff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@release/release-mac`:
- Around line 105-125: The notarization block that runs when NOTARIZE="true"
references unset variables and will cause a cryptic "unbound variable" failure
due to set -u; add an explicit guard at the start of that block to verify
APPLE_ID and APPLE_PASSWORD (and optionally APPLE_TEAM_ID) are set and
non-empty, printing a clear error and exiting if not. Specifically, in the if [
"$NOTARIZE" = "true" ]; then ... fi block, test the ENV vars (APPLE_ID,
APPLE_PASSWORD[, APPLE_TEAM_ID]) and call echo with a helpful message then exit
1 rather than letting the shell expand an unset variable in xcrun notarytool
submit or store-credentials.
---
Duplicate comments:
In `@release/lib/keychain.sh`:
- Around line 45-48: The error message in the setup_signing_keychain function is
being written to stdout and thus swallowed by callers that capture stdout (e.g.,
CODESIGN_IDENT=$(setup_signing_keychain ...)); change the echo that prints
"Error: Could not find Developer ID Application identity" to write to stderr
instead (use shell redirection to stderr) and still return a non-zero status so
callers can detect the failure.
- Line 34: The command substitution $(security list-keychains -d user | tr -d
'"') is unquoted and will split keychain paths that contain spaces; change the
logic to iterate safely over each keychain line and quote variables. For
example, run security list-keychains -d user | tr -d '"' | while IFS= read -r
kc; do security list-keychain -d user -s "$keychain_path" "$kc"; done (ensure
"$keychain_path" and "$kc" are quoted) so paths with spaces are preserved when
calling security and to remove the SC2046 risk.
In `@release/release-linux`:
- Around line 19-32: The script leaks the created container if later commands
fail and unnecessarily spawns a second container to discover the .deb; update
the logic to reuse the created container (use docker exec or docker cp from
CONTAINER_ID instead of running a new container for DEB_PATH) and add a cleanup
trap that removes the container on EXIT or error (ensure CONTAINER_ID is checked
before calling docker rm). Specifically, stop using the `docker run ...
moqbs-linux` for DEB_PATH discovery, derive DEB_PATH by running a shell inside
the created container referenced by CONTAINER_ID (e.g., via docker exec or
docker cp with a path listing), and register a trap to `docker rm
"$CONTAINER_ID"` so the container is always removed even if `docker cp` or other
commands fail.
In `@release/release-mac`:
- Around line 38-39: The command substitution
CODESIGN_IDENT=$(setup_signing_keychain "$TEMP_DIR") swallows error output, so
modify the setup_signing_keychain function in keychain.sh to send any
diagnostic/error messages to stderr instead of stdout (use >&2 for error prints
and ensure only the final identity value is printed to stdout), and also
redirect stderr for any internal commands within setup_signing_keychain so
callers like the release script receive errors on stderr while CODESIGN_IDENT
captures only the intended identity string.
---
Nitpick comments:
In `@release/Dockerfile`:
- Around line 32-40: The CEF version extraction is duplicated because each RUN
starts a new shell; fix by consolidating the jq extractions and subsequent
download/extract steps into a single RUN so CEF_VERSION, CEF_REVISION and
CEF_HASH (and FILENAME) are computed once and reused, or alternatively export
them via ENV/ARG before the separate RUNs; update the Dockerfile to compute
CEF_VERSION/CEF_REVISION/CEF_HASH once and then perform curl, sha256sum and tar
using that single set of variables (referencing the CEF_VERSION, CEF_REVISION,
CEF_HASH and FILENAME symbols).
In `@release/lib/keychain.sh`:
- Line 11: The shell snippet uses "local var=$(openssl ...)" which masks
openssl's exit status (SC2155); for each occurrence (e.g., the keychain_password
assignment to keychain_password and the similar assignments later) generate the
random value first into a temporary/unnamed variable (e.g.,
tmp_password="$(openssl rand -base64 32)") and check its exit code, then declare
the variable local and assign it (local keychain_password="$tmp_password"); if
openssl fails, handle the error (exit or return) before creating the local.
Ensure you apply the same pattern to the other two similar assignments
referenced in the diff.
In `@release/release-mac`:
- Around line 49-51: The cmake invocation uses unquoted variable expansions (the
command shown with "cmake -S . --preset macos -DCMAKE_OSX_ARCHITECTURES=$ARCH
-DCMAKE_BUILD_TYPE=$CONFIG"); update this to quote the expansions so they become
"-DCMAKE_OSX_ARCHITECTURES=\"$ARCH\"" and "-DCMAKE_BUILD_TYPE=\"$CONFIG\"" (and
apply the same quoting to the other cmake call referenced on line 56) to prevent
word-splitting if values contain spaces or are changed later.
In `@release/release-windows.ps1`:
- Line 9: The current OBSVersion assignment using git describe and -replace
'-moqbs.*' can leave commit-distance suffixes when tags lack the -moqbs marker;
update the OBSVersion extraction to run git -C $ProjectRoot describe --tags
--always and then apply a regex that captures only the leading semantic version
(e.g., match digits.digits.digits) and replace the whole string with that
capture so OBSVersion contains just the X.Y.Z part (change the assignment that
defines OBSVersion to use this regex-based extraction).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bunx wranglerbin/release-windows.ps1) using CMake + CPackwrangler.tomlfor Cloudflare R2 configurationbuild-windows,release-windows, andclean-windowsjustfile targetsArtifacts are uploaded to
obs.moq.dev/{platform}/{arch}/{version}/whenR2_BUCKETis set (viaop run). Builds withoutop runskip uploads.Test plan
just releasebuilds ARM64 DMG and uploads to R2just release-intelbuilds x86_64 DMG and uploads to R2just release-windowson Windows machine builds ZIP and uploadshttps://obs.moq.dev/macos/{arch}/32.0.4-2ee93ce/🤖 Generated with Claude Code