FIX: Install-ChocolateyPath won't add a path that is a substring of a… #1275
Conversation
…n existing path New Feature: Check if path exists on disk before adding it. Default behavior is to ignore the install if the path doesnt exist. A Force parameter was added for weird cases where that would be desirable. New Feature: Install-ChocolateyPath now understands cmd-type variables. A complete pester test file is available. I didnt add it to the commit as I haven't seen any for powershell methogs in the project.
|
Test file is available here: aba1d04#diff-94c826cb9ab70d9c220a3d018358890f |
|
I see the Pester file. |
| # Copyright © 2011 - 2015 RealDimensions Software, LLC & original authors/contributors from https://github.com/chocolatey/chocolatey | ||
| # Copyright � 2017 Chocolatey Software, Inc. | ||
| # Copyright � 2015 - 2017 RealDimensions Software, LLC | ||
| # Copyright � 2011 - 2015 RealDimensions Software, LLC & original authors/contributors from https://github.com/chocolatey/chocolatey |
There was a problem hiding this comment.
It looks like you possibly changed line endings on this file. That will break it for authenticode signing.
| [parameter(ValueFromRemainingArguments = $true)][Object[]] $ignoredArguments | ||
| ) | ||
| Write-Debug "Running 'Install-ChocolateyPath' with pathToInstall:`'$pathToInstall`'"; | ||
| $originalPathToInstall = $pathToInstall | ||
| Set-StrictMode -Version 2 |
There was a problem hiding this comment.
This won't work. On PowerShell 4/5 systems, you have to install the v2 engine so that you can do this. This breaks those systems.
There was a problem hiding this comment.
Will remove. Thanks
There was a problem hiding this comment.
@ferventcoder In what circumstances exactly did you encounter problems with that? I run scripts containing
#Requires -Version 2
Set-StrictMode -Version 2
on Windows 10 1511 and 1607 systems and on Server 2012 R2 with both .NET 2.0 and PowerShell 2.0 removed and no issues occur. The docs for Set-StrictMode and #Requires imply that the number is interpreted as a minimum, not an exact version.
There was a problem hiding this comment.
@jberezanski remove the PowerShell v2 engine feature and #Requires -Version 2 fails. I've seen it before. Would love for the minimum to be true, but have not seen it implemented in that way.
There was a problem hiding this comment.
(The line of code in question was Set-StrictMode, not #Requires, but the behavior should be the same for both constructs.)
On what systems exactly have you seen that? I'm asking because, as I wrote before, this works as expected on all machines I have access to.
Besides, two widespread Chocolatey extensions (chocolatey-windowsupdate.extension and chocolatey-visualstudio.extension) use it and there has not been a single issue reported.
Or perhaps you were thinking about executing powershell.exe -version 2? This is asking specifically for the 2.0 engine, so naturally it fails if that engine is not available.
There was a problem hiding this comment.
I highly appreciate these changes. Perhaps you can review some of the notes I've left here and provide fixes. Have you had an opportunity to review our CONTRIBUTING documentation? It's pretty helpful and can save you lots of review fixes - https://github.com/chocolatey/choco/blob/master/CONTRIBUTING.md
| @@ -0,0 +1,126 @@ | |||
| . $PSScriptRoot\Install-ChocolateyPath.ps1 | |||
There was a problem hiding this comment.
This file is in a non-good location. If we are going to introduce Pester tests again (or I should say when we do, as they are in the original chocolatey/chocolatey), they need to be in a test project directory.
There was a problem hiding this comment.
Where do you suggest I create/put the pester file?
| $newPath = $pathToInstall + [System.IO.Path]::PathSeparator + (Get-EnvironmentVariable -Name 'Path' -Scope $pathType) | ||
|
|
||
| Set-EnvironmentVariable -Name 'PATH' -Value $newPath -Scope $pathType | ||
| Update-SessionEnvironment |
There was a problem hiding this comment.
This is already a PR up for this particular file and this appears to be repeating some of what is being handled there. You may wish to review some of their changes as well in this area.
There was a problem hiding this comment.
The PR in waiting (327) should be closed. ctaggart said he is unable to continue the PR (#327 (comment))
There was a problem hiding this comment.
I don't believe that was the one I was talking about.
|
It may look like come of the code is a "styling-rewrite" but the changes were made so the module could be pester-able. This means no more direct access to environment variables to allow mocking those calls. Also, the logic was hard to implement making minimal changes to the existing code... hence the rewrite. |
FIX: Install-ChocolateyPath won't add a path that is a substring of an existing path
FIX: Install-ChocolateyPath adds duplicate paths if the added path ends in a folder separator and the one already in the path does not.
New Feature: Check if path exists on disk before adding it. Default behavior is to ignore the install if the path doesnt exist. A Force parameter was added for weird cases where that would be desirable.
New Feature: Install-ChocolateyPath now understands cmd-type variables.
A complete pester test file is available. I didn't add it to the commit as I haven't seen any for powershell methods in the project.