build: drop support for VS 2013 in v6 LTS

[bnoordhuis]

This is split off from #7484 to discuss the possibility of dropping VS 2013 in v6 LTS. From the other issue:

Proposal: drop support for building node.js with VS 2013.

Motivation: VS 2013 has numerous bugs in its C++11 support and in general. The source tree is full of hacks that work around this broken compiler and pull requests sometimes strand on it (e.g. #5458.)

As to add-ons, we could extend VS 2013 support for our public headers for a while (and test that through test/addons) but V8 will probably force us to update the baseline there too. Chromium currently requires Visual Studio 2015 Update 2 so I expect V8 will follow suit sooner rather than later.

@joaocgreis reported in #7484 (comment):

I tested some of the most downloaded native modules (ws (to test bufferutil and utf-8-validate), node-sass, contextify, thread-sleep, weak, bcrypt, hiredis, sqlite3, v8-profiler, iconv, ref and v8-debug) using VS2013 and they all behave exactly the same whether node itself was built with VS2013 or VS2015.

So I think we should go ahead with this and start building v6 and v7 with VS2015.

Encouraging!

[jasnell]

Fwiw, I'm +1 on dropping support in v6

[ChALkeR]

+1 if this lands before LTS.

[orangemocha]

+1

[adron-orange]

Yes. Drop the support. Please +1.

[eljefedelrodeodeljefe]

+1 since otherwise big baggage for LTS (specifically) and mainstream development (in consequence).

[ChALkeR]

/cc @nodejs/platform-windows

[saghul]

+1

[cjihrig]

SGTM

[jasnell]

@nodejs/build ... what's the status of getting the CI updated to drop vs2013?

[joaocgreis]

@jasnell

• Release CI: Machines done, finetuning the job. Will let you know when it's ready to use.
• Test CI: Already compiles only with 2015 for the correct versions, but addons testing should still use VS2013 (next in my queue).
• CITGM: Stalled for now.

[MylesBorins]

@joaocgreis is there anything i can do to help with CITGM? We are not even building with windows ATM

[joaocgreis]

@thealphanerd If you have any comments on the first draft I made in JaneaSystems/citgm@5d44fc2...janeasystems:joaocgreis-G7R-attempts it'd be great! I'll work more on it, but if you want to finish the work on some of those features and PR them to CITGM, feel free to! (It'd be great to have a way to test modules with both downloaded and compiled binaries, but I'm not sure how to do that, ideas?)

I started a CI job for CITGM on Windows (https://ci.nodejs.org/view/All/job/reis-citgm-native-windows-fanned/) but it is still too tied to running my commits above. I'll make it general once the main canary has all the functionality.

[joaocgreis]

Update:

• Test CI: Pending win,build: improvements to vcbuild.bat #8412 and src: remove VS 2013 compatibility hacks #8067 , should be all done after they land.
• Release CI: Ready. There's a new job, reis-node-release-v6, that uses VS2015. I plan to drop the reis- prefix when it's approved by the @nodejs/release team (or should I right away?).

Node v7 can use the new job right away for the RCs. For node v6 we should perhaps make some test releases with the same commits as the releases. Note that users won't be able to test properly in Windows until #8430 lands, because node-gyp won't be able to download what it needs.

[rvagg]

We should be able to do this within the same release job using build node labels. Already we have this in there:

NODE_VERSION=$(python tools/getnodeversion.py)

if [[ $ARCH =~ s390x && ${NODE_VERSION:0:1} -lt "6" ]]; then
  echo "Not building $disttype on linuxOne slave as only supported on v6.x and later"
  exit 0
fi

# nodejs <1.0 runs on specific builders that has the 'pre-1-release' tag.
if [[ \
    ( $NODE_LABELS =~ pre-1-release && $NODE_VERSION =~ ^[0] ) \
 || ( $NODE_LABELS =~ post-1-release && $NODE_VERSION =~ ^[^0] ) \
]]; then

 ... build ...

fi

That s390x stuff really should be using the labels as well and we can wrap in this v6/v7 stuff too.

So right now we have these restrictions on platforms within the existing job (using "v1" here to separate v0.10 + v0.12 and v4+ btw).

• < v4 gets an older version of smartos
• < v4 gets no arm

• = v4 gets newer smartos

• = v4 gets armv6, armv7 and armv8

• = v6 gets s390x

Now we can add the Windows rules to that too, ideally all with labels and not this mishmash of methods.

[joaocgreis]

@rvagg Thanks for your suggestion, I adapted the Windows script and we'll be able to use the same job.

I created a new job reis-iojs+release that can be used for v7 RCs and v6 test builds. This way, we can use the current iojs+release job until we make the change for VS2015, and then I'll just copy the new parts and delete the new job. We can start testing this when #8430 lands.

[joaocgreis]

Here is a test build of v6.5.0 with VS2015 with #8430 applied: https://nodejs.org/download/test/v6.5.0-test2016091435bb966909/ . This can be used to test native modules.

Is there a good way to make this visible to users, so that they can start testing with it?

cc @nodejs/platform-windows

[joaocgreis]

Here is a test build of v6.6.0 with VS2015: https://nodejs.org/download/test/v6.6.0-test201609140a3ad92292/ . This can be used to test native modules.

[joaocgreis]

@saper Any chance you could give this test build a try with node-sass? I've tested it, and I get exactly the same results as with the official release: 5905 passing, 322 pending (running in a VM with VS2013 installed). Given that node-sass had the only known issue with VS versions in the past, an "all clear" from you would be invaluable. Thanks!

[joaocgreis]

Given that everything is going smoothly, should we move ahead and start releasing v6 with VS2015?

I can activate it in the Release Jenkins, so that the next v6 release will already be built with it. Starting at the moment I activate it, nightlies, tests and so on will also use it.

@nodejs/ctc is there consensus around this? Should I activate it right away or should we set a date (like before a minor release)?

[jasnell]

I'd say we should be good to go.

On Wednesday, September 21, 2016, João Reis [email protected]
wrote:

Given that everything is going smoothly, should we move ahead and start
releasing v6 with VS2015?

I can activate it in the Release Jenkins, so that the next v6 release will
already be built with it. Starting at the moment I activate it, nightlies,
tests and so on will also use it.

@nodejs/ctc https://github.com/orgs/nodejs/teams/ctc is there consensus
around this? Should I activate it right away or should we set a date (like
before a minor release)?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7989 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eRZo2vl_t5L8hmHjvCXhEQU59iBSks5qsaj_gaJpZM4JeMKm
.

[jbergstroem]

@joaocgreis as a build member you have my +1.

[Trott]

@nodejs/ctc is there consensus around this? Should I activate it right away or should we set a date (like before a minor release)?

Looking at the record, I'd say we seem to have consensus to move forward on this. (Anyone who thinks otherwise should say so, of course!)

The last time this was discussed at a CTC meeting was August 3. The minutes indicate that dropping support for VS2013 in Node.js version 7 was uncontroversial. For version 6, it was desirable, but there was a wait-and-see approach:

@jasnell: Let’s get this landed in master and test it out, then make a decision on putting in v6 before October.

That conversation resulted in this issue.

And now would seem to be the time to make that decision.

Looking through the issue, it seems that the following folks would appear to be on board with doing this:

• @jasnell
• @bnoordhuis
• @ChALkeR
• @cjihrig

I'd be inclined to interpret that as consensus until someone expresses something contrary.

[saper]

Few suggestions regarding keeping the ecosystem sane after this change:

• I think that node module binary ABI should be bumped for the C++ runtime ABI change. Probably would be better to do it on some major release version. If we decide against bumping node module ABI, the extensions that provide binary downloads should be informed to differentiate between a (48, v120) binaries and (48, v140) binaries. Can be very cumbersome so I think we should just bump for that.
• We should ensure that PlatformToolset parameter (set at v120 which is VS2013) is passed over to node-gyp and node-pre-gyp extensions now (Windows PlatformToolset: Provide stable C++ API to addons #6045) and actually used.
• Next step could be to run CI under Visual Studio 2015 with v120 enabled, which is effectively using VS2013 compiler (warning: it might not be installed by default).
• Extensions providing binary extensions as a download should be ready to run their CIs in a dual-mode, i.e. VS2015 with v120 and v140. The mode should be picked up automatically by node-gyp and node-pre-gyp. We will take care of node-sass which does not use node-pre-gyp.
• Once the next version is released (with a bumped node module ABI version) it would be good to verify the parallel building of v120 binaries for older nodes and v140 binaries for the new one works.
• We shouldn't probably update node development trails which should be left at v120, because updating node module ABI there is probably not feasible.
• An interesting question appears how does node lifecycle support should react when we want to switch to hypothetical Visual Studio 2017 (hypothetical v160) which no longer supports v120 ABI (and older Windows SDKs matching v120 are pulled).

[bnoordhuis]

I think that node module binary ABI should be bumped for the C++ runtime ABI change.

That won't happen in v6.x, if that is what you are suggesting, but it will in v7.0.0. An ABI bump forces everyone to rebuild their add-ons; that is not something we want in the stable branch.

We should ensure that PlatformToolset parameter (set at v120 which is VS2013) is passed over to node-gyp and node-pre-gyp extensions now (#6045) and actually used.

That is technically possible by setting 'msbuild_toolset' in common.gypi but I don't know, actively blocking people from building their add-ons with a particular version of VS seems anti-social. Why enforce that? As long as it works, it works.

Also, why v120? We're bumping the baseline to VS 2015 in v6 LTS so v140 would make more sense, wouldn't it?

[saper]

If v6.x is going to remain stable we shouldn't be change C++ library in the mean time.

I will review some extensions and will check if msbuild_toolset is working.

The problem is that one cannot support setup where node engine and C++addons have been compiled using different C++ runtime libraries. With most modern Unix systems an attempt to do that fails with a symbol versioning error nodegit/nodegit#1131 nodegit/nodegit#845 nodegit/nodegit#1096 nodegit/nodegit#853 nodegit/nodegit#969 - those are just from one repository.

Windows unfortunately allows to do exactly this. Setting msbuild_toolset only tells you which C++ ABI your node engine is built against. I have rebuild node with two different compilers and tested the crashing add-on - vs2013 node with v120 extension worked, vs2015 node with v140 extension worked, but not when the versions where mixed.

We have enough unhappy folk on node-gyp issue tracker with Windows. I think it is not very difficult to install v120 support with VS2015 today. Maybe node-gyp could provide a better diagnostic if the desired PlatformToolset is not available, that can be done for sure.

Why making sure v120 works? I think that we are already passing PlatformToolset to addons properly. I'd rather test PlatformToolset "enforcement" now and add changes needed to node-gyp and node-pre-gyp (if any needed at all) now without breaking compatibility. So we make sure the PlatformToolset machine is working smoothly.

Then we can switch to v140 and extensions can test CI setup for building some binaries using v120 (for the stable node) and others are built using v140. This can be done with a properly setup Visual Studio 2015.

By the time we get completely of v120 I am sure another Visual Studio gets released and we will be playing the same game again. Once we make sure the supporting infrastructure is in place, future upgrades will much smoother.

My major concern is making sure downloadable module binaries provided by many extensions are built properly for users who don't install compilers at all (big majority on Windows).

Somebody compiling modules by herself with VS2015 can be asked to install VS2013 compatibility package or (if VS2015 v140 is absolutely required) asked to rebuild node with v140 and obey its PlatformToolset.

Had Windows DLL symbol versioning scheme would have made ignoring PlatformToolset hard and this issue should have been resolved somehow anyway.

I understand that given that msbuild_toolset is available probably no work needs to be done with node itself. Adjustments may need to be done node-gyp, node-pre-gyp and popular extensions. But if a timeline will be agreed (for example for launching this some point in 7.x), extension authors will be given chance to prepare and catch up. If C++ interface changes in the middle of the 6.x line without bumping NODE_MODULE_VERSION projects like https://github.com/sass/node-sass would have to provide a 6.x v120 and 6.x v140 module version separately and include the C++ ABI version number in addition to the node module version number. To make this work PlatformToolset identification should be provided at runtime (not sure it's there).

[joaocgreis]

I don't think we can enforce a PlatformToolset, at least with the motivation we've seen so far. I don't know where a "VS2013 compatibility package" can be found, the only way I know to add v120 support to VS2015 was to install the full 8GB of VS2013 alongside (that is no longer available for easy download in the VS site). For now, we should still add the PlatformToolset information to be used by node-gyp – but only if the desired toolset is available, ignored otherwise.

Currently, we build Node with VS2013 and users can compile with 2013 or 2015. It is true that we have no complete assurance of ABI compatibility. However, compiling Node with VS2015 should not create any new problems, will just change where any possible problems happen. Given that we've been recommending users to build with VS2015 for some time, it should help more than it harms.

In the future, if we encounter any problems, we have several options: we can ask everybody to use the same version of VS (breaking scenarios where a version is imposed), release binaries built with every version, or find a way around the problem (would have been possible with the node-sass bug).

Module developers should move to VS2015 simultaneously with Node v6. I understand that distributing a binary is more serious than a local test build, but many of the most downloaded npm modules provide no binaries, so there is still a significant number of users compiling on their machines. Here is a test build of v6.7.0: https://nodejs.org/download/test/v6.7.0-test20160930d87ad0ceea/

@saper were you able to find any issue when using the test builds? I tested the update path (intall v6.6 release, install a module, test it, install v6.7-testVS2015, test again without recompiling) and it worked fine with both VS versions. The known issue with node-sass was with Windows XP, that is no longer supported.

[joaocgreis]

@nodejs/ctc any objection to moving forward with this on Sunday, so that the release next week is already done with VS2015? It would be good to have one before LTS, and if I'm reading it right this will be the last one.

[saper]

i need some time to respond to this re possible bugs; basically we should have an agreement what a "supported configuration" means. When I propose that everyone should be using the same compiler to compile addons as they or node foundation used to build the engine itself, this is a configuration that lets us preclude a certain class of hard to debug problems. I think I needed a week or something to troubleshoot the problem we've had with vs2015 and XP and I am not going to do this again.

I am uncomfortable with a notion "maybe it's just 1% of corner cases" when we can prepare the environment not to have defects in this area.

i fully agree that we should be moving towards VS2015. Actually instead of just saying/recommending "you need to use VS2015", you express the same with the code by adding v140. What could be additionally done is to trap the msbuild error message if the toolkit is not installed and provide an additional info about the need to use a right compiler from node-gyp.

Regarding switching in the 6.x release cycle with the ABI number unchanged can you propose a way how can I distinguish whether the user is running a vs2013 node 6.x or a vs2015 node 6.x in order to download a correct binary?

[joaocgreis]

@saper I completely agree about adding info about the Platform Toolset, v120 or v140, somewhere. It should be used to select the right version (if it is available) when compiling native modules, and perhaps display a warning if it is not available. I only disagree with blocking the build completely, we have no practical reason to break users with a different version of VS installed.

If we decide to move ahead now, v6.7 will be the last version built with VS2013 and v6.8 will be the first with VS2015, so you can use process.version.

[joaocgreis]

@saper If this is to move forward, it should be for the release this Tuesday. LTS is scheduled for October 18 and it would be good to do this one release before so that we can take a step back if absolutely necessary. Are you still investigating this? Is there anything I can do to help? If we do not find anything by Monday, I think we should move ahead.

[joaocgreis]

Landed the changes in the release jenkins and changed the test CI to not use VS2013 for v6. The next release of v6 will be built with VS2015.