Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix VS compilation errors on VS < 2015 #1719

Closed
wants to merge 1 commit into from

Conversation

xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Nov 11, 2015

This header was removed in 827eac5 which subsequently broke node-sass.

Not 100% what our stance is on VS support. Since we have hacks in out build files for VS 2013 I suspect we intend to support it. If that's the case we should try to figure out an way to test against it in CI.

I can work around this in node-sass buy bumping our AppVeyor to VS 2015 but this still an open question for us. I'd love @am11 thoughts.

This header was removed in 827eac5 which subsequently broke
node-sass.
xzyfer added a commit to xzyfer/node-sass that referenced this pull request Nov 11, 2015
This is a work around for sass/libsass#1719 which removed a header
that broke compilation on VS 2013.
xzyfer added a commit to xzyfer/node-sass that referenced this pull request Nov 11, 2015
This is a work around for sass/libsass#1719 which removed a header
that broke compilation on VS 2013.
xzyfer added a commit to xzyfer/node-sass that referenced this pull request Nov 11, 2015
This is a work around for sass/libsass#1719 which removed a header
that broke compilation on VS 2013.
@am11
Copy link
Contributor

am11 commented Nov 11, 2015

TL;DR: With this magnitude of change if the VS2013 support is alive, that is excellent. On a more general note, VS2013 support is a good candidate to be deprecated at some point.


Story:

The vast majority of ISO C99 standard is finally supported in VS2015 as well as many C++11,14 and 17 features were implemented, which were not available in previous versions.

They have also made standalone built tools available nodejs/node-gyp#629 (comment) (so people don't have to download and install the whole VS to compile C/C++ especially in the upcoming windows dockers use-cases), and we have AppVeyor already supporting VS2015. VC guys have specifically tested node-sass with VS2015. 😄

Therefore, I see no point of keeping support for older version of VS moving forward. IMO, if it ever comes to point again where some additional code is required for _MSC_VER < 1900, we should drop old VC compiler support without hesitation and daringly consume C99 and the new language features of C++.

Considering:

  • We already are putting too much effort to provide Windows binaries for 7+ module versions x 2 architectures. Many other npm packages with native modules are not even providing a single prebuild binary, but 100% relying on install-time compilation.
  • In case of node-sass, install-time compilation is a fallback; if the npm install node-sass failed to fetch the binary.
  • On Windows, many node-sass users don't have VS and or Python installed and they rely on prebuild binaries.

So in saying "Instead of VS2013+ AND Python27, we are changing our Windows prerequisite policy for building node-sass to VS2015+ AND Python27" might not even break 1% of existing node-sass consumers on Windows.

@saper
Copy link
Member

saper commented Nov 12, 2015

If this change does not break VS2015, we should go for it I think.

@xzyfer
Copy link
Contributor Author

xzyfer commented Nov 12, 2015

The bigger issue is how do assert we don't break it again the future?
On 12 Nov 2015 3:14 am, "Marcin Cieślak" notifications@github.com wrote:

If this change does not break VS2015, we should go for it I think.


Reply to this email directly or view it on GitHub
#1719 (comment).

@asottile
Copy link
Member

This seems to break us here: sass/libsass-python#99

I can probably update that to use a newer VS without issue. I think most (if not all) of our windows consumers install via python wheels anyway.

@asottile
Copy link
Member

fwiw, we dropped VS2013 and are now using VS2015 :)

@mgreter
Copy link
Contributor

mgreter commented Jan 14, 2016

Is this still needed with latest master?
I checked with MSVC 2013 community!

@mgreter
Copy link
Contributor

mgreter commented Jan 17, 2016

Closing this since I can compile master with MSVC 2013. Plase re-open otherwise!

@mgreter mgreter closed this Jan 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants