Support Invoke-Item -Path <folder> #4262
Conversation
| @@ -1350,9 +1350,10 @@ protected override void InvokeDefaultAction(string path) | |||
| invokeProcess.Start(); | |||
| } | |||
| #elif CORECLR | |||
| catch (Win32Exception ex) when (ex.NativeErrorCode == 193) | |||
| catch (Win32Exception ex) when (ex.NativeErrorCode == 193 || ex.NativeErrorCode == 5) | |||
There was a problem hiding this comment.
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 was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
|
||
| It "Should invoke a folder without error" { | ||
| # can't validate that it actually opened, but no error should be returned | ||
| Invoke-Item -Path $PSHOME |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Cool! I'll make the change
| } | ||
| #elif CORECLR | ||
| catch (Win32Exception ex) when (ex.NativeErrorCode == 193) | ||
| catch (Win32Exception ex) when (ex.NativeErrorCode == 193 || ex.NativeErrorCode == 5) |
There was a problem hiding this comment.
Why can't we check Directory.Exists(path) on Windows (not Nano and IoT) in line 1353 and exclude the exception in the case?
There was a problem hiding this comment.
That might be cleaner so that the overall logic is in one place. I'll change.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can we Directory.Exists(path) above for Windows and leave Nano/IoT here in the catch?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think I figured out a way to make the code more clean and readable
| else | ||
| { | ||
| # can't validate that it actually opened, but no error should be returned | ||
| Invoke-Item -Path $PSHOME |
There was a problem hiding this comment.
Can we enhance Invoke-Item -PassThru to return ProcessInfo?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We could use lsof to detect that the directory is opened in a process.
There was a problem hiding this comment.
I'll add this. Thanks!
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
3f82fe4 to
efea5e1
Compare
| else | ||
| { | ||
| # can't validate on MacOS, so just make sure no error | ||
| Invoke-Item -Path $PSHOME |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
iOS isn't same as MacOS, but found the way to do it on MacOS. Will work on this.
There was a problem hiding this comment.
Will do this later, don't have Mac at work
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see 😕 I suggest don't block the PR, 'pending' the test on MacOS and open new tracking Issue.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I believe the pending status is "We can't do it now, but we have to do it later."
There was a problem hiding this comment.
The problem with marking it Pending is that the test isn't run at all
There was a problem hiding this comment.
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.
| process.StartInfo.FileName = Platform.IsLinux ? "xdg-open" : /* OS X */ "open"; | ||
| if (NativeCommandParameterBinder.NeedQuotes(filename)) | ||
| { | ||
| path = string.Format(CultureInfo.InvariantCulture, quoteFormat, filename); |
There was a problem hiding this comment.
Should we check NeedQuotes? Maybe do that for every filename?
There was a problem hiding this comment.
NeedQuotes check is needed only if you are specifying arguments for ProcessInfo, for example, the argument string may have space in it.
There was a problem hiding this comment.
I thought we could remove the 'if' and each name quoted - "filename.txt", "file name.txt".
| if (Platform.IsNanoServer || Platform.IsIoT) { throw new NotSupportedException(); } | ||
| process.StartInfo.FileName = filename; | ||
| ShellExecuteHelper.Start(process.StartInfo); | ||
| #endif |
There was a problem hiding this comment.
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 :))
There was a problem hiding this comment.
We're almost at the point where we will remove the FullCLR code...
| } | ||
| process.StartInfo.Arguments = filename; | ||
| // xdg-open on Ubuntu outputs debug info to stderr, suppress it | ||
| process.StartInfo.RedirectStandardError = true; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's probably true. I guess we'll just have to have the debug spew coming out.
There was a problem hiding this comment.
What error an user see if run the xdg-open or open from command line?
There was a problem hiding this comment.
It's the same stderr if run directly
| process.StartInfo.RedirectStandardError = true; | ||
| process.Start(); | ||
| #else | ||
| if (Platform.IsNanoServer || Platform.IsIoT) { throw new NotSupportedException(); } |
There was a problem hiding this comment.
NotSupportedException should have a meaningful message with it.
|
How about organizing the code in the following way, so that we can directly use the exception thrown from .NET Core to handle #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
}
} |
|
Travis CI froze at |
fa51134 to
fb22c0e
Compare
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.
|
Any other feedback? |
|
Very interesting approach of using |
| } | ||
| else | ||
| { | ||
| $supportedEnvironment = $false |
There was a problem hiding this comment.
What environments we skip and why? Could you please add comment?
There was a problem hiding this comment.
Maybe I merged too fast :) I guess this can happen on a Linux without a desktop.
There was a problem hiding this comment.
Correct, Travis-CI, for example
|
Many thanks to @mklement0 for the idea to use AppleScript! |
|
@SteveL-MSFT |
|
I don't know for sure, but I presume that the issue arises from startup timing issues in the 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:
|
|
I've submitted a PR to address this. First, I didn't realize the |
|
@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:
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). |
|
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):
|
|
@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. |
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