This repository has been archived by the owner. It is now read-only.

Improved VS detection. Fix for GYP_MSVS_VERSION #504

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
@gigi81
Contributor

gigi81 commented Jul 25, 2012

Added visual studio 2010/2008 detection and set also the GYP_MSVS_VERSION bsed on the version detected

Improved VS detection. Fix for GYP_MSVS_VERSION
Added visual studio 2010/2008 detection and set also the GYP_MSVS_VERSION bsed on the version detected
@mscdex

This comment has been minimized.

Show comment Hide comment
@mscdex

mscdex Jul 25, 2012

Contributor

+100

I can't tell you how many times I've opened up a command prompt to use vcbuild, gotten errors of one kind or another (because GYP_MSVS_VERSION wasn't set), and then had to go and look up the environment variable name from one of the libuv github issues about the problem.

Yes, one could set a permanent environment variable, but it's better if the build tool creates the temporary variable since this is only a gyp-specific (and gyp version-specific too) issue.

Contributor

mscdex commented Jul 25, 2012

+100

I can't tell you how many times I've opened up a command prompt to use vcbuild, gotten errors of one kind or another (because GYP_MSVS_VERSION wasn't set), and then had to go and look up the environment variable name from one of the libuv github issues about the problem.

Yes, one could set a permanent environment variable, but it's better if the build tool creates the temporary variable since this is only a gyp-specific (and gyp version-specific too) issue.

@bnoordhuis

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Jul 26, 2012

Contributor
Contributor

bnoordhuis commented Jul 26, 2012

@piscisaureus

This comment has been minimized.

Show comment Hide comment
@piscisaureus

piscisaureus Jul 26, 2012

Member

I understand that the GYP_MSVS_VERSION fix is important, but what's the point of detecting vs2008 if we can't build with it?

Member

piscisaureus commented Jul 26, 2012

I understand that the GYP_MSVS_VERSION fix is important, but what's the point of detecting vs2008 if we can't build with it?

Setting GYP_MSVS_VERSION on the gyp_uv
We set GYP_MSVS_VERSION to 2010 if not found on the environment variables
@gigi81

This comment has been minimized.

Show comment Hide comment
@gigi81

gigi81 Jul 26, 2012

Contributor

The point is that is that it should be possible to build also with visual studio 2008 if someone hasn't 2010. There is no particular reason why it shouldn't build (at least that I'm aware of). The only problem from what I understood is with the inttypes http://code.google.com/p/msinttypes/. So this can be easily fixed. But it has nothing to do with the build script that it should just be smart enough to detect the latest available system compiler.

It's also a good example on how to add 2012 when it will be supported

Contributor

gigi81 commented Jul 26, 2012

The point is that is that it should be possible to build also with visual studio 2008 if someone hasn't 2010. There is no particular reason why it shouldn't build (at least that I'm aware of). The only problem from what I understood is with the inttypes http://code.google.com/p/msinttypes/. So this can be easily fixed. But it has nothing to do with the build script that it should just be smart enough to detect the latest available system compiler.

It's also a good example on how to add 2012 when it will be supported

@piscisaureus

This comment has been minimized.

Show comment Hide comment
@piscisaureus

piscisaureus Aug 13, 2012

Member

LGTM.
@gigi81, can you sign the CLA? http://nodejs.org/cla.html

For the record, I'd take a patch that adds VS2008 support

Member

piscisaureus commented Aug 13, 2012

LGTM.
@gigi81, can you sign the CLA? http://nodejs.org/cla.html

For the record, I'd take a patch that adds VS2008 support

@piscisaureus

This comment has been minimized.

Show comment Hide comment
@piscisaureus

piscisaureus Aug 30, 2012

Member

ping @gigi81

Member

piscisaureus commented Aug 30, 2012

ping @gigi81

@gigi81

This comment has been minimized.

Show comment Hide comment
@gigi81

gigi81 Aug 31, 2012

Contributor

I'm on it. Waiting for my project manager to approve it.

Contributor

gigi81 commented Aug 31, 2012

I'm on it. Waiting for my project manager to approve it.

@gigi81

This comment has been minimized.

Show comment Hide comment
@gigi81

gigi81 Sep 20, 2012

Contributor

I signed and sent the cla. You can merge it now...

Contributor

gigi81 commented Sep 20, 2012

I signed and sent the cla. You can merge it now...

@piscisaureus

This comment has been minimized.

Show comment Hide comment
@piscisaureus

piscisaureus Sep 20, 2012

Member

Thanks. I changed it a little bit to make it a little simpler. Also, please take care to not mix tabs and spaces next time.
I landed this patch together with #514 in 5bfb7c9.

Unfortunately the test runner doesn't link when a static library is being built, but this is not a regression.

Member

piscisaureus commented Sep 20, 2012

Thanks. I changed it a little bit to make it a little simpler. Also, please take care to not mix tabs and spaces next time.
I landed this patch together with #514 in 5bfb7c9.

Unfortunately the test runner doesn't link when a static library is being built, but this is not a regression.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.