Skip to content

Set Default PowerShell Version on Windows to Latest Available #2094

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

Merged
merged 43 commits into from
Aug 22, 2019

Conversation

SydneyhSmith
Copy link
Collaborator

@SydneyhSmith SydneyhSmith commented Jul 18, 2019

PR Summary

Updated the default PowerShell path on Windows to check for the latest version (previous behavior defaulted to Windows PowerShell). Also removed an SSL check that is no longer necessary.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
  • Summarized changes
  • PR has tests
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

Checks for PowerShell Core on Windows OS, if exists uses the latest version of PowerShell as the default PowerShell path
Removes SSL check
@SydneyhSmith
Copy link
Collaborator Author

Looks like the error might be a result of the intended design-- the default PowerShell path is no longer Windows (as previously expected)
image

@bergmeister
Copy link
Contributor

This makes the assumption that the user has chosen the default MSI installation path. Ideally we should query the registry for it. However, currently the installer does not create a registry key for it, so we can't do that now. Going forward we should fix that though using something like this: https://stackoverflow.com/a/13451210/1810304
cc @TravisEz13

@corbob
Copy link
Contributor

corbob commented Jul 28, 2019

Is it not here, in the registry @bergmeister?:

https://github.com/microsoft/vscode/blob/ffcb9e62b4365e652caeb05c80d4d7582833fdb8/src/vs/workbench/contrib/terminal/electron-browser/terminalService.ts#L120-L139

Well that explains why Win+R > pwsh launches a different pwsh than cmd > pwsh. Windows Run looks to App Paths before it looks to the Path variable, and so it's launching 7 Preview for me, while cmd looks to the path only and thus launches 6.2.2.

I believe what @bergmeister is referring to is the InstallLocation value in Computer\HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\{A55D1349-D294-43E8-B79A-1EC86DC3D18A} for 6.2.2, and Computer\HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Uninstall\{EA126E54-972B-4EC0-A823-5D37B1284BCE} for 7.0-preview. Personally the InstallLocation is where I would expect to find where the path actually is, but then you might need to maintain a list of GUIDs for each version?

Co-Authored-By: Christoph Bergmeister [MVP] <c.bergmeister@gmail.com>
@TylerLeonhardt
Copy link
Member

Yeah that makes sense - I think we should default to the install location (what Sydney already has) rather than switch to the App Paths.

src/platform.ts Outdated
? SysWow64PowerShellPath
: System32PowerShellPath;
psCoreInstallPath =
(platformDetails.isProcess64Bit ? process.env["ProgramFiles(x86)"] : process.env.ProgramFiles)
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor-nit but to maintain the coding style this line and line 81 should be indented four more spaces.

@bergmeister
Copy link
Contributor

Is it not here, in the registry @bergmeister?:

https://github.com/microsoft/vscode/blob/ffcb9e62b4365e652caeb05c80d4d7582833fdb8/src/vs/workbench/contrib/terminal/electron-browser/terminalService.ts#L120-L139

Oh, I forgot we had this one as well. Normally apps, register a key for the uninstall location but the one that you references is perfectly fine and does the job as well :-)
From a standardization perspective, people would still expect to find the InstallLocation registry key as most (>50% of installers do), therefore I'd still recommend the PS installer to do that because people usually do something like this: https://stackoverflow.com/a/55229210/1810304

SydneyhSmith and others added 5 commits August 6, 2019 15:38
Co-Authored-By: Tyler James Leonhardt <tylerl0706@gmail.com>
Co-Authored-By: Tyler James Leonhardt <tylerl0706@gmail.com>
Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM - just a single comment addition

Co-Authored-By: Tyler James Leonhardt <tylerl0706@gmail.com>
@SydneyhSmith SydneyhSmith merged commit 449f7a0 into PowerShell:master Aug 22, 2019
SydneyhSmith added a commit to SydneyhSmith/vscode-powershell that referenced this pull request Aug 22, 2019
Set Default PowerShell Version on Windows to Latest Available
TylerLeonhardt added a commit that referenced this pull request Aug 23, 2019
* Merge pull request #2094 from SydneyhSmith/master

Set Default PowerShell Version on Windows to Latest Available

* Update platform.test.ts

* Update src/platform.ts

Co-Authored-By: Tyler James Leonhardt <tylerl0706@gmail.com>
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.

6 participants