-
Notifications
You must be signed in to change notification settings - Fork 29
add back Visual C++ Redistributable #434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds installation of the Visual C++ Redistributable to the Windows MSVC build and refactors CPack generator selection to use PACKAGE_TYPE_NORMALIZED for distinguishing NSIS and AppX packaging flows. Sequence diagram for NSIS installer running Visual C++ RedistributablesequenceDiagram
actor User
participant NSISInstaller
participant AppInstallerScript
participant VCRedistInstaller
participant Application
User->>NSISInstaller: Run installer.exe
NSISInstaller->>AppInstallerScript: Execute install commands
AppInstallerScript->>VCRedistInstaller: ExecWait vc_redist.x64.exe /q /norestart
VCRedistInstaller-->>AppInstallerScript: Return success
AppInstallerScript->>NSISInstaller: Continue application installation
NSISInstaller->>Application: Install binaries and libraries
AppInstallerScript->>NSISInstaller: Delete vc_redist.x64.exe from INSTDIR
NSISInstaller-->>User: Show installation completed
Flow diagram for CPack generator selection using PACKAGE_TYPE_NORMALIZEDflowchart TD
A[WIN32 build enabled] --> B{MSVC compiler?}
B -- Yes --> C[Install vc_redist.x64.exe into packaging staging area]
C --> D[Configure NSIS extra commands to ExecWait and Delete vc_redist.x64.exe]
B -- No --> E[Skip vc_redist configuration]
D --> F[Set CPACK_SOURCE_GENERATOR to ZIP]
E --> F
F --> G{PACKAGE_TYPE_NORMALIZED}
G -- Nsis --> H[Set CPACK_GENERATOR to NSIS]
H --> I[Configure NSIS-specific settings<br/>- Icons<br/>- Shortcuts<br/>- License file]
G -- AppX --> J[Set CPACK_GENERATOR to External]
J --> K[Configure AppX-specific settings<br/>- APPX_NAME<br/>- External packaging tool]
G -- Other --> L[No NSIS/AppX-specific generator configured]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- The
install(FILES "${WINDEPLOYQT_STAGING}/vc_redist.x64.exe" ...)step assumesWINDEPLOYQT_STAGINGandvc_redist.x64.exealways exist; consider guarding this with anif(EXISTS ...)or a check onWINDEPLOYQT_STAGINGto avoid configuration/install-time failures when the file is missing. - Switching from
if(WITH_UPDATER)toif(PACKAGE_TYPE_NORMALIZED STREQUAL "Nsis")changes the condition under which NSIS packaging is used; confirm that all previousWITH_UPDATERuse cases are still covered and thatPACKAGE_TYPE_NORMALIZEDis always set to one of the expected values to avoid falling through without configuring a generator.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `install(FILES "${WINDEPLOYQT_STAGING}/vc_redist.x64.exe" ...)` step assumes `WINDEPLOYQT_STAGING` and `vc_redist.x64.exe` always exist; consider guarding this with an `if(EXISTS ...)` or a check on `WINDEPLOYQT_STAGING` to avoid configuration/install-time failures when the file is missing.
- Switching from `if(WITH_UPDATER)` to `if(PACKAGE_TYPE_NORMALIZED STREQUAL "Nsis")` changes the condition under which NSIS packaging is used; confirm that all previous `WITH_UPDATER` use cases are still covered and that `PACKAGE_TYPE_NORMALIZED` is always set to one of the expected values to avoid falling through without configuring a generator.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
c3dcbd8 to
4280c81
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #434 +/- ##
==========================================
- Coverage 66.48% 65.48% -1.01%
==========================================
Files 85 86 +1
Lines 4186 3963 -223
Branches 255 264 +9
==========================================
- Hits 2783 2595 -188
+ Misses 1403 1368 -35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This broke when we added AppX support in a86804c
This helps the CI pipeline build for new contributors who did not push tags to their fork.
Summary by Sourcery
Bundle the Visual C++ Redistributable in Windows builds and refine packaging configuration based on normalized package type.
New Features:
Enhancements: