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

Detect visual studio installation using VSSetup module #2957

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

jarig
Copy link
Contributor

@jarig jarig commented Dec 23, 2023

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

Detect visual studio installation using VSSetup module and Get-VSSetupInstance method as a first attempt, this approach works even in systems with Constrained language mode of the powershell.
Original type-base detection requires FullLanguage mode, which quite often not available on the enrolled devices.

@jarig jarig marked this pull request as ready for review December 23, 2023 21:11
@jarig jarig force-pushed the feat/vs-detection-improve branch 2 times, most recently from 0993aa5 to d54e550 Compare December 23, 2023 22:20
@jarig jarig force-pushed the feat/vs-detection-improve branch 2 times, most recently from c2eb63c to 4195a9e Compare December 26, 2023 13:59
@jarig
Copy link
Contributor Author

jarig commented Dec 26, 2023

Verified tests on both windows and linux now
image
image

@jarig
Copy link
Contributor Author

jarig commented Dec 26, 2023

Sry last linting issue also fixed. Btw mind to add .eslintrc into the project? VSCode not scanning automatically it looks

@jarig
Copy link
Contributor Author

jarig commented Jan 14, 2024

@cclauss any other activities or blockers to be done for the PR merge?

Copy link
Contributor

@StefanStojanovic StefanStojanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good overall. I have another PR that works on the same parts of the code, so this one should land first as my changes should be in the new findVisualStudio2017OrNewerUsingSetupModule function as well. I can rebase my changes on top of this one after it lands.

I've noticed something with the tests here though. There are no VSSetup files in fixtures for all of the fixtures we had before. The problem with it is that in our tests we will simulate the VSSetup function to return null when it should return something (eg. having 2019 installed). I'm aware this would be a tedious task to make all those fixtures and I'm not sure it's worth the effort, but just wanted to bring it up in case it comes up later.

lib/find-visualstudio.js Outdated Show resolved Hide resolved
lib/find-visualstudio.js Outdated Show resolved Hide resolved
lib/find-visualstudio.js Outdated Show resolved Hide resolved
lib/find-visualstudio.js Outdated Show resolved Hide resolved
lib/find-visualstudio.js Outdated Show resolved Hide resolved
test/test-find-visualstudio.js Outdated Show resolved Hide resolved
test/test-find-visualstudio.js Outdated Show resolved Hide resolved
test/test-find-visualstudio.js Outdated Show resolved Hide resolved
test/test-find-visualstudio.js Outdated Show resolved Hide resolved
test/test-find-visualstudio.js Outdated Show resolved Hide resolved
@StefanStojanovic
Copy link
Contributor

@jarig my PR is ready for landing. Do you have time to address the feedback I gave? If you are busy or unavailable, I can take over and make the required changes.

@jarig
Copy link
Contributor Author

jarig commented Feb 1, 2024

@jarig my PR is ready for landing. Do you have time to address the feedback I gave? If you are busy or unavailable, I can take over and make the required changes.

I'll try to make and finalize changes this week

…VSSetupInstance method, it works even in systems with Constrained language mode of the powershell
Copy link
Contributor

@StefanStojanovic StefanStojanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes LGTM. Thanks for addressing my comments @jarig. @lukekarrys (since you are a reviewer) if this LGTY too we can go ahead and merge this and I'll rebase and adapt my changes in #2959 accordingly.

lib/find-visualstudio.js Show resolved Hide resolved
lib/find-visualstudio.js Show resolved Hide resolved
@StefanStojanovic
Copy link
Contributor

I plan to land this tomorrow. If there are any concerns, please state them today. cc @lukekarrys

@StefanStojanovic StefanStojanovic merged commit 109e3d4 into nodejs:main Feb 9, 2024
35 checks passed
@Mapuppy09
Copy link

Good!

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

Successfully merging this pull request may close these issues.

4 participants