-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat(release): add windows msi installers #4031
Conversation
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.
It looks to me like icon.ico
is transparent. Is that intentional?
Left a couple of comments, but nothing blocking. LGTM! 👍
@@ -93,13 +98,28 @@ jobs: | |||
profile: minimal | |||
target: ${{ matrix.target }} | |||
|
|||
- name: Setup | Install cargo-wix [Windows] |
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.
A passing thought — not something we have to address in this PR:
Our release-time build process is getting pretty complex with building installers and notarizing, which leads to a lot of unexpected failures at the time of release.
For this, in particular, I think there'd be value in adding this to CI. Maybe not on every commit, like the rest of our suite, but perhaps on every push to master
.
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.
Done.
- name: Setup | Install cargo-wix [Windows] | ||
# aarch64 is only supported in wix 4.0 development builds | ||
if: matrix.os == 'windows-latest' && matrix.target != 'aarch64-pc-windows-msvc' | ||
run: cargo install --version 0.3.2 cargo-wix |
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.
It's unfortunate that this, being a hard-coded version, won't be updated by Dependabot.
I wish we could have cargo subcommands be retrieved from our dev-dependencies. That's not possible though, right?
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 don't think that's possible.
|
@davidkna It looks like the icon svg in the repo is transparent. That's probably my fault 😅 |
@davidkna Done! Feel free to try using the updated |
Description
This PR adds the required configurations files and changes to the release workflow to publish MSI installers. Windows aarch64 builds don't get installers yet, because they're not yet supported by wix.
As there's no easy way to include MSVC-redist in the installer, this PR also changes the Windows builds to use static linking. This is the approach recommended by cargo-wix.
Motivation and Context
Required for #1295
Related #690
Screenshots (if appropriate):
How Has This Been Tested?
Checklist: