Skip to content

Conversation

@nschimme
Copy link
Contributor

@nschimme nschimme commented Dec 19, 2025

Summary by Sourcery

Bundle the Visual C++ Redistributable in Windows builds and refine packaging configuration based on normalized package type.

New Features:

  • Include the Visual C++ Redistributable installer in MSVC-based Windows installation packages.

Enhancements:

  • Gate NSIS and AppX packaging configuration on the normalized package type instead of the updater option.

@sourcery-ai
Copy link

sourcery-ai bot commented Dec 19, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Adds 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++ Redistributable

sequenceDiagram
    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
Loading

Flow diagram for CPack generator selection using PACKAGE_TYPE_NORMALIZED

flowchart 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]
Loading

File-Level Changes

Change Details Files
Install the Visual C++ Redistributable alongside Windows MSVC builds and wire it into the NSIS installer execution flow.
  • Adds an install(FILES ...) rule to include vc_redist.x64.exe from the WINDEPLOYQT staging directory into the installation payload when building with MSVC.
  • Keeps the existing CPACK_NSIS_EXTRA_INSTALL_COMMANDS logic that silently runs vc_redist.x64.exe during installation and deletes it afterwards, now relying on the explicit install of the redistributable file.
src/CMakeLists.txt
Switch CPack generator selection logic from WITH_UPDATER to explicit PACKAGE_TYPE_NORMALIZED checks for NSIS and AppX packaging.
  • Replaces the WITH_UPDATER conditional with a PACKAGE_TYPE_NORMALIZED STREQUAL "Nsis" check to select NSIS as the CPack generator and configure NSIS-specific settings.
  • Changes the fallback branch to an explicit PACKAGE_TYPE_NORMALIZED STREQUAL "AppX" condition for configuring AppX packaging using the External CPack generator.
src/CMakeLists.txt

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a 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 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.
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.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@nschimme nschimme force-pushed the nsis-fix branch 2 times, most recently from c3dcbd8 to 4280c81 Compare December 19, 2025 21:56
@codecov
Copy link

codecov bot commented Dec 19, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.48%. Comparing base (26d1a9d) to head (701409f).
⚠️ Report is 156 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.
@nschimme nschimme merged commit c34220d into MUME:master Dec 19, 2025
20 of 21 checks passed
@nschimme nschimme deleted the nsis-fix branch December 19, 2025 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant