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

Enabling ClangCL compilation testing #55784

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

StefanStojanovic
Copy link
Contributor

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

It's causing linker errors with node.lib in node-gyp and potentially
breaks other 3rd party tools
@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added 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. v8 engine Issues and PRs related to the V8 dependency. labels Nov 8, 2024

This comment was marked as off-topic.

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

Successfully merging this pull request may close these issues.

3 participants