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

build,win: compile with clang #3932

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

StefanStojanovic
Copy link
Contributor

Since nodejs/node#55249 landed, the main branch of the node repo supports compilation with ClangCL with PCH. MSVC and ClangCL compilation times are similar now, so we can start compiling (in some time testing, and eventually releasing) with ClangCL.

This PR adds support for compiling with ClangCL from v23. It also stops compiling v20 with VS2022 since it was temporary (we just never removed it before).

The labels we currently use in CI for VS2022 compilation are win-vs2022 and win-vs2022-arm64. The format will stay the same, only vs2022 will be replaced by vs2022_clang. The way I see it, this is the most consistent way of labeling it compared to what we already have in the CI, but if someone thinks differently we can discuss it.

I already added those labels to the CI machines that will be compiling and made a copy of the Windows fanned job in https://ci.nodejs.org/job/mefi-node-test-commit-windows-fanned/, so I can confirm these changes work. Once this PR lands we can edit the original compile jobs to do the same.

The second part of the plan is to make new labels, similar to these for test jobs and once I test it in the CI, open a PR for them as well.

One more notable thing is that clcache doesn't work with ClangCL, so in parallel with all of this, I'll try and see about alternatives eg. ccache for ClangCL compilation.

Added support for compiling with ClangCL from v23.
Stopped compiling v20 with VS2022 since it was temporary
@richardlau
Copy link
Member

LGTM. Only question I have is whether this is pushed live before or after the Node.js 23 and 22 releases planned for Tuesday/Monday of next week.

cc @RafaelGSS @aduh95

@StefanStojanovic
Copy link
Contributor Author

LGTM. Only question I have is whether this is pushed live before or after the Node.js 23 and 22 releases planned for Tuesday/Monday of next week.

cc @RafaelGSS @aduh95

I do not find it necessarily connected. Adding them will mean each Windows run will have 3 or 4 (depending if we add both x64 and ARM64 at the same time, or one by one) compilations instead of 2 we have now, and there are 6 compilation machines in total (also used for testing), so we may expect a decrease in our throughput. With that in mind, I'll probably wait for releases to go as we're not in a hurry with it, at least as far as I know.

@StefanStojanovic StefanStojanovic merged commit fd6184c into nodejs:main Oct 14, 2024
1 check passed
@StefanStojanovic
Copy link
Contributor Author

Although I've merged it, I'll wait with using it in the CI until after the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants