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

add msvs 2017 support #1964

Closed
wants to merge 1 commit into from

Conversation

dothebart
Copy link
Contributor

No description provided.

@richardlau
Copy link
Member

Based on #1952 (comment) this isn't necessary? cc @joaocgreis

@dothebart
Copy link
Contributor Author

after opening the PR I saw that these changes already there in GYP3. However, as long as you use the node-gyp this is needed.

@joaocgreis
Copy link
Member

This shouldn't be needed, VS detection is not done by GYP but in https://github.com/nodejs/node-gyp/blob/master/lib/find-visualstudio.js.

@dothebart what issue did you see that needs this? Is the detection in node-gyp not working for you? Can you paste the output of a failing node-gyp rebuild --verbose here?

@dothebart
Copy link
Contributor Author

Ah, I don't compile V8 for nodejs - its https://github.com/ArangoDB/ArangoDB - so I don't have this file.
I now continued with gyp3, which has this. So if you don't want it, close it.

@joaocgreis
Copy link
Member

I guess we'll have to land something like this or remove autodetection from the GYP side, but I'd rather wait until we figure out how GYP will be managed here. @dothebart thanks for opening this PR and trying to help.

@joaocgreis joaocgreis closed this Nov 13, 2019
@dothebart
Copy link
Contributor Author

hm, asking a heretic question - how do you run .js before having a ready compiled V8?

@joaocgreis
Copy link
Member

I'm not sure I understand your question... Here in node-gyp, Node.js is required to run, so V8 is always already compiled.

For compiling Node.js itself in https://github.com/nodejs/node/ a different mechanism is used. It uses vswhere from the command line: https://github.com/nodejs/node/blob/ed401236f6988c6ad62a21dbbf808da83782bee0/vcbuild.bat#L247

@dothebart
Copy link
Contributor Author

ah, nice. a shellscript to tell the environment autodetection infrastructure about the infrastructure to work around the autodetection.
Is Gyp3 the way ahead?

@joaocgreis
Copy link
Member

There's discussion about the way forward in #1791. There was a PR to change to GYP3 #1845 but it was closed. From what I see, it looks like we'll be maintaining GYP here specifically to be used in node-gyp at least for a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants