msi: update to wix v5 #5069
msi: update to wix v5 #5069
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
WalkthroughReplaces the Go-based WXS generator and template with a concrete WiX .wixproj and product.wxs, migrates WiX UI files to WiX v4 syntax, updates Makefile to build the MSI via Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (push)
participant CI as CI Runner (windows-2025)
participant Winget as winget
participant DotNetSDK as DotNet.SDK.10
participant DotNetBuild as dotnet build (wixproj)
participant Host as Host Build Dir
participant Pack as Packaging/Artifacts
Dev->>CI: push changes / trigger workflow
CI->>CI: checkout repo
CI->>Winget: run winget install DotNet.SDK.10
Winget-->>DotNetSDK: install SDK
CI->>CI: invoke Makefile target (msidir)
CI->>DotNetBuild: dotnet build packaging/windows/crc-installer.wixproj --property:DefineConstants="Version=$(CRC_VERSION)" --output Host
DotNetBuild-->>Host: produce `crc-windows-amd64.msi`
CI->>Pack: copy runtime assets and MSI to package dir
CI->>Pack: create ZIP including `$(MSI)` and other artifacts
Pack-->>CI: upload artifacts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packaging/windows/product.wxs (2)
13-17: Minor naming inconsistency between version and build numbers.
MINIMUMBUILDis set to1709which is a Windows version identifier, whileCURRENTBUILDfrom the registry contains the actual OS build number (e.g.,19045for Windows 10 22H2). The comparison works correctly because1709 < actual_build_numbers, but the naming could cause confusion for future maintainers.Also note the condition uses
>(greater than), which technically excludes the exact minimum. Consider using>=if build 1709 itself should be supported.
39-40: Consider handling Hyper-V installation failure more explicitly.The
InstallHypervaction usesReturn="ignore", which silently ignores failures. If Hyper-V fails to install (e.g., unsupported hardware, virtualization disabled in BIOS, or permission issues), the user won't be notified during installation but CRC will fail to run afterward.For cleanup actions (
RemoveCrcDaemonTask,RemoveSMBShare),Return="ignore"is appropriate. However, for Hyper-V installation, consider either:
- Using
Return="check"and handling failure gracefully, or- Logging a warning that users should verify Hyper-V is enabled
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/windows-artifacts.yml(2 hunks)Makefile(1 hunks)packaging/.gitignore(0 hunks)packaging/windows/LicenseAgreementDlg_HK.wxs(2 hunks)packaging/windows/WixUI_HK.wxs(2 hunks)packaging/windows/WixUI_en.wxl(0 hunks)packaging/windows/crc-installer.wixproj(1 hunks)packaging/windows/gen_msi_wxs.go(0 hunks)packaging/windows/product.wxs(1 hunks)packaging/windows/product.wxs.template(0 hunks)
💤 Files with no reviewable changes (4)
- packaging/windows/WixUI_en.wxl
- packaging/windows/gen_msi_wxs.go
- packaging/.gitignore
- packaging/windows/product.wxs.template
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 369-369: Target body for "msidir" exceeds allowed length of 5 (6).
(maxbodylength)
⏰ 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). (16)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: verify-devcontainer
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (12)
Makefile (1)
380-385: LGTM!The
MSIvariable path correctly aligns with theOutputNamedefined incrc-installer.wixproj.packaging/windows/WixUI_HK.wxs (2)
1-1: LGTM!Correctly updated to WiX v4 schema namespace.
43-49: LGTM! Dialog flow logic is correct.The conditional navigation handles upgrade detection properly:
- Shows
OldVersionDlgwhenCRCINSTALLEDindicates an existing installation- Skips directly to license agreement for fresh installs
packaging/windows/LicenseAgreementDlg_HK.wxs (1)
1-24: LGTM! Clean migration to WiX v4 syntax.The dialog correctly:
- Uses WiX v4 schema namespace
- Employs attribute-based
DisableCondition/EnableConditioninstead of nested elements- Simplifies the Publish elements to self-closing tags where appropriate
.github/workflows/windows-artifacts.yml (1)
24-24: No action needed. Windows Server 2025 image is generally available for all customers starting April 8, 2025, and you can use the windows-2025 image label in your YAML for standard runners.packaging/windows/crc-installer.wixproj (1)
1-19: Well-structured WiX v6 project file.WixToolset.Sdk 6.0.0 provides the WiX Toolset as an MSBuild SDK for both .NET (v6 or later) and .NET Framework (v4.7.2 or later), confirming compatibility with .NET SDK 10. The configuration correctly targets the x64 platform matching the
crc-windows-amd64output name, suppresses ICE61 (acceptable for upgrade scenarios), disables culture/framework/runtime suffixes for clean output paths, and maintains consistent v6.0.0 versions across SDK and extensions. Consider upgrading to WiX v6.0.2, the latest stable release published 28-Aug-2025, as v6.0.1 fixed a build exception issue present in v6.0.0.packaging/windows/product.wxs (6)
6-8: Consider the security implications ofAllowDowngrades="yes".Allowing downgrades lets users install older, potentially vulnerable versions over newer ones. While this may be intentional for testing or rollback scenarios, it's worth confirming this is the desired behavior for production releases.
If downgrades should be prevented:
🔎 Suggested change
- <MajorUpgrade AllowDowngrades="yes" /> + <MajorUpgrade DowngradeErrorMessage="A newer version of [ProductName] is already installed." />
44-49: LGTM!The execution sequence conditions are well-structured:
- Hyper-V installation and reboot only on fresh installs
- Daemon task removal on uninstall (but preserved during upgrade)
- SMB share cleanup specifically during upgrade path
72-75: Verify aggressive cleanup behavior on uninstall is intentional.The
RemoveFileelement withName="*.*"will delete all files in the installation directory on uninstall, including any files created after installation (logs, crash dumps, user-modified configs, etc.).If this is intentional for a clean uninstall, consider documenting this behavior. If users might want to preserve certain files, consider more targeted cleanup or adding a UI option.
76-80: LGTM!The service installation is configured correctly. The
Stop="both"setting ensures the service is stopped during both install (for upgrades) and uninstall scenarios. WiX handles the case where the service doesn't exist on fresh install gracefully.
93-105: LGTM!The user and group management is well-configured:
crc-usersgroup is created and cleaned up on uninstall- Hyper-V Administrators membership is retained after uninstall (avoiding unexpected permission revocation)
- Uses
[LogonUser]and[%USERDOMAIN]correctly for both local and domain-joined machines
27-29: Remove - PowerShell registry path is current and reliable.The registry path
SOFTWARE\Microsoft\PowerShell\1\ShellIds\Microsoft.PowerShellis the current location for Windows PowerShell 5.1 configuration, not a legacy path. Recent systems (Windows 10.0.22631) confirm the "Path" value at this location contains the Windows PowerShell executable path. This registry search is reliable on Windows 10 1709+ and Windows 11.
31c169a to
86127d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/windows-artifacts.yml (1)
33-34: Add non-interactive flags to prevent CI hangs.The
winget installcommand may prompt for interactive confirmations in CI environments, potentially causing the workflow to hang. Add the--accept-source-agreementsand--accept-package-agreementsflags to ensure non-interactive execution.🔎 Apply this diff to add non-interactive flags:
- name: Install DotNet.SDK.10 - run: winget install --id Microsoft.DotNet.SDK.10 --source winget + run: winget install --id Microsoft.DotNet.SDK.10 --source winget --accept-source-agreements --accept-package-agreements
🧹 Nitpick comments (1)
Makefile (1)
370-375: Consider splitting themsidirrecipe body if makefile conventions are strict.The addition of the
*.wixprojcopy step (line 375) correctly supports the new .NET build system. However, static analysis flags that the target body exceeds the conventional 5-line limit (currently 6 lines). If your project enforces strict makefile conventions, consider extracting some steps into a helper target.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/windows-artifacts.yml(2 hunks)Makefile(1 hunks)packaging/windows/WixUI_en.wxl(0 hunks)packaging/windows/crc-installer.wixproj(1 hunks)packaging/windows/gen_msi_wxs.go(0 hunks)packaging/windows/product.wxs.template(0 hunks)
💤 Files with no reviewable changes (3)
- packaging/windows/WixUI_en.wxl
- packaging/windows/product.wxs.template
- packaging/windows/gen_msi_wxs.go
✅ Files skipped from review due to trivial changes (1)
- packaging/windows/crc-installer.wixproj
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 369-369: Target body for "msidir" exceeds allowed length of 5 (6).
(maxbodylength)
⏰ 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). (16)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build-installer (windows-2025, 1.24)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (5)
Makefile (4)
369-369: LGTM!The updated
msidirtarget dependencies correctly reflect the migration from Go-generated WiX templates to the .NET-basedwixprojbuild system by removing theGenMsiWxsdependency.
377-378: LGTM!The migration to
dotnet buildfor the .NET-based WiX v6 build is correct. TheDefineConstantsparameter properly passes the version to the build, and the past issue with redundantdotnet restorehas been successfully addressed.
380-380: LGTM!The
MSIvariable introduction improves maintainability by centralizing the MSI output path reference.
381-385: LGTM!The Windows installer packaging correctly uses the new
$(MSI)variable, and the PowerShellCompress-Archivecommand is properly structured with the-NoProfileflag for CI reproducibility..github/workflows/windows-artifacts.yml (1)
24-24: No action required—windows-2025is now generally available and stable in GitHub Actions.Windows Server 2025 image is generally available for all customers, and the image stability has been improved and is now ready to be set as the latest. The runner has transitioned from beta status to production-ready. Be aware that some packages have been removed from the Windows Server 2025 image—review the official documentation if your workflow relies on specific tooling.
86127d5 to
e735828
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/windows-artifacts.yml (1)
33-34: Add non-interactive flags to prevent CI hangs.The
winget installcommand should use the--accept-package-agreementsflag when scripting to auto-accept agreements and avoid prompts. Add both--accept-source-agreementsand--accept-package-agreementsflags to ensure the workflow runs unattended:- name: Install DotNet.SDK.10 - run: winget install --id Microsoft.DotNet.SDK.10 --source winget + run: winget install --id Microsoft.DotNet.SDK.10 --source winget --accept-source-agreements --accept-package-agreements
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/windows-artifacts.yml(2 hunks)Makefile(1 hunks)packaging/windows/WixUI_en.wxl(0 hunks)packaging/windows/crc-installer.wixproj(1 hunks)packaging/windows/gen_msi_wxs.go(0 hunks)packaging/windows/product.wxs.template(0 hunks)
💤 Files with no reviewable changes (3)
- packaging/windows/WixUI_en.wxl
- packaging/windows/gen_msi_wxs.go
- packaging/windows/product.wxs.template
🚧 Files skipped from review as they are similar to previous changes (1)
- packaging/windows/crc-installer.wixproj
⏰ 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). (17)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-installer (windows-2025, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (windows-2022, 1.24)
🔇 Additional comments (2)
.github/workflows/windows-artifacts.yml (1)
24-24: No action needed—windows-2025is stable and generally available.The
windows-2025runner is well-established, having been generally available since April 2025 and monitored for stability improvements since then.Makefile (1)
374-374: LGTM!The dotnet build command correctly uses the WiX v6 .NET SDK approach, passing the version via DefineConstants and outputting to the expected build directory.
e735828 to
66f5c26
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packaging/windows/WixUI_HK.wxs (1)
64-64: Typo: "Suceesfully" should be "Successfully".This was already flagged in a previous review.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/windows-artifacts.yml(2 hunks)Makefile(1 hunks)packaging/.gitignore(0 hunks)packaging/windows/LicenseAgreementDlg_HK.wxs(2 hunks)packaging/windows/WixUI_HK.wxs(2 hunks)packaging/windows/WixUI_en.wxl(0 hunks)packaging/windows/crc-installer.wixproj(1 hunks)packaging/windows/gen_msi_wxs.go(0 hunks)packaging/windows/product.wxs(1 hunks)packaging/windows/product.wxs.template(0 hunks)
💤 Files with no reviewable changes (4)
- packaging/windows/gen_msi_wxs.go
- packaging/windows/WixUI_en.wxl
- packaging/.gitignore
- packaging/windows/product.wxs.template
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/windows-artifacts.yml
- Makefile
⏰ 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). (17)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-installer (windows-2025, 1.24)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (6)
packaging/windows/LicenseAgreementDlg_HK.wxs (1)
1-24: LGTM! Clean migration to WiX v4 schema.The namespace update and control attribute changes correctly follow WiX v4 patterns:
DisableCondition/EnableConditionon the Next button properly replaces legacyConditionelements- Self-closing
<Publish>tags with inline conditions are valid v4 syntax<Text SourceFile="..."/>nested inside the control is correctpackaging/windows/WixUI_HK.wxs (1)
1-59: LGTM! UI dialog flow and navigation logic look correct.The migration to WiX v4 schema is properly implemented:
- Dialog references and custom dialogs are correctly defined
- Conditional navigation using
CRCINSTALLEDwith substring matching (<<) handles upgrade detection appropriatelyPublishelements correctly wire up the dialog flow between Welcome → OldVersionDlg/LicenseAgreement → VerifyReadypackaging/windows/product.wxs (3)
8-8: Verify intentional use ofAllowDowngrades="yes".This setting permits installing older versions over newer ones without warning. While useful during development, it can cause issues in production if users accidentally downgrade and encounter state/data incompatibilities.
If this is intentional for upgrade flexibility, consider documenting the rationale.
74-74: Wildcard file removal may delete user-created files.
RemoveFile Id="RemoveInstallFiles" Name="*.*"will delete all files in the install directory on uninstall, including any files users may have created there (logs, configs, etc.).If this is intentional for clean uninstall, it's fine. Otherwise, consider removing only known installed files or using a more specific pattern.
1-109: Overall structure looks correct for the WiX v4 migration.The product definition properly implements:
- Version validation via preprocessor directive
- Launch conditions for Windows build and edition requirements
- Custom actions with correct deferred execution and elevated privileges
- Service installation with appropriate lifecycle controls
- User/group management using WiX Util extension
- Environment variable broadcasting for PATH changes
packaging/windows/crc-installer.wixproj (1)
1-19: Project configuration is correctly configured for WiX v6.WiX v6.0.0 was published 7-Apr-2025 as a stable release. The project properly matches SDK and extension versions.
SuppressIces>ICE61</SuppressIces>suppresses validation for files without MsiFileHash entries, which is acceptable if the embedded files don't require hash verification.- Extension versions (6.0.0) correctly match the SDK version.
- Consider updating to WiX v6.0.1 or later, which include bug fixes from subsequent maintenance releases.
fbb77ca to
3fcec14
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packaging/windows/WixUI_HK.wxs (1)
64-64: Typo: "Suceesfully" should be "Successfully".This typo was previously flagged and still needs to be addressed.
🔎 Apply this diff to fix the typo:
- <CustomAction Id="CA_Set_WIXUI_EXITDIALOGOPTIONALTEXT_NOREBOOT" Property="WIXUI_EXITDIALOGOPTIONALTEXT" Value="Suceesfully installed [ProductName]." /> + <CustomAction Id="CA_Set_WIXUI_EXITDIALOGOPTIONALTEXT_NOREBOOT" Property="WIXUI_EXITDIALOGOPTIONALTEXT" Value="Successfully installed [ProductName]." />
🧹 Nitpick comments (1)
Makefile (1)
380-380: Quote DestinationPath parameter for robustness.While the
Pathparameter is correctly quoted, theDestinationPathparameter should also be quoted to handle potential spaces in paths. AlthoughHOST_BUILD_DIRis unlikely to contain spaces in typical usage, quoting both paths follows PowerShell best practices.🔎 Apply this diff for consistent path quoting:
- pwsh -NoProfile -Command 'Compress-Archive -Path "$(MSI)" -DestinationPath $(HOST_BUILD_DIR)/crc-windows-installer.zip' + pwsh -NoProfile -Command 'Compress-Archive -Path "$(MSI)" -DestinationPath "$(HOST_BUILD_DIR)/crc-windows-installer.zip"'
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/windows-artifacts.yml(2 hunks)Makefile(1 hunks)packaging/.gitignore(0 hunks)packaging/windows/LicenseAgreementDlg_HK.wxs(2 hunks)packaging/windows/WixUI_HK.wxs(2 hunks)packaging/windows/WixUI_en.wxl(0 hunks)packaging/windows/crc-installer.wixproj(1 hunks)packaging/windows/gen_msi_wxs.go(0 hunks)packaging/windows/product.wxs(1 hunks)packaging/windows/product.wxs.template(0 hunks)
💤 Files with no reviewable changes (4)
- packaging/windows/product.wxs.template
- packaging/windows/gen_msi_wxs.go
- packaging/.gitignore
- packaging/windows/WixUI_en.wxl
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/windows-artifacts.yml
- packaging/windows/crc-installer.wixproj
⏰ 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). (11)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-installer (windows-2025, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (12)
Makefile (2)
369-381: Excellent migration to WiX v6 and dotnet-based build workflow.The transition from the legacy WiX candle/light toolchain to
dotnet buildwith a.wixprojfile is well-executed. The new workflow is cleaner, more maintainable, and aligns with modern .NET practices. The dependency chain (msidir→ MSI build → ZIP packaging) is logically structured, and the previous issues flagged in earlier reviews (redundantdotnet restore, quote nesting in PowerShell) have been properly addressed.
373-374: MSI build target configuration verified.The wixproj file exists at the expected path with correct configuration:
- SDK reference is
WixToolset.Sdk/6.0.0- Output filename is configured as
crc-windows-amd64, which will produce the expectedcrc-windows-amd64.msi- Extensions are properly referenced at version 6.0.0
The build command and output path in the Makefile align correctly with the wixproj configuration.
packaging/windows/WixUI_HK.wxs (2)
1-1: Namespace migration to WiX v4 looks correct.The namespace update to
http://wixtoolset.org/schemas/v4/wxsaligns with the WiX v4+ toolset migration.
41-58: Dialog publish flow updates look correct for WiX v4.The migration from inline content to
Valueattributes and the addition ofConditionattributes for dialog navigation is consistent with WiX v4 syntax requirements.packaging/windows/product.wxs (5)
37-42: Custom action pattern correctly uses WiX v4 conventions.The implementation properly uses
DllEntry="WixQuietExec"withBinaryRef="Wix4UtilCA_$(sys.BUILDARCHSHORT)"for platform-matched custom actions, which is the correct WiX v4 pattern. This addresses the concern from the previous review about deprecatedWixQuietExec64.
44-49: Verify custom action ordering and conditions.The
InstallExecuteSequencelogic appears correct:
InstallHypervruns only on fresh install (not upgrade)RemoveCrcDaemonTaskruns on uninstall (not during upgrade)RemoveSMBShareruns during upgrade- Reboot scheduled on fresh install
However, ensure the
ScheduleRebootcondition correctly handles all edge cases.
93-105: User and group management looks correct.The
util:Groupandutil:Userelements properly:
- Create the
crc-usersgroup with cleanup on uninstall- Add the current user to both
crc-usersandHyper-V Administratorsgroups- Use
CreateUser="no"to reference existing users rather than creating new ones
70-75: Consider implications ofRemoveFilewith wildcard on uninstall.The
RemoveFileelement withName="*.*"will remove all files in theINSTALLDIRon uninstall, including any user-generated files. Ensure this is the intended behavior and that important user data isn't stored in this directory.
8-8: ReconsiderAllowDowngrades="yes"or document the rationale.The Windows Installer team has issued a warning against allowing downgrades, as they consider it bad practice. Allowing downgrades can result in mixed file versions that were not designed to work together.
If downgrades are intentionally supported for this product (e.g., for development workflows or emergency rollbacks), add a comment explaining the decision. Otherwise, remove
AllowDowngrades="yes"and rely on the standard upgrade behavior that blocks downgrades.Note:
AllowSameVersionUpgradescannot be used withAllowDowngrades—they are mutually exclusive attributes per WiX specifications.Likely an incorrect or invalid review comment.
packaging/windows/LicenseAgreementDlg_HK.wxs (3)
1-1: Namespace migration to WiX v4 is correct.
7-9: Next button enablement logic is correctly implemented.The
DisableConditionandEnableConditionattributes properly gate the Next button based on license acceptance, and theSpawnWaitDialogpublish correctly waits for costing to complete before proceeding.
14-16: License text sourcing updated to WiX v4 syntax.The nested
<Text SourceFile="..." />element is the correct WiX v4 pattern for embedding RTF content in a scrollable text control.
3fcec14 to
68d2bcf
Compare
68d2bcf to
8fd3d7d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packaging/windows/product.wxs (1)
16-17: The MINIMUMBUILD value must match the documented Windows version requirement.OpenShift Local requires Windows 10 Fall Creators Update (version 1709) or later, which corresponds to build 10.0.16299. However, the code sets
MINIMUMBUILDto19041(version 2004), which directly contradicts the error message stating "Fall Creators Update (version 1709)".This mismatch will incorrectly reject valid installations on Windows 10 builds 16299–19040. Update the build number to match the documented requirement:
- <Property Id="MINIMUMBUILD" Value="19041" Secure="yes"></Property> + <Property Id="MINIMUMBUILD" Value="16299" Secure="yes"></Property>If version 2004 is truly required due to Hyper-V or other dependencies, update the error message instead to reflect the actual requirement.
🧹 Nitpick comments (1)
Makefile (1)
380-380: Add quotes around DestinationPath for robustness.The
-DestinationPathparameter should be quoted to handle potential spaces in$(HOST_BUILD_DIR). While this is unlikely in practice, adding quotes improves robustness and aligns with the quoting already applied to the-Pathparameter.🔎 Proposed fix
- pwsh -NoProfile -Command 'Compress-Archive -Path "$(MSI)" -DestinationPath $(HOST_BUILD_DIR)/crc-windows-installer.zip' + pwsh -NoProfile -Command 'Compress-Archive -Path "$(MSI)" -DestinationPath "$(HOST_BUILD_DIR)/crc-windows-installer.zip"'
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/windows-artifacts.yml(2 hunks)Makefile(1 hunks)packaging/.gitignore(0 hunks)packaging/windows/LicenseAgreementDlg_HK.wxs(2 hunks)packaging/windows/WixUI_HK.wxs(2 hunks)packaging/windows/WixUI_en.wxl(0 hunks)packaging/windows/crc-installer.wixproj(1 hunks)packaging/windows/gen_msi_wxs.go(0 hunks)packaging/windows/product.wxs(1 hunks)packaging/windows/product.wxs.template(0 hunks)
💤 Files with no reviewable changes (4)
- packaging/windows/product.wxs.template
- packaging/.gitignore
- packaging/windows/WixUI_en.wxl
- packaging/windows/gen_msi_wxs.go
🚧 Files skipped from review as they are similar to previous changes (2)
- packaging/windows/LicenseAgreementDlg_HK.wxs
- packaging/windows/crc-installer.wixproj
⏰ 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). (18)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build-installer (windows-2025, 1.24)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (3)
packaging/windows/WixUI_HK.wxs (1)
1-71: LGTM! WiX v4 migration is well-executed.The schema migration from WiX v3 to v4 is correct, with proper namespace updates, attribute-based Publish actions, and conditional dialog flow logic. The XML entity escaping and dialog navigation are appropriate for the new schema.
packaging/windows/product.wxs (1)
38-42: LGTM! WiX v4 custom action references are correct.The custom actions properly use
DllEntry="WixQuietExec"with the platform-matchedBinaryRef="Wix4UtilCA_$(sys.BUILDARCHSHORT)"pattern, which is the correct WiX v4 approach for quiet execution custom actions..github/workflows/windows-artifacts.yml (1)
33-34:and
8fd3d7d to
74557a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/windows-artifacts.yml (1)
33-34: Add non-interactive flags to prevent CI hangs.The
winget installcommand may prompt for source and package agreements, which will hang the CI workflow. Add--accept-source-agreementsand--accept-package-agreementsflags to ensure non-interactive execution.🔎 Proposed fix
- name: Install DotNet.SDK.10 - run: winget install --id Microsoft.DotNet.SDK.10 --source winget + run: winget install --id Microsoft.DotNet.SDK.10 --source winget --accept-source-agreements --accept-package-agreements
🧹 Nitpick comments (1)
Makefile (1)
369-371: Consider adding defensive checks for robustness.While
embed-download-windowsdependency should populate$(EMBED_DOWNLOAD_DIR), the wildcard copy could fail if the directory is unexpectedly empty. Additionally, ensure$(PACKAGE_DIR)exists before copying.🔎 Optional: Add defensive checks
msidir: clean_windows_msi embed-download-windows windows-release-binary + @mkdir -p $(PACKAGE_DIR) + @test -d $(EMBED_DOWNLOAD_DIR) && test -n "$$(ls -A $(EMBED_DOWNLOAD_DIR) 2>/dev/null)" || { echo "Error: $(EMBED_DOWNLOAD_DIR) is empty or missing"; exit 1; } cp $(EMBED_DOWNLOAD_DIR)/* $(PACKAGE_DIR)/ cp $(HOST_BUILD_DIR)/crc.exe $(PACKAGE_DIR)/
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/windows-artifacts.yml(2 hunks)Makefile(1 hunks)packaging/.gitignore(0 hunks)packaging/windows/LicenseAgreementDlg_HK.wxs(2 hunks)packaging/windows/WixUI_HK.wxs(2 hunks)packaging/windows/WixUI_en.wxl(0 hunks)packaging/windows/crc-installer.wixproj(1 hunks)packaging/windows/gen_msi_wxs.go(0 hunks)packaging/windows/product.wxs(1 hunks)packaging/windows/product.wxs.template(0 hunks)
💤 Files with no reviewable changes (4)
- packaging/.gitignore
- packaging/windows/product.wxs.template
- packaging/windows/gen_msi_wxs.go
- packaging/windows/WixUI_en.wxl
🚧 Files skipped from review as they are similar to previous changes (1)
- packaging/windows/crc-installer.wixproj
⏰ 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). (17)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build-installer (windows-2025, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (6)
packaging/windows/LicenseAgreementDlg_HK.wxs (1)
1-25: LGTM! WiX v4 migration correctly applied.The migration to WiX v4 schema is properly implemented:
- Correct namespace update to
http://wixtoolset.org/schemas/v4/wxs- Proper use of
EnableCondition/DisableConditionattributes for flow control- Publish events correctly refactored with
Valueattributes- Text element properly uses
SourceFileattributepackaging/windows/product.wxs (2)
37-42: LGTM! CustomActions properly migrated to WiX v4.The custom actions correctly use:
DllEntry="WixQuietExec"(WiX v4 pattern)BinaryRef="Wix4UtilCA_$(sys.BUILDARCHSHORT)"for platform-appropriate CA selection- Proper
SetPropertyactions to prepare command linesThis follows WiX v4 best practices for quiet execution custom actions.
71-113: LGTM! Component structure is well-organized.The component definitions properly handle:
- File installation with appropriate KeyPath settings
- Service installation and control for the admin helper
- PATH environment variable modification
- Registry entries for Hyper-V guest communication services
- User/group management for proper permissions
The mix of explicit GUIDs and auto-generated GUIDs (
Guid="*") is appropriate for the respective component types.packaging/windows/WixUI_HK.wxs (1)
1-71: LGTM! WiX v4 UI migration successfully completed.The UI definition correctly implements WiX v4 patterns:
- Proper namespace migration to v4 schema
- Dialog navigation using
Publishelements withEvent/Valueattributes- Conditional routing based on
CRCINSTALLEDandLicenseAcceptedproperties- Custom actions properly defined and sequenced in
InstallUISequence- Dialog references correctly included
The custom OldVersionDlg for handling upgrades integrates well with the overall flow.
Makefile (2)
373-374: LGTM! .NET-based MSI build properly configured.The transition from WiX toolchain to
dotnet buildis correctly implemented:
- Uses the new
crc-installer.wixprojproject file- Passes version via
DefineConstantsproperty (required by product.wxs)- Specifies output directory appropriately
- No redundant
dotnet restore(implicitly handled by build)
376-381: LGTM! PowerShell quoting correctly handles path with spaces.The MSI variable and PowerShell command are properly structured:
MSIvariable defined without surrounding quotes (avoids nesting issues)- PowerShell
-Commanduses single quotes to contain the command string- File paths within the command use double quotes for proper PowerShell handling
- SHA256 checksum generated alongside the ZIP file
This pattern correctly avoids the shell quote nesting problems that would occur with double quotes throughout.
| <Property Id="MINIMUMBUILD" Value="19041" Secure="yes"></Property> | ||
| <Launch Condition="Installed OR (CURRENTBUILD > MINIMUMBUILD)" Message="Red Hat OpenShift Local requires the Windows 10 Fall Creators Update (version 1709) or newer." /> |
There was a problem hiding this comment.
Inconsistency between MINIMUMBUILD value and error message.
The MINIMUMBUILD is set to 19041 (Windows 10 version 2004/May 2020 Update), but the Launch condition message states "Windows 10 Fall Creators Update (version 1709)". Build number 19041 corresponds to version 2004, not 1709.
Either update the message to reflect the actual minimum version requirement, or if Windows 10 1709 is the actual requirement, change MINIMUMBUILD to 16299.
🔎 Proposed fix (if 2004 is the actual requirement)
<Property Id="MINIMUMBUILD" Value="19041" Secure="yes"></Property>
- <Launch Condition="Installed OR (CURRENTBUILD > MINIMUMBUILD)" Message="Red Hat OpenShift Local requires the Windows 10 Fall Creators Update (version 1709) or newer." />
+ <Launch Condition="Installed OR (CURRENTBUILD > MINIMUMBUILD)" Message="Red Hat OpenShift Local requires Windows 10 version 2004 (May 2020 Update) or newer." />🔎 Alternative fix (if 1709 is the actual requirement)
- <Property Id="MINIMUMBUILD" Value="19041" Secure="yes"></Property>
+ <Property Id="MINIMUMBUILD" Value="16299" Secure="yes"></Property>
<Launch Condition="Installed OR (CURRENTBUILD > MINIMUMBUILD)" Message="Red Hat OpenShift Local requires the Windows 10 Fall Creators Update (version 1709) or newer." />📝 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.
| <Property Id="MINIMUMBUILD" Value="19041" Secure="yes"></Property> | |
| <Launch Condition="Installed OR (CURRENTBUILD > MINIMUMBUILD)" Message="Red Hat OpenShift Local requires the Windows 10 Fall Creators Update (version 1709) or newer." /> | |
| <Property Id="MINIMUMBUILD" Value="19041" Secure="yes"></Property> | |
| <Launch Condition="Installed OR (CURRENTBUILD > MINIMUMBUILD)" Message="Red Hat OpenShift Local requires Windows 10 version 2004 (May 2020 Update) or newer." /> |
| <Property Id="MINIMUMBUILD" Value="19041" Secure="yes"></Property> | |
| <Launch Condition="Installed OR (CURRENTBUILD > MINIMUMBUILD)" Message="Red Hat OpenShift Local requires the Windows 10 Fall Creators Update (version 1709) or newer." /> | |
| <Property Id="MINIMUMBUILD" Value="16299" Secure="yes"></Property> | |
| <Launch Condition="Installed OR (CURRENTBUILD > MINIMUMBUILD)" Message="Red Hat OpenShift Local requires the Windows 10 Fall Creators Update (version 1709) or newer." /> |
🤖 Prompt for AI Agents
In packaging/windows/product.wxs around lines 16-17, the MINIMUMBUILD is set to
19041 but the Launch message says "Windows 10 Fall Creators Update (version
1709)", creating a mismatch; fix by either (A) updating the Message text to
state the correct version for build 19041 (e.g., "Windows 10 version 2004 (build
19041) or newer") if 19041 is the intended minimum, or (B) changing the
MINIMUMBUILD value to 16299 if the actual requirement is version 1709 — pick the
correct requirement and make the corresponding single-line edit so the build
number and message match.
74557a5 to
6f22c68
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/windows-artifacts.yml(2 hunks)Makefile(1 hunks)packaging/.gitignore(0 hunks)packaging/windows/LicenseAgreementDlg_HK.wxs(2 hunks)packaging/windows/WixUI_HK.wxs(2 hunks)packaging/windows/WixUI_en.wxl(0 hunks)packaging/windows/crc-installer.wixproj(1 hunks)packaging/windows/gen_msi_wxs.go(0 hunks)packaging/windows/product.wxs(1 hunks)packaging/windows/product.wxs.template(0 hunks)
💤 Files with no reviewable changes (4)
- packaging/windows/product.wxs.template
- packaging/.gitignore
- packaging/windows/WixUI_en.wxl
- packaging/windows/gen_msi_wxs.go
🚧 Files skipped from review as they are similar to previous changes (3)
- packaging/windows/product.wxs
- .github/workflows/windows-artifacts.yml
- Makefile
⏰ 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). (17)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: verify-devcontainer
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: build-installer (windows-2025, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (7)
packaging/windows/WixUI_HK.wxs (4)
1-1: Verify schema version compatibility with WiX v5.This file also uses the WiX v4 schema, consistent with
LicenseAgreementDlg_HK.wxs. Confirm whether v4 schemas are supported in WiX v5 or if an update to v5 schema is needed.(Same verification as in LicenseAgreementDlg_HK.wxs - confirming v4/v5 schema compatibility)
29-32: LGTM: OldVersionDlg text migration.The migration of text content from inline to a Value attribute is consistent with WiX v4/v5 patterns. The dialog message clearly communicates the upgrade scenario.
51-52: LGTM: Proper conditional Back navigation.The VerifyReadyDlg Back button correctly routes to different dialogs based on installation state—LicenseAgreementDlg_HK for new installations and MaintenanceTypeDlg for existing installations. This follows proper installer UX patterns.
43-44: The<<operator in the condition is valid WiX/MSI syntax. It checks if the left string starts with the right string (e.g.,CRCINSTALLED << "#1"evaluates to true if CRCINSTALLED begins with "#1"). No changes needed.Likely an incorrect or invalid review comment.
packaging/windows/LicenseAgreementDlg_HK.wxs (2)
1-1: No action needed—the v4 schema is correct for WiX v5.WiX v5 uses the same XML namespace as WiX v4 and is backward compatible with the WiX v4 language, so you don't need to translate your WiX v4 projects to use WiX v5. The xmlns declaration is intentional and standard.
14-16: No action needed. The LICENSE.rtf file exists at the expected location and the path resolves correctly.packaging/windows/crc-installer.wixproj (1)
1-7: Document the rationale for suppressing ICE61.The project suppresses ICE61 validation. ICE61 validates the Upgrade table of a Windows Installer database, which relates to product upgrade and downgrade scenarios. Add a comment explaining why this suppression is necessary for the installer configuration.
fedc1fd to
d07663d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Makefile (1)
369-371: Consider adding defensive checks for the wildcard copy.The wildcard expansion
$(EMBED_DOWNLOAD_DIR)/*will causecpto fail if the directory is empty or doesn't exist. Whileembed-download-windowsis a dependency that should populate this directory, adding explicit error handling would make the build more robust.🔎 Optional: Add defensive check
msidir: clean_windows_msi embed-download-windows windows-release-binary + @test -d $(EMBED_DOWNLOAD_DIR) && test -n "$$(ls -A $(EMBED_DOWNLOAD_DIR) 2>/dev/null)" || { echo "Error: $(EMBED_DOWNLOAD_DIR) is missing or empty"; exit 1; } cp $(EMBED_DOWNLOAD_DIR)/* $(PACKAGE_DIR)/ cp $(HOST_BUILD_DIR)/crc.exe $(PACKAGE_DIR)/packaging/windows/product.wxs (1)
17-18: MINIMUMBUILD value and error message are inconsistent.The
MINIMUMBUILDis set to19041(Windows 10 version 2004/May 2020 Update), but the Launch condition message states "Windows 10 Fall Creators Update (version 1709)". Build number 19041 corresponds to version 2004, not 1709.Either update the message to reflect the actual minimum version requirement (2004), or if Windows 10 1709 is the actual requirement, change
MINIMUMBUILDto16299.
🧹 Nitpick comments (1)
packaging/windows/WixUI_HK.wxs (1)
63-67: Minor grammar improvement for exit dialog message.Consider adding a comma for better readability.
🔎 Proposed fix
- <CustomAction Id="CA_Set_WIXUI_EXITDIALOGOPTIONALTEXT" Property="WIXUI_EXITDIALOGOPTIONALTEXT" Value="To finish installation of [ProductName] please restart your computer." /> + <CustomAction Id="CA_Set_WIXUI_EXITDIALOGOPTIONALTEXT" Property="WIXUI_EXITDIALOGOPTIONALTEXT" Value="To finish installation of [ProductName], please restart your computer." />
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/windows-artifacts.yml(2 hunks)Makefile(1 hunks)packaging/.gitignore(0 hunks)packaging/windows/LicenseAgreementDlg_HK.wxs(2 hunks)packaging/windows/WixUI_HK.wxs(2 hunks)packaging/windows/WixUI_en.wxl(0 hunks)packaging/windows/crc-installer.wixproj(1 hunks)packaging/windows/gen_msi_wxs.go(0 hunks)packaging/windows/product.wxs(1 hunks)packaging/windows/product.wxs.template(0 hunks)
💤 Files with no reviewable changes (4)
- packaging/windows/WixUI_en.wxl
- packaging/windows/gen_msi_wxs.go
- packaging/windows/product.wxs.template
- packaging/.gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/windows-artifacts.yml
- packaging/windows/crc-installer.wixproj
⏰ 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). (17)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build (ubuntu-22.04, 1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: build-qe (linux, amd64)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: build-qe (windows, amd64)
- GitHub Check: build-qe (darwin, amd64)
- GitHub Check: build-qe (darwin, arm64)
- GitHub Check: build (macOS-15, 1.24)
- GitHub Check: build-qe (linux, arm64)
- GitHub Check: build-installer (windows-2025, 1.24)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (ubuntu-latest, 1.24)
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (9)
packaging/windows/product.wxs (3)
38-55: LGTM! Custom actions properly use WiX v4 patterns.The
WixQuietExecwithBinaryRef="Wix4UtilCA_$(sys.BUILDARCHSHORT)"correctly follows WiX v4 conventions. UsingReturn="ignore"is appropriate for these optional system configuration actions.
56-64: Install sequence logic is well-structured.The conditions correctly handle fresh install, uninstall, and upgrade scenarios. The rollback action is properly scheduled, and the reboot is appropriately limited to fresh installations where Hyper-V may require it.
116-125: Verifyutil:Userdomain attribute behavior in WiX v4.The
Domain="[%USERDOMAIN]"syntax uses environment variable expansion at install time. Ensure this resolves correctly in all deployment scenarios (domain-joined vs. workgroup machines). On workgroup machines,USERDOMAINtypically equals the computer name, which should work correctly with local user operations.packaging/windows/WixUI_HK.wxs (2)
1-1: LGTM! Schema correctly updated to WiX v4.The namespace update to
http://wixtoolset.org/schemas/v4/wxsis correct for WiX v4/v5 compatibility.
43-52: Dialog navigation flow is well-structured.The condition-based navigation between WelcomeDlg, OldVersionDlg, LicenseAgreementDlg_HK, and VerifyReadyDlg correctly handles both fresh install and maintenance scenarios using WiX v4
Valueattribute syntax.packaging/windows/LicenseAgreementDlg_HK.wxs (2)
7-8: LGTM! Next button correctly uses WiX v4 enable/disable conditions.The
DisableConditionandEnableConditionattributes properly gate the Next button based on license acceptance, and theSpawnWaitDialogensures costing completes before navigation.
14-16: LGTM! License text control updated for WiX v4.The
Textchild element withSourceFileattribute is the correct WiX v4 syntax for embedding the license file.Makefile (2)
374-374: LGTM! MSI build correctly uses dotnet with version parameter.The
dotnet buildcommand with--property:DefineConstantsproperly passes the version to the WiX project, aligning with the WiX v4/v5 SDK-style build approach.
376-380: Quote nesting issue resolved.The MSI variable is now unquoted and the PowerShell command uses single quotes with proper inner quoting, addressing the previous shell parsing concerns.
this updates the wix scripts for the msi to use wix v5.0.2 as v3 is deprecated
in wix v5 the candle.exe, light.exe tool were removed and there functionality is now part of the wix cli this removes the golang code to inject the version into the wix script and uses dotnet to run the wix tool, this allows to set the version using properties for a dotnet project
we donot have translations for the installer and only building it for en-US
we need a DotNet environment to build the MSI the simplest way to get it installed is using winget which is pre-installed in windows-2025 runner image
d07663d to
e321314
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/windows-artifacts.yml (1)
33-34: Add non-interactive flags to prevent CI workflow hangs.The
winget installcommand is missing the--accept-source-agreementsand--accept-package-agreementsflags. Without these flags, winget may prompt for interactive confirmation in the GitHub Actions environment, causing the workflow to hang or fail.- name: Install DotNet.SDK.10 - run: winget install --id Microsoft.DotNet.SDK.10 --source winget + run: winget install --id Microsoft.DotNet.SDK.10 --source winget --accept-source-agreements --accept-package-agreementspackaging/windows/product.wxs (1)
17-18: Inconsistency between MINIMUMBUILD value and error message remains unresolved.The
MINIMUMBUILDis set to19041(Windows 10 version 2004, May 2020 Update), but the Launch condition message states "Windows 10 Fall Creators Update (version 1709)" which corresponds to build16299. This mismatch was flagged in previous reviews but is still present.
🧹 Nitpick comments (1)
packaging/windows/product.wxs (1)
38-42: Consider simplifying group management with WiX util extensions.The installer uses both WiX
util:Groupelements (Lines 86-87) and custom actions withnet localgroupcommands to manage thecrc-usersgroup. Theutil:Groupextension in WiX should handle group creation automatically when users are added viautil:GroupRef.The explicit custom actions with
Return="ignore"may provide better error handling, but consider whether the built-in WiX group management is sufficient to reduce maintenance complexity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.github/workflows/windows-artifacts.yml(2 hunks)Makefile(1 hunks)packaging/.gitignore(0 hunks)packaging/windows/LicenseAgreementDlg_HK.wxs(2 hunks)packaging/windows/WixUI_HK.wxs(2 hunks)packaging/windows/WixUI_en.wxl(0 hunks)packaging/windows/crc-installer.wixproj(1 hunks)packaging/windows/gen_msi_wxs.go(0 hunks)packaging/windows/product.wxs(1 hunks)packaging/windows/product.wxs.template(0 hunks)
💤 Files with no reviewable changes (4)
- packaging/.gitignore
- packaging/windows/WixUI_en.wxl
- packaging/windows/gen_msi_wxs.go
- packaging/windows/product.wxs.template
🚧 Files skipped from review as they are similar to previous changes (3)
- packaging/windows/WixUI_HK.wxs
- Makefile
- packaging/windows/crc-installer.wixproj
⏰ 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). (6)
- GitHub Check: build (windows-2022, 1.24)
- GitHub Check: build (macOS-26, 1.24)
- GitHub Check: build (macOS-15-intel, 1.24)
- GitHub Check: Run OKD bundle with crc (1.24)
- GitHub Check: verify-devcontainer
- GitHub Check: Konflux kflux-prd-rh02 / crc-binary-on-pull-request
🔇 Additional comments (6)
.github/workflows/windows-artifacts.yml (1)
24-24: Ensure the workflow is compatible with Windows Server 2025 breaking changes.The
windows-2025runner is now generally available. However, verify that your workflow doesn't depend on the D: drive, as the Windows Server 2025 image only includes the C: drive.packaging/windows/LicenseAgreementDlg_HK.wxs (3)
1-1: LGTM!The namespace update to WiX v4 schema (
http://wixtoolset.org/schemas/v4/wxs) is correct and required for the WiX v5 migration.
7-9: LGTM!The migration to WiX v4 attribute-based conditions (
DisableConditionandEnableCondition) is correct. The button is properly disabled until the license is accepted, and the costing dialog is spawned only after cost calculation completes.
11-11: LGTM!The self-closing
Publishelement (Line 11) and theTextelement as a child (Line 15) are both correct WiX v4 syntax patterns. These changes maintain the same functionality while conforming to the updated schema.Also applies to: 15-15
packaging/windows/product.wxs (2)
60-61: Confirm whether SMB share should be removed during uninstall.The
RemoveCrcDaemonTaskcustom action removes the daemon task during all uninstalls, including manual uninstalls via Programs and Features (line 11:Installed AND NOT UPGRADINGPRODUCTCODE). However,RemoveSMBShareonly removes the share during upgrade scenarios (line 12:Installed AND UPGRADINGPRODUCTCODE), not during manual uninstalls.This means the SMB share persists on the system after a user manually uninstalls the application. Verify whether this is intentional (e.g., the share is meant to be preserved) or if the condition should match the daemon task cleanup to ensure consistent resource removal on all uninstall scenarios.
102-105: The explicit GUID for theAddToPathcomponent is required, not optional.The component contains
<CreateFolder />and<Environment />elements, neither of which qualify as valid key path elements for auto-generated GUIDs. Generatable GUIDs are supported only for components with a single file as the component's keypath or no files and a registry value as the keypath. This is a technical constraint, not a backward compatibility preference. By contrast, theHvsock9pRegistryEntrycomponent can useGuid="*"because it has aRegistryValuewithKeyPath="yes".Likely an incorrect or invalid review comment.
|
@anjannath: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Summary by CodeRabbit
Chores
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.