-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Using Visual Studio BuildTools as a MinGW alternative #23212
Conversation
We were not able to find or create Copr project
Please check your configuration for:
|
/packit build-failed |
$wslCheckboxVar = $($installWSL ? "1" : "0") | ||
$hypervCheckboxVar = $($installHyperV ? "1" : "0") | ||
$allowOldWinVar = $($skipWinVersionCheck ? "1" : "0") |
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.
@l0rd these 3 lines seem to be failing tests.
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.
@lsm5 thank you. I suspect that's related to the version of PowerShell that's used by CI (< v7.0) that doesn't support the ternary operator syntax I am going to put this PR back in draft mode until I fix it.
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.
are you planning to update the cirrus windows image with latest powershell? Looks like github actions windows env does have required min-version of powershell
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.
This is an option, yes. But no, I think it's simpler to update the PowerShell script introduced with this PR to support old and new versions of PWSH. So that it has a wider compatibility of windows dev environments.
Building the MSI hook on Windows (`contrib/win-installer/podman-msihooks/check.c`) currently requires MinGW. This commit updates the build script so that, when MinGW is absent but the C compiler included in Visual Studio BuildTools is installed, the latter is used to build the MSI hook. Other than that, `winmake.ps1` has a new `installertest` target to run the Windows installer tests that are currently verified by Cirrus CI. Signed-off-by: Mario Loriedo <[email protected]>
Tests are passing. Re-opening for review. @containers/podman-maintainers PTAL. |
LGTM, though my unfamiliarity with Powershell makes me less able to find issues than normal |
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.
I removed mingw tools from my windows env and installed the VSSetup extension as mentioned in the PR. Was able to build the installer just fine.
I won't be able to review the powershell details, but I can say it works for me :) . Setting a hold in case anyone else has the time/expertise to review it.
@n1hility PTAL
/lgtm
/approve
/hold
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: l0rd, lsm5 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
Building the MSI hook on Windows currently requires MinGW CC. This commit updates the build script (build-hooks.bat is now build-hooks.ps1) so that, when MinGW is absent but the C compiler included in Visual Studio BuildTools is installed, the latter is used to build the MSI hook.
Documentation in
build_windows.md
has been updated with the instructions to install VS BuildTools (not MSYS2/MinGW).Another change included in the PR is the
installertest
target in windows Makefile (winmake.ps1
) to run the Windows installer tests that are part of Cirrus CI.Does this PR introduce a user-facing change?