Add Windows CI#55
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a Windows GitHub Actions workflow that provisions Clang and Vulkan, configures and builds the debug target with CMake and Ninja, runs unit tests, and uploads the resulting artifact. Linux checkout is updated to avoid persisting credentials. ChangesCross-Platform CI Workflows
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2
🧹 Nitpick comments (3)
.github/workflows/windows_build.yml (3)
26-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStandardize shell selection.
This step specifies
shell: powershell(Windows PowerShell 5.1), while the rest of the workflow usesshell: pwsh(PowerShell Core). It is recommended to usepwshconsistently across all steps for uniform behavior, as both shells fully support the Chocolatey commands and syntax used here.♻️ Proposed consistency fix
- name: Install System Dependencies - shell: powershell + shell: pwsh🤖 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 @.github/workflows/windows_build.yml around lines 26 - 27, Update the “Install System Dependencies” workflow step to use shell `pwsh` instead of `powershell`, matching the shell selection used throughout the workflow while preserving its existing Chocolatey commands.
16-19: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDisable credential persistence to improve security posture.
It is a security best practice to set
persist-credentials: falsewhen checking out the code, especially for public repositories. This ensures that the GitHub API token is not left lingering in the local Git configuration, eliminating the risk of it being inadvertently read or exported by subsequent build scripts or uploaded artifacts.🛡️ Proposed fix to secure the checkout
- name: Checkout Source Code uses: actions/checkout@v4 with: submodules: recursive + persist-credentials: false🤖 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 @.github/workflows/windows_build.yml around lines 16 - 19, Update the “Checkout Source Code” step using actions/checkout@v4 to set persist-credentials to false, while preserving the existing recursive submodules configuration.Source: Linters/SAST tools
48-64: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider using
sccachefor native Windows caching stability.The
hendrikmuhs/ccache-actiondocumentation explicitly recommendssccachefor stable caching on Windows. The standardccachedistribution provided by the action relies on MSYS2, which can occasionally introduce path translation quirks or certificate issues in native Windows CI pipelines.If you adopt this, ensure both the action variant and the CMake compiler launchers are updated to use
sccache.♻️ Proposed refactoring to use sccache
- name: Cache Build Artifacts uses: hendrikmuhs/ccache-action@v1.2 with: - key: ${{ runner.os }}-ccache-${{ hashFiles('**/CMakeLists.txt', '**/*.cmake') }} + variant: sccache + key: ${{ runner.os }}-sccache-${{ hashFiles('**/CMakeLists.txt', '**/*.cmake') }} restore-keys: | - ${{ runner.os }}-ccache- + ${{ runner.os }}-sccache- max-size: "500M" - name: Configure Build System shell: pwsh run: | cmake --preset debug ` -G Ninja ` -DCMAKE_C_COMPILER=clang ` -DCMAKE_CXX_COMPILER=clang++ ` - -DCMAKE_C_COMPILER_LAUNCHER=ccache ` - -DCMAKE_CXX_COMPILER_LAUNCHER=ccache + -DCMAKE_C_COMPILER_LAUNCHER=sccache ` + -DCMAKE_CXX_COMPILER_LAUNCHER=sccache🤖 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 @.github/workflows/windows_build.yml around lines 48 - 64, Replace the Windows cache setup using hendrikmuhs/ccache-action and the ccache compiler launchers with the action’s sccache-compatible variant. Update both CMAKE_C_COMPILER_LAUNCHER and CMAKE_CXX_COMPILER_LAUNCHER in the Configure Build System step to use sccache, while preserving the existing cache key and size settings.
🤖 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 @.github/workflows/windows_build.yml:
- Around line 32-36: Update the Vulkan version extraction in the PowerShell
block guarded by Test-Path so Get-ChildItem is coerced to an array before
selecting the first item's Name. Preserve the existing VULKAN_SDK and
GITHUB_PATH assignments while ensuring a single installed SDK directory yields
its complete version name rather than the first character.
- Around line 32-36: Update the Vulkan SDK version assignment in the Test-Path
block to use the full selected child item's Name rather than indexing the Name
string with [0]. Preserve selecting the installed SDK directory and use that
complete version when setting VULKAN_SDK and VULKAN_PATH.
---
Nitpick comments:
In @.github/workflows/windows_build.yml:
- Around line 26-27: Update the “Install System Dependencies” workflow step to
use shell `pwsh` instead of `powershell`, matching the shell selection used
throughout the workflow while preserving its existing Chocolatey commands.
- Around line 16-19: Update the “Checkout Source Code” step using
actions/checkout@v4 to set persist-credentials to false, while preserving the
existing recursive submodules configuration.
- Around line 48-64: Replace the Windows cache setup using
hendrikmuhs/ccache-action and the ccache compiler launchers with the action’s
sccache-compatible variant. Update both CMAKE_C_COMPILER_LAUNCHER and
CMAKE_CXX_COMPILER_LAUNCHER in the Configure Build System step to use sccache,
while preserving the existing cache key and size settings.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7c9c4e0-3930-47c4-a305-dffa10953d2d
📒 Files selected for processing (1)
.github/workflows/windows_build.yml
There was a problem hiding this comment.
The used versions of actions/checkout and hendrikmuhs/ccache-action trigger a warning due to the use of the deprecated NodeJS 20.
actions/checkout could be updated to v7 and hendrikmuhs/ccache-action could be pinned to v1.2.23 to fix the warning (for some reason v1.2 doesn't pick the latest patch release which runs on Node24).
2 participants
Adds CI for Windows
Summary by CodeRabbit