-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Added WindowsPS version check for WinCompat #11148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added WindowsPS version check for WinCompat #11148
Conversation
| { | ||
| #if !UNIX | ||
| var winPSVersionString = Utils.GetWindowsPowerShellVersionFromRegistry(); | ||
| if (!winPSVersionString.StartsWith("5.1", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In CreateRunspacesForUseWindowsPowerShellParameterSet() we request connectionInfo.PSVersion = new Version(5, 1)
This is not enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
SteveL-MSFT
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Andrew, is this needed for 7.0... please change the milestone on any PR's that are to |
|
Codacy failure seems to be false positive because it does not correctly handle code branches based on preprocessor directives. |
|
@anmenaga can you respond to comments from Ilya |
|
🎉 Handy links: |
PR Summary
WinCompat functionality needs Windows PS 5.1
This change modifies
Import-Moduleso that WinCompat returns an error if Windows PS on the current system is not 5.1 (with recommendation to install WMF).PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.