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.

Sign up for GitHub

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

Jump to bottom

Enable UseShellExecute on all platforms #7198

Merged
merged 5 commits into from
Jul 2, 2018
Merged

Enable UseShellExecute on all platforms #7198

merged 5 commits into from
Jul 2, 2018

Conversation

SteveL-MSFT
Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Jun 28, 2018 •
edited
Loading

PR Summary

Previously, we limited use of UseShellExecute when starting a new process to only Windows Desktop. However, corefx now supports UseShellExecute (appropriately calling open on macOS and xdg-open on Linux) so we can have consistent code and remove some of the existing redundant code or unnecessary limiting code.

There is a bug in corefx where Arguments is passed to the executable used to ShellExecute such as open on macOS instead of the FileName intended to be started, so don't use ShellExecute if arguments is passed.

Also, because of difference in behavior with using ShellExecute with console apps compared to Windows where a new console window is opened, we only use ShellExecute if FileName doesn't refer to an actual command, like a URL.

Fix #5715

PR Checklist

@SteveL-MSFT SteveL-MSFT requested a review from daxian-dbw June 28, 2018 01:27
iSazonov
iSazonov reviewed Jun 28, 2018
View reviewed changes
test/powershell/Modules/Microsoft.PowerShell.Utility/Invoke-Item.Tests.ps1 Outdated
@@ -219,4 +189,26 @@ Describe "Invoke-Item tests on Windows" -Tags "CI","RequireAdminOnWindows" {
Start-Process $testfilepath -Wait
Test-Path $renamedtestfilepath | Should -BeTrue
}

It "Should invoke an executable file without error" -Skip:(!$IsWindows) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please clarify why we remove previous all-platform test and add this new for Windows only?

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 previous test expected UseShellExecute to only be available on Windows and the previous code was checking for different behavior on non-Windows. Since I've changed it to only use ShellExecute on non-Windows if the target is not an executable, I should put back this test.

@SteveL-MSFT
[feature]
b2a5a6f 
put back Invoke-Item test after recent code change
iSazonov
iSazonov approved these changes Jun 29, 2018
View reviewed changes
Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

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

LGTM with minor comment.

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs Outdated
//so don't use ShellExecute if arguments are specified

//Linux relies on `xdg-open` and macOS relies on `open` which behave differently than Windows ShellExecute when running console commands
//as a new console will be opened. So to avoid that, we only use ShellExecute on non-Windows if the filename is not an actual command (like a URI)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add space after // in comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was following the existing comments

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I see. But if we continue use CodeFactor we have to fix every code block without following file style.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll update this file with whitespace as a separate commit so it doesn't make the overall code review noisy

iSazonov
iSazonov reviewed Jun 29, 2018
View reviewed changes
src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs Outdated
@@ -428,7 +428,7 @@ internal static bool TryHasExited(Process process)
}

#endregion Internal
}//ProcessBaseCommand
} // ProcessBaseCommand
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would delete such comments. They are useless in modern IDEs.

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs Outdated
@@ -520,7 +520,7 @@ public SwitchParameter IncludeUserName
private bool _includeUserName = false;

///<summary>
///To display the modules of a process
/// To display the modules of a process
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add final dot.

src/Microsoft.PowerShell.Commands.Management/commands/management/Process.cs Outdated
@@ -530,7 +530,7 @@ public SwitchParameter IncludeUserName
public SwitchParameter Module { get; set; }

///<summary>
///To display the fileversioninfo of the main module of a process
/// To display the fileversioninfo of the main module of a process
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add final dot.

@SteveL-MSFT
Copy link
Member Author

@iSazonov did a pass through the comments, removed a bunch of unnecessary ones and added punctuation to the summaries. However, this PR has grown much larger than the original issue. I think in general, we want CodeFactor issues outside of the changes to be a separate PR.

@daxian-dbw daxian-dbw merged commit 0696034 into PowerShell:master Jul 2, 2018
@iSazonov
Copy link
Collaborator

iSazonov commented Jul 3, 2018

@SteveL-MSFT I agree that large style fixes we should place in separate PR. A separate style fix commit is suitable for small changes.

@SteveL-MSFT SteveL-MSFT deleted the use-shellexecute branch July 9, 2018 18:31
This was referenced Dec 8, 2018
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iSazonov iSazonov iSazonov approved these changes

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan adityapatwardhan is a code owner

@anmenaga anmenaga Awaiting requested review from anmenaga

@BrucePay BrucePay Awaiting requested review from BrucePay

@Francisco-Gamino Francisco-Gamino Awaiting requested review from Francisco-Gamino

Assignees

@daxian-dbw daxian-dbw

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Start-Process http://localhost:8080 fails to open browser on macOS
3 participants
@SteveL-MSFT @iSazonov @daxian-dbw