Preserve PSModulePath environment variable #10300

iSazonov · 2019-08-05T13:47:21Z

PR Summary

Fix #9957
Fix #9921
Fix #7082
Fix #6850
Fix #10620

For reference #9845

The fix was approved by PowerShell Committee.
The fix is that now we do not change PSModulePath environment variable. Now we only read it and add to a module path cache. Also we add Powershell standard paths in the cache.

Because now we don't change PSModulePath environment variable we have no needs to manipulate strings and the module path cache is List<string>.

There was discovered some bugs. I hope they is related only tests.

We use string.Contains() but correct way is to use PathContainsSubstring()

PowerShell/src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs 8000

Lines 1357 to 1363 in 17fb524

    
           // If on Windows, we want to add the System32 Windows PowerShell module directory 
        
           // so that Windows modules are discoverable 
        
           string windowsPowerShellModulePath = GetWindowsPowerShellPSHomeModulePath(); 
        
           if (!newModulePathString.Contains(windowsPowerShellModulePath, StringComparison.OrdinalIgnoreCase)) 
        
           { 
        
               newModulePathString += Path.PathSeparator + windowsPowerShellModulePath; 
        
           }

As result the system path was not added in tests because we added a module path like c:\tmp\root\modulename then we were tried to add c:\tmp\root (come from test hook) but string.Contains() returns true.

We don't removed a root module in CompatiblePSEditions.Module.Tests.ps1

PowerShell/test/powershell/Modules/Microsoft.PowerShell.Core/CompatiblePSEditions.Module.Tests.ps1

Lines 735 to 738 in 17fb524

    
           AfterEach { 
        
               Get-Module $moduleName | Remove-Module -Force 
        
               Restore-ModulePath 
        
           }

As result some tests below failed after the PR fix. I hope it is only test issue - please review in depth.

Perhaps it is not bug but we need a conclusion. If we add a path to module in PSModulePath and then add a root path from the module path like c:\tmp\root\modulename and c:\tmp\root - Get-Module -Listavailable -All returns the module twice. (No regression - Windows PowerShell works the same way.)

The PR fix doesn't remove path to PSHome/Module if 7.0 starts from 6.0:

I think it makes no sense to remove the path because 6.0 will deprecated (EOL) soon.

PR Context

See #9957

PR Checklist

src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs

iSazonov · 2019-08-06T10:29:09Z

Current issues with test:

DSC tests failed on Linux. It is not clear why we run its only on Linux. /cc @amitsara
Remove-Module test failed on Windows - really an issue is with Get-Module, it returns 2 values instead of 1. It is very amazing because Get-Module tests was passed. Locally the tests is passed too.
Some HelpSystem tests failed on Windows. Don't know why and how fix.

@SteveL-MSFT @daxian-dbw @rjmholt @adityapatwardhan I hope you help.

@mklement0 @KirkMunro @Jaykul for information.

src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs

rjmholt · 2019-08-07T20:40:54Z

It is very amazing because Get-Module tests was passed. Locally the tests is passed too.

That sounds like we have different expectations in different tests. Ideally we should fix that.

I think it makes no sense to remove the path because 6.0 will deprecated (EOL) soon.

The powershell/6/ module path would also be for 6.1 or 6.2 though -- but I might have misunderstood what you're saying here. Could you clarify? (Also worth noting that deprecated =/= EOL; the first means it's not recommended for use, but continues to be supported, while the second means there's no support).

Would you be able to describe the nature of this change @iSazonov? I understand the issues it addresses, and got some information from the description, but would you be able to write a paragraph at the top of the description describing:

what specific behaviours this changes
- specifically, not just what is fixed, but what the new behaviour is, for each fixed item
any implementation details that are important

iSazonov · 2019-08-08T12:41:49Z

The powershell/6/ module path would also be for 6.1 or 6.2 though

I meant all 6.*. After 7.0 release 6.* will be out of support soon - it is not LTS version. And I suggest don't clean up the specific 6.* module path. (But we could address this if needed in follow PR. The problem here is that an user could want to preserve the path)

Would you be able to describe the nature of this change

There is reworded PowerShell Committee conclusion in #9957 and original PowerShell/PowerShell-RFC#133 (comment)
In short PowerShel shouldn't change PSModule environment variable to exclude impact on other PowerShell versions (side-by-side scenario). Now PowerShell creates internal module path cache and add there all the version and session specific paths and paths from PSModulePath in right order. The order is the same as previously - I don't change the logic.

SteveL-MSFT · 2019-11-11T21:21:52Z

@iSazonov after some discussion with @JamesWTruher on what we think is the right behavior, I'm going to author a small RFC

SteveL-MSFT · 2019-11-12T01:16:38Z

Please take look at PowerShell/PowerShell-RFC#233, I'll submit a code PR to validate my logic

iSazonov · 2019-11-12T04:16:34Z

@SteveL-MSFT I have take look the RFC. I see you consider only SxS for Core vs Windows PowerShell, not third-party soft. In the case my proposal resolve all the SxS scenarios in simplest way. I still don't understand why we should expose modified PSModulePath environment variable. If needed for UX and scripts we could introduce $PSModulePath variable (also we discussed cmdlets to manipulate paths).

SteveL-MSFT · 2019-11-14T00:47:22Z

@iSazonov we modify $env:PSModulePath because existing utilities use it to determine module search path

iSazonov · 2019-11-14T08:46:49Z

because existing utilities use it

What is the utilities?

SteveL-MSFT · 2019-11-15T02:01:35Z

@iSazonov I don't have any specific utilities in mind, but a search on GitHub shows lots of scripts manipulating $env:PSModulePath

iSazonov · 2019-11-15T05:34:28Z

@SteveL-MSFT I see that they add custom paths but not extract "system"/PowerShell paths.

SteveL-MSFT · 2019-11-21T21:34:23Z

@iSazonov I believe #11057 should resolve the related issues and also conforms to the desire by @PowerShell/powershell-committee to respect the env var.

iSazonov · 2019-11-22T03:47:37Z

@SteveL-MSFT Thanks! I hope the long story is resolved now for years :-)

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Aug 5, 2019

iSazonov added this to the 7.0.0-preview.3 milestone Aug 5, 2019

iSazonov requested review from daxian-dbw, rjmholt, SteveL-MSFT and adityapatwardhan August 5, 2019 13:47

iSazonov requested a review from PaulHigin as a code owner August 5, 2019 13:47

PoshChan reviewed Aug 5, 2019

View reviewed changes

8000 src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs Show resolved Hide resolved

PoshChan reviewed Aug 5, 2019

View reviewed changes