Enabling ClangCL compilation testing

[StefanStojanovic]

This PR is a draft to discuss the changes needed to run test jobs in the CI for the ClangCL-produced binaries. It is not expected to land like this, just to discuss before opening production-ready PRs:

1. Enforce MSVC to be used for node-gyp. As far as I can tell, this is the intended way of using node-gyp on Windows. The issue with running the native suites is that now we use Clang to compile Node.js, so config.variables.clang is set to 1, which makes node-gyp generate project files for ClangCL. While we'd like to enable this, that is currently not the priority so for now enforcing MSVC would be the way to go the way I see it. The changes in create-config-gypi.js would be done as a PR in node-gyp and then floated in Node.js until the next node-gyp update.
2. Disable V8_PRESERVE_MOST on Windows. We already have something similar for V8_NODISCARD as a floating patch, so it should not be a problem as I see it. If left, this results in functions that use it having __swift_2 calling conventions rather than the expected __cdecl. This was noticed in cppgc-object native suites test. This change, if agreed upon, would be come one of our V8 floating patches we use on each V8 update.

Tagging relative teams/people: @nodejs/platform-windows @nodejs/node-gyp @targos

[nodejs-github-bot]

Review requested:

• @nodejs/security-wg
• @nodejs/v8-update

[StefanStojanovic]

FYI marked as ready for review so people do not consider it WIP and review it. Still not planning to land it like this, but I want to get the discussion started.

[targos]

Changes SGTM

[RafaelGSS]

LGTM

[StefanStojanovic]

Can someone who already approved this approve my PR in node-gyp with the changes already included here (I will port it to Node.js afterward so we do not wait for the next node-gyp release). The CI failures in that PR are not connected to that and the fix for that has landed in gyp-next.

Thanks in advance

[StefanStojanovic]

Can someone who already approved this approve my other PR with part of changes from here (the ones from node-gyp). Thanks in advance!

[SuibianP]

Prohibiting ClangCL would prevent nodejs/llnode from working on Windows, which, as an lldb plugin, has to be compiled with the same suite I suppose. I am unsure if node-gyp is capable of correctly generating other types of build descriptions on Windows to compile with unwrapped clang.

[StefanStojanovic]

Prohibiting ClangCL would prevent nodejs/llnode from working on Windows, which, as an lldb plugin, has to be compiled with the same suite I suppose. I am unsure if node-gyp is capable of correctly generating other types of build descriptions on Windows to compile with unwrapped clang.

Thanks for the information, I wasn't aware of this. I've investigated it a bit and will open an issue in llnode to discuss this further.

[targos]

The best solution is probably not to prohibit ClangCL, but to prevent it from being inherited as the default from Node.js

[StefanStojanovic]

The best solution is probably not to prohibit ClangCL, but to prevent it from being inherited as the default from Node.js

My understanding is that it picks clang variable from the config.gypi generated during the configuration phase of Node.js compilation. One way to do it is to edit the test job or vcbuild.bat, or something like that to ensure we have 'clang': 0, before running tests but that is also not the nicest solution (not saying prohibiting clang from node-gyp is).

Also, from what I saw, llnode generates a project file with <PlatformToolset>v143</PlatformToolset>, and then sets clang-cl as a compiler with msbuild options (which is currently broken), so I do not think this change would affect it.

[StefanStojanovic]

Closing this since all the changes from here landed.