Add applicaton icon for Windows build - #95
Conversation
- Add -prerelease flag to vswhere to detect preview/insider VS - Explicitly specify Visual Studio 17 2022 CMake generator - Add prerequisite validation for VS and Qt installations - Add error handling after CMake configuration and build steps - Fix trailing whitespace
WalkthroughBuild configuration updated to include Windows-specific icon resources in CMake executable targets. Windows PowerShell build script completely rewritten to implement preflight environment validation (Ninja and Visual Studio detection), use Ninja generator with Visual Studio environment setup via vcvars64.bat, and add packaging steps with windeployqt and Inno Setup support. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds Windows application icon support and improves the Windows build process by integrating Ninja as the CMake generator and adding support for Visual Studio preview versions.
- Adds multi-resolution icon.ico file with embedded PNG images
- Updates Windows build script to detect VS preview versions using vswhere with
-prereleaseflag - Switches to Ninja generator for faster Windows builds with proper environment setup via vcvars64.bat
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/resources/icon.ico | New multi-resolution icon file with embedded PNG images at various sizes (16x16, 32x32, 48x48, 64x64, 128x128, 256x256) |
| src/project/build.ps1 | Enhanced Windows build with Ninja generator support, VS preview detection via vswhere, improved error handling, and vcvars64.bat environment setup |
| src/project/CMakeLists.txt | Added Windows icon resource (app_icon_resource_windows) to executable compilation |
| } | ||
|
|
||
| if ($Package) { | ||
| mkdir .\build\Release |
There was a problem hiding this comment.
The mkdir command can fail if the directory already exists. Add -Force parameter to prevent errors on subsequent builds: mkdir .\build\Release -Force
| mkdir .\build\Release | |
| mkdir .\build\Release -Force |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/project/build.ps1 (2)
14-20: Verify vswhere.exe availability before use.The script uses a hardcoded path to vswhere.exe without first verifying it exists. If Visual Studio Installer is missing or in a non-standard location, this will fail with a cryptic error.
Apply this diff to check vswhere existence first:
+ # Check if vswhere is available + $vswherePath = "C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe" + if (-not (Test-Path $vswherePath)) { + Write-Error "vswhere.exe not found. Visual Studio Installer may not be installed correctly." + exit 1 + } + # Check if Visual Studio is installed (including preview versions) for compiler - $vsPath = & "C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe" -latest -prerelease -property installationPath 2>$null + $vsPath = & $vswherePath -latest -prerelease -property installationPath 2>$null
49-53: LGTM! Consider consolidating vcvars calls.The build command is correctly implemented with good error handling.
For efficiency, consider running both CMake commands in a single cmd session:
- # Setup Visual Studio environment and run CMake with Ninja generator - & cmd /c "`"$vcvars`" && cmake . -G `"$generator`" -DCMAKE_PREFIX_PATH=C:\Qt\6.9.0\msvc2022_64 -DCMAKE_CXX_STANDARD=17 -DCMAKE_CXX_FLAGS=`"/Zc:__cplusplus /permissive-`" -DCMAKE_BUILD_TYPE=Release -B build" - if ($LASTEXITCODE -ne 0) { - Write-Error "CMake configuration failed" - exit $LASTEXITCODE - } - - & cmd /c "`"$vcvars`" && cmake --build build --parallel 32" + # Setup Visual Studio environment and run CMake configure + build + & cmd /c "`"$vcvars`" && cmake . -G `"$generator`" -DCMAKE_PREFIX_PATH=C:\Qt\6.9.0\msvc2022_64 -DCMAKE_CXX_STANDARD=17 -DCMAKE_CXX_FLAGS=`"/Zc:__cplusplus /permissive-`" -DCMAKE_BUILD_TYPE=Release -B build && cmake --build build --parallel 32" if ($LASTEXITCODE -ne 0) { - Write-Error "Build failed" + Write-Error "CMake configuration or build failed" exit $LASTEXITCODE }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/resources/icon.icois excluded by!**/*.ico
📒 Files selected for processing (2)
src/project/CMakeLists.txt(1 hunks)src/project/build.ps1(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (5)
src/project/CMakeLists.txt (2)
65-65: LGTM!The fallback
add_executablepath correctly includes the Windows icon resource, maintaining consistency with theqt_add_executablepath.
59-59: LGTM. Icon resource file verified to exist.The Windows icon resource is correctly added using CMake generator expressions, and verification confirms the icon resource file exists at
src/resources/icon.rc.src/project/build.ps1 (3)
7-12: LGTM!The Ninja preflight check is well-implemented with clear error messages and installation guidance.
35-40: LGTM!The vcvars64.bat detection and validation is properly implemented with clear error messaging.
42-47: CMake configuration approach is sound, but inherits hardcoded Qt path.The CMake configuration with Ninja generator and MSVC environment setup is correctly implemented. However, it uses the same hardcoded Qt path flagged earlier.
Note: This comment approves the configuration approach. The hardcoded Qt path issue is addressed in a separate comment (lines 28-33).
| mkdir .\build\Release | ||
| Copy-Item .\build\SQLiteQueryAnalyzer.exe .\build\Release\SQLiteQueryAnalyzer.exe |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling and safe directory creation.
The packaging steps lack error handling and may fail if the directory already exists.
Apply this diff to improve robustness:
- mkdir .\build\Release
+ if (-not (Test-Path .\build\Release)) {
+ New-Item -ItemType Directory -Path .\build\Release -Force | Out-Null
+ }
+
+ if (-not (Test-Path .\build\SQLiteQueryAnalyzer.exe)) {
+ Write-Error "Build output not found: .\build\SQLiteQueryAnalyzer.exe"
+ exit 1
+ }
+
Copy-Item .\build\SQLiteQueryAnalyzer.exe .\build\Release\SQLiteQueryAnalyzer.exe📝 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.
| mkdir .\build\Release | |
| Copy-Item .\build\SQLiteQueryAnalyzer.exe .\build\Release\SQLiteQueryAnalyzer.exe | |
| if (-not (Test-Path .\build\Release)) { | |
| New-Item -ItemType Directory -Path .\build\Release -Force | Out-Null | |
| } | |
| if (-not (Test-Path .\build\SQLiteQueryAnalyzer.exe)) { | |
| Write-Error "Build output not found: .\build\SQLiteQueryAnalyzer.exe" | |
| exit 1 | |
| } | |
| Copy-Item .\build\SQLiteQueryAnalyzer.exe .\build\Release\SQLiteQueryAnalyzer.exe |
🤖 Prompt for AI Agents
In src/project/build.ps1 around lines 56 to 57, the script unconditionally calls
mkdir and Copy-Item which will fail if the directory already exists or if copy
errors occur; update to create the Release folder safely (use Test-Path or
New-Item -ItemType Directory -Force) and wrap the directory creation and
Copy-Item in a try/catch with -ErrorAction Stop so failures are caught; on
error, write a clear error message to stderr (Write-Error or Write-Host with
red) and exit with a non-zero code to fail the build.
Summary by CodeRabbit
New Features
Build & Infrastructure