doc: drop support for VS2015 #16868
doc: drop support for VS2015 #16868
Conversation
nodejs-github-bot
added
build
gibfahn
added
the
semver-major
|
Previous issue/PR (to move from 2013 to 2015) was #7484 and #8049 In that @joaocgreis confirmed that node compiled with 2015 still worked with modules compiled with 2013 (#7484 (comment)). We should probably do something similar. From #7484 (comment), it sounds like node-sass is a good test-case, cc @saper FWIW now seems like a good time to do this, and if V8 is forcing our hand then +1 on this for Node 10.0. |
There was a problem hiding this comment.
LGTM
Didn't we decide last time that semver doesn't apply?
- it only affects people that build from source, and
- it's not unreasonable to expect an up-to-date toolchain
The situation for add-ons is a bit different because it affects a much larger group of people, and they don't always have the option of simply downloading binaries if compiling doesn't work.
edit: basically what @gibfahn said but with more words.
|
AFAIR the decision to make this Besides that:
/CC @nodejs/v8 |
|
@seishun I added two commits that remove more mentions of VS2015. Feel free to push them out. |
|
@refack The discussion in #14798 is still ongoing and "Allow edits from maintainers" seems to be checked by default, so I'd avoid pushing to PR branches for now. Also, I disagree with squashing Build Tools into "any edition" since it's not an edition of VS2017. Plus "Build Tools SKU" is a confusing name (what exactly is "SKU"?). |
As I wrote, I added this only as a suggestion, feel free to push it out, or take what you like. I only moved some sentences around, did not add anything new, and I don't feel strongly about any of the phrasing. AFAIK SKU is is a term used like catalog-number, that is a label given to a specific version of a product including all its parts. Microsoft uses it as the full name of each variation of it's products (for example Product = |
FWIW it's become pretty common to push suggestions to branches (obviously not force pushing), I should probably update that issue to reflect that. |
|
There was already some discussion in #13052 (comment) (but now we're actually dropping VS2015). Do we need to land a version of V8 that doesn't work with VS2015 in v9.x? If not, it would be better to move as semver major in v10.0.0. This is because we're hopeful that there are no issues with ABI compatibility for native modules compiled with VS2015, but we can't be completely sure. There are still some steps that need to happen before this PR can land:
|
I was thinking about doing it in a separate PR to avoid mixing discussions.
This is necessary to make this non-major, right?
Why does it need to be a separate machine? What if I just build node with VS2017, then make sure CitGM runs with VS2015?
Sure, but ideally I think it should be done by someone with more experience with these things. |
|
I have no idea how to run Not sure if it passed or failed. Also, I got the same result when I ran it on the machine with VS2017. Plus it's unclear if it even built the binary since Since Anyway, I don't want to spend more time on this since this blog post and these two answers claim that VS2017 is binary compatible with VS2015, which should be sufficient. I'm removing the |
seishun
removed
the
semver-major
The main "power" of CitGM is it's doing batch tests. Basicly what it does is downloads a package, runs npm install, then npm test. You can do that manually and the output should be more readable. |
|
BTW: removing VS2015 support from |
#8067 wasn't semver-major, and it doesn't make sense to keep VS2015 "support" in vcbuild in 9.x if it won't actually compile. |
This should do the trick: |
That's helpful. So, v140 and v141 are guaranteed to be binary compatible, which should make this transition much less risky. The second answer leaves it quite clear that we'll have the issue again in v150, hopefully by then N-API will help mitigate this. I still think we shouldn't rush this. Let me get the release machine ready, we'll start building nightlies of master (v10) with it and see if any issue surfaces. Is this needed for v9?
How is it different discussion? I would do it in the same commit, because one does not make sense without the other. If one is semver major, how can the other not be? |
|
N-API is just a first step. C++ runtime support will be needed even if pure C interfaces will be used and that's why it is still important to keep binary compatibility between components. |
|
If we assume compiled addons will be compatible, as I see it there are two issues
I'm +1 on this PR as is, since it's only addresses issue (1). |
We run CI on VS2017, what kind of issues do you expect other than build infra issues?
If V8 is going to be upgraded in 9.x, then yes.
There might be some technical discussion regarding the
Not exactly. After this change lands,
I refer to @bnoordhuis's comment above which I fully agree with. |
I not sure either way, I would advise for cautions, but I'm not blocking either way. As for issues, there is at least one bug that reproduces only in VS2017 build binaries #15558 (comment) |
|
I've updated the commit to include necessary changes in the other sections. I've also removed the space in "VS2015" because it looks nicer. I wasn't sure how to update the "Visual Studio 2015 or Visual C++ Build Tools 2015 or newer" line. It's not clear what the actual name of the Build Tools product is - it's "Build Tools for Visual Studio 2017" in the downloads page, and "Visual Studio Build Tools 2017" in the installer, so I just went with a short version. I've also dropped "and newer" since there's nothing newer than 2017. @nodejs/platform-windows I'm going to land this in 24 hours if there are no objections. |
👍 that's prudent as build might not work out of the box with the next version. |
|
Having only skipped through this thread, is there a consensus on requiring VS2017 for Node.js at this point? We are currently adding Windows and Mac CI bots on our end at V8, and it would be awesome if we didn't have to set up an additional one for VS2015. Chromium no longer requires VS2015, for reference. |
|
@hashseed apparently V8 contains code that doesn't compile in VS2015 to begin with - so even if we wanted to (short of Node.js forking V8 or PRing against that file) - we can't support VS2015. Given VS2017 is free and supports exactly the same platforms as VS2015 the consensus in this thread is to support only VS2017 moving forward. This is beneficial for Node.js anyway since it is one less thing to maintain. |
|
I get that part. I'm trying to get a feeling of whether that's a regression we can live with, by dropping VS2015 support in Node.js, or something we need to fix in V8 until Node.js drops support. The current situation is unfortunate in that V8 went ahead and stopped testing for VS2015, and we broke Node.js' expectations. The question is where we go from here. From your response I gather that we can just drop support for VS2015 like we already have. |
Yeah, I agree it's unfortunate but given there are no platforms VS2015 runs that VS2017 doesn't run on I don't think it's that unfortunate either :) |
PR-URL: #16868 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
|
Landed in c5a49e1. |
@hashseed it depends on whether you CI 6.2 and 6.3, as we are dropping official support for VS2017, which is certain for node10, but it is still left to be decided if we can start building node8 and node9 with VS2017. So it only the tip of V8 is broken, it's possible to live with that, but it might limit our options for upgrading V8 for the active version lines (TDB). As a post mortem, I was wondering if we could get a heads up when chromium/V8 bumps requirements, so that we can follow suit as soon as possible. |
|
Since we are only trying to make sure updating V8 does not break Node.js, we only test updating V8 in a reasonably recent version of Node.js (which we sync with upstream every 2-3 months). That seems to have worked well so far aside from this Windows build issue. I'll keep in mind next time V8 changes build requirements. |
PR-URL: #16868 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
CI runs only node's addon tests. CitGM is only run for releases, but even that is no guarantee that nothing in the wild will break. The issue in node-sass with VS2015 shows that compatibility issues can cause a lot of pain and be very hard to debug. Even with the guarantee of binary compatibility, I'd rather err on the safe side. Furthermore, there's the possibility of issues with VS2017 like the one @refack pointed. Doing a documentation only removal still doesn't make complete sense to me, as this is something that we can know for sure, not a platform that we'll keep working while there are only minor issues to address. But it's good as a way to start warning, if it turns out that V8 actually needs to be updated to an incompatible version in v9.x. @seishun please don't land PRs in a hurry when there's no need to. This PR was open only 5 days and discussion was still taking place. @hashseed I think we're generally ok with moving node to VS2017, for compiling node core. A different issue is requiring it for users, who need to compile native modules. Even if we compile node core with VS2017, there can be cases where the V8 headers use features that are not supported by VS2015, thus modules will fail to compile with it. Ref nodejs/node-v8#4 for an issue like this with VS2013. It would be great if this could be tested on the V8 side as well. |
|
I'm marking don't land for 6.x and 8.x as I'm assuming we will not be changing the VS support for either please LMK if this is a mistake |
V8 master no longer builds with VS2015: nodejs/node-v8#19 (comment) There are two main issues:
constexprinbits.h(introduced in v8/v8@51f4d2e). Supported since VS2017. There is no trivial way to work around it while keeping the functionconstexpr.Visual Studio Community 2017 is free and supports all the operating systems VS2015 does, so spending extra effort to support VS2015 doesn't seem justifiable.
Checklist
Affected core subsystem(s)
doc