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

feat: prohibit compiling with ClangCL on Windows #3098

Merged
merged 1 commit into from
Nov 29, 2024

Conversation

StefanStojanovic
Copy link
Contributor

This PR is opened based on nodejs/node#55784 as it is required to enable running native (addon) tests for the ClangCL-produced binaries on Windows. The original PR is reviewed and approved in the Node.js repo and I'm moving to the next phase now - landing the changes in their respective repositories. Node-gyp is the only dependency that needs changing outside Node.js, so I've decided to start with it.

Disables trying to compile with ClangCL on Windows. Was required to enable native tests in the Node.js CI for the ClangCL produced binaries.

Refs: nodejs/node#55784
@StefanStojanovic
Copy link
Contributor Author

FYI the Tests / Lint Python workflow failure is not connected to my changes, as I didn't make changes in any Python file in this PR.

@cclauss
Copy link
Contributor

cclauss commented Nov 25, 2024

The failing Python lint issues are fixed in

@StefanStojanovic
Copy link
Contributor Author

The failing Python lint issues are fixed in

Thanks for letting me know. I'll rebase after that PR lands.

@cclauss
Copy link
Contributor

cclauss commented Nov 25, 2024

That PR has landed but a gyp-next --> node-gyp update is also required. This PR can proceed despite those unrelated lint errors.

@StefanStojanovic
Copy link
Contributor Author

StefanStojanovic commented Nov 29, 2024

Could someone please review and approve this so it can be landed? It was already approved as part of a bigger change (this is the change needed in node-gyp) in nodejs/node#55784 and I'd like to push this forward so we can enable ClangCL in the Windows CI.

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

@StefanStojanovic StefanStojanovic merged commit 88260bf into nodejs:main Nov 29, 2024
9 of 10 checks passed
StefanStojanovic added a commit to JaneaSystems/node that referenced this pull request Nov 29, 2024
This change cherry-picks a small change from node-gyp needed for
running native tests on ClangCL compiled binaries.

Refs: nodejs/node-gyp#3098
lukekarrys pushed a commit that referenced this pull request Dec 2, 2024
Disables trying to compile with ClangCL on Windows. Was required to enable native tests in the Node.js CI for the ClangCL produced binaries.

Refs: nodejs/node#55784
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants