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

deps: cherry-pick ARM64 Windows changes to node-gyp #46995

Closed
wants to merge 1 commit into from

Conversation

StefanStojanovic
Copy link
Contributor

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

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
@nodejs-github-bot nodejs-github-bot added dont-land-on-v14.x fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. npm Issues and PRs related to the npm client dependency or the npm registry. labels Mar 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2023

Fast-track has been requested by @nodejs-github-bot. Please 👍 to approve.

@pbo-linaro
Copy link
Contributor

pbo-linaro commented Mar 7, 2023

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).

Copy link
Member

@anonrig anonrig left a 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.

@StefanStojanovic
Copy link
Contributor Author

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.

@StefanStojanovic
Copy link
Contributor Author

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.

@StefanStojanovic
Copy link
Contributor Author

The original PR in node-gyp is closed. Closing this one as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track PRs that do not need to wait for 48 hours to land. needs-ci PRs that need a full CI run. npm Issues and PRs related to the npm client dependency or the npm registry.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants