Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Use Visual Studio 2015 for Node 6+ #1870

Merged
merged 1 commit into from
Jan 28, 2017
Merged

Use Visual Studio 2015 for Node 6+ #1870

merged 1 commit into from
Jan 28, 2017

Conversation

xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Jan 27, 2017

Node change to Visual Studio 2015 when Node 6 went to LTS.

See nodejs/node#7989
Fixes #1854

Copy link
Contributor

@nschonni nschonni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beat me to it 😄
Two things but I can't comment on the line since it's out of the diff

  • The tag matrix above should be updated too
  • Confirm if the --msvs_version=2013 should be changed

@xzyfer
Copy link
Contributor Author

xzyfer commented Jan 27, 2017

The tag matrix above should be updated too

Yeah, I'm just confirming this syntax is even valid. The docs are non-existent on this use case.

Confirm if the --msvs_version=2013 should be changed

True.

@nschonni
Copy link
Contributor

Not sure if it would be faster to spit the matrix with os: Visual Studio 2015

@xzyfer
Copy link
Contributor Author

xzyfer commented Jan 27, 2017

You might be right. It's easy enough to test.

@@ -114,9 +114,9 @@
- nodejs_version: 0.12
- nodejs_version: 4
- nodejs_version: 6
APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015
os: Visual Studio 2013
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think you got this reversed. These ones should be 2015

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep 😞

@xzyfer
Copy link
Contributor Author

xzyfer commented Jan 27, 2017

The builds appear to be comparable. I've opted ENV vars just because GYP_MSVS_VERSION needs to be an ENV var also.

image

image

@xzyfer
Copy link
Contributor Author

xzyfer commented Jan 27, 2017

Paging @am11 and @saper for a sanity check

@nschonni
Copy link
Contributor

Sounds good, I'm just playing with something else for fun

@nschonni
Copy link
Contributor

Not sure if it's picking it up properly, it looks like it's going back to auto https://ci.appveyor.com/project/sass/node-sass/build/2412/job/eecvsxmwbfyaawjy#L243

@nschonni
Copy link
Contributor

Not sure if it was changing the os: from the global to the matrix, or adding back the --msvcs flag, but this was working nschonni@a2f420f

@xzyfer
Copy link
Contributor Author

xzyfer commented Jan 27, 2017

The node-gyp code suggest GYP_MSVS_VERSION should be enough. If msvs_version defaults to "auto" if not set. If the msvs version is auto when GYP_MSVS_VERSION is looked up. I'm digging in a little more atm.

Node change to Visual Studio 2015 when Node 6 went to LTS.

See nodejs/node#7989
Fixes sass#1854
@xzyfer
Copy link
Contributor Author

xzyfer commented Jan 27, 2017

Looks like the ENV vars don't cascade which is what was tripping me up.

Copy link
Contributor

@am11 am11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks Mike! 👍

@xzyfer xzyfer merged commit 167812b into sass:master Jan 28, 2017
@xzyfer xzyfer deleted the vs2015 branch January 28, 2017 00:02
xzyfer added a commit to xzyfer/node-sass that referenced this pull request Jan 28, 2017
The AppVeyor config was updated in sass#1870 but I missed some changes
for the release config.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants