FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Added WindowsPS version check for WinCompat by anmenaga · Pull Request #11148 · PowerShell/PowerShell · GitHub

Added WindowsPS version check for WinCompat - #11148

Merged
Aditya Patwardhan (adityapatwardhan) merged 6 commits into
PowerShell:masterfrom
anmenaga:WinCompatVersion
Dec 10, 2019
Merged

Added WindowsPS version check for WinCompat#11148
Aditya Patwardhan (adityapatwardhan) merged 6 commits into
PowerShell:masterfrom
anmenaga:WinCompatVersion

Conversation

Copy link
Copy Markdown

PR Summary

WinCompat functionality needs Windows PS 5.1
This change modifies Import-Module so that WinCompat returns an error if Windows PS on the current system is not 5.1 (with recommendation to install WMF).

PR Checklist

{
#if !UNIX
var winPSVersionString = Utils.GetWindowsPowerShellVersionFromRegistry();
if (!winPSVersionString.StartsWith("5.1", StringComparison.OrdinalIgnoreCase))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In CreateRunspacesForUseWindowsPowerShellParameterSet() we request connectionInfo.PSVersion = new Version(5, 1)
This is not enough?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Unfortunately, not enough. Error is generated too late - when remote WinPS process is already starting and parsing arguments and Job's code (that we are reusing in WinCompat and that starts remote process) not doing a great job at reporting it, so user experience is not good - user gets an impression that everything is fine, when in fact WinCompat is not working.
Also, detecting the incompatible environment and reporting a clear error sooner than later is better performance-wise.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I guess most of user systems already have PS 5.1 so version check is an edge case.
I guess PowerShell processes Version parameter before other parameters so it doesn't extra work.
Also I think it is useful to catch Windows PowerShell exit code and write an error if needed - do we this? In PR case we could catch -1 exit code, catch message "Cannot start Windows PowerShell. No version of Windows PowerShell compatible to 5.1 is installed." and then write appropriate error to user.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You are right, theoretically on the high level that's probably the right approach. The devil is in details, special-casing WinCompat needs small changes in remoting code and it is already complex-enough. Just for the risk of regressions in high-use features (remoting, jobs) I would like to avoid doing changes in remoting code unless absolutely necessary.

Steve Lee (SteveL-MSFT) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Copy link
Copy Markdown
Member

Andrew, is this needed for 7.0... please change the milestone on any PR's that are to rc.1-consider.

Copy link
Copy Markdown
Author

Codacy failure seems to be false positive because it does not correctly handle code branches based on preprocessor directives.

Copy link
Copy Markdown
Member

Andrew (@anmenaga) can you respond to comments from Ilya

Copy link
Copy Markdown

🎉v7.0.0-rc.1 has been released which incorporates this pull request.:tada:

Handy links:

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL