Rework PSModulePath path building by jborean93 · Pull Request #24568 · PowerShell/PowerShell

Jordan Borean (jborean93) · 2024-11-12T02:38:35Z

PR Summary

Fixes the parsing of the powershell.config.json PSModulePath entry when it contains the path separator char. Instead of crashing this is able to handle entries with those characters and treat them as multiple entries.

This also rewrites the logic when building the process PSModulePath to
simplify the way entries were added and deal with canonicalisation of
the path entries.

Fixes: #20706

PR Context

Using powershell.config.json with a PSModulePath entry that contains the Path.Separator char (; for Windows and : for *nix) crashes the process on startup. This is because the current logic adds the value provided as is to the path but then attempts to find the index for where it is located. This lookup splits the new path value by the Path.Separator so the entry being added is never found (the raw value is never a split component of the new path).

This new PR consolidates the various path addition logic and tries to make it more consistent in how it handles new values. It also attempts to remove as much string allocations by working on spans where possible to hopefully improve the memory allocation at startup.

Ultimately the behaviour of this logic when building the PSModulePath is

Add the current user path
- Uses the current user .config.json and falls back to the default user's modules dir
Add the shared machine wide path
- On Windows this is C:\Program Files\PowerShell\Modules
- This not controlled by any config
Add the machine/system path
- Uses the $PSHome/powershell.config.json entry and falls back to $PSHome/Modules
Adds the existing process PSModulePath if present
- On Windows this is typically the machine wide PSModulePath (System32/Program Files\WindowsPowerShell)
- Will inherit from the parent process that spawned PowerShell
- On Linux this is not set by default but can be using whatever shell that is spawning PowerShell

There are a few behavioural changes introduced by this PR which I think aren't breaking changes but are behaviour differences for the better.

Paths are canonicalised and only added if not already present
- Whitespace is trimmed
- Trailing / or \ (on Windows) is removed
- On Windows / is converted to \
- All duplicates and empty entries are removed
- In the past some paths were checked before adding while others are just appended as is leading to mixed behaviour
- This new logic is consistent with all stages of building the PSModulePath
On non-Windows paths are compared in a case sensitive way
- This is because these OS’ are typically run on a filesystem where the case does matter
When PSModulePath is not set as a process env var (Linux mostly) and there is an explicit PSModulePath in the $PSHome/powershell.config.json the shared module path is now included
- This was omitted before but I cannot understand why
- Making this change keeps things consistent
- Should be a rare scenario in any case which is probably why it was never picked up

PR Checklist

Jordan Borean (jborean93) · 2024-11-12T04:29:54Z

Tests work locally on my macOS host so I need to figure out what the differences are in CI to cause it to fail.

Jordan Borean (jborean93) · 2024-11-13T07:25:35Z

I've pushed a commit that removed the existing logic and reimplemented it in what I hope is a more sane manner. I'll have to sit down and write some more comprehensive tests for this API to test out edge cases like cases, inserting the system/shared path earlier and so on. I'll also need to look into things like trailing backslash and whitespace handling as I think before this was considered equal when compared.

The only change that I can tell this makes is that the shared path C:\Program Files\PowerShell\Modules is added all the time whereas before it was skipped if the PSModulePath env var was unset and there was an explicit machine env var path. I think the change is better consistency wise but will have to think about it some more.

Ilya (iSazonov) · 2024-11-13T08:53:02Z

As for tests, I'd consider xUnit tests for GetModulePath(). It saves a lot your time and makes design intentions explicit.

Jordan Borean (jborean93) · 2024-11-21T09:14:55Z

I've updated the PR title and summary to sum up the changes I've made here. The tests are passing except for the CodeFactor checks which is complaining about the lack of docs on the new unit tests added. No other tests have the docs so I'm just ignoring this failure.

This is now ready for review.

Ilya (iSazonov) · 2024-11-21T12:25:46Z

It seems there is very optimized overload for two arguments so we can eliminate dirSeparatorChars.

Fair point I’ll use the params overload which I think is the same one used but avoids the span allocation.

Please see my comment about TrimEndingDirectorySeparator()

Ah there reason why I did this is because the params overload is for a string, we are operating on a ReadOnlySpan and MemoryExtensions.TrimEnd doesn't have such an overload.

I've kept the current code because TrimEndingDirectorySeparator() doesn't handle trimming more than 1 trailing separator.

I think it makes no sense for us to try to fix user errors. In fact, there may be a lot of user errors that we don't fix. I suggest following the same logic as you suggested for duplicated paths - just ignore it.
The main thing for us here is to correctly add the necessary paths and no more.

I suggest to use more simple code with TrimEndingDirectorySeparator() here. It is well tested (I see one issue in your code for Windows).

I suggest to use more simple code with TrimEndingDirectorySeparator() here. It is well tested (I see one issue in your code for Windows)

What's the issue for Windows?

I'll wait until the WG weighs in to see whether they prefer the current code that trims all trailing dir path separators or whether to swap to TrimEndingDirectorySeparator which only trims the last.

// On Windows we always want to trim '/' or '\'. Only exception // is to keep one trailing char if the path is the drive root // 'C:\+'. entry = entry.Length > 1 && entry[1] == Path.VolumeSeparatorChar && trimmedEntry.Length == 2 ? entry.Slice(0, 3) : trimmedEntry;

The code is not correct.

You will wait indefinitely.
I suggest making the code as simple as possible and accepting it now.
Since we are talking about an extreme case, you can create an issue for it and make changes to the code after additional discussion if it really turns out to be useful for many users.

