Report error when PowerShell built-in modules are missing

[daxian-dbw]

PR Summary

When a PowerShell built-in module is getting imported in the WinCompat mode, we check whether a core-edition compatible version of this module is available in module paths. If not, report error instead of blindly loading the module under System32 module path, because that could result in very confusing errors later on.

Fix #13157. Related to #16525.
We also had a stack overflow issue reported from internal team when they were testing with PowerShell 7.2. The investigation showed the root cause was that they didn't recursively copy the $PSHOME\Modules when deploying their test infrastructure, and the stack overflow exception was caused by auto-loading attempts to load Microsoft.PowerShell.Utility from the System32 module path.

With this change, we should be able to avoid the confusing errors when somehow a user has the built-in modules missing. The error will explicitly call out that a core-edition compatible version of the built-in module cannot be found.

Here is an example to show the UX:

Basically, for built-in modules, PowerShell blocks the loading of the same module in System32\WindowsPowerShell\v1.0\Modules via WinCompat when a core-edition compatible version of such module cannot be found. When the core-edition compatible built-in modules are available, loading of the same module in System32 module path will proceed normally.

Where is the check done?

The check is done in PrepareNoClobberWinCompatModuleImport.
When importing a module remotely via WinCompat, if the module is a built-in module, or it's listed in WindowsPowerShellCompatibilityNoClobberModuleList in powershell.config.json, we first attempt to load a core-edition compatible version of such module, and then proceed with loading the module remotely. This is how WInCompat works today.

This PR made an improvement to the existing code:

• skip the System32 module path when attempting to load a core-compatible version of the module, so that
  1. a Desktop module listed in the no-clobber list won't be loaded as remote module twice.
  2. a core-compatible module can be discovered even if it's in a module path that appears behind the System32 module path in $env:PSModulePath.
• when a core-compatible version of the module cannot be found,
  1. if the module to be loaded remotely is a built-in module, we throw exception because that means core-compatible built-in modules are not available.
  2. if the module to be loaded remotely is NOT a built-in module, then it's OK to proceed with loading it remotely.

PR Checklist

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: or [ WIP ] to the beginning of the title (the WIP bot will keep its status check at Pending while the prefix is present) and remove the prefix when the PR is ready.
• Breaking changes
  • None
  • OR
  • Experimental feature(s) needed
    • Experimental feature name(s):
• User-facing changes
  • Not Applicable
  • OR
  • Documentation needed
    • Issue filed:
• Testing - New and feature
  • N/A or can only be tested interactively
  • OR
  • Make sure you've added a new test if existing tests do not effectively test the code changed
• Tooling
  • I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.

[PaulHigin]

LGTM

[iSazonov]

I do not understand why even try to look for built-in modules if they are hardwired to a particular version of SMA and the paths to them must be hardcoded.

[daxian-dbw]

Turning the PR to a draft for now, as I'm working on a different solution without assuming the built-in modules have to be under $PSHOME module path.

change of design was made, so need a new review

[daxian-dbw]

@iSazonov The design was changed to not assume the built-in modules are always under $PSHOME module path. Please see details in the "Where is the check done?" section in the PR description.

[daxian-dbw]

@anmenaga @SteveL-MSFT, please review when you have time.
@PaulHigin the fix was changed to avoid assuming the built-in modules are always in $PSHOME module path, so I dismissed your previous approval. Please review again when you have time.
Thanks in advance to all of you!

[PaulHigin]

Just some minor comments.

[PaulHigin]

LGTM

[iSazonov]

What about Appx module? :-)

[daxian-dbw]

What about Appx module?

@iSazonov Not sure what you mean. Appx module is not in the picture of the problem I tried to resolve here.

[iSazonov]

What about Appx module?

@iSazonov Not sure what you mean. Appx module is not in the picture of the problem I tried to resolve here.

I think we need add the module to stop list and load in wincompat mode..

[pull-request-quantifier-deprecated]

This PR has 466 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

---

Quantification details

Label      : Extra Large
Size       : +310 -156
Percentile : 82.2%

Total files changed: 10

Change summary by file extension:
.cs : +179 -155
.resx : +7 -0
.ps1 : +124 -1

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

• Fast and predictable releases to production:
  • Optimal size changes are more likely to be reviewed faster with fewer
    iterations.
  • Similarity in low PR complexity drives similar review times.
• Review quality is likely higher as complexity is lower:
  • Bugs are more likely to be detected.
  • Code inconsistencies are more likely to be detetcted.
• Knowledge sharing is improved within the participants:
  • Small portions can be assimilated better.
• Better engineering practices are exercised:
  • Solving big problems by dividing them in well contained, smaller problems.
  • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

• Use the PullRequestQuantifier to quantify your PR accurately
  • Create a context profile for your repo using the context generator
  • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
  • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
  • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
• Change your engineering behaviors
  • For PRs that fall outside of the desired spectrum, review the details and check if:
    • Your PR could be split in smaller, self-contained PRs instead
    • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

• One line was added: +1 -0
• One line was deleted: +0 -1
• One line was modified: +1 -1 (git diff doesn't know about modified, it will
  interpret that line like one addition plus one deletion)
• Change percentiles: Change characteristics (addition, deletion, modification)
  of this PR in relation to all other PRs within the repository.

---

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

[daxian-dbw]

@anmenaga I think this PR is ready. Can you please take a look? Thanks!

[Fs00]

Jumping in to make you aware that this PR would also fix #13157 🙂

[daxian-dbw]

Thanks @Fs00, I've updated the PR description with that info.

[iSazonov]

As final notices:

• ReadCoreEngineSnapIn() - doesn't support WU and looks weird in whole.
• x32 vs x64 paths (including Program Files/x32 and System32/SysWow64 folders) - The existing logic is contradictory. See 7418 It would be more reliable to rely on the declared architecture in the module manifest

[ghost]

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

Handy links:

• Release Notes