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.

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

Merged

SydneyhSmith merged 43 commits into PowerShell:master from SydneyhSmith:master

Aug 22, 2019

Merged

Set Default PowerShell Version on Windows to Latest Available #2094

SydneyhSmith merged 43 commits into PowerShell:master from SydneyhSmith:master

Aug 22, 2019

Conversation

Copy link

Collaborator

@SydneyhSmith SydneyhSmith commented Jul 18, 2019 •

edited by TylerLeonhardt

Loading

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

SydneyhSmith added 3 commits

July 18, 2019 09:28


 Update platform.ts

93b9e20

Checks for PowerShell Core on Windows OS, if exists uses the latest version of PowerShell as the default PowerShell path


 Update session.ts

f5e99d4

Removes SSL check


 Merge pull request #1 from SydneyhSmith/WindowsCoreCheck

fc0e83b

Windows core check

@SydneyhSmith SydneyhSmith requested review from rjmholt and TylerLeonhardt

July 18, 2019 16:44


 Update platform.ts

8e829bd

Copy link

Collaborator Author

SydneyhSmith commented Jul 18, 2019

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

corbob reviewed

View reviewed changes

src/platform.ts Outdated Show resolved Hide resolved

TylerLeonhardt reviewed

View reviewed changes

src/platform.ts Show resolved Hide resolved

TylerLeonhardt reviewed

View reviewed changes

src/platform.ts Outdated Show resolved Hide resolved

SydneyhSmith and others added 9 commits

July 19, 2019 09:38

@SydneyhSmith @TylerLeonhardt


 Update src/platform.ts

be41e5b

Co-Authored-By: Tyler James Leonhardt <tylerl0706@gmail.com>


 Update platform.ts

962bac7


 Update platform.test.ts

decb0ac


 Update platform.test.ts

29cde1b


 Update platform.ts

df67d6b


 Update platform.ts

2ddc9c0


 Update platform.test.ts

a373034


 Update platform.test.ts

a7931ae


 Update platform.test.ts

38a9cf7

bergmeister reviewed

View reviewed changes

src/platform.ts Outdated Show resolved Hide resolved

Copy link

Contributor

bergmeister commented Jul 27, 2019

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

rjmholt approved these changes

View reviewed changes

@TylerLeonhardt

Copy link

Member

TylerLeonhardt 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

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?

@SydneyhSmith @bergmeister


 Update src/platform.ts

9981f80

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

@TylerLeonhardt

Copy link

Member

TylerLeonhardt commented Jul 29, 2019

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

rkeithhill reviewed

View reviewed changes

src/platform.ts Outdated

? SysWow64PowerShellPath

: System32PowerShellPath;

psCoreInstallPath =

(platformDetails.isProcess64Bit ? process.env["ProgramFiles(x86)"] : process.env.ProgramFiles)

Copy link

Contributor

@rkeithhill rkeithhill Jul 29, 2019

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.

rkeithhill reviewed

View reviewed changes

src/platform.ts Outdated Show resolved Hide resolved

rkeithhill reviewed

View reviewed changes

src/platform.ts Show resolved Hide resolved

rkeithhill reviewed

View reviewed changes

src/platform.ts Outdated Show resolved Hide resolved

SydneyhSmith added 7 commits

July 31, 2019 11:45


 Update platform.test.ts

6b5e3a9


 Update platform.test.ts

b9b93ee


 Update platform.test.ts

c70aeca


 Update platform.test.ts

ed355ae


 Update platform.test.ts

d788bb7


 Update platform.test.ts

9efa922


 Update platform.test.ts

69f9342

TylerLeonhardt reviewed

View reviewed changes

src/platform.ts Outdated Show resolved Hide resolved

src/platform.ts Outdated Show resolved Hide resolved

src/platform.ts Outdated Show resolved Hide resolved

Copy link

Contributor

bergmeister commented Aug 2, 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

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

@SydneyhSmith @TylerLeonhardt


 Update src/platform.ts

bd8eeb0

Co-Authored-By: Tyler James Leonhardt <tylerl0706@gmail.com>

@SydneyhSmith @TylerLeonhardt


 Update src/platform.ts

059c782

Co-Authored-By: Tyler James Leonhardt <tylerl0706@gmail.com>


 Update platform.ts

35c5e10


 Update platform.ts

aa3f354


 Update platform.ts

9c5ca2c

TylerLeonhardt reviewed

View reviewed changes

src/platform.ts Show resolved Hide resolved

TylerLeonhardt approved these changes

View reviewed changes

Copy link

Member

@TylerLeonhardt TylerLeonhardt left a comment •

edited

Loading

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

@TylerLeonhardt TylerLeonhardt added the Backport-Legacy label

@SydneyhSmith @TylerLeonhardt


 Update src/platform.ts

b405e00

Co-Authored-By: Tyler James Leonhardt <tylerl0706@gmail.com>

rjmholt approved these changes

View reviewed changes

src/platform.ts Outdated Show resolved Hide resolved

src/platform.ts Outdated Show resolved Hide resolved

SydneyhSmith added 6 commits

August 22, 2019 09:26


 Update platform.ts

40f1649


 Update platform.ts

af96894


 Update platform.ts

b15caad


 Update platform.ts

768bacf


 Update platform.ts

b3df4bb


 Update platform.ts

1c620bb

@SydneyhSmith SydneyhSmith merged commit 449f7a0 into PowerShell:master

SydneyhSmith added a commit to SydneyhSmith/vscode-powershell that referenced this pull request


 Merge pull request PowerShell#2094 from SydneyhSmith/master

7f7db75

Set Default PowerShell Version on Windows to Latest Available

TylerLeonhardt added a commit that referenced this pull request

@SydneyhSmith @TylerLeonhardt


 Backporting #2094 to set default PowerShell to latest available ( #2149 )

5d509e4

* 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>

@rjmholt rjmholt added Backport-Complete and removed Backport-Legacy labels

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

6 participants

@SydneyhSmith @bergmeister @TylerLeonhardt @corbob @rkeithhill @rjmholt