Fix local build setup for Windows - #94
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
|
Warning Rate limit exceeded@christianhelle has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces the Windows CMake path with a gated Windows-specific flow: checks for Ninja, Visual Studio (via vswhere), and Qt 6.9.0 MSVC2022 x64; configures environment with vcvars64.bat; runs CMake with Ninja generator and flags; builds, copies artifacts into Release, and runs windeployqt only after successful build. Linux/macOS branches unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as build.ps1
participant vswhere
participant Ninja
participant Qt
participant vcvars as vcvars64.bat
participant CMake
participant NinjaBuild as Ninja/CMakeBuild
participant Deployer as windeployqt
User->>Script: run build.ps1
rect rgb(240,248,255)
Note over Script: Prerequisite validation
Script->>Ninja: check installed
Script->>vswhere: locate Visual Studio
Script->>Qt: verify Qt 6.9.0 MSVC2022 x64
Script->>vcvars: locate vcvars64.bat
alt any check fails
Script->>User: error & exit
end
end
rect rgb(235,255,240)
Note over Script: Environment & Configure
Script->>vcvars: call vcvars64.bat
vcvars->>Script: env set
Script->>CMake: configure (Ninja, Qt prefix, C++ flags)
alt configure fails
Script->>User: error & exit
end
end
rect rgb(255,250,235)
Note over Script: Build & Package
Script->>NinjaBuild: build via CMake -G Ninja
alt build fails
Script->>User: error & exit
else
Script->>Script: create Release/, copy exe
Script->>Deployer: run windeployqt on artifact
Deployer->>Script: deployed
Script->>User: success
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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: 0
🧹 Nitpick comments (3)
src/project/build.ps1 (3)
28-33: Consider making the Qt path configurable.The hard-coded Qt installation path reduces flexibility for developers who may install Qt in different locations. Consider using an environment variable (e.g.,
$env:QT_PATHor$env:CMAKE_PREFIX_PATH) with a fallback to the current default.Example refactor:
+ # Allow Qt path override via environment variable + $qtPath = if ($env:QT_ROOT) { $env:QT_ROOT } else { "C:\Qt\6.9.0\msvc2022_64" } + # Check if Qt is installed - if (-not (Test-Path "C:\Qt\6.9.0\msvc2022_64")) { - Write-Error "Qt 6.9.0 MSVC2022 64-bit is not installed at C:\Qt\6.9.0\msvc2022_64" + if (-not (Test-Path $qtPath)) { + Write-Error "Qt 6.9.0 MSVC2022 64-bit is not installed at $qtPath" Write-Error "Please install Qt 6.9.0 for MSVC 2022 64-bit from: https://www.qt.io/download" + Write-Error "Or set QT_ROOT environment variable to your Qt installation path" exit 1 }
49-53: Consider making the parallel job count dynamic.The hard-coded
--parallel 32may not be optimal for all systems. Developers with fewer CPU cores may experience system slowdowns, while those with more cores won't fully utilize their hardware.Consider using the number of processors dynamically:
- & cmd /c "`"$vcvars`" && cmake --build build --parallel 32" + $processorCount = $env:NUMBER_OF_PROCESSORS + & cmd /c "`"$vcvars`" && cmake --build build --parallel $processorCount"
55-60: Add validation for packaging tools and improve error handling.The packaging section should validate that required tools exist before attempting to use them, similar to how the Linux section checks for
snapcraft(lines 78-83).Apply these improvements:
if ($Package) { - mkdir .\build\Release + New-Item -ItemType Directory -Path .\build\Release -Force | Out-Null Copy-Item .\build\SQLiteQueryAnalyzer.exe .\build\Release\SQLiteQueryAnalyzer.exe - C:\Qt\6.9.0\msvc2022_64\bin\windeployqt.exe .\build\Release\SQLiteQueryAnalyzer.exe - ../../deps/innosetup/ISCC.exe setup.iss + + $windeployqt = "$qtPath\bin\windeployqt.exe" + if (Test-Path $windeployqt) { + & $windeployqt .\build\Release\SQLiteQueryAnalyzer.exe + } else { + Write-Warning "windeployqt not found at $windeployqt. Qt dependencies will not be deployed." + } + + $innoSetup = "..\..\deps\innosetup\ISCC.exe" + if (Test-Path $innoSetup) { + & $innoSetup setup.iss + } else { + Write-Warning "InnoSetup not found at $innoSetup. Installer will not be created." + } }This change:
- Uses
New-Item -Forceto safely create the directory- Validates windeployqt existence before running it
- Validates InnoSetup (ISCC.exe) existence before running it
- Uses
$qtPathvariable for consistency (if the earlier refactor is applied)- Provides clear warnings rather than silent failures
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/project/build.ps1(1 hunks)
🔇 Additional comments (5)
src/project/build.ps1 (5)
7-12: LGTM: Clear prerequisite validation with helpful error messages.The Ninja validation correctly checks for availability and provides actionable installation instructions.
14-20: Successfully handles Visual Studio preview versions.The
-prereleaseflag correctly enables detection of preview VS installations, achieving the PR objective. The hard-coded vswhere.exe path is the standard location and is acceptable since vswhere.exe ships with Visual Studio 2017 and later.
22-26: LGTM: Successfully switches to Ninja generator.This change achieves the second PR objective by explicitly using Ninja as the CMake generator.
35-40: LGTM: Proper validation of Visual Studio C++ tools.The vcvars64.bat validation ensures the C++ development workload is installed before proceeding.
42-47: CMake configuration is correct; consider parameterizing the Qt path.The CMake configuration properly sets up the build environment with appropriate C++ standard and compiler flags. The use of
cmd /cto preserve the vcvars64 environment is correct.If the Qt path is made configurable (as suggested for lines 28-33), remember to update the hard-coded path in
-DCMAKE_PREFIX_PATHon line 43 to use the$qtPathvariable.
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the Windows build script to support Visual Studio preview versions and switches to the Ninja build system for improved local build performance. The changes add comprehensive validation checks for build dependencies and improve error handling.
Key changes:
- Adds VS preview version detection using
vswhere.exewith-prereleaseflag - Switches from Visual Studio generator to Ninja generator for CMake builds
- Adds validation for Ninja, Visual Studio, and Qt installation before build
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/project/build.ps1 (3)
42-53: Consider combining vcvars64.bat invocations and adjusting parallel build count.Two performance improvements to consider:
vcvars64.bat overhead: Each
cmd /cinvocation starts a new process, requiring vcvars64.bat to be loaded twice. Combining the configure and build steps into a single shell invocation would reduce this overhead.Excessive parallelism: The
--parallel 32flag may be excessive on systems with fewer cores. Consider using a dynamic value like$env:NUMBER_OF_PROCESSORSor allowing it to be configurable.Based on past review comments
Apply this diff to combine the commands:
- # 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 and build in a single shell + $parallelJobs = if ($env:NUMBER_OF_PROCESSORS) { $env:NUMBER_OF_PROCESSORS } else { 32 } + & 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 $parallelJobs" if ($LASTEXITCODE -ne 0) { - Write-Error "Build failed" + Write-Error "CMake configure or build failed" exit $LASTEXITCODE }
56-57: Verify the necessity of copying the executable to a Release subdirectory.With the Ninja generator, the executable is output directly to
.\build\SQLiteQueryAnalyzer.exe(not in aReleasesubdirectory like with MSBuild). The current logic copies it to.\build\Release\and runswindeployqton the copy.Consider whether this copy operation is necessary:
- If
setup.iss(line 60) expects files in.\build\Release\, then keep the copy but ensure the directory creation is robust (see previous comment).- If not, simplify by running
windeployqtdirectly on.\build\SQLiteQueryAnalyzer.exeand remove the copy operation.Based on past review comments
To verify what setup.iss expects, run:
#!/bin/bash # Check setup.iss for expected file paths if [ -f "src/project/setup.iss" ]; then cat src/project/setup.iss else fd -t f "setup.iss" --exec cat {} fi
55-55: Fix mkdir to handle existing directory.The
mkdircommand will fail on subsequent builds if the directory already exists, causing the build to fail.Based on past review comments
Apply this diff:
- mkdir .\build\Release + New-Item -ItemType Directory -Path .\build\Release -Force | Out-Null
🧹 Nitpick comments (1)
src/project/build.ps1 (1)
29-29: Extract hardcoded Qt path to a variable.The Qt installation path
C:\Qt\6.9.0\msvc2022_64is repeated on lines 29, 43, and 57. Extracting it to a variable improves maintainability and makes it easier to update the version.Apply this diff at the top of the Windows block:
if ($IsWindows) { + $qtPath = "C:\Qt\6.9.0\msvc2022_64" + # Check if Ninja is installedThen update the three occurrences:
# Check if Qt is installed - if (-not (Test-Path "C:\Qt\6.9.0\msvc2022_64")) { + if (-not (Test-Path $qtPath)) { - Write-Error "Qt 6.9.0 MSVC2022 64-bit is not installed at C:\Qt\6.9.0\msvc2022_64" + Write-Error "Qt 6.9.0 MSVC2022 64-bit is not installed at $qtPath"- & 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" + & cmd /c "`"$vcvars`" && cmake . -G `"$generator`" -DCMAKE_PREFIX_PATH=$qtPath -DCMAKE_CXX_STANDARD=17 -DCMAKE_CXX_FLAGS=`"/Zc:__cplusplus /permissive-`" -DCMAKE_BUILD_TYPE=Release -B build"- C:\Qt\6.9.0\msvc2022_64\bin\windeployqt.exe .\build\Release\SQLiteQueryAnalyzer.exe + & "$qtPath\bin\windeployqt.exe" .\build\Release\SQLiteQueryAnalyzer.exe
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
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). (2)
- GitHub Check: build / template
- GitHub Check: build / build
🔇 Additional comments (3)
src/project/build.ps1 (3)
7-12: LGTM! Clear prerequisite check with helpful guidance.The Ninja installation check is well-implemented with actionable error messages for users.
24-26: LGTM!Clear generator selection with informative logging.
35-40: LGTM!Proper verification of Visual Studio C++ tools with clear error messaging.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/project/build.ps1 (1)
49-53: Consider using dynamic parallelism based on available processors.The build uses a hardcoded
--parallel 32, which may overwhelm systems with fewer CPU cores.Consider using
$env:NUMBER_OF_PROCESSORS:- & cmd /c "`"$vcvars`" && cmake --build build --parallel 32" + & cmd /c "`"$vcvars`" && cmake --build build --parallel $env:NUMBER_OF_PROCESSORS"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
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). (2)
- GitHub Check: build / build
- GitHub Check: build / template
🔇 Additional comments (3)
src/project/build.ps1 (3)
7-12: LGTM! Clear prerequisite check with helpful error messages.The Ninja installation check is well-implemented with appropriate error handling and actionable guidance for users.
35-40: LGTM! Proper validation of C++ development tools.The vcvars64.bat check correctly validates that the C++ workload is installed with clear error messaging.
42-47: LGTM! Correct CMake configuration with proper error handling.The CMake configuration correctly runs within the Visual Studio environment and includes appropriate MSVC flags (
/Zc:__cplusplus /permissive-) for modern C++ compliance. Error handling via$LASTEXITCODEis proper.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Summary by CodeRabbit