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.

Sign up for GitHub

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

Jump to bottom

This will look for Visual Studio in the Correct directory. #143479

Merged
merged 14 commits into from Feb 28, 2022
Merged

This will look for Visual Studio in the Correct directory. #143479

merged 14 commits into from Feb 28, 2022

Conversation

ghost
Copy link

@ghost ghost commented Feb 20, 2022

Previously running yarn install was looking for visual studio workload in the programfilesx86 folder now this fix will check for visual studio in the programfilesx86 in the first and then will look on programFiles folder.

This fix will help Visual Studio 2022 users as it is a 64b bit software so it goes to programFiles folder not in ProgramFilesx86.

This PR fixes #143478

ghost
ghost commented Feb 20, 2022
View reviewed changes
Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I have checked the code and it works in my case

@samarmohan
Copy link

I've tried this before, and the preinstall.js code works fine, but Electron and node-gyp don't like the 64 bit version.
There was an issue regarding this a while ago but I can't find it.

@samarmohan
Copy link

Ah well, I was behind on this issue. #138572 and #142667

@ghost
Copy link
Author

ghost commented Feb 20, 2022

Yaa this will actually fix the issue and the next time no other will have the same issue. Just waiting for the maintainer to merge this pr

@ghost ghost changed the title This is look for Visual Studio in the Correct directory. This will look for Visual Studio in the Correct directory. Feb 20, 2022
@ghost
Copy link
Author

ghost commented Feb 20, 2022

I tested the code and it worked totally fine. Let's see when the maintainer merges this pr.

@ghost
Copy link
Author

ghost commented Feb 20, 2022

After this fix the issue will completely resolve.

@ghost
Copy link
Author

ghost commented Feb 20, 2022

@paulacamargo25 please approve all the pending workflow.

@ghost
Copy link
Author

ghost commented Feb 20, 2022

@paulacamargo25 if there's something wrong in the code please let me know.

rzhao271
rzhao271 reviewed Feb 22, 2022
View reviewed changes
build/npm/preinstall.js Outdated Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Feb 23, 2022

@rzhao271 waiting for your approval.

@rzhao271
Copy link
Contributor

It is currently endgame/testing week. PR LGTM, but I plan on merging it next week.

@rzhao271 rzhao271 added this to the March 2022 milestone Feb 23, 2022
@rzhao271 rzhao271 added the engineering VS Code - Build / issue tracking / etc. label Feb 23, 2022
@ghost
Copy link
Author

ghost commented Feb 23, 2022

No issues. Waiting for the endgame to end.

@deepak1556
Copy link
Collaborator

Merging to address build failure https://github.com/microsoft/vscode/runs/5352655839?check_suite_focus=true

@deepak1556
Copy link
Collaborator

Thanks for the PR!

@deepak1556 deepak1556 merged commit d35ee52 into microsoft:main Feb 28, 2022
bpasero referenced this pull request Feb 28, 2022
@meganrogge
xterm@4.18.0-beta.15
8a9180f
@aasimkhan30 aasimkhan30 mentioned this pull request Mar 2, 2022
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@rzhao271 rzhao271

Labels
engineering VS Code - Build / issue tracking / etc.
Projects
None yet
Milestone
March 2022
4 participants
@samarmohan @rzhao271 @deepak1556 @paulacamargo25