-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
msi: migrate to WiX4 #45943
msi: migrate to WiX4 #45943
Conversation
@nodejs/platform-windows |
@StefanStojanovic We can help with testing on this. Do you have an installer to share (built from that PR) that would could try? |
I made a release with the installer on JaneaSystems/node fork here. Anyone from @nodejs/platform-windows-arm, feel free to chip in. |
I tried your installer on a Windows on Arm machine, and it works great. Node is added to PATH, and is accessible from any prompt. Great work 👍 |
An update on WiX v4: release candidate 2 was released on January 20th. I've tried it and it generated the same MSI file as release candidate 1. Since there are no notable changes for the Node.js installer, this PR will keep relying on the RC1. |
5474db7
to
17cdadc
Compare
This is required to migrate from WiX3 to WiX4 for building the Node msi. Refs: nodejs/node#45943
17cdadc
to
e3abcc7
Compare
Just a quick update - I've made a few changes, the most important one regarding the NuGet.Config file, which makes sure nodemsi.sln is able to resolve WixToolset.SDK regardless of the NuGet configuration on the machine it's being built on. I've also tested everything in the CI and the job passed https://ci.nodejs.org/job/node-test-commit-windows-fanned/53633/ |
This is required to migrate from WiX3 to WiX4 for building the Node msi. Refs: nodejs/node#45943 PR-URL: nodejs#3211 Reviewed-By: Richard Lau <[email protected]>
This is required to migrate from WiX3 to WiX4 for building the Node msi. Refs: nodejs/node#45943 PR-URL: #3211 Reviewed-By: Richard Lau <[email protected]>
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 did discuss the implementation with @StefanStojanovic, but reviewed this independently and I believe this is correct.
@nodejs/tsc what is the best way of moving this forward? How can we help? We would like to bring ARM64 Windows to tier 2 support, and this is needed to be able to build an MSI installer. |
Let me make a test release. If that works fine, I'm ready to rubberstamp this PR. |
https://nodejs.org/download/test/v20.0.0-test20230317e3abcc7400/ Can someone test with a Windows sandbox? |
Is this semver-patch? Is there a risk to break upgrades from installs done with a previous MSI? |
Per semver definition, this should be minor. The only visible change is to add support for ARM64 MSI, so it only adds functionality. Though the change is contained to the ARM64 Windows platform, so could it be patch? As with any change, there is risk that there is some configuration out there where this breaks. However, this only changes the software used to build the MSI. The MSI itself should be very similar (we checked). There were no changes in the mechanism that handles upgrades. We tested the upgrade scenario and found no issues. @targos I checked the test release and it worked well for me (upgraded from latest v19). Would be good to have other people test it as well! |
I also just tested it myself (x64) with an upgrade from v18. Looks good! |
@targos, since neither Joao nor I are collaborators, can you please land this? |
Commit Queue failed- Loading data for nodejs/node/pull/45943 ✔ Done loading data for nodejs/node/pull/45943 ----------------------------------- PR info ------------------------------------ Title msi: migrate to WiX4 (#45943) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch StefanStojanovic:mefi-wix-migration-wixproj -> nodejs:main Labels doc, windows, install, semver-minor, build, meta, tools, needs-ci, commit-queue-squash Commits 2 - msi: migrate to WiX4 - msi: fixup WiX4 migration Committers 1 - StefanStojanovic PR-URL: https://github.com/nodejs/node/pull/45943 Refs: https://github.com/nodejs/build/issues/2540 Reviewed-By: Michaël Zasso ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/45943 Refs: https://github.com/nodejs/build/issues/2540 Reviewed-By: Michaël Zasso -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last review: ⚠ - msi: migrate to WiX4 ⚠ - msi: fixup WiX4 migration ℹ This PR was created on Thu, 22 Dec 2022 18:05:22 GMT ✔ Approvals: 1 ✔ - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/45943#pullrequestreview-1346956766 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2023-03-22T17:46:58Z: https://ci.nodejs.org/job/node-test-pull-request/50554/ - Querying data for job/node-test-pull-request/50554/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/4494886452 |
Landed in 0b66df6 |
Refs: nodejs#45943 Refs: nodejs#47020 Refs: nodejs/build#3046 Refs: nodejs/build#3246 Refs: nodejs/build#3250 Refs: nodejs/build#3211
Refs: nodejs/node#45943 PR-URL: nodejs#3251 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>
Refs: nodejs/node#45943 PR-URL: #3251 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>
To be able to build x86, x64, and ARM64 MSI installers with the same WiX version, migration to WiX4 is required. PR-URL: #45943 Refs: nodejs/build#2540 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Notable changes: events: * (SEMVER-MINOR) add getMaxListeners method (Khafra) #47039 lib: * (SEMVER-MINOR) add tracing channel to diagnostics\_channel (Stephen Belanger) msi: * (SEMVER-MINOR) migrate to WiX4 (Stefan Stojanovic) #45943 node-api: * (SEMVER-MINOR) deprecate napi\_module\_register (Vladimir Morozov) #46319 stream: * (SEMVER-MINOR) add setter & getter for default highWaterMark (Robert Nagy) #46929 url: * (SEMVER-MINOR) implement URL.canParse (Khafra) #47179 wasi: * (SEMVER-MINOR) no longer require flag to enable wasi (Michael Dawson) #47286 PR-URL: TBD
Notable changes: events: * (SEMVER-MINOR) add getMaxListeners method (Khafra) #47039 lib: * (SEMVER-MINOR) add tracing channel to diagnostics\_channel (Stephen Belanger) msi: * (SEMVER-MINOR) migrate to WiX4 (Stefan Stojanovic) #45943 node-api: * (SEMVER-MINOR) deprecate napi\_module\_register (Vladimir Morozov) #46319 stream: * (SEMVER-MINOR) add setter & getter for default highWaterMark (Robert Nagy) #46929 url: * (SEMVER-MINOR) implement URL.canParse (Khafra) #47179 wasi: * (SEMVER-MINOR) no longer require flag to enable wasi (Michael Dawson) #47286 PR-URL: TBD
Notable changes: events: * (SEMVER-MINOR) add getMaxListeners method (Khafra) #47039 lib: * (SEMVER-MINOR) add tracing channel to diagnostics\_channel (Stephen Belanger) msi: * (SEMVER-MINOR) migrate to WiX4 (Stefan Stojanovic) #45943 node-api: * (SEMVER-MINOR) deprecate napi\_module\_register (Vladimir Morozov) #46319 stream: * (SEMVER-MINOR) add setter & getter for default highWaterMark (Robert Nagy) #46929 url: * (SEMVER-MINOR) implement URL.canParse (Khafra) #47179 wasi: * (SEMVER-MINOR) no longer require flag to enable wasi (Michael Dawson) #47286 PR-URL: #47441
Notable changes: events: * (SEMVER-MINOR) add getMaxListeners method (Khafra) #47039 lib: * (SEMVER-MINOR) add tracing channel to diagnostics\_channel (Stephen Belanger) msi: * (SEMVER-MINOR) migrate to WiX4 (Stefan Stojanovic) #45943 node-api: * (SEMVER-MINOR) deprecate napi\_module\_register (Vladimir Morozov) #46319 stream: * (SEMVER-MINOR) add setter & getter for default highWaterMark (Robert Nagy) #46929 url: * (SEMVER-MINOR) implement URL.canParse (Khafra) #47179 wasi: * (SEMVER-MINOR) no longer require flag to enable wasi (Michael Dawson) #47286 PR-URL: #47441
To be able to build x86, x64, and ARM64 MSI installers with the same WiX version, migration to WiX4 is required. PR-URL: #45943 Refs: nodejs/build#2540 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Notable changes: events: * (SEMVER-MINOR) add getMaxListeners method (Khafra) #47039 lib: * (SEMVER-MINOR) add tracing channel to diagnostics\_channel (Stephen Belanger) msi: * (SEMVER-MINOR) migrate to WiX4 (Stefan Stojanovic) #45943 node-api: * (SEMVER-MINOR) deprecate napi\_module\_register (Vladimir Morozov) #46319 stream: * (SEMVER-MINOR) add setter & getter for default highWaterMark (Robert Nagy) #46929 url: * (SEMVER-MINOR) implement URL.canParse (Khafra) #47179 wasi: * (SEMVER-MINOR) no longer require flag to enable wasi (Michael Dawson) #47286 PR-URL: #47441
Notable changes: events: * (SEMVER-MINOR) add getMaxListeners method (Khafra) #47039 lib: * (SEMVER-MINOR) add tracing channel to diagnostics\_channel (Stephen Belanger) msi: * (SEMVER-MINOR) migrate to WiX4 (Stefan Stojanovic) #45943 node-api: * (SEMVER-MINOR) deprecate napi\_module\_register (Vladimir Morozov) #46319 stream: * (SEMVER-MINOR) add setter & getter for default highWaterMark (Robert Nagy) #46929 url: * (SEMVER-MINOR) implement URL.canParse (Khafra) #47179 PR-URL: #47441
Notable changes: events: * (SEMVER-MINOR) add getMaxListeners method (Khafra) #47039 lib: * (SEMVER-MINOR) add tracing channel to diagnostics\_channel (Stephen Belanger) msi: * (SEMVER-MINOR) migrate to WiX4 (Stefan Stojanovic) #45943 node-api: * (SEMVER-MINOR) deprecate napi\_module\_register (Vladimir Morozov) #46319 stream: * (SEMVER-MINOR) add setter & getter for default highWaterMark (Robert Nagy) #46929 url: * (SEMVER-MINOR) implement URL.canParse (Khafra) #47179 test_runner: * (SEMVER-MINOR) expose reporter for use in run api (Chemi Atlow) #47238 PR-URL: #47441 Signed-off-by: RafaelGSS <[email protected]>
Notable changes: events: * (SEMVER-MINOR) add getMaxListeners method (Khafra) #47039 lib: * (SEMVER-MINOR) add tracing channel to diagnostics\_channel (Stephen Belanger) msi: * (SEMVER-MINOR) migrate to WiX4 (Stefan Stojanovic) #45943 node-api: * (SEMVER-MINOR) deprecate napi\_module\_register (Vladimir Morozov) #46319 stream: * (SEMVER-MINOR) add setter & getter for default highWaterMark (Robert Nagy) #46929 url: * (SEMVER-MINOR) implement URL.canParse (Khafra) #47179 test_runner: * (SEMVER-MINOR) expose reporter for use in run api (Chemi Atlow) #47238 PR-URL: #47441 Signed-off-by: RafaelGSS <[email protected]>
Notable changes: events: * (SEMVER-MINOR) add getMaxListeners method (Khafra) #47039 lib: * (SEMVER-MINOR) add tracing channel to diagnostics\_channel (Stephen Belanger) msi: * (SEMVER-MINOR) migrate to WiX4 (Stefan Stojanovic) #45943 node-api: * (SEMVER-MINOR) deprecate napi\_module\_register (Vladimir Morozov) #46319 stream: * (SEMVER-MINOR) add setter & getter for default highWaterMark (Robert Nagy) #46929 url: * (SEMVER-MINOR) implement URL.canParse (Khafra) #47179 test_runner: * (SEMVER-MINOR) expose reporter for use in run api (Chemi Atlow) #47238 PR-URL: #47441 Signed-off-by: RafaelGSS <[email protected]>
Currently, two versions of the WiX Toolset are used to build the MSI installers. WiX v3.11 is used for x86 and x64, and WiX v3.14 is used for ARM64, which is not a stable release. This PR aims to migrate Node's MSI installer to WiX4 to unify all installers under a single version and remove all dependencies to WiX3, e.g. it can be removed from the CI machines.
In addition, using any WiX3 version requires it to be installed on the machine. Also, the WiX team is dropping support for v3 and they are encouraging users to move to v4.
This PR uses WiX rc-1, released on 16th December 2022. No big changes are expected until GA other than documentation updates and minor fixes. I've tested this extensively, including upgrade scenarios, and I believe this is good to be used on Node as is. Anyway, I plan to update to the stable version when it is released.
In this PR, the way to download WiX is changed to leverage MSBuild, as recommended for WiX v4. This includes an update to BUILDING.md, making it simpler, and is expected to work out of the box in CI machines.
About semver, this should only add support for ARM64. Everything else should stay the same. So, I think this should be semver-minor.
Refs: nodejs/build#2540