Support Invoke-Item -Path <folder> by SteveL-MSFT · Pull Request #4262 · PowerShell/PowerShell

SteveL-MSFT · 2017-07-15T01:58:26Z

On CoreFx, UseShellExecute for Process start is false by default to be cross platform compatible.
In the case of a folder, Process.Start() returns Access Denied as it's not an executable.
On Windows we can use the ShellExecute path to have explorer open the folder.

Fix #4252
Fix #4282

daxian-dbw · 2017-07-15T05:18:33Z

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

Can you add || ex.NativeErrorCode == 5 to the catch in Unix block as well? ii <folder-path> doesn't work in Linux, and I believe it's because the NativeErrorCode in that case on Unix is also 5 (or a different value).

There's code above this for Unix that is supposed to catch Access Denied for the same situation. However, [System.Diagnostics.Process]::Start("/home/steve") just forks powershell with the new folder. Need to follow-up with corefx.

Opened issue in corefx https://github.com/dotnet/corefx/issues/22299

For now, we can special case directories as the existing Unix code path will work using xdg-open but it currently doesn't fall through to that code

daxian-dbw · 2017-07-15T05:25:31Z

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

On windows we can validate it by using our COM adapter:

$s = New-Object -ComObject "Shell.Application" $w = $s.Windows() $before = $w.Count Invoke-Item -Path $PSHOME $after = $w.Count $before + 1 | Should Be $after $item = $w.Item($after - 1) $item.LocationURL | Should Match ($PSHOME -replace '\\', '/') ## close the windows explorer $item.Quit()

But no easy way to do the same on non-windows platforms.

Cool! I'll make the change

iSazonov

Leave a comment

iSazonov · 2017-07-15T17:50:52Z

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

Why can't we check Directory.Exists(path) on Windows (not Nano and IoT) in line 1353 and exclude the exception in the case?

That might be cleaner so that the overall logic is in one place. I'll change.

Actually, I think this will actually make the code a bit more complex as I have to check if it's a Directory & not Nano/IoT (headless) for Windows. The exception is handling cases where you invoke a .gif or .doc so that also needs to check.

Can we Directory.Exists(path) above for Windows and leave Nano/IoT here in the catch?

We'd have to do a check if Directory and Not Nano/IoT to exercise the Desktop Windows code path so you'd special case Nano/IoT twice unless I'm misunderstanding you

I guess if we miss the test (Nano/IoT) above and only check Directory , we'll get the exception for Nano/IoT.
Also we can cache the Nano/IoT check and reuse in the catch.
In fact, I don't see perf problems in double check - invoke-item is single operation vs Dir.

I think I figured out a way to make the code more clean and readable

iSazonov · 2017-07-15T18:00:18Z

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

Can we enhance Invoke-Item -PassThru to return ProcessInfo?

Not sure how well this will work in practice. When you invoke a file that is not an executable, it gets handed off to something else to start it (xdg-open, open, or shellexecute on Windows) so if .gif starts mspaint.exe you would like to get mspaint.exe ProcessInfo back, but there's no way to do that as it's decoupled.

We could use lsof to detect that the directory is opened in a process.

I'll add this. Thanks!

Not able to figure out correct command to get this to work. It looks like lsof only works with files and not directories. If you specify a directory, it's looking for any files within that directory and not the directory itself. Also, if I get Ubuntu GUI to open $pshome directory, it doesn't show up in lsof output at all.

lsof +d $pshome | grep -v "^powershel"

It's probably because the GUI is only enumerating the files and don't have any actually open so lsof doesn't see it

lsof (w/o parameters) should show directories too - in Unix a directory is a file.

It's probably because the GUI is only enumerating

I think GUI open the directory, enumerate files and close directory. Then we run lsof...

Another idea is to change a directory default app, ex. script which create a flag file. I did this in ' "Describe "Invoke-Item tests on Windows" ' - maybe we can do the same on Unix.

iSazonov · 2017-07-16T09:57:43Z

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

