-
Notifications
You must be signed in to change notification settings - Fork 168
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
Adding ClangCL to the CI #4001
Comments
An update on this. Both x64 and arm64 architectures are compiled with ClangCL now, and thanks to Next step is enabling the tests. I've already prepared everything, but there is a question of bandwidth as we will run double the set of tests now. I will probably start by enabling 1-2 configurations to begin with. VS2022 configurations need to be enabled last as those machines are already under a bigger load due to the increased number of compilations per run. Also just want to note that I've already tested both debug compilation and testing in my temporary job and all passed fine so I'm not expecting any big issues with it other than the bandwidth. |
Thanks for the update. Is the goal for Node.js 24 to remove MSVC support? Asking since I still got no answer about the Windows issues on nodejs/node#57114, where Clang builds pass, but MSVC builds fail. |
Yes that is the idea. Next week I can try and resolve the issues with MSVC, in case ClangCL doesn't get ready for v24, but I would like to see v24 release happen with ClangCL. That is also the reason why I still haven't got to that as I focused on Clang CI changes. By the way, should I start some kind of discussion with TSC (or someone else) regarding the transition to Clang in v24? It is still not ready (will be ready in time I think), but maybe it'll be too late to open that question later? |
I guess it doesn't hurt to ping @nodejs/tsc since the change will be semver-major. |
In general transitioning to Clang in v24 SGTM. Though I wonder if there are any problems with addons? I recall that @nodejs/node-api did a lot of investigation in the ecosystem during the development of N-API. Is there some kind of list of popular projects using addons that we can look into and double check? |
You mean that addons built with MSVC might not be compatible with Node.js built with Clang? |
Yes, for example the ones that might be using MSVC options in the configurations for Windows. I think some breakage should be expected, but it would be better to provide something for them to workaround/adapt to Clang in 24 (e.g. do we expose the clang == 1 variable to them to create some conditions?). Theoratically it should be possible to link an MSVC-compiled addon to Clang build still, but I am not sure if the options we use for clang + the options from common.gypi for MSVC etc. can produce ABI compatible builds? If not they might need some kind of indicator to switch to a different toolchain in their script when being built against v24 too and do some adjustment in their config for v24. |
Node-apis are just C APIs and it might be a different case from C++ Addon APIs. From https://clang.llvm.org/docs/MSVCCompatibility.html, it reads to me that Clang provides ABI compatibility guarantees. For |
I meant that the N-API effort was initially a migration effort so the team may have an idea about what tricky things the ecosystem is doing with their addon build config that might be relevant :) |
From the discussion in the node-api team today, this is one of the issues were we looked at popular modules with native code, including how we identified them - nodejs/abi-stable-node#22 |
This directory also includes some historical information about the assessment done in the early days of node-api - https://github.com/nodejs/abi-stable-node/tree/doc/data |
Plan
Since Node.js can now be compiled with ClangCL on Windows, I'll start adapting the CI to start using that. Here is the plan in short and we'll work out the details as we go forward:
Preparation
This part is done, as a part of it I did the following:
Other work
Besides the main plan, there are some things to take care of as well (this list might get new entries later on):
obj
directory which interferes with PCH).The text was updated successfully, but these errors were encountered: