Skip to content

Conversation

@anmenaga
Copy link

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

@anmenaga anmenaga added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Nov 21, 2019
{
#if !UNIX
var winPSVersionString = Utils.GetWindowsPowerShellVersionFromRegistry();
if (!winPSVersionString.StartsWith("5.1", StringComparison.OrdinalIgnoreCase))
Copy link
Collaborator

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?

Copy link
Author

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.

Copy link
Collaborator

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.

Copy link
Author

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.

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@TravisEz13 TravisEz13 added this to the rc.1-consider milestone Dec 3, 2019
@TravisEz13
Copy link
Member

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

@anmenaga
Copy link
Author

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

@adityapatwardhan
Copy link
Member

@anmenaga can you respond to comments from Ilya

@adityapatwardhan adityapatwardhan merged commit 0c46e3e into PowerShell:master Dec 10, 2019
TravisEz13 pushed a commit that referenced this pull request Dec 11, 2019
@TravisEz13 TravisEz13 modified the milestones: rc.1-approved, 7.0.0-rc.1 Dec 11, 2019
@ghost
Copy link

ghost commented Dec 16, 2019

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

Handy links:

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