Windows PlatformToolset: Provide stable C++ API to addons

[saper]

This is a followup to #2365

When building nodejs addons it is very important to use the same C++ ABI as the original node engine installed. Currently node releases are done with Visual Studio 2013 so I'd propose we add v120 as the PlatformToolset.

Maybe we could store the PlatformToolset used in the config.gypi file, so that all add-ons can pick it up and configure themselves accordingly?

Example of a real problem caused by this issue: sass/node-sass#1326

[mscdex]

/cc @nodejs/platform-windows @joaocgreis?

[bnoordhuis]

@saper To clear up any misunderstandings: you're suggesting to put PlatformToolset in process.config.variables? That's reasonable, I think.

We don't currently set it (i.e., PlatformToolset - GYP calls it msbuild_toolset) explicitly, I believe?

[benjamingr]

Sounds reasonable to have this exposed.

[oksite]

@saper I also encountered similar problems

[joaocgreis]

Fixing the PlatformToolset would prevent users from building with any other version of Visual Studio.

I've been trying to get node-sass and other modules to fail when they are build with a different version of VS than node, but I couldn't find any problem so far. Note that the issue in node-sass was in Windows XP, which we no longer support.

Given this, I don't see a pressing reason to force all users to use the same version of VS, since doing that would cause major pain to users. If we ever find some issue like the one in node-sass again, I would much rather find a way to work around it with something like sass/node-sass#1283 (comment) .

[saper]

@joacgreis this is not about "fixing" PlatformToolset, it is about informing extensions about the C++ runtime ABI version used to compile. When debugging C++ ABI compatibility problems I have actually been building v120 and v140 versions of node in parallel. One can easily flip the switch, just extensions have to be compiled against the same C++ binary ABI as the node engine itself.

[joaocgreis]

As discussed in #7989 , we should move ahead with this and include PlatformToolset information.

When a mismatch is detected when building a module, there should only be a warning instead of a failure (if possible, use the correct the correct platform if available). We can change this later if we find a pressing reason.

[jasnell]

@nodejs/n-api ... does this need to remain open?

[jasongin]

This won't be a problem for modules using N-API, because all N-API exported functions are "extern C", so the ABI is unaffected by the PlatformToolset version.

However I'm not sure that means this issue can be closed, because Node needs to continue supporting non-N-API modules for a long time.

[saper]

@jasongin Correct. But whenever N-API module decides to use C++ (via the wrapper or just for itself), we come back to square one and the requirement to use the same C++ compiler and runtime libraries.

[Trott]

This issue has been inactive for almost a year, but I suspect the issue is still valid and not yet addressed. If anyone has information to the contrary, by all means, close this or leave a comment. I'll also ping one more possibly-relevant group: @nodejs/addon-api

[saper]

Let me check this. Even with N-API if the addon is using C++ for some other reason (wrapping C++ library) it still needs C++ compiler and the runtime environment, which should be the same as node's.

[gabrielschulhof]

@saper why should it be the same as Node.js' if the N-API only ever calls extern "C" functions from Node.js? I've always built my N-API module with the packages installed via npm -g install windows-build-tools, and run it against an official Node.js release, and I have never had any issues even though my module uses the STL and thus needs to be built with a C++ compiler.

[gabrielschulhof]

... and I build my module against Node.js 4, 5.5.0, 6, and 8. So, unless all those Node.js versions were built using the same version of the C++ compiler, which is also the same version that my module is built with, I don't think the versions have to line up.

[jasnell]

What is the status on this? Does this need to remain open?

[apapirovski]

I'm going to close this out since there's basically no movement here and I can't really parse the conversation very well. If someone still feels strongly about this, do feel free to reopen but it would be good to have more information so that this conversation can move forward.