I believe if we have to skip the IOS test we should split the It block by platforms and use -Pending switch.
But we can configure a type association on IOS too - let's start with https://developer.apple.com/library/content/documentation/FileManagement/Conceptual/DocumentInteraction_TopicsForIOS/Articles/RegisteringtheFileTypesYourAppSupports.html

iOS isn't same as MacOS, but found the way to do it on MacOS. Will work on this.

Will do this later, don't have Mac at work

It appears that lsregister is what one would use to work with existing registrations with Launch Services, however, it appears you need to build an app in Xcode to declare registrations with specific MIME types and there's no api to do it (let alone a command line tool). The other problem is that lsregister -dump isn't user friendly. I don't see this happening for Mac.

I see 😕 I suggest don't block the PR, 'pending' the test on MacOS and open new tracking Issue.

Created #4282

I don't think test should be Pending on Mac, we should still not get an exception on Mac, we just can't validate Finder actually opened it to the right location.

I believe the pending status is "We can't do it now, but we have to do it later."

The problem with marking it Pending is that the test isn't run at all

I'm sorry I wasn't clear. I think we should split the test by platforms because every platform has its own way of testing in the case.

iSazonov · 2017-07-16T10:49:46Z

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

Should we check NeedQuotes? Maybe do that for every filename?

NeedQuotes check is needed only if you are specifying arguments for ProcessInfo, for example, the argument string may have space in it.

I thought we could remove the 'if' and each name quoted - "filename.txt", "file name.txt".

daxian-dbw · 2017-07-17T16:01:28Z

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

So we are not differentiating FullCLR and CoreCLR on Windows. Are we supposed to ignore the FullCLR condition from now on? (which I'm totally fine with :))

We're almost at the point where we will remove the FullCLR code...

daxian-dbw · 2017-07-17T16:03:48Z

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

What if xdg-open or open fails to open a file? In that case, user won't see anything after running Invoke-Item and will suspect that Invoke-Item doesn't work.

That's probably true. I guess we'll just have to have the debug spew coming out.

What error an user see if run the xdg-open or open from command line?

It's the same stderr if run directly

daxian-dbw · 2017-07-17T16:09:18Z

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

NotSupportedException should have a meaningful message with it.

daxian-dbw · 2017-07-17T16:39:29Z

How about organizing the code in the following way, so that we can directly use the exception thrown from .NET Core to handle NanoServer and IoT cases.

#if UNIX
        // Error code 13 -- Permission denied.
        const int NOT_EXECUTABLE = 13;
#else
        // Error code 193 -- BAD_EXE_FORMAT (not a valid Win32 application).
        const int NOT_EXECUTABLE = 193;
#endif

        if (ShouldProcess(resource, action))
        {
            var invokeProcess = new System.Diagnostics.Process();
            bool invokeDefaultProgram = false

            if (Directory.Exists(path) && !Platform.IsNanoServer && !Platform.IsIoT)
            {
                // Path points to a directory and it's not NanoServer or IoT, so we can opne the file explorer
                invokeDefaultProgram = true;
            }
            else
            {
                try
                {
                    // Try Process.Start first. This works for executables on Win/Unix platforms
                    invokeProcess.StartInfo.FileName = path;
                    invokeProcess.Start();
                }
                catch (Win32Exception ex) when (ex.NativeErrorCode == NOT_EXECUTABLE)
                {
                    // The file is possibly not an executable. If it's headless SKUs, rethrow.
                    if (Platform.IsNanoServer || Platform.IsIoT) { throw; }
                    // Otherwise, try invoking the default program that handles this file.
                    invokeDefaultProgram = true
                }
            }

            if (invokeDefaultProgram)
            {
#if UNIX
                const string quoteFormat = "\"{0}\"";
                invokeProcess.StartInfo.FileName = Platform.IsLinux ? "xdg-open" : /* OS X */ "open";
                if (NativeCommandParameterBinder.NeedQuotes(path))
                {
                    path = string.Format(CultureInfo.InvariantCulture, quoteFormat, path);
                }
                invokeProcess.StartInfo.Arguments = path;
                invokeProcess.Start();
#else
                ShellExecuteHelper.Start(invokeProcess.StartInfo);
#endif
            }
        }

daxian-dbw · 2017-07-18T18:42:06Z

Travis CI froze at Describing Web cmdlets tests using the cmdlet's aliases for several runs, I'm afraid it could be a side effect from the xdg-mime ...

On CoreFx, UseShellExecute for Process start is false by default to be cross platform compatible. In the case of a folder, Process.Start() returns Access Denied as it's not an executable. On Windows we can use the ShellExecute path to have explorer open the folder.

SteveL-MSFT · 2017-07-27T23:42:55Z

Any other feedback?

daxian-dbw · 2017-07-28T05:16:17Z

Very interesting approach of using xdg-mime and AppleScript to validate results!

iSazonov · 2017-07-28T05:20:19Z

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

+                }
+                else
+                {
+                    $supportedEnvironment = $false


What environments we skip and why? Could you please add comment?

Maybe I merged too fast :) I guess this can happen on a Linux without a desktop.

