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

feat: add support for native windows arm64 build tools #2650

Merged
merged 1 commit into from
May 25, 2023

Conversation

dennisameling
Copy link
Contributor

@dennisameling dennisameling commented Apr 25, 2022

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

Visual Studio 2022 17.4 adds a native C++ compiler for Windows on ARM. This allows arm64 devices to leverage native build tools, leading to a 35% (or more) speed increase.

Related announcement from Microsoft: https://devblogs.microsoft.com/visualstudio/arm64-visual-studio-is-officially-here/

Building Signal Desktop's native dependencies on a Surface Pro X (arm64 device):

x86 build tools (emulation): Done in 108.97s.
arm64 build tools (native): Done in 69.91s.
That's ~35.8% faster with the native tools.

@dennisameling
Copy link
Contributor Author

@rvagg any chance this could be reviewed & merged? Now that nodejs/node#46228 has been merged and NodeJS itself can be built on arm64 (cross-compilation was already possible), having first-class support in node-gyp as well would be great. I just rebased the PR and updated the test with the most recent VS2022 version (17.4.33213.308). Thanks!

Visual Studio 2022 17.4 adds a native C++ compiler for Windows on ARM.
This allows arm64 devices to leverage native build tools, leading to
a 35% (or more) speed increase.
https://devblogs.microsoft.com/visualstudio/arm64-visual-studio-is-officially-here/

Signed-off-by: Dennis Ameling <dennis@dennisameling.com>
@richardlau
Copy link
Member

richardlau commented Apr 22, 2023

cc @StefanStojanovic @nodejs/platform-windows-arm

Copy link
Contributor

@StefanStojanovic StefanStojanovic left a comment

Choose a reason for hiding this comment

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

LGTM! Since GitHub actions are failing in general (not caused by this PR), I pulled this branch and ran tests locally - they passed.

@StefanStojanovic
Copy link
Contributor

I'm planning to land this PR in one week (26 May), if someone has any objections to this being landed, please express them by then.

@StefanStojanovic
Copy link
Contributor

@rvagg @cclauss I'm changing tests from tap to mocha in #2851. Since this PR adds some tests and it's already approved, what are your thoughts on landing it so I can change those tests in my PR as well? I've already planned to land this on Friday since there were no objections, so I can do it a few days earlier.

@cclauss
Copy link
Contributor

cclauss commented May 23, 2023

You want to merge a PR with 30 failing and 1 successful checks?

@StefanStojanovic
Copy link
Contributor

You want to merge a PR with 30 failing and 1 successful check?

Yes. Those tests (as is the case in other more recent PRs) are failing because the new make-fetch-happen version and tap we use not working well together (I'm working on a fix for that in #2851). I reviewed this and tested it locally and all looks good.

@StefanStojanovic StefanStojanovic merged commit bb76021 into nodejs:main May 25, 2023
@dennisameling dennisameling deleted the win-arm64-build-tools branch May 25, 2023 11:38
@cclauss cclauss added the Arm label Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants