Skip to content

Cleanup: remove the unneeded type 'RemotingCommandUtils' #7029

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.

Sign up for GitHub

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

Merged
iSazonov merged 2 commits into from
Jun 14, 2018
Merged

Cleanup: remove the unneeded type 'RemotingCommandUtils' #7029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4b65ea
[Feature] Cleanup: remove the unneeded type 'RemotingCommandUtils'
daxian-dbw Jun 9, 2018
7be287b
[Feature] Minor fix
daxian-dbw Jun 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions src/System.Management.Automation/engine/Utils.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,6 @@ internal static int CombineHashCodes(int h1, int h2, int h3, int h4, int h5, int
return CombineHashCodes(CombineHashCodes(h1, h2, h3, h4), CombineHashCodes(h5, h6, h7, h8));
}

/// <summary>
/// The existence of the following registry confirms that the host machine is a WinPE
/// HKLM\System\CurrentControlSet\Control\MiniNT
/// </summary>
internal static string WinPEIdentificationRegKey = @"System\CurrentControlSet\Control\MiniNT";

/// <summary>
/// Allowed PowerShell Editions
/// </summary>
Expand Down Expand Up @@ -344,7 +338,7 @@ internal static bool IsWinPEHost()
{
// The existence of the following registry confirms that the host machine is a WinPE
// HKLM\System\CurrentControlSet\Control\MiniNT
winPEKey = Registry.LocalMachine.OpenSubKey(WinPEIdentificationRegKey);
winPEKey = Registry.LocalMachine.OpenSubKey(@"System\CurrentControlSet\Control\MiniNT");

return winPEKey != null;
}
Expand Down
4 changes: 2 additions & 2 deletions src/System.Management.Automation/engine/remoting/commands/CustomShellCommands.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2298,10 +2298,10 @@ public Version PSVersion
get { return psVersion; }
set
{
RemotingCommandUtils.CheckPSVersion(value);
RemotingCommandUtil.CheckPSVersion(value);

// Check if specified version of PowerShell is installed
RemotingCommandUtils.CheckIfPowerShellVersionIsInstalled(value);
RemotingCommandUtil.CheckIfPowerShellVersionIsInstalled(value);

psVersion = value;
isPSVersionSpecified = true;
Expand Down
62 changes: 0 additions & 62 deletions src/System.Management.Automation/engine/remoting/commands/PSRemotingCmdlet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4208,68 +4208,6 @@ public enum SessionFilterState

#endregion

#region RemotingCommandUtils

internal static class RemotingCommandUtils
{
internal static void CheckPSVersion(Version version)
{
// PSVersion value can only be 2.0, 3.0, 4.0, 5.0, or 5.1
if (version != null)
{
// PSVersion value can only be 2.0, 3.0, 4.0, 5.0, or 5.1
if (!(version.Major >= 2 && version.Major <= 4 && version.Minor == 0) &&
!(version.Major == 5 && version.Minor <= 1))
{
throw new ArgumentException(
StringUtil.Format(RemotingErrorIdStrings.PSVersionParameterOutOfRange, version, "PSVersion"));
}
}
}

/// <summary>
/// Checks if the specified version of PowerShell is installed
/// </summary>
/// <param name="version"></param>
internal static void CheckIfPowerShellVersionIsInstalled(Version version)
{
// Check if PowerShell 2.0 is installed
if (version != null && version.Major == 2)
{
#if CORECLR
// PowerShell 2.0 is not available for CoreCLR
throw new ArgumentException(
PSRemotingErrorInvariants.FormatResourceString(
RemotingErrorIdStrings.PowerShellNotInstalled,
version, "PSVersion"));
#else
// Because of app-compat issues, in Win8, we will have PS 2.0 installed by default but not .NET 2.0
// In such a case, it is not enough if we check just PowerShell registry keys. We also need to check if .NET 2.0 is installed.
try
{
RegistryKey engineKey = PSSnapInReader.GetPSEngineKey(PSVersionInfo.RegistryVersion1Key);
// Also check for .NET 2.0 installation
if (!PsUtils.FrameworkRegistryInstallation.IsFrameworkInstalled(2, 0, 0))
{
throw new ArgumentException(
PSRemotingErrorInvariants.FormatResourceString(
RemotingErrorIdStrings.NetFrameWorkV2NotInstalled));
}
}
catch (PSArgumentException)
{
throw new ArgumentException(
PSRemotingErrorInvariants.FormatResourceString(
RemotingErrorIdStrings.PowerShellNotInstalled,
version, "PSVersion"));
}
#endif
}
}
}

#endregion

#endregion Helper Classes
}

