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

Code: Fixes include guard condition #1495

Merged
merged 1 commit into from
Aug 30, 2015
Merged

Code: Fixes include guard condition #1495

merged 1 commit into from
Aug 30, 2015

Conversation

am11
Copy link
Contributor

@am11 am11 commented Aug 29, 2015

With this change, we can compile src/c99func.c without any extra condition in down-streams' build process.

With this change, we can compile `src/c99func.c` without any extra
condition in down-streams' build process.
@xzyfer xzyfer added this to the 3.3 milestone Aug 30, 2015
xzyfer added a commit that referenced this pull request Aug 30, 2015
Fixes include guard condition for VS
@xzyfer xzyfer merged commit 9ed0093 into sass:master Aug 30, 2015
@saper
Copy link
Member

saper commented Aug 30, 2015

Well, the whole point of the c99func.c is not to compile that file when not needed... you can leave ifdefs if you Iike but I'd leave msbuild conditional rules in place.

@am11
Copy link
Contributor Author

am11 commented Aug 30, 2015

Yeah I remember the point of c99func.c. However, the point of this PR is to save every downstream from checking the condition to be able to build with both VS2013 and VS2015. See https://github.com/am11/node-sass/blob/request-ci-artifacts/src/libsass.gyp#L82-L84, there is no decent condition to check MSC_VER there and this approach saved me sometime and hopefully will save others' as well.

@saper
Copy link
Member

saper commented Aug 30, 2015

That is why I think we can keep MSBuild rule and leave conditionals inside. I will deal with libsass.gyp once our gyp fully supports vs2015.
I want to make sure we don't get any linker errors when not linking c99func.obj

@am11
Copy link
Contributor Author

am11 commented Aug 30, 2015

I found this guy trying to figure out a way to detect toolset version in gyp: http://stackoverflow.com/q/26311665/863980 (received no answer).
Then tested with VS2013 (on AppVeyor) and VS2015 (both locally and on AppVeyor) with this change, there is no linker error so far; compilation succeeds (only some tests are failing atm).

@saper
Copy link
Member

saper commented Aug 31, 2015

Here it goes: saper/node-sass@edf7127 - can you check that with VS2015 if c99func.c gets excluded from the build? This is sass/node-sass#1040

@am11
Copy link
Contributor Author

am11 commented Sep 1, 2015

Yup c99func gets excluded from VS2015, when ps: $env:GYP_MSVS_VERSION=2015. Nicely done!

BTW, AppVeyor is also building with VS2015, but the file pangyp was emitting VS2013 proj files, with c99func. I have sent a PR on your fork.

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.

3 participants