-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
deps: cherry-pick ARM64 Windows changes to node-gyp #46995
Conversation
This change cherry-picks a part of node-gyp changes needed to enable the find visual studio script to run on Windows on ARM64. This is needed for running Node.js native test suites in the CI on Windows ARM64 machines. Original commit message: fix: find visual studio arm64 support This enables running find visual studio script on Windows on ARM64 Refs: nodejs/node-gyp#2810
Fast-track has been requested by @nodejs-github-bot. Please 👍 to approve. |
Good catch @StefanStojanovic. When I stumbled on this issue, I installed VS2022 and it found it, but I was not finding the root cause (see some details here). |
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 think this change should be made in node-gyp
, later to npm
and later be added to the Node.js core, since this commit will be overridden with the next NPM release.
I agree, there's already a PR in node-gyp for that, and as you said it will eventually make its way into Node.js core via the NPM update. The issue with that is the time it will take for this fix to become available in Node.js since it's blocking our CI tests. I can help by adding this commit on top of the main branch whenever NPM gets updated until the fix becomes an official part of NPM. |
I just wanted to add that a precedent similar to this one was done before in #28604 and that's why I opened this floating patch PR thinking we can do something like that now. Of course, the node-gyp PR would still need to land before moving this forward. |
The original PR in node-gyp is closed. Closing this one as well. |
As a part of an ongoing effort to make Windows on ARM a tier 2 supported platform, we've added Windows 11 ARM64 machines to the CI for testing. When running tests, we saw that native suites were failing because of the following error: Could not find any Visual Studio installation to use. One run showcasing this behavior can be observed in this build.
Upon further investigation, we've discovered that this is an issue with COM classes and interfaces used for this, which weren't registered correctly for ARM64. (for more details on that, please see the PR being cherry-picked nodejs/node-gyp#2810). With the changes proposed here, we're experiencing no problems running native test suites. One run showcasing this behavior can be observed in this build.
We plan to have this fix added as a floating patch inside the node repo and to keep it here until, in some future release, npm is upgraded to a version with the same changes being part of its node-gyp module.
cc @nodejs/platform-windows-arm