Expand Down
4 changes: 2 additions & 2 deletions src/System.Management.Automation/engine/remoting/commands/StartJob.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,10 +466,10 @@ public virtual Version PSVersion
get { return _psVersion; }
set
{
RemotingCommandUtils.CheckPSVersion(value);
RemotingCommandUtil.CheckPSVersion(value);

// Check if specified version of PowerShell is installed
RemotingCommandUtils.CheckIfPowerShellVersionIsInstalled(value);
RemotingCommandUtil.CheckIfPowerShellVersionIsInstalled(value);

_psVersion = value;
}
Expand Down
65 changes: 57 additions & 8 deletions src/System.Management.Automation/engine/remoting/commands/remotingcommandutil.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@

using System;
using System.Management.Automation;
using System.Management.Automation.Internal;
using System.Management.Automation.Remoting;
using System.Management.Automation.Runspaces;
using Microsoft.Win32;
using Dbg = System.Management.Automation.Diagnostics;
using System.Management.Automation.Internal;

namespace Microsoft.PowerShell.Commands
{
Expand All @@ -31,12 +31,6 @@ internal enum RunspaceParameterSet
/// </summary>
internal static class RemotingCommandUtil
{
/// <summary>
/// The existence of the following registry confirms that the host machine is a WinPE
/// HKLM\System\CurrentControlSet\Control\MiniNT
/// </summary>
internal static string WinPEIdentificationRegKey = @"System\CurrentControlSet\Control\MiniNT";

internal static bool HasRepeatingRunspaces(PSSession[] runspaceInfos)
{
if (runspaceInfos == null)
Expand Down Expand Up @@ -170,6 +164,61 @@ internal static void CheckHostRemotingPrerequisites()
throw new InvalidOperationException(errorRecord.ToString());
}
}

internal static void CheckPSVersion(Version version)
{
// PSVersion value can only be 2.0, 3.0, 4.0, 5.0, or 5.1
if (version != null)
{
// PSVersion value can only be 2.0, 3.0, 4.0, 5.0, or 5.1
if (!(version.Major >= 2 && version.Major <= 4 && version.Minor == 0) &&
!(version.Major == 5 && version.Minor <= 1))
{
throw new ArgumentException(
StringUtil.Format(RemotingErrorIdStrings.PSVersionParameterOutOfRange, version, "PSVersion"));
}
}
}

/// <summary>
/// Checks if the specified version of PowerShell is installed
/// </summary>
/// <param name="version"></param>
internal static void CheckIfPowerShellVersionIsInstalled(Version version)
{
// Check if PowerShell 2.0 is installed
if (version != null && version.Major == 2)
{
#if CORECLR
// PowerShell 2.0 is not available for CoreCLR
throw new ArgumentException(
PSRemotingErrorInvariants.FormatResourceString(
RemotingErrorIdStrings.PowerShellNotInstalled,
version, "PSVersion"));
#else
// Because of app-compat issues, in Win8, we will have PS 2.0 installed by default but not .NET 2.0
// In such a case, it is not enough if we check just PowerShell registry keys. We also need to check if .NET 2.0 is installed.
try
Copy link
Collaborator

@iSazonov iSazonov Jun 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Seems we can remove the unused code after "else".

Copy link
Collaborator

@rjmholt rjmholt Jun 11, 2018

Choose a reason for hiding this comment

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

And if it gets removed, this method will just throw when given v2 and do nothing otherwise. I assume that's all the method should do for PowerShell 6?

Copy link
Member Author

@daxian-dbw daxian-dbw Jun 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, throwing that exception in #if CORECLR was a decision when porting the code to NanoServer targeting .NET Core 1.0. Back then, PSv2 was definitely not available on NanoServer, which was the only place that PowerShell Core edition was available, so throwing the exception had no problem. Now PowerShell Core is on the desktop, so I'm not sure if we should just throw the exception there (win 10 has removed PSv2, but downlevel OS still has it as an optional windows feature). Besides, if we decide to go with the exception, we probably need to change the error message.

I have updated #3565 to keep track of this cleanup work.

Copy link
Collaborator

@rjmholt rjmholt Jun 11, 2018

Choose a reason for hiding this comment

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

Is this the behaviour designed to support powershell.exe -version 2?

Copy link
Member Author

@daxian-dbw daxian-dbw Jun 12, 2018

Choose a reason for hiding this comment

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

@rjmholt I didn't look into why the PSv2 is taken into consideration here, but possibly related to the scenario where the remote server is PSv2.

Copy link
Collaborator

@rjmholt rjmholt Jun 12, 2018

Choose a reason for hiding this comment

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

Yes that makes sense! Well I suppose the behaviour without the #ifdef makes sense then -- no-op unless trying to remote into v2, in which case, exception.

Copy link
Collaborator

@iSazonov iSazonov Jun 13, 2018

Choose a reason for hiding this comment

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

I believe we don't support PSv2 at all.

Copy link
Collaborator

@rjmholt rjmholt Jun 13, 2018

Choose a reason for hiding this comment

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

Yes -- so I'm guessing an exception makes sense then? Something that is red and tells the user we don't support this, but doesn't crash the shell

Copy link
Collaborator

@iSazonov iSazonov Jun 13, 2018

Choose a reason for hiding this comment

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

Seems the method is used to run jobs. I guess the remoting (with both local and remote hosts) with PSv2 doesn't work at all.
/cc @PaulHigin Could you please comment too?

Copy link
Member Author

@daxian-dbw daxian-dbw Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I briefly examined the code that depends on this method.

  • for the use in StartJob, it looks to me is possible to start a job running in PSv2 if it's available on the machine. However, in PowerShell Core, we can only start job using pwsh. So the whole -PSVersion parameter doesn't really make sense. It turns out this is a bug that should be fixed. (see PowerShell Core cannot start a job using Windows PowerShell #7059) That means we still need to check for PSv2 availability for Start-Job -PSEdition.

  • for the use in CustomShellCommand, it allows to register a PSSessionConfiguration for PSv2. However, PowerShell Core remoting depends on WinRM features from WMF 4.0, so it won't be able to connect to PSv2 remoting endpoint. My comment about the remoting was not accurate. WMF 4.0 is required to create a PSCore endpoint (using PSCore as a server in remoting), however, it might not be required for PSCore to connect to a PSv2 server (using PSCore as a client). If PSCore is able to connect to PSv2 server, then the PSv2 related code is still relevant.

So, it looks to me is likely that we still need the PSv2 related code. As to this PR, I think it can be merged as is for now.

{
RegistryKey engineKey = PSSnapInReader.GetPSEngineKey(PSVersionInfo.RegistryVersion1Key);
// Also check for .NET 2.0 installation
if (!PsUtils.FrameworkRegistryInstallation.IsFrameworkInstalled(2, 0, 0))
{
throw new ArgumentException(
PSRemotingErrorInvariants.FormatResourceString(
RemotingErrorIdStrings.NetFrameWorkV2NotInstalled));
}
}
catch (PSArgumentException)
{
throw new ArgumentException(
PSRemotingErrorInvariants.FormatResourceString(
RemotingErrorIdStrings.PowerShellNotInstalled,
version, "PSVersion"));
}
#endif
}
}
}
}