Correct, Travis-CI, for example

iSazonov · 2017-07-28T05:24:33Z

Many thanks to @mklement0 for the idea to use AppleScript!

TravisEz13 · 2017-07-28T21:31:56Z

@SteveL-MSFT Should invoke text file '<TestFile>' without error on Mac is failing intermittently after this change
https://travis-ci.org/PowerShell/PowerShell/jobs/258709720
Also, @daxian-dbw PR here:
https://travis-ci.org/PowerShell/PowerShell/jobs/258716345

mklement0 · 2017-07-28T22:28:16Z

I don't know for sure, but I presume that the issue arises from startup timing issues in the 'tell application "TextEdit" to count of windows' AppleScript command, where the count returned may - unpredictably - either be 0 or 1 (or greater), if previously opened windows are automatically being restored (which macOS apps do by default).

Though - unless a previous run failed - a previous test run shouldn't leave an open window behind (I don't know how these tests are being run).

Note that in a given macOS session this is not a concern with Finder, which can be assumed to be always running.

A more defensive approach that may fix the issue is:

to start TextEdit with an initial, separate command that explicitly bypasses restoring previously opened windows: open -F -a TextEdit.
after the test, to quit TextEdit with 'tell application "TextEdit" to quit' | osascript

SteveL-MSFT · 2017-07-29T00:04:15Z

I've submitted a PR to address this. First, I didn't realize the invoke-item tests were CI, they should have been Feature. Second, I increased the timeout waiting for Finder since it seems slower on Travis-CI.

mklement0 · 2017-07-29T01:41:50Z

@SteveL-MSFT: If the increased timeout (for TextEdit, not Finder) fixes the issue, that's great.

Again, speaking in the abstract, not knowing the details of how Travis-CI runs, purely from a macOS / AppleScript perspective:

Note how the failing test expected 3 open windows, suggesting that TextEdit either was already open with 2 windows, or, more likely, that it had been open in a previous session with 2 windows, which, on restarting, TextEdit implicitly tries to reopen, which is a standard macOS feature named Resume.

Both scenarios can cause unpredictable behavior:

If TextEdit happens not to be running at the time of executing AppleScript tell application "TextEdit" to count windows, the window count reported is unreliable, because there can be race conditions that result in a lower window count being reported than the actual count of windows ultimately restored.
If TextEdit is already running and you're reopening an already opened document, the window count will not change, because the existing window is simply activated.

For that reason, the defensive approaches I recommended earlier should generally make for a more robust solution (open TextEdit without restoring previously opened windows, quit TextEdit after the test is done).

mklement0 · 2017-07-29T01:57:00Z

@SteveL-MSFT:

P.S.: Taking a step back: Arguably, in a CI environment, any macOS image should be configured with the Resume feature turned off system-wide (that feedback is probably for Travis CI):

