Skip to content

Conversation

@edolnx
Copy link

@edolnx edolnx commented Jun 11, 2022

Checklist
  • npm install && npm test passes
  • tests are included
  • commit message follows commit guidelines
Description of change

This fixes building on win32_arm64 platform by finding the correct VC tools for the platform. The documentation was already correct in terms of necessary tools to install, but the code would not find/use the ARM64 version of the tools, or worse build x86 code instead. This corrects that by looking for the ARM64 tools when the platform is arm64, otherwise default to the x86.x84 tools. This also allows for future platforms to be added in the if tree later if needed. All of the existing tests apply, and the log messages now show which version of the tools it's looking for in the event of a failure.

This fix works with VS2019 and VS2022. ARM64 support is not available on older versions of VS. This supersedes #2650 since VS2022 has shipped and the pre-release version is no longer needed.

Happy to make any adjustments in order to get this, or a similar patch, merged so that more NodeJS packages are available on the win32_arm64 platform.

@edolnx
Copy link
Author

edolnx commented Jun 12, 2022

I don't think these failures are due to my change, but I'm unsure. Could someone give me some guidance?

@dennisameling
Copy link
Contributor

@edolnx did you see #2650? I think we're both trying to accomplish the same 😄

@edolnx
Copy link
Author

edolnx commented Jun 14, 2022

@dennisameling yes, I did see that which is why I mentioned it in the PR. The 2022 pre-release works different than the GA release. This patch works for both the GA release of VS2022 and VS2019, and should also work for future versions

@dennisameling
Copy link
Contributor

I actually think we need a combination of this PR and #2650. In my testing, the 32-bit version of MSBuild is still triggered because node-gyp explicitly references MSBuild\Current\Bin\MSBuild.exe, which is 32-bit/x86:

image

The native MSBuild for ARM64 is located in MSBuild\Current\Bin\arm64\MSBuild.exe, which I added to my PR.

As far as I can see, your PR only touches the toolset detection logic, which only checks if the ARM64 tools are installed, but does not actually use them.

Were you able to trigger the native ARM64 MSBuild by using this PR only (can be checked in Task Manager when building a project)?

@edolnx
Copy link
Author

edolnx commented Jun 14, 2022

@dennisameling You are correct that the x86 MSBuild is used, since I'm only using the GA releases of VS2019 and VS2022 on my system ARM64 based system. I did not try the VS2022 Preview which has ARM64 native MSBuild, however even with the x86 MSBuild the binaries created are ARM64 native since I only have those compilers installed. Otherwise, it will default to the x86.x86 compilers, as you noted. The reason why I didn't go down the path of trying with VS2022 is that most other dependencies still require VS2019 so sticking with the existing toolset which doesn't have a native MSBuild seems to make more sense. Also while the VS2022 compilers and tools are installed, the build system seems to prefer the VS2019 versions, probably because that is what build the Python 3.11b1 ARM64 release I used for this (which I only used because while the Python 3.10 code will compile for ARM64, the installer will not build and backporting the updated installer code failed). I'm relatively new to the NodeJS/npm ecosystem so I'll dig in a bit more and see if I can merge in some changes and I'll upgrade my VS2022 from the GA release to the Preview release with ARM64 native tooling.

@edolnx
Copy link
Author

edolnx commented Jun 14, 2022

But I am happy to work with you @dennisameling on trying to come up with a combined effort. I didn't want to discount your work, but also didn't want to depend on a Preview release since the ARM64 preview releases of VS2019 never reached GA so I figured path of least resistance makes more sense...

@edolnx
Copy link
Author

edolnx commented Jun 15, 2022

Ah! After reviewing your code a bit mode @dennisameling your PR is in better shape than mine. There are some things that need to be addressed (mainly that most code won't compile on VS2022 currently because of compiler changes) but I'll put comments and make suggestions based off yours instead.

@edolnx edolnx closed this Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants