Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "Invoke-Item" to accept a file path with spaces on Unix platforms #3850

Merged
merged 4 commits into from May 24, 2017

Conversation

daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented May 23, 2017

Address #2900

Root Cause

On Unix platforms, we are currently depending on xdg-open on Linux and open on OSX to open the default program for a registered file type, so the file path is served as an argument to xdg-open and open. However, we didn't handle the case when path contains spaces.

Fix

Use the method NativeCommandParameterBinder.NeedQuotes, which is used by powershell native command processor, to check if quote is needed. If yes, add quotes in the same way as our native command processor (see the code here).
There is another problem with the current Invoke-Item on Linux and OSX -- it cannot invoke an executable properly. The fix is to first run Process.Start directly with the file path, and if it fails, then try xdg-open or open.

Additional Info

We are focusing on 2 scenarios for "Invoke-Item":

  • it can start an executable
  • it can open a file with the default program that is registered to the file type.

Copy link
Member

@adityapatwardhan adityapatwardhan left a comment

Choose a reason for hiding this comment

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

Looks good, except one small comment.

// If it's headless SKUs, rethrow.
if (Platform.IsNanoServer || Platform.IsIoT) { throw; }
// If it's full Windows, then try ShellExecute.
ShellExecuteHelper.Start(invokeProcess.StartInfo);
}
#else
invokeProcess.StartInfo.FileName = path;
invokeProcess.Start();
finally { /* Nothing to do in FullCLR */ }
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a empty finally block?

Copy link
Member Author

Choose a reason for hiding this comment

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

The finally block is kept there for FullCLR section only to match the try block above, so that we don't need to enclose try { and } in if/def. I will add one more comment to make the purpose more clear.
An empty finally block will be ignored during compilation (release version), so there won't be any performance concern. See the code below:

static void Main(string[] args)
{
    Process p = null;
    try { p = new Process(); } finally {  }
}

Corresponding IL

.method private hidebysig static void  Main(string[] args) cil managed
{
  .entrypoint
  // Code size       7 (0x7)
  .maxstack  8
  IL_0000:  newobj     instance void [System.Diagnostics.Process]System.Diagnostics.Process::.ctor()
  IL_0005:  pop
  IL_0006:  ret
} // end of method Program::Main

@iSazonov
Copy link
Collaborator

The cmdlet seems to have the most unpredictable behavior😕

@daxian-dbw
Copy link
Member Author

@iSazonov I think we are focusing on 2 scenarios for this cmdlet:

  • it can start an executable
  • it can open a file with the default program that is registered to the file type.

@daxian-dbw daxian-dbw merged commit 99f9ef2 into PowerShell:master May 24, 2017
@daxian-dbw daxian-dbw deleted the invoke-item branch May 24, 2017 20:31
@iSazonov
Copy link
Collaborator

@daxian-dbw Thanks for clarify. Could you move the comment to the PR description for future docs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants