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:05:23Z

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


            // -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:18:33Z

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:22:07Z

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:26:37Z

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:27:17Z

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

            }
        }

+        internal static bool IsWindowsDesktop


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

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

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:58:34Z

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:04Z

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:32:01Z

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


            // -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:22:56Z

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:32:34Z

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:34:04Z

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:45:23Z

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 August 4, 2017 21:21

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 August 7, 2017 23:25

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

adityapatwardhan requested changes Aug 8, 2017

View reviewed changes

[Feature] Update tests

7d375eb

daxian-dbw force-pushed the shellexecute branch from 1ffa36b to 7d375eb Compare August 8, 2017 00:43

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 reviewed changes

address one more comment

16f459e

adityapatwardhan requested changes Aug 8, 2017

View reviewed changes

adityapatwardhan approved these changes Aug 8, 2017

View reviewed 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 reviewed changes

Address some more comments

c7c9394

adityapatwardhan approved these changes Aug 9, 2017

View reviewed changes

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

daxian-dbw deleted the shellexecute branch August 10, 2017 20:47

CAD97 mentioned this pull request Sep 8, 2022

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

Closed

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

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

Uh oh!

Conversation

daxian-dbw commented Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

daxian-dbw commented Aug 8, 2017

Uh oh!

daxian-dbw commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityapatwardhan left a comment

Choose a reason for hiding this comment

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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

daxian-dbw commented Aug 7, 2017 •

edited

Loading

daxian-dbw Aug 9, 2017 •

edited

Loading