Support Windows Arm64 Build #1801
Conversation
There was a problem hiding this comment.
This looks reasonable to me overall! I had a bunch of questions, but they are questions where I literally just do not understand the reasoning, so do not take them as implying you’re wrong in making those changes—merely that I would like to understand them and make sure we have the reason for the change captured for posterity!
Also, @charlespierce can you take a gander here?
| <?if $(var.Platform) = x64 ?> | ||
| <?define Win64 = "yes" ?> | ||
| <?if $(sys.BUILDARCH) = x64 or $(sys.BUILDARCH) = arm64 ?> | ||
| <?define PlatformProgramFilesFolder = "ProgramFiles64Folder" ?> | ||
| <?else ?> | ||
| <?define Win64 = "no" ?> |
There was a problem hiding this comment.
Why the Win64 changes here?
There was a problem hiding this comment.
The Win64 and Platform settings have been removed as they are now deprecated in wix. The details are described in the following pull request from cargo-wix, which is shared here.
volks73/cargo-wix#113 (comment)
I took the opportunity to remove use of Win64 and Platform in the wxs: those attributes are deprecated according to the wix documentation, passing -arch on the command line should be prefered (which this PR now does). var.Platform is replaced by sys.BUILDARCH.
| <?if $(sys.BUILDARCH) = x64 ?> | ||
| <Feature Id='VCRedistributable' Title='Visual C++ Runtime' AllowAdvertise='no' Display='hidden' Level='1'> | ||
| <MergeRef Id='VCRedist'/> | ||
| </Feature> | ||
| <?endif ?> |
There was a problem hiding this comment.
Likewise, why the addition of the BUILDARCH check here?
There was a problem hiding this comment.
Added a condition that the VC++ redistribution runtime should not be included on arm64 since it was included for x64.
However, I do not know why the redistribution runtime is included. In my opinion, it seems unnecessary.
There was a problem hiding this comment.
It looks like that was added in #592, as it's needed to actually run Volta and not always available on the system.
There was a problem hiding this comment.
I looked into it because I couldn't find a merge module for Arm64, and it seems that it is now deprecated.
Merge modules (.msm files) for Visual C++ Redistributable files are deprecated. We don't recommend you use them for application deployment. Instead, we recommend central deployment of the Visual C++ Redistributable package.
| Languages='1033' | ||
| Compressed='yes' | ||
| InstallScope='perMachine' | ||
| SummaryCodepage='1252' | ||
| Platform='$(var.Platform)'/> |
There was a problem hiding this comment.
Why are you dropping the Platform value here?
There was a problem hiding this comment.
I am deleting it for the same reason as here: #1801 (comment)
| <?if $(sys.BUILDARCH) = x64 ?> | ||
| <Merge Id='VCRedist' SourceFile='wix\Microsoft_VC140_CRT_x64.msm' DiskId='1' Language='0'/> | ||
| <?endif ?> |
There was a problem hiding this comment.
Why are you dropping this check?
There was a problem hiding this comment.
Since the VC++ redistribution runtime was only available for x64, I have added a condition so that it is only included for x64 as before.
| </Directory> | ||
|
|
||
| <ComponentGroup Id='Binaries' Directory='INSTALLDIR'> | ||
| <Component Id='voltaBinary' Guid='*' Win64='$(var.Win64)'> | ||
| <Component Id='voltaBinary' Guid='*'> |
There was a problem hiding this comment.
Here and throughout, why are you removing Win64=?
There was a problem hiding this comment.
I am deleting it for the same reason as here: #1801 (comment)
|
Excellent – and @shibayan answered all my questions (thank you!). |
Relate #1270
A build for Windows on Arm will be added to Volta.
The changes I made include creating an msi and zip for Arm64 in my repository, and I have confirmed that it works on Windows on Arm machines.