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 (1)
📝 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
| if (Test-Path "C:\VulkanSDK") { | ||
| $vulkanVersion = (Get-ChildItem "C:\VulkanSDK").Name[0] | ||
| echo "VULKAN_SDK=C:\VulkanSDK\$vulkanVersion" >> $env:GITHUB_ENV | ||
| echo "C:\VulkanSDK\$vulkanVersion\Bin" >> $env:GITHUB_PATH | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix PowerShell array indexing bug to prevent invalid Vulkan SDK paths.
In PowerShell, when Get-ChildItem returns a single item (which is standard after a clean Vulkan SDK install), it returns a single DirectoryInfo object instead of an array. Accessing the .Name property returns a string, and [0] extracts the first character of that string (e.g., "1" instead of "1.3.268.0"). This results in a broken environment path like C:\VulkanSDK\1.
Coerce the result into an array using @(...) to ensure consistent indexing behavior regardless of the number of child items.
🐛 Proposed fix for the path extraction
# Add Vulkan SDK paths to environment
if (Test-Path "C:\VulkanSDK") {
- $vulkanVersion = (Get-ChildItem "C:\VulkanSDK").Name[0]
+ $vulkanVersion = @(Get-ChildItem "C:\VulkanSDK")[0].Name
echo "VULKAN_SDK=C:\VulkanSDK\$vulkanVersion" >> $env:GITHUB_ENV
echo "C:\VulkanSDK\$vulkanVersion\Bin" >> $env:GITHUB_PATH
}📝 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.
| if (Test-Path "C:\VulkanSDK") { | |
| $vulkanVersion = (Get-ChildItem "C:\VulkanSDK").Name[0] | |
| echo "VULKAN_SDK=C:\VulkanSDK\$vulkanVersion" >> $env:GITHUB_ENV | |
| echo "C:\VulkanSDK\$vulkanVersion\Bin" >> $env:GITHUB_PATH | |
| } | |
| if (Test-Path "C:\VulkanSDK") { | |
| $vulkanVersion = @(Get-ChildItem "C:\VulkanSDK")[0].Name | |
| echo "VULKAN_SDK=C:\VulkanSDK\$vulkanVersion" >> $env:GITHUB_ENV | |
| echo "C:\VulkanSDK\$vulkanVersion\Bin" >> $env:GITHUB_PATH | |
| } |
🤖 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 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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify PowerShell string indexing behavior when only one item is present.
pwsh -Command '
New-Item -ItemType Directory -Path "TestVulkan\1.3.261.0" -Force | Out-Null;
$vulkanVersion = (Get-ChildItem "TestVulkan").Name[0];
Write-Host "Extracted version: $vulkanVersion";
if ($vulkanVersion -eq "1") { Write-Host "BUG DETECTED" } else { Write-Host "OK" }
'Repository: Redot-Engine/DraconicEngine
Length of output: 199
Avoid indexing the Vulkan SDK name
If Get-ChildItem "C:\VulkanSDK" returns a single folder, .Name[0] takes the first character of that folder name (1 from 1.3.261.0), so VULKAN_SDK points to C:\VulkanSDK\1 instead of the installed SDK version. Use the full name (or explicitly select the first item before reading .Name).
🤖 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 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.
1 participant
Adds CI for Windows
Summary by CodeRabbit