Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix reading msvs version on Windows #2644

Merged
merged 2 commits into from
Jun 25, 2023

Conversation

jgcook935
Copy link
Contributor

@jgcook935 jgcook935 commented Apr 13, 2022

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

Ever since #2547 went in for version 9 our Windows machines can't properly read msvs_version from the .npmrc. Since names with '_' are now changed to '-' we need to change the opts definition. If there's a different way you'd like to handle this let me know and I can update.

@cclauss
Copy link
Contributor

cclauss commented May 25, 2023

@StefanStojanovic Your review, please.

Also, what are the currently supported values for MSVS?

Why is this in our tests?

echo 'GYP_MSVS_VERSION=2015' >> $Env:GITHUB_ENV
echo 'GYP_MSVS_OVERRIDE_PATH=C:\\Dummy' >> $Env:GITHUB_ENV

@cclauss cclauss changed the title Fix reading msvs version on windows Fix reading msvs version on Windows May 25, 2023
@StefanStojanovic
Copy link
Contributor

Why is this in our tests?

echo 'GYP_MSVS_VERSION=2015' >> $Env:GITHUB_ENV
echo 'GYP_MSVS_OVERRIDE_PATH=C:\\Dummy' >> $Env:GITHUB_ENV

I've made a draft PR to test this. From what I see, it is required for Python tests to pass. The same code exists in the visual-studio workflow file, but since Python tests are not run there it can be removed. By the way, the last change on that part of the code was done 3 years ago.

@StefanStojanovic
Copy link
Contributor

The change looks good. @jgcook935 could you rebase to the latest main (will require rewriting your test to mocha) so it can run through CI, please?

@StefanStojanovic
Copy link
Contributor

@jgcook935 I can go ahead and apply the changes I mentioned above and move this forward. Please let me know if you want to do it yourself. Thanks.

@StefanStojanovic
Copy link
Contributor

After rebasing and rewriting the test to mocha everything's good. I'm planning to land this on Friday unless someone objects before that.

@StefanStojanovic StefanStojanovic merged commit 53c99ae into nodejs:main Jun 25, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants