build: remove support for VS2015 #16969

seishun · 2017-11-12T16:22:57Z

Previous similar PRs for the curious:
#156
#8067

@nodejs/build VS2015 needs to be removed from CI (at least on master for now. I'm not sure how "backporting" to 9.x works). Do I need to create an issue in nodejs/build for that?

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

build

refack · 2017-11-12T22:15:48Z

/CC @nodejs/lts for consideration whether this is semver-major or not.
Summary: it doesn't not change compatibility with native-addons (they will not need to be recompiled), but it does require users who build from source to upgrade their toolset.

refack · 2017-11-12T22:16:47Z

/CC @nodejs/build we need to have the nightly release machine ready, and exclude VS2015 from CI.

rvagg · 2017-11-13T03:39:47Z

@joaocgreis this one's for you, could you document any actions you take in this thread for us for future reference please?

Re semver-major, on balance I'd prefer this be done at the semver-major boundary to cause minimal disruption.

gibfahn · 2017-11-13T17:36:34Z

/CC @nodejs/lts for consideration whether this is semver-major or not.

I'd say it's not semver-major, but I think we should only change 9.x and up.

joaocgreis · 2017-11-13T18:34:21Z

About semver: it doesn't apply to what we support to build (but it'd be nice if we could treat removals like this as breaking, to minimize impact for developers and custom build farms). Semver should apply to what users need to have installed on their machines to use node, but this time we are oddly safe in that because VS2015 and VS2017 are binary compatible, and for now we still support VS2015 for building native modules. Still, it wouldn't be surprising to see odd issues appear after a change like this.

So @nodejs/lts my recommendation is that we do this for v10, and possibly also for v9.x but only if needed. Do we need to update V8 in v9.x to an incompatible version (6.4)? Do we know when? If we need to, having it done for v10 before will make the transition smoother.

I am working on the release machine, as agreed before.

joaocgreis

This cannot land until the test and release infra is ready. Also, this should be treated as semver-major unless there is a strong reason not to (update V8 to an incompatible version in 9.x).

refack · 2017-11-13T22:10:59Z

I'd argue that embedders and vendors are users, and breaking changes to the build process should be semver-major.
Equivalent to node having breakage when V8 breaks build compatibility upstream, i.e. if V8 would have communicated them dropping VS2015 support we could have pushed this change for node9.

bnoordhuis · 2017-11-14T10:29:15Z

breaking changes to the build process should be semver-major.

I disagree. There are parallels with the centos5 debate: it's reasonable to expect your users to have an up-to-date system.

Downstream users not willing to do that can put in the work, that's not our responsibility.

joaocgreis · 2017-11-30T19:14:34Z

Release machines ready, the next nightlies of v10 should be built with VS2017. If all goes well and no issue appears in the next few days, I'll go ahead with the modifications to the test CI required for this PR to pass CI and land.

joaocgreis

Infrastructure is ready. LGTM pending CI (needs a rebase).

seishun · 2017-12-07T09:11:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11939/

seishun · 2017-12-07T10:38:37Z

CI is green, landing in 24 hours.

joaocgreis · 2017-12-07T16:53:24Z

vcbuild.bat

@@ -185,20 +181,19 @@ if %target_arch%==x64 if %msvs_host_arch%==amd64 set vcvarsall_arg=amd64

 @rem Look for Visual Studio 2017
 :vs-set-2017
-if defined target_env if "%target_env%" NEQ "vs2017" goto vs-set-2015


This line should not be removed, it'll be needed again when we add a new VS version.

It would need to be changed now anyway. And when we add a new VS version, it will need to be changed again. I'd rather remove it now and add it back if and when it's needed than keep a useless line around for likely at least a year.

That said, I don't really mind either way. If anyone else feels strongly about it, I'll put it back.

FWIW it was also removed in #8067.

A new version would go on top, so it would stay the same if changed now. I'd rather these blocks behaved more atomically, and I see this as part of the vs2017 parameter. But it works either way, feel free to go ahead.

refack · 2017-12-07T18:49:04Z

