Clean up ShellExecute code to use the .NET Core implementation by daxian-dbw · Pull Request #4523 · PowerShell/PowerShell

daxian-dbw · Aug 7, 2017

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 · Aug 8, 2017


            // -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 · Aug 8, 2017

 #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 · Aug 8, 2017

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


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

Good point. Will update.

adityapatwardhan · Aug 8, 2017

+            & $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 · Aug 8, 2017

            }
        }

+        internal static bool IsWindowsDesktop


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

adityapatwardhan · Aug 8, 2017

+            & $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 · Aug 8, 2017

+                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 · Aug 8, 2017

+                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 · Aug 8, 2017

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 · Aug 8, 2017

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

adityapatwardhan · Aug 8, 2017


            // -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 · Aug 9, 2017

+            }
+            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 · Aug 9, 2017

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


Again obvious comment. Please remove or enhance.

iSazonov · Aug 9, 2017

-#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 · Aug 9, 2017

+                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 · Aug 9, 2017

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 · Aug 10, 2017

LGTM.

daxian-dbw · Aug 10, 2017

@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

Search code, repositories, users, issues, pull requests...

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

Uh oh!

Choose a reason for hiding this comment

daxian-dbw commented Aug 7, 2017 •

edited

Loading

daxian-dbw Aug 9, 2017 •

edited

Loading