Skip to content

AddToPath re-implementation in install-powershell.ps1#8081

Merged
TravisEz13 merged 10 commits intoPowerShell:masterfrom
glachancecmaisonneuve:install-powershell-addtopath-fix
Jan 17, 2019
Merged

AddToPath re-implementation in install-powershell.ps1#8081
TravisEz13 merged 10 commits intoPowerShell:masterfrom
glachancecmaisonneuve:install-powershell-addtopath-fix

Conversation

@glachancecmaisonneuve
Copy link
Contributor

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:

  • it expands all variables in the path to their full value (%SystemRoot% ==> C:\Windows for example) and saves the new path with all variables expanded.
  • it changes the path type from REG_EXPAND_SZ to REG_SZ, disabling all variables (wich are now gone in any event).
  • it fails o add the path to the newly installed powershell if the user has a path that has the same base path (it won't add c:\pwsh of you have c:\pwsh6 in your path).
  • it will add a duplicate path for path values that have a DirectorySeparatorChar at the end (for example, it will add c:\pwsh6\ in your path even if you have c:\pwsh6).

This PR remedies all of the above by

  • strictly adding the new path to the current path value, without changing the current path value in any other way.
  • saving the new path with the same REG_EXPAND_SZ/REG_SZ it had before, if appropriate.
  • not adding a duplicate path by understanding that C:\1 and C:\1\ is the same path.

Non-Breaking change

  • The updated script will attempt to add the path to the newly installed pwsh in the user's path. This is entirely new, as the previous version only attempted to save the path in the machine wide path, and very likely failed most of the time, lacking sufficient rights to do so.
  • This last bit also explains why these issues and side-efects have gone unnoticed until now ... you have to be in an elevated console for the path-rewrite to happen. Since the script is mostly used while building (via build.psm1), and since most people don't use an elevated console for building, ... I beleive few users were affected.

An alternate fix (dont do anything for -AddToPath when $WinEnv -eq $true)

  • I am not convinced of the pertinence of saving the path to the registry. It isn't required by the build script (adding the path to the current process is enough to satisfy it's needs). It depends who the target audience for install-powershell.ps1 is. If the script is used by the public at large as an easy way to install powershell, then it's pertinent. If it's really only used by the build script, then I wasted quite a bit of time getting this right, and the -AddToPath switch should not do anything for $WinEnv -eq $true.

@TravisEz13 TravisEz13 self-assigned this Oct 29, 2018
TravisEz13 and others added 2 commits October 30, 2018 06:32
Co-Authored-By: glachancecmaisonneuve <glachance@cmaisonneuve.qc.ca>
Co-Authored-By: glachancecmaisonneuve <glachance@cmaisonneuve.qc.ca>
@TravisEz13 TravisEz13 added the Breaking-Change breaking change that may affect users label Oct 30, 2018
Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

see updated comments

TravisEz13 and others added 3 commits October 30, 2018 21:52
Co-Authored-By: glachancecmaisonneuve <glachance@cmaisonneuve.qc.ca>
Co-Authored-By: glachancecmaisonneuve <glachance@cmaisonneuve.qc.ca>
@glachancecmaisonneuve
Copy link
Contributor Author

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.

@TravisEz13
Copy link
Member

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.
Thanks.

@iSazonov iSazonov added the CL-Tools Indicates that a PR should be marked as a tools change in the Change Log label Nov 16, 2018
@glachancecmaisonneuve
Copy link
Contributor Author

glachancecmaisonneuve commented Nov 24, 2018

I just found out that the Set-ItemProperty cmdlet honnors the registryvaluekind when provided.

Would you have preferred that over a call to [Microsoft.Win32.RegistryKey]::SetValue?

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?

Copy link
Member

Choose a reason for hiding this comment

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

should we return after this if Add-Path was successful?

Copy link
Member

Choose a reason for hiding this comment

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

Add a description of what each function does and the expected values of the parameters.

Copy link
Member

Choose a reason for hiding this comment

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

NIT: convention is to have a space after the # in a comment

Copy link
Member

Choose a reason for hiding this comment

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

NIT: convention is to have a newline before a comment
(NIT = minor issues not blocking the PR)

Copy link
Member

Choose a reason for hiding this comment

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

blocks of comments just have one newline before the first comment

@TravisEz13
Copy link
Member

Thanks for the contribution. This fixes a regression in the code I introduced.

@TravisEz13 TravisEz13 removed the Breaking-Change breaking change that may affect users label Nov 26, 2018
@TravisEz13
Copy link
Member

saving the path is needed even in the build systems we use. They often spawn new processes from a central build orchestrator.

@TravisEz13
Copy link
Member

Sorry, We were focusing on the release of 6.1.1 and 6.0.5 and then I took vacation.
I have two comments that I would like addressed before merging, but I think the change is good. There are two additional style issues with the label NIT: that I would love if you fixed but I'm won't block the PR for.

@glachancecmaisonneuve
Copy link
Contributor Author

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

@stale
Copy link

stale bot commented Jan 6, 2019

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.
Thank you for your contributions.
Community members are welcome to grab these works.

@stale stale bot added the Stale label Jan 6, 2019
- 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
@stale stale bot removed the Stale label Jan 7, 2019
@TravisEz13
Copy link
Member

The script is failing on windows.

@TravisEz13 TravisEz13 merged commit 80cabc4 into PowerShell:master Jan 17, 2019
@TravisEz13
Copy link
Member

Thanks for your contribution. Feel free to ping me if I lose you PR.

@glachancecmaisonneuve glachancecmaisonneuve deleted the install-powershell-addtopath-fix branch March 23, 2019 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-Tools Indicates that a PR should be marked as a tools change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

Comments