I disagree. There are parallels with the centos5 debate: it's reasonable to expect your users to have an up-to-date system.

Downstream users not willing to do that can put in the work, that's not our responsibility.

AFAIR the push to stop supporting centos5 was because it went EoL, and that's why we added the platform EoL disclaimer - #12672

IMHO we should minimize breaking build flow, unless there's a good reason to. In this example if we want to bump V8 to 6.4 in node9. Until then I'm Ok with landing this as is with the don't-land-on-X labels.

BridgeAR · 2017-12-10T02:26:34Z

@seishun this needs a rebase

seishun · 2017-12-10T18:35:53Z

New CI due to rebase: https://ci.nodejs.org/job/node-test-pull-request/12030/

PR-URL: #16969 Refs: #16868 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added build Issues and PRs related to build files or the CI. windows Issues and PRs related to the Windows platform. labels Nov 12, 2017

jasnell approved these changes Nov 12, 2017

View reviewed changes

joaocgreis suggested changes Nov 13, 2017

View reviewed changes

joaocgreis added dont-land-on-v4.x labels Nov 13, 2017

seishun mentioned this pull request Nov 16, 2017

build, win: vcbuild improvements #17015

Closed

2 tasks

joaocgreis mentioned this pull request Nov 17, 2017

build,win,msi: support WiX with VS2017 #17101

Closed

3 tasks

joaocgreis mentioned this pull request Dec 5, 2017

Skip VS2015 build if node version is >= 10 nodejs/build#1029

Closed

joaocgreis approved these changes Dec 7, 2017

View reviewed changes

bnoordhuis approved these changes Dec 7, 2017

View reviewed changes

joaocgreis reviewed Dec 7, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

BridgeAR approved these changes Dec 10, 2017

View reviewed changes

build: remove support for VS2015

b13233a

PR-URL: #16969 Refs: #16868 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: João Reis <reis@janeasystems.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

seishun merged commit b13233a into nodejs:master Dec 12, 2017

seishun deleted the drop-vs2015 branch December 12, 2017 18:45

refack mentioned this pull request Nov 23, 2018

chore: list compiler versions used to build each node version on each platform nodejs/build#1522

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: remove support for VS2015 #16969

build: remove support for VS2015 #16969

seishun commented Nov 12, 2017

refack commented Nov 12, 2017

refack commented Nov 12, 2017

rvagg commented Nov 13, 2017

gibfahn commented Nov 13, 2017

joaocgreis commented Nov 13, 2017

joaocgreis left a comment

refack commented Nov 13, 2017

bnoordhuis commented Nov 14, 2017

joaocgreis commented Nov 30, 2017

joaocgreis left a comment

seishun commented Dec 7, 2017

seishun commented Dec 7, 2017

joaocgreis Dec 7, 2017

seishun Dec 8, 2017

joaocgreis Dec 9, 2017

refack commented Dec 7, 2017

BridgeAR commented Dec 10, 2017

seishun commented Dec 10, 2017

build: remove support for VS2015 #16969

build: remove support for VS2015 #16969

Conversation

seishun commented Nov 12, 2017

Checklist

Affected core subsystem(s)

refack commented Nov 12, 2017

refack commented Nov 12, 2017

rvagg commented Nov 13, 2017

gibfahn commented Nov 13, 2017

joaocgreis commented Nov 13, 2017

joaocgreis left a comment

Choose a reason for hiding this comment

refack commented Nov 13, 2017

bnoordhuis commented Nov 14, 2017

joaocgreis commented Nov 30, 2017

joaocgreis left a comment

Choose a reason for hiding this comment

seishun commented Dec 7, 2017

seishun commented Dec 7, 2017

joaocgreis Dec 7, 2017

Choose a reason for hiding this comment

seishun Dec 8, 2017

Choose a reason for hiding this comment

joaocgreis Dec 9, 2017

Choose a reason for hiding this comment

refack commented Dec 7, 2017

BridgeAR commented Dec 10, 2017

seishun commented Dec 10, 2017