Stop altering the PATH env variable by honoring the PSHome path in CommandDiscovery - #27757
Stop altering the PATH env variable by honoring the PSHome path in CommandDiscovery#27757Dongbo Wang (daxian-dbw) wants to merge 8 commits into
PSHome path in CommandDiscovery#27757Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR stops PowerShell from modifying the PATH environment variable at startup and instead ensures the current PowerShell installation directory ($PSHOME) is prioritized during command lookup inside CommandDiscovery, addressing issues with tools (e.g., CMake) caching an MSIX-versioned pwsh path. It also updates/extends tests to validate pwsh resolution and behavior when PATH is unset.
Changes:
- Removed startup-time
PATHmutation fromConsoleHost. - Updated
CommandDiscovery.GetLookupDirectoryPaths()to always prioritize the current pwsh location (including global tool path correction) without editingPATH. - Updated Pester tests to validate
pwshversion resolution and that startup does not populatePATHwhen it is unset.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHost.cs |
Removes logic that prepended $PSHOME to PATH during startup. |
src/System.Management.Automation/engine/CommandDiscovery.cs |
Prioritizes $PSHOME in lookup paths and streamlines path caching/tilde expansion. |
test/powershell/Host/ConsoleHost.Tests.ps1 |
Adjusts/adds tests for pwsh resolution and verifies PATH remains unaltered when unset. |
| if (!isPathCacheValid) | ||
| { | ||
| // Reset the cached lookup paths | ||
| _pathCacheKey = null; |
There was a problem hiding this comment.
- Is isPathCacheValid evaluated correctly now?
- If we evaluate _psHome only for saving in _cachedLookupPaths why do we save it permanently in field?
There was a problem hiding this comment.
For 1, I think it's correct now.
For 2, _cachedLookupPaths will be invalidated and reconstructed when the PATH env var is changed, but we don't want to recalculate _psHome in that case.
There was a problem hiding this comment.
For 2, the question is rather what if we search for pshome throughout the source code, there is more than one place where it is used according to the old pattern. I.e., I would expect that pshome should be cached in Utils.DefaultPowerShellAppBase, taking into account regular, dotnet tool and hosted scenarios.
There was a problem hiding this comment.
I renamed the _psHome variable to _psExeHome to differentiate it from the $PSHOME concept.
_psExeHome is the directory that contains the pwsh executable, which is not necessarily under $PSHOME. For example, the built-in pwsh in the .NET SDK docker container image is located at C:\Program Files\powershell for Windows Server Core, and $PSHOME\pwsh.exe doesn't exist.
The only place I plan to update next is the PwshExePath in PowerShellProcessInstance. The current code is incorrect. I plan to reuse what I have here in the CommandDiscovery, in a separate PR.
There was a problem hiding this comment.
Thanks!
Perhaps the best thing that could be done is to have a common code for each scenario somewhere in Utils. (Actually, it was originally like that once.)
|
Would this effect embedded pwsh runspaces and their command discovery look up order? For embedded pwsh, I believe the location of SMA.dll would be the pshome that gets prepended to the path by command discovery. Any name collision in that SMA.dll location would be unintended. I can't imagine this happening often. |
|
That's a good point Justin Chung (@jshigetomi). Let me think about the impact to the applications that host PowerShell using the NuGet packages. |
|
Justin Chung (@jshigetomi) I've updated the changes to take into account the "PowerShell hosted by application via NuGet packages" scenario. In that case, we will use the PATH as is. Can you please review again? |
Context
Altering the
PATHenv variable causes a problem to cmake-based build system when running in the MSIX PowerShell installation because they cache the location of PowerShell sometimes.At startup, PowerShell adds
$PSHOMEto the beginning ofPATH, and for MSIX installation,$PSHOMEcontains version numbers that change when PowerShell is updated.When
cmakeis started from MSIX PowerShell, the path it caches will be that$PSHOME, which will become invalid after an update of the MSIX PowerShell.PR Summary
The main change of this PR is to move the logic for ensuring the current PowerShell's executable directory (
$PSHOME) is prioritized in command lookup from prepend it toPATHat startup to theCommandDiscoveryprocess itself. This results in a more accurate and less intrusive handling ofPATH. The PR also improves test coverage for these scenarios.Refactoring of PATH handling:
PSHOMEto thePATHenvironment variable is removed fromConsoleHost.cs, so PowerShell no longer altersPATHduring startup.PSHOMEis always the first lookup directory is now implemented inCommandDiscovery.cswithin theGetLookupDirectoryPathsmethod, regardless of the state of thePATHvariable. This includes handling cases wherePATHis unset and expanding~to the user home directory._psHomewhen constructing_cachedLookupPaths.Code cleanup and caching improvements:
_cachedPathfield and streamlined the caching logic for lookup paths inCommandDiscovery.cs.Test updates:
ConsoleHost.Tests.ps1to verify that runningpwshalways starts the current version and thatPATHremains unaltered during startup, including whenPATHis unset.PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header