Make GetWindowsPowerShellModulePath compatible with multiple PS installations #12280

anmenaga · 2020-04-07T23:16:19Z

PR Summary

NativeCommandProcessor and WinCompat rely on GetWindowsPowerShellModulePath function to filter out PS-Core-specific module paths so that WindowsPS loads its own version of modules instead of PS-Core versions.
However, when several side-by-side PS-Core installations (eg: MSI-installed, daily build, manual zip-based installation, etc...) started from each-other the filtering function does not work because it does not filter out the $PSHOME\Modules of the parent PS process.
The fix is to add additional check for each component of PSModulePath (that is set for WinPS process) - if it is has pwsh.exe in the parent directory, then it is considered another PS Core installation and this location is also filtered out.

Before the change:

PS C:\> $PSHome
c:\PowerShell-7.1.0-preview.1-win-x64

PS C:\> E:\UserA-PowerShell\src\powershell-win-core\bin\Debug\netcoreapp5.0\win7-x64\publish\pwsh.exe
PowerShell 7.0.0-preview.6-324-g5f89a10f5b872406a9ac6cb1fcdea8078ea6a4ca

PS C:\> $env:psmodulepath -split ';'
C:\Users\UserA\Documents\PowerShell\Modules
C:\Program Files\PowerShell\Modules
e:\UserA-powershell\src\powershell-win-core\bin\debug\netcoreapp5.0\win7-x64\publish\Modules
c:\powershell-7.1.0-preview.1-win-x64\Modules
C:\Program Files\WindowsPowerShell\Modules
C:\windows\system32\WindowsPowerShell\v1.0\Modules

PS C:\> powershell.exe -c "'WinPS PSModulePath:';`$env:PSModulePath -split ';';Get-ChildItem | Out-Null;'';'Module in WinPS loaded from:';(Get-Module Microsoft.PowerShell.Management).Path"
WinPS PSModulePath:
c:\powershell-7.1.0-preview.1-win-x64\Modules
C:\Program Files\WindowsPowerShell\Modules
C:\windows\system32\WindowsPowerShell\v1.0\Modules

Module in WinPS loaded from:
C:\powershell-7.1.0-preview.1-win-x64\Modules\Microsoft.PowerShell.Management\Microsoft.PowerShell.Management.psd1
PS C:\> # Module in WinPS loaded from parent PS Core installation

After the change:

PS C:\> $PSHome
c:\PowerShell-7.1.0-preview.1-win-x64

PS C:\> E:\UserA-PowerShell\src\powershell-win-core\bin\Debug\netcoreapp5.0\win7-x64\publish\pwsh.exe
PowerShell 7.0.0-preview.6-324-g5f89a10f5b872406a9ac6cb1fcdea8078ea6a4ca

PS C:\> $env:psmodulepath -split ';'
C:\Users\UserA\Documents\PowerShell\Modules
C:\Program Files\PowerShell\Modules
e:\UserA-powershell\src\powershell-win-core\bin\debug\netcoreapp5.0\win7-x64\publish\Modules
c:\powershell-7.1.0-preview.1-win-x64\Modules
C:\Program Files\WindowsPowerShell\Modules
C:\windows\system32\WindowsPowerShell\v1.0\Modules

PS C:\> powershell.exe -c "'WinPS PSModulePath:';`$env:PSModulePath -split ';';Get-ChildItem | Out-Null;'';'Module in WinPS loaded from:';(Get-Module Microsoft.PowerShell.Management).Path"
WinPS PSModulePath:
C:\Users\UserA\Documents\WindowsPowerShell\Modules
C:\Program Files\WindowsPowerShell\Modules
C:\windows\system32\WindowsPowerShell\v1.0\Modules

Module in WinPS loaded from:
C:\windows\system32\WindowsPowerShell\v1.0\Modules\Microsoft.PowerShell.Management\Microsoft.PowerShell.Management.psd1
PS C:\> # Module in WinPS loaded from WinPS module path

PR Checklist

anmenaga · 2020-04-07T23:17:17Z

This is needed by #12269

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

daxian-dbw · 2020-04-08T05:58:32Z

@PoshChan please retry windows

PoshChan · 2020-04-08T05:58:49Z

@daxian-dbw, successfully started retry of PowerShell-CI-Windows

rjmholt

This seems reasonable if we accept that we're not engineering ourselves into a corner by doing yet more strange magic with the PSModulePath

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

daxian-dbw · 2020-04-09T20:47:26Z

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

+                    }
+                    else
+                    {
+                        if (!File.Exists(Path.Combine(possiblePwshDir, "pwsh.exe")))


It may be a nitpick here, but what if a parent PowerShell session starts with the FxDependent package where only pwsh.dll is present?
How about checking for pwsh.dll instead?

Think about the powershell global tool.

Is there a need to consider things built with the SDK in general here too? Those will have a concept of the module path.

Very good point. Updated to check for pwsh.dll.

Good question. That seems to be right in terms of completeness, but I'm not sure if it's really necessary.
@SteveL-MSFT @anmenaga thoughts?

@anmenaga All applications that built against Microsoft.PowerShell.SDK nuget package can potentially have modules Microsoft.PowerShell.Management and Microsoft.PowerShell.Utility in their $pshome\modules path. This is because we ship all built-in modules along with Microsoft.PowerShell.SDK since 7.0.
For example, PowerShell Jupyter kernel have all those built-in modules in the $pshome\modules folder.

With that being said, the real question is -- what is the chance to start pwsh from one of those applications (e.g. PS Jupyter) and then start powershell.exe from that pwsh? This feels unlikely to me. So I'm fine with just changing to pwsh.dll. @SteveL-MSFT can decide if we really want to go 100% completeness.

Every time I see once again a change in this code, I understand that I will never understand :-) why to be based on the variable change if this is a fundamentally insoluble problem in an SxS scenarios. What if there are several SDK applications in the row? There are an infinite number of application, modules and combinations.

And more tricky question: if PowerShell engine does the variable manipulation should all SDK applications follow the logic to get compatible behavior?

I still think it is better for PowerShell Engine to import/consume the variable values but does not export it - this should be delegated to applications/users who better know how address their specific scenario.
If we think about services, then everything becomes easier as there are independent processes. It is unclear why then complicate the behavior of subprocesses.

@iSazonov I personally agree with you, but the consensus is that we must proceed for now in a way that offers maximum compatibility with the existing concepts in PowerShell versions being used in the wild (particularly PS 5.1).

I worry that trying to nip and tuck the PSModulePath variable around other processes in the process stack is always going to have edge cases, but for something so fundamental to PowerShell this seems to be the more conservative option for now.

As for SDK embedding, I wonder if a scenario like Start-Job is going to be motivating. I'm not sure of what exact assemblies are published with the SDK, but we've defined $PSHome as where System.Management.Automation.dll lives and my thought is that perhaps that's the right assembly to anchor to...

I personally agree with you, but the consensus is

Thanks :-). For me it is a breaking change in grace area: nobody pointed me any affected critical scenario. I am amazed that team does not even start offer alternatives. My proposal was to expose $PSModulePath variable and GetPSModulePath() public method. Then 7.1 is nice milestone to remove these tricky PSModulePath env manipulations or make it opt-in. Otherwise, this behavior will always be problematic for years to come.

Then 7.1 is nice milestone to remove these tricky PSModulePath env manipulations or make it opt-in. Otherwise, this behavior will always be problematic for years to come.

Very much agreed. I'm not sure we have this work on the board for the coming milestone, but I'll see if I can get the team to discuss it.

iSazonov · 2020-04-14T06:08:31Z

Do we want to have this in 7.0 Servicing?

anmenaga · 2020-04-14T19:08:46Z

Probably yes;
... considering this is essentially a fix for a bug in code that was part of 7.0, so it would be good to have it in servicing.
@SteveL-MSFT thoughts?

iSazonov · 2020-04-15T03:16:18Z

I moved to 7.0.x-Servicing-Consider to consider.

ghost · 2020-04-23T18:02:59Z

🎉v7.1.0-preview.2 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

Updated GetWindowsPowerShellModulePath

e4c4c35

anmenaga added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Apr 7, 2020

anmenaga added this to the 7.1.0-preview.2 milestone Apr 7, 2020

anmenaga requested a review from SteveL-MSFT April 7, 2020 23:16

ghost assigned iSazonov Apr 7, 2020

SteveL-MSFT requested review from daxian-dbw and rjmholt April 7, 2020 23:27

iSazonov reviewed Apr 8, 2020

View reviewed changes

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

codefactor fixes

80f21c1

rjmholt reviewed Apr 8, 2020

View reviewed changes

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

rjmholt approved these changes Apr 9, 2020

View reviewed changes

SteveL-MSFT approved these changes Apr 9, 2020

View reviewed changes

daxian-dbw reviewed Apr 9, 2020

View reviewed changes

changed detection of pwsh.exe to pwsh.dll

dc02c90

daxian-dbw approved these changes Apr 9, 2020

View reviewed changes

iSazonov merged commit 14487bf into PowerShell:master Apr 14, 2020

iSazonov modified the milestones: 7.1.0-preview.2, 7.0.x-Servicing-Consider Apr 15, 2020

Make GetWindowsPowerShellModulePath compatible with multiple PS installations #12280

Make GetWindowsPowerShellModulePath compatible with multiple PS installations #12280

Uh oh!

Conversation

anmenaga commented Apr 7, 2020

PR Summary

PR Checklist

Uh oh!

anmenaga commented Apr 7, 2020

Uh oh!

Uh oh!

daxian-dbw commented Apr 8, 2020

Uh oh!

PoshChan commented Apr 8, 2020

Uh oh!

rjmholt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

daxian-dbw Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Apr 14, 2020

Uh oh!

anmenaga commented Apr 14, 2020

Uh oh!

iSazonov commented Apr 15, 2020

Uh oh!

ghost commented Apr 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

daxian-dbw Apr 9, 2020 •

edited

Loading