AddToPath re-implementation in install-powershell.ps1#8081
AddToPath re-implementation in install-powershell.ps1#8081TravisEz13 merged 10 commits intoPowerShell:masterfrom glachancecmaisonneuve:install-powershell-addtopath-fix
Conversation
Co-Authored-By: glachancecmaisonneuve <glachance@cmaisonneuve.qc.ca>
Co-Authored-By: glachancecmaisonneuve <glachance@cmaisonneuve.qc.ca>
Co-Authored-By: glachancecmaisonneuve <glachance@cmaisonneuve.qc.ca>
Co-Authored-By: glachancecmaisonneuve <glachance@cmaisonneuve.qc.ca>
|
Sorry it took so long to implemented the simple changes you were asking for. I am not accustomed to this form of review were code changes can be accepted... and forgot that I could just push the requested changes by myself, just as usual. It must have seemed like I was either being uncommonly stubborn/dimwitted/uncooperative. Thank you for taking the time and your efforts towards integrating this code review. Reminder: There is one proposed change that has not been implement, i.e. using [Environment]::SaveEnvironmentVariable to write to the registry as it lacks the ability to assign a RegistryValueKind. |
|
I restarted appveyor due to an intermittent failure. Thanks for your changes. Yes, I know understand why your are using the Registry API instead of the environment. |
|
Nevermind. There is no way to get an unexpanded path out of the registry save for a direct call to [Microsoft.Win32.RegistryKey]::GetKey. My part is done right? I don't have to add anything for this to eventually go through? |
tools/install-powershell.ps1
Outdated
There was a problem hiding this comment.
should we return after this if Add-Path was successful?
tools/install-powershell.ps1
Outdated
There was a problem hiding this comment.
Add a description of what each function does and the expected values of the parameters.
tools/install-powershell.ps1
Outdated
There was a problem hiding this comment.
NIT: convention is to have a space after the # in a comment
tools/install-powershell.ps1
Outdated
There was a problem hiding this comment.
NIT: convention is to have a newline before a comment
(NIT = minor issues not blocking the PR)
There was a problem hiding this comment.
blocks of comments just have one newline before the first comment
|
Thanks for the contribution. This fixes a regression in the code I introduced. |
|
saving the path is needed even in the build systems we use. They often spawn new processes from a central build orchestrator. |
|
Sorry, We were focusing on the release of |
|
Sorry it's the end of the term and I haven't had much time at all. I'll implement the requested changes as soon as i'm able |
|
This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days. |
- Test-PathInRegistryPath was in fact a validator - Update-Path should not be a function but in the script's body - Add-PathToSettings was the only real function Added: - parameter validation - function documentation Other: - formatting changes as requested
|
The script is failing on windows. |
|
Thanks for your contribution. Feel free to ping me if I lose you PR. |
PR Summary
This PR is a re-implementation of the -AddToPath switch in tools/install-powershell.ps1, without the side-effects the current implementation has. The changes only affects windows users.
Summarized changes
As currently implemented, the -AddToPath switch has quite a few undesirable side-effects and 1 or 2 issues that prevents it from adding the path to the newly installed powershell to the registry. Specifically:
This PR remedies all of the above by
Non-Breaking change
An alternate fix (dont do anything for -AddToPath when $WinEnv -eq $true)