Skip to content
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

Build arm64 MSI packages additionally to the current amd64 ones #482

Merged
merged 24 commits into from
Oct 24, 2024

Conversation

philipbalinov
Copy link
Contributor

@philipbalinov philipbalinov commented Oct 16, 2024

  • remove hardcoded amd64 string everywhere

  • parametrize builds to run for both amd64 and arm64

  • Successfully tested against local windows 10 arm64 VM

Caveats

@philipbalinov philipbalinov marked this pull request as ready for review October 17, 2024 08:00
@philipbalinov philipbalinov changed the title Initial commit for adding support for arm64 MSI packages Build arm64 MSI packages additionally to the current amd64 ones Oct 18, 2024
Copy link
Contributor

@afiune afiune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@@ -53,12 +53,17 @@ jobs:
echo "trimmed_version=$(echo ${V} | sed 's/-.*//')" >> $GITHUB_OUTPUT
- name: Ensure version of cnquery and cnspec are available
run: |
curl -sL --head --fail https://github.com/mondoohq/cnquery/releases/download/v${{ steps.version.outputs.version }}/cnquery_${{ steps.version.outputs.version }}_windows_amd64.zip
curl -sL --head --fail https://github.com/mondoohq/cnspec/releases/download/v${{ steps.version.outputs.version }}/cnspec_${{ steps.version.outputs.version }}_windows_amd64.zip
curl -sL --head --fail https://github.com/mondoohq/cnquery/releases/download/v11.26.0/cnquery_11.26.0_windows_amd64.zip \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do I feel this command went from being parameterized to be hardcoded? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops! fixed 👍

<?define RegistrationTokenRequired = "0"?>
<?define OtherSKU = "$(var.UpgradeCodeEnterprise)"?>
<?else?>
<?define InstallerVersion="200"?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for my own learning, what are these InstallerVersion for and, why these numbers 200 and 500?¿

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apparently - I'm not expert mysel - that is the minimum MS Installer version, these are the recommendations for x64 and arm as per https://wixtoolset.org/docs/v3/xsd/wix/package/
In reality, 200 seems to be Windows 2000-era, and 500 - Windows 7, so these numbers are super conservative.

cd msi
# delete previous intermediate files
Remove-Item .\Product.wixobj -ErrorAction Ignore
Remove-Item .\mondoo.wixpdb -ErrorAction Ignore
# build package
dir 'C:\Program Files (x86)\'
info "run candle (standard)"
& 'C:\Program Files (x86)\WiX Toolset v3.14\bin\candle' -nologo -arch x64 -dMondooSKU="standard" -dProductVersion="$version" -ext WixUtilExtension Product.wxs
& 'C:\Program Files (x86)\WiX Toolset v3.14\bin\candle' -nologo -dMondooSKU="standard" -darch="$platform" -dProductVersion="$version" -ext WixUtilExtension Product.wxs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit extra whitespace

Suggested change
& 'C:\Program Files (x86)\WiX Toolset v3.14\bin\candle' -nologo -dMondooSKU="standard" -darch="$platform" -dProductVersion="$version" -ext WixUtilExtension Product.wxs
& 'C:\Program Files (x86)\WiX Toolset v3.14\bin\candle' -nologo -dMondooSKU="standard" -darch="$platform" -dProductVersion="$version" -ext WixUtilExtension Product.wxs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

<Wix xmlns="http://schemas.microsoft.com/wix/2006/wi"
xmlns:util="http://schemas.microsoft.com/wix/UtilExtension">
<Product Name="$(var.ProductName)" Version="$(var.ProductVersion)" Manufacturer="Mondoo, Inc." Language="1033" Codepage="1252" Id="*" UpgradeCode="$(var.UpgradeCode)">
<!-- custom action do not work if its not privileged-->
<Package
Description="Mondoo verifies your system for known vulnerabilities"
Manufacturer="Mondoo, Inc."
InstallerVersion="200"
InstallerVersion="$(var.InstallerVersion)"
Compressed="yes"
Comments="Windows Installer Package"
Platform="x64"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't there be an arm section too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the Platform attribute entirely - it's considered deprecated and is overriden by passing -arch to candle.exe anyway

@@ -16,22 +19,24 @@ function info($msg) { Write-Host $msg -f white }

info "build msi package $version"
# delete previous build
Remove-Item .\mondoo.msi -ErrorAction Ignore
Remove-Item ".\mondoo_${arch}.msi" -ErrorAction Ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the intend is to delete previous builds, for x64, should we try to remove the previous location too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kept both lines just in case, although in reality this runs in CI against an empty directory each time, so none of the files will ever be there

- Try to remove old installer filename `mondoo.msi' as well
- Remove some extra whitespace
Copy link
Contributor

@afiune afiune left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a windows machine to test with, but everything looks good to me.

Copy link
Member

@benr benr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fully tested cnquery installed by MSI on an Azure ARM Windows 11 Enterprise instance. Worked perfectly. Binaries are properly signed.

image

Great job @philipbalinov !!!

@benr benr merged commit ee25760 into main Oct 24, 2024
9 checks passed
@benr benr deleted the philip/arm64-msi-installer branch October 24, 2024 15:56
@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants