Cmake: Fixes Windows cmake install path config#45
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- If the goal is to normalize the Windows install prefix to forward slashes, consider using
cmake_path(CONVERT $ENV{ALLUSERSPROFILE} TO_CMAKE_PATH ALLUSERSPROFILE_PATH)rather thancmake_path(SET ...), which only assigns the value without conversion. - The Windows build instructions hard-code specific tool versions and generator names (e.g.,
visualstudio2026buildtools,Visual Studio 18 2026); consider clarifying the actual supported Visual Studio version or making the instructions version-agnostic to avoid confusion as these names change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- If the goal is to normalize the Windows install prefix to forward slashes, consider using `cmake_path(CONVERT $ENV{ALLUSERSPROFILE} TO_CMAKE_PATH ALLUSERSPROFILE_PATH)` rather than `cmake_path(SET ...)`, which only assigns the value without conversion.
- The Windows build instructions hard-code specific tool versions and generator names (e.g., `visualstudio2026buildtools`, `Visual Studio 18 2026`); consider clarifying the actual supported Visual Studio version or making the instructions version-agnostic to avoid confusion as these names change.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. WalkthroughAdds 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2
🧹 Nitpick comments (2)
WINDOWS.md (2)
1-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMissing prerequisite: PowerShell execution policy and administrator elevation.
Consider adding a note that some commands (Chocolatey install, system-wide PATH modification) may require running PowerShell as Administrator. Also, the
Set-ExecutionPolicy Bypasson line 6 is scoped toProcess, which is good practice, but users may need to permanently allow script execution for subsequent build steps.🤖 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 `@WINDOWS.md` around lines 1 - 38, The Windows build guide is missing prerequisite notes for elevated PowerShell access and script execution permissions. Update the build setup section around the Chocolatey install and related setup steps to explicitly mention when PowerShell must be run as Administrator, and clarify that the current Set-ExecutionPolicy command is process-scoped while noting any cases where users may need broader script execution permissions for subsequent setup commands.
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAutomate nightly toolchain selection instead of manual instruction.
Replace the manual "choose the nightly variant" step with an explicit command to ensure reproducibility:
rustup default nightlyOr if multiple toolchains are needed:
rustup toolchain install nightly rustup default nightly🤖 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 `@WINDOWS.md` at line 13, The Windows setup docs still tell users to manually choose the nightly Rust toolchain during rustup installation; replace that instruction with an explicit command sequence in WINDOWS.md so the setup is reproducible. Update the relevant toolchain setup step to use rustup default nightly, or rustup toolchain install nightly followed by rustup default nightly if you want to preserve multiple toolchains, and remove the manual selection wording.
🤖 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 `@WINDOWS.md`:
- Line 9: The Windows setup instructions are out of sync with the repo’s actual
toolchain and reference the wrong Visual Studio version. Update the guidance in
WINDOWS.md to use the 2022 build tools/package names and workload names that
match the existing Windows preset and any related references to the Visual
Studio toolchain, so the documented install path aligns with the repo
configuration.
- Line 21: The build command in WINDOWS.md is redundant and inconsistent because
it hardcodes a CMake generator while the windows-x64 preset already defines the
generator in CMakePresets.json. Remove the explicit -G flag from the documented
cmake invocation and keep relying on the windows-x64 preset’s generator settings
so the command matches the preset configuration.
---
Nitpick comments:
In `@WINDOWS.md`:
- Around line 1-38: The Windows build guide is missing prerequisite notes for
elevated PowerShell access and script execution permissions. Update the build
setup section around the Chocolatey install and related setup steps to
explicitly mention when PowerShell must be run as Administrator, and clarify
that the current Set-ExecutionPolicy command is process-scoped while noting any
cases where users may need broader script execution permissions for subsequent
setup commands.
- Line 13: The Windows setup docs still tell users to manually choose the
nightly Rust toolchain during rustup installation; replace that instruction with
an explicit command sequence in WINDOWS.md so the setup is reproducible. Update
the relevant toolchain setup step to use rustup default nightly, or rustup
toolchain install nightly followed by rustup default nightly if you want to
preserve multiple toolchains, and remove the manual selection wording.
🪄 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: fd801b15-2073-409a-9cac-22361ba672c5
📒 Files selected for processing (2)
WINDOWS.mdcmake/windows/defaults.cmake
| Set-ExecutionPolicy Bypass -Scope Process -Force; [System.Net.ServicePointManager]::SecurityProtocol = [System.Net.ServicePointManager]::SecurityProtocol -bor 3072; iex ((New-Object System.Net.WebClient).DownloadString('https://community.chocolatey.org/install.ps1')) | ||
| choco install -y git | ||
| choco install -y cmake --installargs 'ADD_CMAKE_TO_PATH=System' | ||
| choco install visualstudio2026buildtools --package-parameters "--add Microsoft.VisualStudio.Workload.VCTools --includeRecommended" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify if visualstudio2026buildtools package exists on Chocolatey
curl -s "https://community.chocolatey.org/packages/visualstudio2026buildtools" -o /dev/null -w "%{http_code}"Repository: moq-dev/obs
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the Windows docs and CMake preset/generator definitions
git ls-files | rg '^(WINDOWS\.md|CMakePresets\.json|CMakeLists\.txt|.*preset.*\.json)$'
# Read the relevant files with line numbers
for f in WINDOWS.md CMakePresets.json CMakeLists.txt; do
if [ -f "$f" ]; then
echo "===== $f ====="
cat -n "$f" | sed -n '1,220p'
fi
done
# Search for Visual Studio generator strings and buildtool package references
rg -n 'Visual Studio 18 2026|visualstudio2026buildtools|Visual Studio 17 2022|CMAKE_GENERATOR|generator' .Repository: moq-dev/obs
Length of output: 15330
Use the 2022 Visual Studio toolchain names here WINDOWS.md is out of sync with the repo’s Windows preset (Visual Studio 17 2022), so visualstudio2026buildtools / Visual Studio 18 2026 will fail for anyone following this guide.
🤖 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 `@WINDOWS.md` at line 9, The Windows setup instructions are out of sync with
the repo’s actual toolchain and reference the wrong Visual Studio version.
Update the guidance in WINDOWS.md to use the 2022 build tools/package names and
workload names that match the existing Windows preset and any related references
to the Visual Studio toolchain, so the documented install path aligns with the
repo configuration.
|
|
||
| ```powershell | ||
| cd obs-studio | ||
| cmake -G "Visual Studio 18 2026" -A x64 --preset windows-x64 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if preset defines generator in CMakePresets.json
fd -i -g "*presets*" -e json | head -5Repository: moq-dev/obs
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CMakePresets.json outline =="
ast-grep outline CMakePresets.json --view expanded || true
echo
echo "== Locate windows-x64 preset and generator fields =="
python3 - <<'PY'
import json
from pathlib import Path
path = Path("CMakePresets.json")
data = json.loads(path.read_text())
for section in ("configurePresets", "buildPresets", "testPresets", "packagePresets", "workflowPresets"):
if section in data:
print(f"[{section}]")
for item in data[section]:
if item.get("name") == "windows-x64":
print(json.dumps(item, indent=2))
PYRepository: moq-dev/obs
Length of output: 993
🏁 Script executed:
python3 - <<'PY'
import json
from pathlib import Path
data = json.loads(Path("CMakePresets.json").read_text())
for section in ("configurePresets", "buildPresets", "testPresets", "packagePresets", "workflowPresets"):
for item in data.get(section, []):
if item.get("name") == "windows-x64":
print(section)
print(json.dumps(item, indent=2))
PYRepository: moq-dev/obs
Length of output: 871
Remove the redundant -G flag. windows-x64 already sets generator: "Visual Studio 17 2022" in CMakePresets.json, so this line is inconsistent with the preset and should rely on the preset generator alone.
🤖 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 `@WINDOWS.md` at line 21, The build command in WINDOWS.md is redundant and
inconsistent because it hardcodes a CMake generator while the windows-x64 preset
already defines the generator in CMakePresets.json. Remove the explicit -G flag
from the documented cmake invocation and keep relying on the windows-x64
preset’s generator settings so the command matches the preset configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@README.md`:
- Around line 33-56: The build instructions mix preset-based CMake invocation
with explicit generator/architecture flags, so update the OBS build steps to use
the preset consistently via the relevant preset name and remove the redundant
-G/-A options from the CMake command. Check the build section in README.md and
align the Windows command with the preset-defined generator/architecture in
CMakePresets.json, then verify the macOS build step uses the correct binary
directory for the macOS preset instead of assuming build_macos.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| 2. Build the OBS fork: | ||
|
|
||
| ```bash | ||
| cd obs-studio | ||
|
|
||
| # Replace with your platform | ||
| # Configure for Windows | ||
| cmake -G "Visual Studio 18 2026" -A x64 --preset windows-x64 | ||
| # Configure for macos | ||
| cmake --preset macos | ||
| cmake --build --preset macos | ||
|
|
||
| # Build for Windows | ||
| cmake --build --preset windows-x64 | ||
|
|
||
| # Build for macOS | ||
| cd build_macos | ||
| xcodebuild \ | ||
| -configuration RelWithDebInfo \ | ||
| -scheme obs-studio \ | ||
| -parallelizeTargets \ | ||
| -destination "generic/platform=macOS,name=Any Mac" | ||
| ``` | ||
|
|
||
| 3. Configure the plugin: | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix Windows CMake command: remove conflicting -G/-A flags when using --preset.
Line 39 combines -G "Visual Studio 18 2026" -A x64 with --preset windows-x64. When --preset is used, CMake ignores explicit -G and -A flags because the preset already defines generator and architecture (per your CMakePresets.json: "generator": "Visual Studio 17 2022", "architecture": "x64,version=10.0.22621"). The generator name "Visual Studio 18 2026" also doesn't match the preset or your Supported Build Environments table.
Additionally, the macOS build command cd build_macos doesn't match the windows-x64 preset's binaryDir of build_x64. Verify the macOS preset's binaryDir is build_macos.
🔧 Proposed fix
# Configure for Windows
- cmake -G "Visual Studio 18 2026" -A x64 --preset windows-x64
+ cmake --preset windows-x64
# Configure for macos
cmake --preset macos
# Build for Windows
cmake --build --preset windows-x64
# Build for macOS
- cd build_macos
+ cd build_macos # verify this matches your macOS preset binaryDir
xcodebuild \
-configuration RelWithDebInfo \
-scheme obs-studio \📝 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.
| 2. Build the OBS fork: | |
| ```bash | |
| cd obs-studio | |
| # Replace with your platform | |
| # Configure for Windows | |
| cmake -G "Visual Studio 18 2026" -A x64 --preset windows-x64 | |
| # Configure for macos | |
| cmake --preset macos | |
| cmake --build --preset macos | |
| # Build for Windows | |
| cmake --build --preset windows-x64 | |
| # Build for macOS | |
| cd build_macos | |
| xcodebuild \ | |
| -configuration RelWithDebInfo \ | |
| -scheme obs-studio \ | |
| -parallelizeTargets \ | |
| -destination "generic/platform=macOS,name=Any Mac" | |
| ``` | |
| 3. Configure the plugin: | |
| 2. Build the OBS fork: |
🤖 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 `@README.md` around lines 33 - 56, The build instructions mix preset-based
CMake invocation with explicit generator/architecture flags, so update the OBS
build steps to use the preset consistently via the relevant preset name and
remove the redundant -G/-A options from the CMake command. Check the build
section in README.md and align the Windows command with the preset-defined
generator/architecture in CMakePresets.json, then verify the macOS build step
uses the correct binary directory for the macOS preset instead of assuming
build_macos.
|
I've updated the build readme from the other PR. the current branch of obs-studio to build now requires Xcode 26.5 it was decided on eventually. So the docs need to be updated for that also. Still checking mac building what it needs. |
|
The documented obs-studio fork doesnt compile in Windows and broken. The plugin won't load with the merged obs upstream. It's missing a dependency I think and won't load but no error produced other claiming it's not found. I've added back in the windows console debugger to see the rust logs. https://github.com/danrossi/obs-studio/tree/moq-merged |
1 participant
The Windows cmake install config needs the path reformatted to forward slashes to compile and install correctly.
Adds Windows specific build docs