Disables Thread-Safe for local static variables #1350
Conversation
|
I don't think this is a good idea for one reason. As long as the node releases are built with VS2013 we have to keep compatibility with C++. I don't want to waste 3 days on yet-another-C++-runtime incompatibility the next time we encounter something. We should simply build with the same compiler and possibly with the same flags as the node engine. I'll be submitting patch to node and node-gyp to let us know the original PlatformToolset used, so we can possibly use VS2015 in |
|
The cflag is only applied to 2015. This doesn't effect VS2013. |
|
I understand. We should really use PlatformToolset to set things straight on VS2015 (if that is confirmed working, I didn't try yet). |
|
@saper, I don't think you are understanding it correctly |
|
On Wed, 20 Jan 2016, Adeel Mujahid wrote:
@saper, I don't think you are understanding it correctly
v120 = VS2013
v140 = VS2015.
If someone has BuiltTools2015 installed, they will not be able to build node-sass because it doesn't have v120 at all. If someone has VS2015 installed without VS2013 toolset (which is optional in the install menu), they will missed out too. Having a compiler flag in gyp is not yet-another-hassle.
We actually need to tell people using node-gyp to use vs2013 or vs2015
with platformtoolset v120 (which is as you say
the C++ compiler taken out of vs2013).
I cannot have C++ compatibility otherwise and that's what we are using.
|
If someone has VS2015 installed without legacy stuff or BuildTools2015, they will not have v120 targets. |
|
On Wed, 20 Jan 2016, Adeel Mujahid wrote:
> We actually need to tell people using node-gyp to use vs2013 or vs2015
If someone has VS2015 installed without legacy stuff or BuildTools2015, they will not have v120 targets.
In other words: I care less about people installing VS2015 with the default or limited settings
and compiling node-sass on their own
I care a lot more about folk using our binaries as is.
|
|
This change should not effect our binaries built from VS2013. |
|
On Wed, 20 Jan 2016, Adeel Mujahid wrote:
This change should not effect our binaries built from VS2013.
Let me be a bit blunt here:
I've wasted enough time to work on the "assumed" VS2015 compatibility.
Things were supposed to run smooth, they didn't.
I don't really care if somebody at the Microsoft tried node-sass
personally with VS2015 or not. They haven't obviously used XP
for their testing.
If Microsoft comes with a statement "We do support dynamic loading
of C++ DLLs compiled with Visual Studio 2015 into the address space
of the process runing C++ code compiled with Visual Studio 2013"
then I am fine.
The problem is, nobody at Microsoft will give us such a guarantee
for C++. They may guarantee compatibility for pure C modules only.
Keep in mind that
https://connect.microsoft.com/VisualStudio/feedback/details/1789709/visual-c-2015-runtime-broken-on-windows-server-2003-c-11-magic-statics
got closed as "(works) by design". The workaround is given for this particular
C++ feature.
As for this patch, I don't know why we are setting globally
that npm variable. Building node modules with v140 is simply
wrong, as long as v120 is what node is using.
I am not going to elaborate further on this, sorry.
|
|
TLDR: the change in appveyor.yml was not meant for this PR. I have reverted it back. #1352 will take care of AppVeyor. This change remained with zero side effects. |
Disables Thread-Safe for local static variables
Fix special eval case for `String_Schema`
Fixes #1283.