iframe-proxy

glachancecmaisonneuve · 2018-10-19T03:28:49Z

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.

Co-Authored-By: glachancecmaisonneuve <glachance@cmaisonneuve.qc.ca>

TravisEz13

see updated comments

Co-Authored-By: glachancecmaisonneuve <glachance@cmaisonneuve.qc.ca>

glachancecmaisonneuve · 2018-10-31T08:46:14Z

TravisEz13 · 2018-11-01T19:54:24Z

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.

glachancecmaisonneuve · 2018-11-24T22:29:27Z

~~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?

TravisEz13 · 2018-11-26T20:52:25Z

+            (-not ($Path.StartsWith($env:LOCALAPPDATA)))) {
+            try {
+                [string] $KeyName = 'SYSTEM\CurrentControlSet\Control\Session Manager\Environment\'
+                Add-Path([Microsoft.Win32.Registry]::LocalMachine.OpenSubKey($KeyName), $Path)


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

TravisEz13 · 2018-11-26T20:53:26Z

+    return $False
+}
+
+Function Add-Path([Microsoft.Win32.RegistryKey] $Key, [string] $Path) {


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

TravisEz13 · 2018-11-26T20:54:34Z

 }

+Function Test-PathInRegistryPath([string] $Path) {
+    #Remove ending DirectorySeparatorChar for comparison purposes


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

TravisEz13 · 2018-11-26T20:55:24Z

+    #Remove ending DirectorySeparatorChar for comparison purposes
+    $Path = [System.Environment]::ExpandEnvironmentVariables([regex]::Replace($Path, '\\;$|\\$', ''));
+    $InstalledPaths = @()
+    #[System.Environment]::GetEnvironmentVariable automatically expands all variables


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

blocks of comments just have one newline before the first comment

TravisEz13 · 2018-11-26T20:57:10Z

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

TravisEz13 · 2018-11-26T20:58:48Z

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

TravisEz13 · 2018-11-26T21:02:20Z

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 · 2018-12-06T12:42:20Z

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 · 2019-01-06T06:22:03Z

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.

- 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

TravisEz13 · 2019-01-08T01:14:34Z

The script is failing on windows.

TravisEz13 · 2019-01-17T22:25:34Z

AddToPath fix

79f57fb

glachancecmaisonneuve requested review from PaulHigin, TravisEz13, adityapatwardhan, anmenaga and daxian-dbw as code owners October 19, 2018 03:28

kvprasoon reviewed Oct 19, 2018

View reviewed changes

Comment thread tools/install-powershell.ps1

Update install-powershell.ps1

ed45583

kvprasoon approved these changes Oct 23, 2018

View reviewed changes

TravisEz13 reviewed Oct 29, 2018

View reviewed changes

Comment thread tools/install-powershell.ps1 Outdated

TravisEz13 requested changes Oct 29, 2018

View reviewed changes

Comment thread tools/install-powershell.ps1 Outdated

Comment thread tools/install-powershell.ps1 Outdated

Comment thread tools/install-powershell.ps1 Outdated

Comment thread tools/install-powershell.ps1 Outdated

TravisEz13 self-assigned this Oct 29, 2018

TravisEz13 and others added 2 commits October 30, 2018 06:32

Update tools/install-powershell.ps1

b7a1bfb

Co-Authored-By: glachancecmaisonneuve <glachance@cmaisonneuve.qc.ca>

Update tools/install-powershell.ps1

c7ceae0

Co-Authored-By: glachancecmaisonneuve <glachance@cmaisonneuve.qc.ca>

TravisEz13 added the Breaking-Change breaking change that may affect users label Oct 30, 2018

TravisEz13 reviewed Oct 30, 2018

View reviewed changes

Comment thread tools/install-powershell.ps1 Outdated

TravisEz13 reviewed Oct 30, 2018

View reviewed changes

Comment thread tools/install-powershell.ps1 Outdated

TravisEz13 reviewed Oct 30, 2018

View reviewed changes

Comment thread tools/install-powershell.ps1 Outdated

TravisEz13 requested changes Oct 30, 2018

View reviewed changes

TravisEz13 and others added 3 commits October 30, 2018 21:52

Update tools/install-powershell.ps1

e4be053

Co-Authored-By: glachancecmaisonneuve <glachance@cmaisonneuve.qc.ca>

Update tools/install-powershell.ps1

aaf53e5

Co-Authored-By: glachancecmaisonneuve <glachance@cmaisonneuve.qc.ca>

Code-Review changes

2fe05aa

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

TravisEz13 requested changes Nov 26, 2018

View reviewed changes

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

stale Bot added the Stale label Jan 6, 2019

stale Bot removed the Stale label Jan 7, 2019

Merge branch 'master' into install-powershell-addtopath-fix

7eea40e

A few style changes and bug fix with invalid destination

1539351

TravisEz13 approved these changes Jan 17, 2019

View reviewed changes

TravisEz13 merged commit 80cabc4 into PowerShell:master Jan 17, 2019

glachancecmaisonneuve deleted the install-powershell-addtopath-fix branch March 23, 2019 02:39

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

glachancecmaisonneuve commented Oct 19, 2018

PR Summary

Summarized changes

Non-Breaking change

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TravisEz13 left a comment

Choose a reason for hiding this comment

Uh oh!

glachancecmaisonneuve commented Oct 31, 2018

Uh oh!

TravisEz13 commented Nov 1, 2018

Uh oh!

glachancecmaisonneuve commented Nov 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TravisEz13 Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

TravisEz13 Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

TravisEz13 Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

TravisEz13 Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

TravisEz13 Nov 26, 2018

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Nov 26, 2018

Uh oh!

TravisEz13 commented Nov 26, 2018

Uh oh!

TravisEz13 commented Nov 26, 2018

Uh oh!

glachancecmaisonneuve commented Dec 6, 2018

Uh oh!

stale Bot commented Jan 6, 2019

Uh oh!

TravisEz13 commented Jan 8, 2019

Uh oh!

TravisEz13 commented Jan 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

glachancecmaisonneuve commented Nov 24, 2018 •

edited

Loading