Clean up ShellExecute code to use the .NET Core implementation #4523

daxian-dbw · 2017-08-07T23:25:13Z

This PR includes following major changes:

ProcessStartInfo.UseShellExecute is now supported in .NET Core on Windows. So we need to remove our implementation of ShellExecute and use the .NET Core API directly.
Enable NativeCommandProcessor to open files using default program associated with the shell.
Fix NativeCommandProcessor to properly clean up in case an exception is thrown in Prepare and ProcessRecord.

Note that, UseShellExecute works as full .NET on windows desktop, but it doesn't work properly on Unix and the improvement work is tracked by dotnet/corefx#19956. So on Linux/OSX, we still need to rely on xdg-open/open

…rocessor

… is thron

adityapatwardhan · 2017-08-08T00:30:23Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

@@ -1877,7 +1865,7 @@ protected override void BeginProcessing()
            string message = string.Empty;

            // -Verb and -WindowStyle are not supported on non-Windows platforms as well as Windows headless SKUs
-            if (_isOnFullWinSku)
+            if (Platform.IsWindowsDesktop)


Maybe cache the value in a variable instead of looking it up twice in the same method.

Updated the code to cache the value in Platform.IsWindowsDesktop.

@daxian-dbw This does not seem to be fixed.

The value is cached in a static field in Platform.IsWindowsDesktop property.

adityapatwardhan · 2017-08-08T00:30:23Z

src/System.Management.Automation/namespaces/FileSystemProvider.cs

 #else
-                    ShellExecuteHelper.Start(invokeProcess.StartInfo);
+                // Use ShellExecute when it's not a headless SKU
+                invokeProcess.StartInfo.UseShellExecute = !Platform.IsNanoServer && !Platform.IsIoT;


Can we use Platform.IsWindowsDesktop here?

Sure. Will update the code.

adityapatwardhan · 2017-08-08T00:30:23Z

test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1

+                xdg-mime default nativeCommandProcessor.desktop text/plain
+            }
+        }
+        elseif ($IsWindows) {


We can use Platform.IsWindowsDesktop if it is made public.

Good point. Will update.

adityapatwardhan · 2017-08-08T00:30:23Z

test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1

+            & $dllFile
+            throw "No Exception!"
+        } catch {
+            $_.FullyQualifiedErrorId | should be "NativeCommandFailed"


Use ShouldBeErroId. Example :

PowerShell/test/powershell/engine/Attributes.Tests.ps1

Line 22 in 0cd7a4a

{ & $testdrive/test.ps1 } | ShouldBeErrorId "Argument"

adityapatwardhan · 2017-08-08T00:30:23Z

src/System.Management.Automation/CoreCLR/CorePsPlatform.cs

@@ -148,6 +148,18 @@ internal static bool IsInbox
            }
        }

+        internal static bool IsWindowsDesktop


Should we consider making this public? This will be very useful in tests.

adityapatwardhan · 2017-08-08T00:30:23Z

test/powershell/Language/Scripting/NativeExecution/NativeCommandProcessor.Tests.ps1

+            & $TestFile
+            $startTime = Get-Date
+            # It may take time for handler to start
+            while (((Get-Date) - $startTime).TotalSeconds -lt 10 -and (-not (Test-Path "$HOME/nativeCommandProcessor.Success"))) {


Polling for a file can be a common pattern. Consider implementing a generic file watcher function in HelpersCommon.psm1

Sounds reasonable. Opened #4524. Will address in a separate PR.

iSazonov · 2017-08-08T06:59:44Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1

+                Get-Process -Name notepad | Stop-Process -Force
+                Invoke-Item -Path $notepad
+                $notepadProcess = Get-Process -Name notepad
+                $notepadProcess.Name | Should Be notepad


Maybe use quotas - "notepad"?

OK, will update.

iSazonov · 2017-08-08T06:59:44Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1

+                Invoke-Item -Path $notepad
+                $notepadProcess = Get-Process -Name notepad
+                $notepadProcess.Name | Should Be notepad
+                Stop-Process -InputObject $notepadProcess


Should we move this in AfterAll?

I think it should stay here, as it's specific to this test when running on full desktop.

I don't like commands after Should 😄

daxian-dbw · 2017-08-08T15:31:25Z

There are 2 failures in AppVeyor feature test run and both are related to Invoke-RestMethod, not related to the changes made in this PR. So the feature test run is good.

daxian-dbw · 2017-08-08T15:42:33Z

@adityapatwardhan @iSazonov Your comments have been addressed. Please take another look. Thanks!

adityapatwardhan · 2017-08-08T16:33:48Z

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs

@@ -1877,7 +1865,7 @@ protected override void BeginProcessing()
            string message = string.Empty;

            // -Verb and -WindowStyle are not supported on non-Windows platforms as well as Windows headless SKUs
-            if (_isOnFullWinSku)
+            if (Platform.IsWindowsDesktop)


@daxian-dbw This does not seem to be fixed.

adityapatwardhan

LGTM

iSazonov

Since the PR is a cleanup we could add fix - we use "UseShellExecute" constant three times in Process.cs and could move it to private static variable.

iSazonov · 2017-08-09T04:49:16Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

+            }
+            catch (Exception)
+            {
+                // Do the cleanup and rethrow the exception


It is obvious comment. Please add in the comment why we do this.

Fixed. More comment added to Cleanup() to explain what we are cleaning up.

iSazonov · 2017-08-09T04:49:16Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

-                    this.Command.Context.EngineHostInterface.NotifyEndApplication();
-                }
-                // Do the clean up...
+                // Do the cleanup...


Again obvious comment. Please remove or enhance.

iSazonov · 2017-08-09T04:49:16Z

src/System.Management.Automation/engine/NativeCommandProcessor.cs

-#else
-            if (Platform.IsNanoServer)
-                return false;
+            if (!Platform.IsWindowsDesktop) { return false; }


Formatting - Can we use the one line pattern for if?

Yes, as long as the if block is a single simple statement like this one.

iSazonov · 2017-08-09T04:49:16Z

test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1

+                Get-Process -Name notepad | Stop-Process -Force
+                Invoke-Item -Path $notepad
+                $notepadProcess = Get-Process -Name notepad
+                $notepadProcess.Name | Should Be "notepad"


We use "notepad" in three lines (66, 68, 69 and maybe 65) so maybe use a variable?

daxian-dbw · 2017-08-09T16:25:41Z

The "Default" constant string is used in 9 places and should be cleaned too (change to const variable, instead of static). But I think they should be in separate PRs as they will introduce too many diffs and make it hard to see the focus of this PR.

adityapatwardhan

LGTM

iSazonov · 2017-08-10T03:10:09Z

LGTM.

daxian-dbw · 2017-08-10T18:52:52Z

@iSazonov @adityapatwardhan Thanks for the review! Aditya, can you please merge this PR? My follow-up work on the file watcher test helper needs to update the tests in this PR (#4524).

daxian-dbw added 3 commits Aug 5, 2017

Clean up ShellExecuteHelper and enable ShellExecute in NativeCommandP…

82636e3

…rocessor

Minor fix

6ac56ab

[Feature] Fix NativeCommandProcessor to clean up in case an exception…

f99c35a

… is thron

daxian-dbw assigned adityapatwardhan Aug 7, 2017

daxian-dbw requested review from SteveL-MSFT and adityapatwardhan Aug 7, 2017

msftclas added the cla-not-required label Aug 7, 2017

adityapatwardhan requested changes Aug 8, 2017

View changes

[Feature] Update tests

7d375eb

daxian-dbw force-pushed the shellexecute branch from 1ffa36b to 7d375eb Compare Aug 8, 2017

daxian-dbw mentioned this pull request Aug 8, 2017

Add a generic file watcher function in HelpersCommon.psm1 #4524

Closed

[Feature] Address comments

5494c7f

iSazonov reviewed Aug 8, 2017

View changes

address one more comment

16f459e

adityapatwardhan requested changes Aug 8, 2017

View changes

adityapatwardhan approved these changes Aug 8, 2017

View changes

daxian-dbw changed the title ~~Shellexecute~~ Clean up ShellExecute code to use the .NET Core implementation Aug 8, 2017

iSazonov reviewed Aug 9, 2017

View changes

Address some more comments

c7c9394

adityapatwardhan approved these changes Aug 9, 2017

View changes

adityapatwardhan merged commit 384a9fe into PowerShell:master Aug 10, 2017

daxian-dbw deleted the shellexecute branch Aug 10, 2017

CAD97 mentioned this pull request Sep 8, 2022

Nushell no longer respects file associations on Windows nushell/nushell#6440

Closed

Nov	DEC	Jan
	07
2021	2022	2023

Clean up ShellExecute code to use the .NET Core implementation #4523

Clean up ShellExecute code to use the .NET Core implementation #4523

Conversation

daxian-dbw commented Aug 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw commented Aug 8, 2017

daxian-dbw commented Aug 8, 2017

Choose a reason for hiding this comment

adityapatwardhan left a comment

iSazonov left a comment

Choose a reason for hiding this comment

daxian-dbw Aug 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw commented Aug 9, 2017

adityapatwardhan left a comment

iSazonov commented Aug 10, 2017

daxian-dbw commented Aug 10, 2017

daxian-dbw commented Aug 7, 2017 •

edited

daxian-dbw Aug 9, 2017 •

edited