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: 54 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. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughAdds 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
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
|
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. |
|
I've got a Windows PC so I'll test this a bit more. Note that you don't need the OBS fork any longer, because I added a MoQ dock. |
- Drop the redundant `-G "Visual Studio 18 2026" -A x64` from the docs: with `--preset`, the generator and architecture come from the preset (Visual Studio 17 2022), and a non-matching `-G` makes CMake error. - Align the documented Visual Studio version with the preset (2022, not 2026), including the chocolatey build-tools package. - Revert the macOS build doc to `cmake --build --preset macos` (matching the justfile) instead of a raw xcodebuild invocation. - Fix obs-moq.cpp formatting to satisfy clang-format (IndentPPDirectives: None / right pointer alignment): de-indent the preprocessor blocks, use `const char *`, and drop the stray blank line. - Include <cstdlib> for std::getenv rather than relying on a transitive include. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 `@src/obs-moq.cpp`:
- Around line 56-58: The console redirection in the obs-moq startup path should
only happen after confirming `AllocConsole()` succeeded and a valid console is
available. Update the `AllocConsole`/`freopen` sequence in `src/obs-moq.cpp` so
`stdout` and `stderr` are reopened to `CONOUT$` only when the console allocation
succeeds; otherwise skip the redirection and keep the existing process streams
intact. Use the existing startup block around `AllocConsole()` and the two
`freopen` calls as the place to add the guard.
🪄 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: e2277cdd-51ce-4028-98f7-34cf4de85b2d
📒 Files selected for processing (5)
CMakeLists.txtREADME.mdWINDOWS.mdsrc/moq-dock.cppsrc/obs-moq.cpp
✅ Files skipped from review due to trivial changes (1)
- WINDOWS.md
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
| AllocConsole(); | ||
| freopen("CONOUT$", "w", stdout); | ||
| freopen("CONOUT$", "w", stderr); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard the freopen calls behind a verified console handle.
Lines 56-58 rewrite process-wide stdout/stderr without checking whether AllocConsole() actually produced a usable console. If that open path fails, freopen can leave OBS's stdio streams unusable for the rest of the process on this debug path.
Suggested fix
`#ifdef` _WIN64
const char *logLevel = std::getenv("RUST_LOG");
if (logLevel && strcmp(logLevel, "debug") == 0) {
- AllocConsole();
- freopen("CONOUT$", "w", stdout);
- freopen("CONOUT$", "w", stderr);
+ if (AllocConsole() || GetLastError() == ERROR_ACCESS_DENIED) {
+ FILE *out = freopen("CONOUT$", "w", stdout);
+ FILE *err = freopen("CONOUT$", "w", stderr);
+ if (!out || !err) {
+ // Leave the module loaded; console logging just stays unavailable.
+ }
+ }
}
`#endif`📝 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.
| AllocConsole(); | |
| freopen("CONOUT$", "w", stdout); | |
| freopen("CONOUT$", "w", stderr); | |
| if (AllocConsole() || GetLastError() == ERROR_ACCESS_DENIED) { | |
| FILE *out = freopen("CONOUT$", "w", stdout); | |
| FILE *err = freopen("CONOUT$", "w", stderr); | |
| if (!out || !err) { | |
| // Leave the module loaded; console logging just stays unavailable. | |
| } | |
| } |
🤖 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 `@src/obs-moq.cpp` around lines 56 - 58, The console redirection in the obs-moq
startup path should only happen after confirming `AllocConsole()` succeeded and
a valid console is available. Update the `AllocConsole`/`freopen` sequence in
`src/obs-moq.cpp` so `stdout` and `stderr` are reopened to `CONOUT$` only when
the console allocation succeeds; otherwise skip the redirection and keep the
existing process streams intact. Use the existing startup block around
`AllocConsole()` and the two `freopen` calls as the place to add the guard.
libmoq is a Rust static library whose std runtime references Windows system symbols (e.g. NtCreateNamedPipeFile from ntdll) that MSVC does not auto-resolve when the archive is linked into the plugin module, causing LNK2001/LNK1120 at link time. Link the OS import libraries reported by `rustc --print native-static-libs` that aren't pulled in by default. They resolve by name from the Windows SDK (no hard-coded paths), and linking them here keeps the plugin buildable against any libmoq whose package config predates the Windows link declaration. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Drop the redundant `-G "Visual Studio 18 2026" -A x64` from the docs: with `--preset`, the generator and architecture come from the preset (Visual Studio 17 2022), and a non-matching `-G` makes CMake error. - Align the documented Visual Studio version with the preset (2022, not 2026), including the chocolatey build-tools package. - Revert the macOS build doc to `cmake --build --preset macos` (matching the justfile) instead of a raw xcodebuild invocation. - Fix obs-moq.cpp formatting to satisfy clang-format (IndentPPDirectives: None / right pointer alignment): de-indent the preprocessor blocks, use `const char *`, and drop the stray blank line. - Include <cstdlib> for std::getenv rather than relying on a transitive include. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Allocate the console before moq_log_level() so the Rust logger binds to a valid stderr, and reopen stdout/stderr onto CONOUT$ -- AllocConsole sets the process std handles but does not redirect the C runtime streams, so without this the debug console stayed empty. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Change the MoQ dock defaults so the plugin works against the public relay out of the box: relay URL defaults to https://cdn.moq.dev/anon, and the broadcast name defaults to "obs-<rand>" (a 6-char suffix) so distinct setups don't collide on the shared relay. Saved settings still override these on subsequent launches. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kixelated
force-pushed
the
win-build-fix-new
branch
from
e0ab0e7 to
489f29f
Compare
|
thanks @danrossi , lemme know if any of my changes don't work for you. |
3 participants
The Windows cmake install config needs the path reformatted to forward slashes to compile and install correctly.
Adds Windows specific build docs