-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: remove implied support for win 2012 not R2 #19378
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Not familiar with our Windows builds, but the documentation should reflect the supported systems.
I'd be +1 on this, but it definitely needs review from @nodejs/build and @nodejs/platform-windows . The question is when/if we should do this. I'd suggest it be a semver-major that we land in 10.x. I think the CI failures we've seen on Win 2012 (non-R2) are enough to make this worthwhile. |
This does not require a CI run due to obvious change. |
Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/468/ (That runs the markdown linter on this file, I'm pretty sure.) |
@gibfahn do you think this should land? |
Would like @joaocgreis to weigh in if possible, otherwise should be good to go. |
Trying Lite CI again: https://ci.nodejs.org/job/node-test-pull-request-lite/471/ |
I'm -0 on this. The failing tests should be addressed, but I agree it's not likely to happen soon. |
Hm, I am unsure if this should land now due to the -0 @joaocgreis |
I think this should land, @joaocgreis was (I think) expressing that this isn't the ideal solution (to which I agree), but that he wouldn't block it as it's the best option we have. |
PR-URL: nodejs#19378 Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Nikolai Vavilov <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Landed in 4a0dff7 🎉 |
PR-URL: #19378 Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Nikolai Vavilov <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Raising this primarily for the discussion of whether to remove the implied support for Win 2012 not R2. The basis for this is that CI does not run on Win 2012 not R2, and some failures have been deemed specific to this platform.
See:
/cc @gibfahn
Checklist