GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Support Invoke-Item -Path <folder> by SteveL-MSFT · Pull Request #4262 · PowerShell/PowerShell

Conversation

@SteveL-MSFT

@SteveL-MSFT Steve Lee (SteveL-MSFT) commented Jul 15, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT Steve Lee (SteveL-MSFT) assigned Dongbo Wang (daxian-dbw) Jul 15, 2017
@SteveL-MSFT
Steve Lee (SteveL-MSFT) requested a review from Dongbo Wang (daxian-dbw) July 15, 2017 01:58
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Jul 15, 2017
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Jul 15, 2017
View reviewed changes
Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Jul 15, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 15, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 15, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Jul 15, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 15, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! I'll make the change

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the invoke-item branch from 4e1450d to b555671 Compare July 15, 2017 17:11
iSazonov
Ilya (iSazonov) reviewed Jul 15, 2017
View reviewed changes

@iSazonov Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave a comment

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated

@iSazonov Ilya (iSazonov) Jul 15, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 15, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 15, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jul 15, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 15, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jul 15, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 15, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 Outdated

@iSazonov Ilya (iSazonov) Jul 15, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 15, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jul 15, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 15, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add this. Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 15, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jul 15, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the invoke-item branch 4 times, most recently from 3f82fe4 to efea5e1 Compare July 15, 2017 23:30
iSazonov
Ilya (iSazonov) reviewed Jul 16, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 Outdated

@iSazonov Ilya (iSazonov) Jul 16, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 17, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 18, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 18, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jul 18, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 18, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jul 18, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 18, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jul 19, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated

@iSazonov Ilya (iSazonov) Jul 16, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Jul 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jul 17, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
daxian-dbw
Dongbo Wang (daxian-dbw) reviewed Jul 17, 2017
View reviewed changes
Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Jul 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :))

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 17, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Jul 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 17, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Jul 17, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 18, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the same stderr if run directly

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/System.Management.Automation/namespaces/FileSystemProvider.cs Outdated

@daxian-dbw Dongbo Wang (daxian-dbw) Jul 17, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NotSupportedException should have a meaningful message with it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Jul 17, 2017

Copy link
Copy Markdown
Member

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
            }
        }
SteveL-MSFT reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the invoke-item branch from bb4ab97 to 0861e12 Compare July 17, 2017 23:28
@SteveL-MSFT Steve Lee (SteveL-MSFT) mentioned this pull request Jul 18, 2017
Add validation that Invoke-Item opens directory in Finder on Mac #4282
Closed
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Jul 18, 2017

Copy link
Copy Markdown
Member

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 ...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the invoke-item branch 5 times, most recently from fa51134 to fb22c0e Compare July 19, 2017 20:03
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the invoke-item branch from fb22c0e to 5950ef8 Compare July 25, 2017 23:01
Steve Lee (SteveL-MSFT) added 7 commits July 25, 2017 22:26
@SteveL-MSFT
Support Invoke-Item -Path <folder>
65513b8 
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
added validation that shell window opened on Windows
706a7b2
@SteveL-MSFT
special case directories to work on Unix
1a3134c
@SteveL-MSFT
reorganized code to make it more readable
08f6a43
@SteveL-MSFT
changed fixed sleep to polling to make test more reliable
1aa5e77
@SteveL-MSFT
added validation for Linux
c04d1be
@SteveL-MSFT
reorganized code based on feedback
186f8ff
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the invoke-item branch from 024cf25 to 03cd9b5 Compare July 26, 2017 05:33
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the invoke-item branch from 03cd9b5 to eecc082 Compare July 26, 2017 16:17
@SteveL-MSFT
added MacOS test
46442da
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the invoke-item branch from eecc082 to 9965921 Compare July 27, 2017 20:47
@SteveL-MSFT
increased timeout for waiting for window to open
4a4cd71
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the invoke-item branch from 9965921 to 4a4cd71 Compare July 27, 2017 22:04
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Jul 27, 2017

Copy link
Copy Markdown
Member Author

Any other feedback?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
daxian-dbw
Dongbo Wang (daxian-dbw) approved these changes Jul 28, 2017
View reviewed changes
@daxian-dbw

Dongbo Wang (daxian-dbw) commented Jul 28, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit 03d4e91 into PowerShell:master Jul 28, 2017
iSazonov
Ilya (iSazonov) reviewed Jul 28, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1
}
else
{
$supportedEnvironment = $false

@iSazonov Ilya (iSazonov) Jul 28, 2017

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@daxian-dbw Dongbo Wang (daxian-dbw) Jul 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 28, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, Travis-CI, for example

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@iSazonov

Ilya (iSazonov) commented Jul 28, 2017

Copy link
Copy Markdown
Collaborator

Many thanks to Michael Klement (@mklement0) for the idea to use AppleScript!
daxian-dbw and mklement0 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT
Steve Lee (SteveL-MSFT) deleted the invoke-item branch July 28, 2017 18:00
@TravisEz13

Travis Plunk (TravisEz13) commented Jul 28, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Steve Lee (@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, Dongbo Wang (@daxian-dbw) PR here:
https://travis-ci.org/PowerShell/PowerShell/jobs/258716345
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mklement0

Michael Klement (mklement0) commented Jul 28, 2017

Copy link
Copy Markdown
Contributor

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Jul 29, 2017

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mklement0

Michael Klement (mklement0) commented Jul 29, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Steve Lee (@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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mklement0

Michael Klement (mklement0) commented Jul 29, 2017

Copy link
Copy Markdown
Contributor

Steve Lee (@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Jul 29, 2017

Copy link
Copy Markdown
Member Author

Michael Klement (@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.
mklement0 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT Steve Lee (SteveL-MSFT) mentioned this pull request Jul 31, 2017
add defensive measures to Mac invoke-item tests #4435
Merged
@iricigor Igor (iricigor) mentioned this pull request May 6, 2018
Invoke-Item /tmp fails on Debian with strange errors #6827
Closed
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@SteveL-MSFT @daxian-dbw
Support Invoke-Item -Path <folder> (PowerShell#4262)
eae8ef5 
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. On Unix, we use `xdg-open` and `open` for Linux and macOS respectively.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) daxian-dbw approved these changes
@iSazonov Ilya (iSazonov) iSazonov left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@daxian-dbw Dongbo Wang (daxian-dbw)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@SteveL-MSFT @daxian-dbw @iSazonov @TravisEz13 @mklement0 @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.