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

feat: add VCToolsVersion for msvs (#2906) #2910

Closed
wants to merge 1 commit into from

Conversation

tr-takatsuka
Copy link

@tr-takatsuka tr-takatsuka commented Oct 6, 2023

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

Add the ability to specify the 'MSVC toolset version' <VCToolsVersion> for the VisualStudio platform.

Usage example for binding.gyp

'targets': [
 {
   'configurations': {
     'Debug': {
       "msvs_configuration_attributes": {
         "VCToolsVersion": "14.36.32532",

ps. The following FAILED is displayed in npm test, but I think it is unrelated to this case.

  Failed test details
    1.'addon build simple addon in path with non-ascii characters'
      Name: TypeError
      Message: Callback must be a function. Received undefined
      Code: ERR_INVALID_CALLBACK
      Stack: TypeError [ERR_INVALID_CALLBACK]: Callback must be a function. Received undefined

Thank you and best regards.

Allows "MSVC Toolset version" <VCToolsVersion> to be specified for VisualStudio platform.

Usage example for binding.gyp

```
'targets': [
 {
   'configurations': {
     'Debug': {
       "msvs_configuration_attributes": {
         "VCToolsVersion": "14.36.32532",
```
@cclauss
Copy link
Contributor

cclauss commented Oct 6, 2023

@rzhao271 @ryzokuken Please review. Would this land in this repo or in https://github.com/nodejs/gyp-next and then get vendored back into this repo?

@rzhao271
Copy link
Contributor

rzhao271 commented Oct 6, 2023

PR LGTM. Any change made to a gyp folder in this repo can be backported to gyp-next, whereas any change made to gyp-next would need to wait for another patch release to be brought back to here. At the same time, I've been putting gyp changes in gyp-next first.

@cclauss
Copy link
Contributor

cclauss commented Oct 6, 2023

That is as clear as mud to me.

@rzhao271
Copy link
Contributor

rzhao271 commented Oct 6, 2023

I recommend first creating this PR in nodejs/gyp-next unless anyone has any objections.

@tr-takatsuka
Copy link
Author

tr-takatsuka commented Oct 10, 2023

Thank you all for your advice.

I am not yet knowledgeable enough about gyp-next, but I understand that submitting a pull request to gyp-next is the right way to go.

I will try my best to submit a pull request to gyp-next.

Hopefully this will be reflected in node-gyp as soon as possible.

@StefanStojanovic
Copy link
Contributor

I recommend first creating this PR in nodejs/gyp-next unless anyone has any objections.

I also agree with this approach. The change itself looks good to me. 👍

@cclauss
Copy link
Contributor

cclauss commented Oct 10, 2023

@StefanStojanovic Please go to https://github.com/nodejs/node-gyp/pull/2910/files and in the upper right click on Review files --> Approve --> Submit review

@StefanStojanovic
Copy link
Contributor

@cclauss I thought we were going to make this change in nodejs/gyp-next? In that case, does it make sense to approve this PR here?

@StefanStojanovic
Copy link
Contributor

Closing this, since nodejs/gyp-next#209 landed. Thank you @tr-takatsuka for the initiative!

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.

4 participants