Update PowerShell Direct to try pwsh then fallback to powershell #7241

TravisEz13 · 2018-07-06T19:38:57Z

PR Summary

Update PowerShell Direct to try pwsh then fallback to powershell

Forward port changes from Windows Powershell to fall back
change the order from powershell->pwsh to pwsh->powershell

PR Checklist

…n PowerShell direct fixes PowerShell#7237

bergmeister · 2018-07-08T22:34:25Z

If I understand correctly, this partially resolves also #6669 but it also means this is a breaking change because the default is no longer Windows PowerShell?

TravisEz13 · 2018-07-09T01:27:01Z

@bergmeister Yes, it is part of the resolution for the issue you mention. I've added the breaking change label for the reason you mentioned.

dantraMSFT · 2018-07-09T17:05:35Z

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

-                    // This code executes `powershell.exe` as it exists in the container which currently is
-                    // expected to be Windows PowerShell as it's inbox in the container.
+                    // This code executes `pwsh.exe` as it exists in the container which currently is
+                    // expected to be PowerShell Core as it's inbox in the container.
                    //
                    cmd = string.Format(System.Globalization.CultureInfo.InvariantCulture,


Shouldn't this logic be in a separate function?

dantraMSFT · 2018-07-09T17:09:27Z

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

                    }
                    else
                    {
-                        processId = Convert.ToInt32(ProcessInformation.ProcessId);
+                        processId = 0;
+                        error = result;


I think you should set ErrorMessage here and in the failure path for failing to launch powershell.exe; otherwise, it won't be diagnosable (i.e., which path failed? pwsh.exe or powershell.exe after not finding pwsh.exe)

BrucePay

Subject to Dan's comments, looks ok to me.

make the code to get the process command a method make the error when we fail to launch the container process include the process that we were trying to launch.

SteveL-MSFT · 2018-07-09T19:37:22Z

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

-                        error = result;
+                        processId = Convert.ToInt32(ProcessInformation.ProcessId);
+                    }
+                    else if (result == 2147942402) // 0x80070002 (2147942402) - The system cannot find the file specified.


Prefer to just have the number on RHS in hex as it's easier to perform a web search against a hex error code. The comment can then be simplified.

SteveL-MSFT · 2018-07-09T19:41:24Z

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

+                        Executable = "powershell.exe";
+                        cmd = GetContainerProcessCommand();
+
+                        result = HcsCreateProcess(ComputeSystem, cmd, ref ProcessInformation, ref Process, ref resultString);


Seems like this can be simplified as a loop (having only one HcsCreateProcess and checking of the error). Have Executable as an array containing pwsh.exe, powershell.exe. Once process creation succeeds, break out of the loop, otherwise try the next executable.

SteveL-MSFT · 2018-07-09T20:15:05Z

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

+                        {
+                            processId = 0;
+                            error = result;
+                            // the executable was found but did not work


We should return the HRESULT within the exception message for future troubleshooting

SteveL-MSFT · 2018-07-09T20:15:20Z

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

+                            // the process was started, exit the loop.
+                            break;
+                        }
+                        else if (result == 0x80070002) // 0x80070002 (2147942402) - The system cannot find the file specified.


remove the "0x80070002 (2147942402) -" as it's not needed now

SteveL-MSFT · 2018-07-09T20:15:51Z

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

+            return string.Format(System.Globalization.CultureInfo.InvariantCulture,
+                        @"{{""CommandLine"": ""{0} {1} -NoLogo {2}"",""RestrictedToken"": {3}}}",
+                        Executable, 
+                        (RuntimeId != Guid.Empty) ? "-so -NoProfile" : "-NamedPipeServerMode",


Can we expand -so?

bergmeister · 2018-07-09T21:12:00Z

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

+        /// </summary>
+        private string GetContainerProcessCommand()
+        {
+            return string.Format(System.Globalization.CultureInfo.InvariantCulture,


Using string interpolation and FormattableString.Invariant would make this string construction easier to read:

using static System.FormattableString; private string foo() { return Invariant($"bar {expression} {baz == null ? 1 : 0}"); }

PaulHigin · 2018-07-09T22:16:42Z

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

-                        (RuntimeId != Guid.Empty) ? "-so -NoProfile" : "-NamedPipeServerMode",
-                        String.IsNullOrEmpty(ConfigurationName) ? String.Empty : String.Concat("-Config ", ConfigurationName),
-                        (RunAsAdmin) ? "false" : "true");
+                    string[] executables = new string[]("pwsh.exe"."powershell.exe")


I am unfamiliar with this syntax. Shouldn't the string array initializer be?:

var executables = new string[] {"pwsh.exe", "powershell.exe"}

Also this can be a static readonly string array.

PaulHigin · 2018-07-09T22:16:50Z

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

+        {
+            return string.Format(System.Globalization.CultureInfo.InvariantCulture,
+                        @"{{""CommandLine"": ""{0} {1} -NoLogo {2}"",""RestrictedToken"": {3}}}",
+                        Executable, 


It feels cleaner if the Executable loop variable is passed in as a parameter. I know we need the property set for error handling, but this code is a bit confusing without realizing Executable is a loop variable.

PaulHigin · 2018-07-09T22:17:23Z

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

+                            // the process was started, exit the loop.
+                            break;
+                        }
+                        else if (result == 0x80070002)


Can we create a const variable for this expected error, with a descriptive name?

Also, set executable property in that function

PaulHigin · 2018-07-10T16:04:03Z

src/System.Management.Automation/engine/remoting/common/RunspaceConnectionInfo.cs

+                        {
+                            // "The system cannot find the file specified", try the next one
+                            // or exit the loop of none are left to try.
+                            continue;


If both pwsh.exe and powershell.exe files are not found then this will exit without error. It is unlikely but not impossible and I feel we should handle this error case so as not to change behavior.

Fixed, it is actually fairly likely as NanoServer containers don't have PowerShell by default if they are 1709 or newer.

PaulHigin

LGTM

TravisEz13 · 2018-07-10T23:00:08Z

Restarted CI due to #7260

SteveL-MSFT · 2018-07-11T22:15:41Z

@PowerShell/powershell-committee reviewed this and the order makes sense. There was some discussion about supporting pwsh-preview, however, we'll defer that until that is customer feedback requiring that change.

Port change to use pwsh and then to fall back to Windows PowerShell i…

ec7a234

…n PowerShell direct fixes PowerShell#7237

TravisEz13 requested review from dantraMSFT, mirichmo and PaulHigin as code owners July 6, 2018 19:38

TravisEz13 added the Breaking-Change breaking change that may affect users label Jul 9, 2018

TravisEz13 added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jul 9, 2018

dantraMSFT suggested changes Jul 9, 2018

View reviewed changes

BrucePay reviewed Jul 9, 2018

View reviewed changes

make the process a property

20dde80

make the code to get the process command a method make the error when we fail to launch the container process include the process that we were trying to launch.

SteveL-MSFT requested changes Jul 9, 2018

View reviewed changes

TravisEz13 added 2 commits July 9, 2018 13:04

Convert CreateContainerProcessInternal to use a loop

06a3f77

Add comments

9bed542

SteveL-MSFT requested changes Jul 9, 2018

View reviewed changes

TravisEz13 added 4 commits July 9, 2018 13:28

Add error code to cannot create process resource

4e0530c

format cannot create process resource with error code

1b3cd96

expand -so to -SocketServerMode

74b597c

simplify comments about cannot find file case

8e370cc

bergmeister reviewed Jul 9, 2018

View reviewed changes

dantraMSFT approved these changes Jul 9, 2018

View reviewed changes

PaulHigin suggested changes Jul 9, 2018

View reviewed changes

add const for file not found hresult

bf06fc7

TravisEz13 force-pushed the port_ps_direct_changes branch from d6d49f1 to cd90133 Compare July 10, 2018 01:30

TravisEz13 added 2 commits July 9, 2018 18:32

make executable array static readyonly

99acff5

pass executableToTry to function to generate command

239f8a7

Also, set executable property in that function

TravisEz13 force-pushed the port_ps_direct_changes branch from cd90133 to 239f8a7 Compare July 10, 2018 01:32

PaulHigin suggested changes Jul 10, 2018

View reviewed changes

make sure we return an error if we not found any PowerShell host

e7c32b1

PaulHigin approved these changes Jul 10, 2018

View reviewed changes

SteveL-MSFT approved these changes Jul 10, 2018

View reviewed changes

Fix StyleCop issues

b638ecd

TravisEz13 force-pushed the port_ps_direct_changes branch from 002412f to b638ecd Compare July 11, 2018 21:41

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Jul 11, 2018

TravisEz13 merged commit 79d20a1 into PowerShell:master Jul 11, 2018

TravisEz13 deleted the port_ps_direct_changes branch July 11, 2018 22:44

stknohg mentioned this pull request Oct 2, 2018

Is really PowerShell Direct tries to use PowerShell Core first? #7919

Closed

SteveL-MSFT mentioned this pull request Mar 29, 2019

Using Enter-PSSession -ContainerId is hardcoded to powershell.exe #6669

Closed

Update PowerShell Direct to try pwsh then fallback to powershell #7241

Update PowerShell Direct to try pwsh then fallback to powershell #7241

Uh oh!

Conversation

TravisEz13 commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

bergmeister commented Jul 8, 2018

Uh oh!

TravisEz13 commented Jul 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BrucePay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PaulHigin left a comment

Choose a reason for hiding this comment

Uh oh!

TravisEz13 commented Jul 10, 2018

Uh oh!

SteveL-MSFT commented Jul 11, 2018

Uh oh!

Uh oh!

TravisEz13 commented Jul 6, 2018 •

edited

Loading