win: update supported vs versions #2959
Conversation
| } | ||
|
|
||
| // Invoke the PowerShell script to get information about Visual Studio 2017 | ||
| async findVisualStudio2017 () { |
There was a problem hiding this comment.
This function should be removed. Also this should be a breaking change. commit. msg should be like
win!: update supported vs versions
There was a problem hiding this comment.
I've changed the commit message. Should I also add the semver-major label?
Regarding the removal of findVisualStudio2017, why do you think it should be removed? I've added it in this PR so that VS2017 can be prohibited on Node v22+, while VS2019 and VS2022 will be enabled. On previous Node versions all 3 VS versions should work, like they are working in the current main branch.
There was a problem hiding this comment.
I don't follow the reasoning why this would be a breaking change. @gengjiawen Can you elaborate on this?
StefanStojanovic
force-pushed
the
mefi-vs2017-support-drop
branch
from
4c9bf4a
to
69ed7ef
Compare
|
I re-ran the failed jobs...
All tests pass. |
There was a problem hiding this comment.
LGTM. I would like others' thoughts on if this is a breaking change as referenced here #2959 (comment).
I don't think it is, in which case I can squash and merge with the PR title as the commit so the automated release does not tag this as a semver major.
this mainly follow Node.js main repo for compiler change. |
|
@StefanStojanovic Do you think this should be merged while #2957 is awaiting updates to your feedback? |
This can be merged. The thing is, if that happens, the function other PR adds ( Just a note, before merging I should squash the fixup commit and probably change |
StefanStojanovic
force-pushed
the
mefi-vs2017-support-drop
branch
from
69ed7ef
to
2a980c3
Compare
|
@lukekarrys I've made the changes preparing this for merge. As I mentioned earlier, I have no problem making adjustments in #2957 (or in a separate PR) to get it in line with this. |
StefanStojanovic
force-pushed
the
mefi-vs2017-support-drop
branch
from
2a980c3
to
8780f4b
Compare
Drop VS2017 support for Node.js v22 and above. Refs: nodejs/build#3603 Refs: nodejs/node#45427
StefanStojanovic
force-pushed
the
mefi-vs2017-support-drop
branch
from
8780f4b
to
fa87296
Compare
|
I've rebased this PR on top of the main branch, which now has the changes from #2957. @lukekarrys, since you've already approved my previous changes, I'd like to ask you to review the new ones and if it LGTY, land this. |
|
A reminder to @nodejs/node-gyp about this PR. I've adjusted it after #2957 landed, and as far as I can tell, it should be ready for merging. |
|
Since this is already approved and no there were no objections, I plan to land this tomorrow. If there are any complaints please state them until then. |
|
Sorry for the delay on review @StefanStojanovic and thank you for landing this. I will work on getting this published this week. |
Checklist
npm install && npm run lint && npm testpassesDescription of change
Since Node.js v22 will use C++20 standard, VS2017 can no longer be used for compiling native add-ons. This is already changed in the docs. This PR drop VS2017 support for Node.js v22 and above for node-gyp.
Note: Node-gyp with this change should be released and consumed by Node for the v22 RC to avoid potential problems on Windows.
cc @targos, @nodejs/node-gyp
Refs: nodejs/build#3603
Refs: nodejs/node#45427