To turn Resume off persistently system-wide:
- programmatically:
```
defaults write NSGlobalDomain NSQuitAlwaysKeepsWindows -bool false
```
- interactively:
  
  Go to System Preferences > General and check Close windows when quitting an app.

SteveL-MSFT · 2017-07-29T03:51:16Z

@mklement0 I see what you're saying now. I'll update it if it becomes an issue, currently have other issues to address. I appreciate the information you're providing.

SteveL-MSFT assigned daxian-dbw Jul 15, 2017

SteveL-MSFT requested a review from daxian-dbw July 15, 2017 01:58

msftclas added the cla-not-required label Jul 15, 2017

daxian-dbw reviewed Jul 15, 2017

View reviewed changes

SteveL-MSFT force-pushed the invoke-item branch from 4e1450d to b555671 Compare July 15, 2017 17:11

iSazonov reviewed Jul 15, 2017

View reviewed changes

SteveL-MSFT force-pushed the invoke-item branch 4 times, most recently from 3f82fe4 to efea5e1 Compare July 15, 2017 23:30

iSazonov reviewed Jul 16, 2017

View reviewed changes

daxian-dbw reviewed Jul 17, 2017

View reviewed changes

SteveL-MSFT force-pushed the invoke-item branch from bb4ab97 to 0861e12 Compare July 17, 2017 23:28

SteveL-MSFT mentioned this pull request Jul 18, 2017

Add validation that Invoke-Item opens directory in Finder on Mac #4282

Closed

SteveL-MSFT force-pushed the invoke-item branch 5 times, most recently from fa51134 to fb22c0e Compare July 19, 2017 20:03

SteveL-MSFT force-pushed the invoke-item branch from fb22c0e to 5950ef8 Compare July 25, 2017 23:01

SteveL-MSFT added 7 commits July 25, 2017 22:26

added validation that shell window opened on Windows

706a7b2

special case directories to work on Unix

1a3134c

reorganized code to make it more readable

08f6a43

changed fixed sleep to polling to make test more reliable

1aa5e77

added validation for Linux

c04d1be

reorganized code based on feedback

186f8ff

SteveL-MSFT force-pushed the invoke-item branch from 024cf25 to 03cd9b5 Compare July 26, 2017 05:33

SteveL-MSFT force-pushed the invoke-item branch from 03cd9b5 to eecc082 Compare July 26, 2017 16:17

added MacOS test

46442da

SteveL-MSFT force-pushed the invoke-item branch from eecc082 to 9965921 Compare July 27, 2017 20:47

increased timeout for waiting for window to open

4a4cd71

SteveL-MSFT force-pushed the invoke-item branch from 9965921 to 4a4cd71 Compare July 27, 2017 22:04

daxian-dbw approved these changes Jul 28, 2017

View reviewed changes

daxian-dbw merged commit 03d4e91 into PowerShell:master Jul 28, 2017

iSazonov reviewed Jul 28, 2017

View reviewed changes

SteveL-MSFT deleted the invoke-item branch July 28, 2017 18:00

SteveL-MSFT mentioned this pull request Jul 31, 2017

add defensive measures to Mac invoke-item tests #4435

Merged

iricigor mentioned this pull request May 6, 2018

Invoke-Item /tmp fails on Debian with strange errors #6827

Closed

Conversation

SteveL-MSFT commented Jul 15, 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!

iSazonov 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!

SteveL-MSFT Jul 15, 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!

iSazonov Jul 15, 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!

iSazonov Jul 18, 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

Uh oh!

Choose a reason for hiding this comment

SteveL-MSFT commented Jul 15, 2017 •

edited

Loading

SteveL-MSFT Jul 15, 2017 •

edited

Loading

iSazonov Jul 15, 2017 •

edited

Loading

iSazonov Jul 18, 2017 •

edited

Loading

SteveL-MSFT Jul 28, 2017 •

edited

Loading

TravisEz13 commented Jul 28, 2017 •

edited

Loading