Retarget windows CRT to MultiThreaded(Debug) for compat with future plugin brokers#331
Conversation
|
Warning Review limit reached
More reviews will be available in 18 minutes and 1 second. 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 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (29)
📝 WalkthroughWalkthroughThe PR refreshes the Alchemy viewer's build infrastructure: renames all vcpkg triplets from ChangesBuild and dependency refresh
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
indra/cmake/00-Common.cmake (1)
163-169: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe new option still cannot control Windows Debug builds.
Lines 163-165 already force
DISABLE_WEBRTC=1for every WindowsDebugbuild, so the newif(DISABLE_WEBRTC)block below never becomes the single source of truth for that configuration. That keeps thellvoiceclient.cppfallback paths compiled out in Debug even when the cache option is off. Fold both paths into one policy.🤖 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 `@indra/cmake/00-Common.cmake` around lines 163 - 169, The Windows Debug build is still being forced to define DISABLE_WEBRTC=1, so the new cache option cannot override that configuration. Update the compile-definition logic in the Common CMake block so that llvoiceclient.cpp sees a single source of truth: make the Debug behavior and the DISABLE_WEBRTC option flow through the same conditional, instead of unconditionally adding the Debug definition separately. Use the existing DISABLE_WEBRTC option check and the Windows Debug compile definition section together so Debug builds only disable WebRTC when the option says so.
🤖 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 `@indra/cmake/Copy3rdPartyLibs.cmake`:
- Around line 44-77: Do not remove the Windows VC runtime staging in
Copy3rdPartyLibs.cmake; the commented-out InstallRequiredSystemLibraries /
to_staging_dirs / to_viewer_staging_dirs logic is still needed for /MD-based
packages. Keep the existing system runtime copy flow intact so cef-bin,
webrtc-bin, vlc-bin, and the llplugin staging path continue to receive the
required MSVC DLLs on clean machines.
In `@indra/newview/llvoiceclient.cpp`:
- Around line 190-195: The current DISABLE_WEBRTC guard leaves an empty
VoiceServerType unresolved and lets getOutgoingCallInterface() silently fall
back to the local module, so update llvoiceclient.cpp to compute one effective
server type up front and use it consistently in both the startup path and
getOutgoingCallInterface(). If WEBRTC is unavailable and no non-WebRTC fallback
exists, reject the unsupported type immediately instead of allowing the empty
value to propagate; otherwise, route the resolved type through the same
selection logic everywhere voiceServerType is used.
In `@indra/vcpkg/ports/tracy/portfile.cmake`:
- Around line 49-53: The Tracy portfile is branching on VCPKG_CRT_LINKAGE for
copying the debug artifact, but this decision should be based on
VCPKG_LIBRARY_LINKAGE instead. Update the conditional in the portfile logic
around the TracyClient copy step so the .lib path is used for static library
linkage and the .dll path is used for dynamic library linkage, keeping the
existing file copy behavior otherwise.
In `@indra/vcpkg/ports/webrtc-bin/vcpkg.json`:
- Line 3: The WebRTC port manifest version is out of sync with the artifacts
fetched by the portfile, so update the version metadata in vcpkg.json and the
download references in portfile.cmake to the same upstream build identifier. Use
the webrtc-bin manifest and the download URLs in portfile.cmake as the source of
truth, and make sure the version-string matches the binaries being installed.
In `@indra/vcpkg/ports/zlib/portfile.cmake`:
- Around line 26-33: Exclude MinGW from the Windows static rename logic in the
zlib portfile so only the MSVC path is renamed. Update the conditional around
the file(RENAME) calls in portfile.cmake to also check VCPKG_TARGET_IS_MINGW,
keeping the existing VCPKG_TARGET_IS_WINDOWS and VCPKG_LIBRARY_LINKAGE STREQUAL
static guards intact. This should ensure the rename block only runs for the
zlibstatic library names produced by the MSVC toolchain and not for MinGW
builds.
---
Outside diff comments:
In `@indra/cmake/00-Common.cmake`:
- Around line 163-169: The Windows Debug build is still being forced to define
DISABLE_WEBRTC=1, so the new cache option cannot override that configuration.
Update the compile-definition logic in the Common CMake block so that
llvoiceclient.cpp sees a single source of truth: make the Debug behavior and the
DISABLE_WEBRTC option flow through the same conditional, instead of
unconditionally adding the Debug definition separately. Use the existing
DISABLE_WEBRTC option check and the Windows Debug compile definition section
together so Debug builds only disable WebRTC when the option says so.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c72fb95c-38fe-453b-9f5e-8bd6fb9fdcc3
📒 Files selected for processing (29)
.github/workflows/build.yamlindra/cmake/00-Common.cmakeindra/cmake/BootstrapVcpkg.cmakeindra/cmake/CURL.cmakeindra/cmake/Collada-Dom.cmakeindra/cmake/Copy3rdPartyLibs.cmakeindra/newview/llvoiceclient.cppindra/vcpkg.jsonindra/vcpkg/ports/apr/portfile.cmakeindra/vcpkg/ports/apr/vcpkg.jsonindra/vcpkg/ports/cef-bin/portfile.cmakeindra/vcpkg/ports/faudio/portfile.cmakeindra/vcpkg/ports/faudio/vcpkg.jsonindra/vcpkg/ports/freetype/portfile.cmakeindra/vcpkg/ports/tracy/portfile.cmakeindra/vcpkg/ports/webrtc-bin/portfile.cmakeindra/vcpkg/ports/webrtc-bin/vcpkg.jsonindra/vcpkg/ports/zlib/portfile.cmakeindra/vcpkg/ports/zlib/vcpkg-cmake-wrapper.cmakeindra/vcpkg/ports/zlib/vcpkg.jsonindra/vcpkg/triplets/arm64-osx-alchemy-release.cmakeindra/vcpkg/triplets/arm64-osx-alchemy.cmakeindra/vcpkg/triplets/x64-linux-alchemy-release.cmakeindra/vcpkg/triplets/x64-linux-alchemy.cmakeindra/vcpkg/triplets/x64-osx-alchemy-release.cmakeindra/vcpkg/triplets/x64-osx-alchemy.cmakeindra/vcpkg/triplets/x64-windows-alchemy-release.cmakeindra/vcpkg/triplets/x64-windows-alchemy.cmakevcpkg
💤 Files with no reviewable changes (1)
- indra/cmake/Collada-Dom.cmake
RyeMutt
force-pushed
the
rye/static-crt
branch
3 times, most recently
from
22dc7d6 to
531e06c
Compare
…aemon Bump GHA xcode version to 26.6 Bump macos min deploy target to 14.0 Update various vcpkg override ports
1 participant
Description
Retarget windows CRT to MultiThreaded(Debug) for compat with future plugin brokers
Bump GHA xcode version to 26.6
Bump macos min deploy target to 14.0
Update various vcpkg override ports
Related Issues
Issue Link:
Checklist
Please ensure the following before requesting review:
Additional Notes