Add cross-compilation support for Windows on Arm

[richard-townsend-arm]

Fixes#32582. CC @richardlau@joaocgreis@jkunkee

This PR fixes the Windows on Arm build of Node.js by applying all the necessary command line flags and workarounds, then adds cross-compilation support for the MSVC backend of node-gyp alongside supporting changes. The way this is implemented is that host GYP targets now end with _host.exe (e.g. mksnapshot_host.exe) and actions are re-written accordingly. In addition to the standard node.sln, a special node_host.sln is also generated with the host-only targets (this because MSBuild doesn't like building solutions with projects targeted at a mix of different architectures).

Current issues

• Declaring support for cross-compilation means that some host-only projects are generated for x64/x64 too. The current solution is to compile host tools for all architectures, which increases build time and isn't strictly necessary (although a benefit is that somebody will definitely notice if they break cross-compilation in this setup). Possible changes may be needed to remove _host.exe tools before distribution.
• node-gyp changes will need to be contributed upstream (?) and sent back down here.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/30766/

[joaocgreis]

Thanks for moving this forward @richard-townsend-arm! I see several minor issues that should be easy to fix, but this fails to build in my x64 machine. WhenI run vcbuild arm64 I get this error building the host part:

msbuild node_host.sln /m:2 /p:Configuration=Release /p:Platform=x64 /clp:NoItemAndPropertyList;Verbosity=minimal /nologo
C:\Users\Administrator\Desktop\Node.js\node\node_host.sln : Solution file error MSB4051: Project {3DA0E800-074E-9F25-3378-2102A04DED74} is referencing a project with GUID {F4CAEC9A-07E1-7F49-5630-116B9D21C367}, but a project with this GUID was not found in the .SLN file.

That seems to be the icudata project.

The commit messages should follow https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-message-guidelines. The V8 backport should follow https://github.com/nodejs/node/blob/master/doc/guides/maintaining-V8.md#backporting-to-abandoned-branches. I would squash the two gyp commits and all the commits that are not deps together. I see you put some effort into the commit descriptions (thanks!), would be good to keep them. So, what I suggest is:

- 5f8ef62671
deps: cherry-pick 2cdbb99 from V8 upstream
- b8e7a6c1a2+0ebdb4443e
tools,gyp: add support for MSVC cross-compilation
- e71a369bc5+67a696acfa+58426be36d+426244cd3a+9627415441
build,win: support cross-compiling for ARM64

I believe gyp is, or will be, maintained at https://github.com/nodejs/gyp-next. I'm not sure if the gyp here is in sync with that version, but in any case would be good to PR these changes there. I think this can still go forward independently as a floating patch to unblock the process. Any thoughts @nodejs/gyp @targos@cclauss?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/30909/

[richard-townsend-arm]

OK, looks like the MacOS CI failure is a flake, ready for review.

[joaocgreis]

@richard-townsend-arm I pushed to your branch to make things easier, but I can change it back to 9effddd if you prefer. I changed the first line of the commit messages to follow the Node.js rules, and moved the vcbuild and configure changes to the last commit, so the middle commit is only gyp changes. We usually do this with commits that change dependencies, so it can be easily applied elsewhere or re-floated here if needed.

This looks good and works well for me. I can build x86, x64 and arm64 in my x64 machine. However, the node_host.sln file does not have any projects, it is only the minimal solution boilerplate. All the projects are built as part of the main solution, without errors. Do we really need a separate host sln file?

[richard-townsend-arm]

@richard-townsend-arm I pushed to your branch to make things easier, but I can change it back to 9effddd if you prefer. I changed the first line of the commit messages to follow the Node.js rules, and moved the vcbuild and configure changes to the last commit, so the middle commit is only gyp changes. We usually do this with commits that change dependencies, so it can be easily applied elsewhere or re-floated here if needed.

Cool, that's great, thank you! (I guess I must have just mixed one of the commits up whilst rebasing).

This looks good and works well for me. I can build x86, x64 and arm64 in my x64 machine. However, the node_host.sln file does not have any projects, it is only the minimal solution boilerplate. All the projects are built as part of the main solution, without errors. Do we really need a separate host sln file?

So I've just double-checked this on my machine and... yeah, it seems to be working fine, without node_host.sln. I think that fixing the x86 build must have also fixed something about how the solution's set up, because I now see that it contains the expected mix of x64 and arm64 projects:

[richard-townsend-arm]

Cool, so the next push will remove the node_host.sln file, and implement Christian's recommendations for improving node-gyp. Hopefully will get to it tomorrow.

[cclauss]

RSLGTM

[richard-townsend-arm]

Cool, thanks @cclauss. I'm not authorized to merge. @joaocgreis, any further thoughts?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31321/

[joaocgreis]

I've force pushed from c7e48545d62 to e71e4d365ba.

@richard-townsend-arm I didn't understand the change in configure, if I read it well if the host is x86 or x64 it never defaults to cross-compiling. I think this would be an issue for cross-compiling for the Raspberry Pi Linux, but apparently that job is not running at the moment in CI. I'd much rather leave the logic there as currently is and leave the line in vcbuild that it replaces.

I've rebased and added a fixup commit with my suggestion. The v8 commit is no longer necessary. Does this LGTY?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31325/

[richard-townsend-arm]

Does look good to me, the commit you've pushed though is not building for me due to an unrelated error:

C:\Users\rictow01\node\deps\v8\src\base\platform\platform-win32.cc(1407,5): error C2039: 'GetCurrentThreadStackLimits': is not a member of '`global namespace'' [C:\Us
ers\rictow01\node\tools\v8_gypfiles\v8_libbase.vcxproj]
C:\Users\rictow01\node\deps\v8\src\base\platform\platform-win32.cc(1407,32): error C3861: 'GetCurrentThreadStackLimits': identifier not found [C:\Users\rictow01\node\
tools\v8_gypfiles\v8_libbase.vcxproj]

... but that's separately fixable.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31352/

[joaocgreis]

Rebased to get a fresh CI run (e71e4d365ba to 87f3389).

@richard-townsend-arm if this passes CI, I'll land this PR, as the new failure happens only on ARM64.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31387/

[joaocgreis]

Landed in d093e78...8833551

@richard-townsend-arm thanks for pushing this forward! The current failure building for ARM64 was caused by 1d6adf7#diff-4fa95c2da0a6e4a70f97811e38ef0ba8R1407 , the V8 update that landed while this PR was open. Do you know why that doesn't fail in V8 upstream (I believe they have ARM64 CI) and how to fix that here?

[richard-townsend-arm]

I do super-vaguely remember fixing something like this on upstream V8, I’ll double-check and back port if necessary.

[richard-townsend-arm]

Found it: https://chromium-review.googlesource.com/c/v8/v8/+/2124321

[richard-townsend-arm]

Wait, sorry, that may not be quite the right fixup. I'll take a closer look in the next day or so.

[richard-townsend-arm]

Actually, turned out to be something quite simple: this function is relatively new, so we need to define _WIN32_WINNT=0x0602 when compiling code involving GetCurrentThreadStackLimits. Will fix up and post a PR.