You will wait indefinitely.

I'm assuming I'm waiting indefinitely to get this reviewed by the WG anyway :) I appreciate you reviewing these PRs though and I'm not trying to be obstructionist it's just if they have to review this anyway I prefer to wait for the final verdict before changing things.

I have picked up the bug if it's C: then the Slice will go out of bounds and will fix up. Not sure if that's the incorrect part you are referring to though.

Ilya (iSazonov) · 2024-11-21T17:03:16Z

Can we use [IO.Path.IsPathRooted](https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs,132)?

And what about C:\ on Windows?

Also there is TrimEndingDirectorySeparator() - it checks for size and root.

One downside of TrimEndingDirectorySeparator is that it seems like it only trims 1 char versus TrimEnd which trims them all if present. I don't think someone having C:\foo\\ would be common but it depends if we want to handle it or not (I think we should).

I've updated the code to handle Windows absolute paths and their trailing chars and expanded the comments that explains what it does. I didn't use TrimEndingDirectorySeparator() because it will only trim 1 char at the end.

Fixes the parsing of the powershell.config.json PSModulePath entry when it contains the path separator char. Instead of crashing this is able to handle entries with those characters and treat them as multiple entries. This also rewrites the logic when building the process PSModulePath to simplify the way entries were added and deal with canonicalisation of the path entries.

Jordan Borean (jborean93) mentioned this pull request Nov 12, 2024

Error starting PowerShell 7.4.0+ #20706

Closed

5 tasks done

Jordan Borean (jborean93) force-pushed the config-modulepath branch from 11ffe1d to 231f0bd Compare November 12, 2024 04:10

Ilya (iSazonov) reviewed Nov 12, 2024

View reviewed changes

Comment thread src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs Resolved

Jordan Borean (jborean93) changed the title ~~Fix config.json PSModulePath path with sep char~~ WIP - Fix config.json PSModulePath path with sep char Nov 13, 2024

Jordan Borean (jborean93) requested review from Travis Plunk (TravisEz13), Aditya Patwardhan (adityapatwardhan), Andrew (anmenaga) and Dongbo Wang (daxian-dbw) as code owners November 21, 2024 05:00

Jordan Borean (jborean93) force-pushed the config-modulepath branch 3 times, most recently from 2ee70e7 to cbd1fd7 Compare November 21, 2024 09:03

Jordan Borean (jborean93) changed the title ~~WIP - Fix config.json PSModulePath path with sep char~~ Rework PSModulePath path building Nov 21, 2024

Ilya (iSazonov) reviewed Nov 21, 2024

View reviewed changes

Comment thread src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs Resolved

Ilya (iSazonov) reviewed Nov 22, 2024

View reviewed changes

Comment thread src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs Resolved

xtqqczze mentioned this pull request Nov 24, 2024

Set LangVersion compiler option to 13.0 #24617

Merged

Jordan Borean (jborean93) force-pushed the config-modulepath branch from 5554bb1 to e46c27b Compare November 26, 2024 05:54

Jordan Borean (jborean93) added 4 commits November 27, 2024 03:25

Better handle path roots and improve path canonicalisation on Windows

7c3a15b

More special handling for Windows drive root entries

102d59a

Use simpler string creation code now that LangVersion is updated

9019fe4

Jordan Borean (jborean93) force-pushed the config-modulepath branch from e46c27b to 9019fe4 Compare November 26, 2024 17:37

microsoft-github-policy-service Bot added the Review - Needed The PR is being reviewed label Dec 3, 2024

Patrick Meinecke (SeeminglyScience) added WG-Engine core PowerShell engine, interpreter, and runtime WG-NeedsReview Needs a review by the labeled Working Group labels May 21, 2025

Staffan Gustafsson (powercode) assigned Patrick Meinecke (SeeminglyScience) Oct 20, 2025

James Truher (JamesWTruher) added the Needs-Investigation The behavior reported in the issue is unexpected and needs further investigation. label Oct 20, 2025

Jordan Borean (jborean93) closed this Nov 17, 2025

Conversation

Jordan Borean (jborean93) commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

Jordan Borean (jborean93) commented Nov 12, 2024

Uh oh!

Uh oh!

Jordan Borean (jborean93) commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ilya (iSazonov) commented Nov 13, 2024

Uh oh!

Jordan Borean (jborean93) commented Nov 21, 2024

Uh oh!

Uh oh!

Uh oh!

Ilya (iSazonov) Nov 21, 2024 • 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!

Jordan Borean (jborean93) Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

Jordan Borean (jborean93) Nov 21, 2024 • 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!

Ilya (iSazonov) Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

Jordan Borean (jborean93) Nov 24, 2024

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Nov 26, 2024

Choose a reason for hiding this comment

Uh oh!

Jordan Borean (jborean93) Nov 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Ilya (iSazonov) Nov 21, 2024 • 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!

Jordan Borean (jborean93) Nov 21, 2024 • 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!

Jordan Borean (jborean93) Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Jordan Borean (jborean93) commented Nov 12, 2024 •

edited

Loading

Jordan Borean (jborean93) commented Nov 13, 2024 •

edited

Loading

Ilya (iSazonov) Nov 21, 2024 •

edited

Loading

Jordan Borean (jborean93) Nov 21, 2024 •

edited

Loading

Ilya (iSazonov) Nov 21, 2024 •

edited

Loading

Jordan Borean (jborean93) Nov 21, 2024 •

edited

Loading