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

UseShellExecute on macOS passes Arguments to open instead of specified filename #26640

Closed
SteveL-MSFT opened this issue Jun 28, 2018 · 18 comments

Comments

@SteveL-MSFT
Copy link
Contributor

Using PSCore6:

$si = [System.Diagnostics.ProcessStartInfo]::new()
$si.UseShellExecute = $true
$si.FileName = (Get-Command pwsh).Source
$si.Arguments = "-c 1+1;read-host"
[System.Diagnostics.Process]::Start($si)

Expected:

Start new instance of PowerShell outputting answer of 2 and waiting for Enter, then exit.

Actual:

Help for open command is displayed since -c is not valid.

When using open, arguments to the executable are passed as --args <arguments>

The above script works as expected on Windows.

@karelz
Copy link
Member

karelz commented Jul 3, 2018

@SteveL-MSFT how does 'open' get into play here? Is that something .NET Core invokes when UseShellExecute=true?
Is the bug that on Mac we should prepend args with --args?

@SteveL-MSFT
Copy link
Contributor Author

@karelz Yes, .NET Core invokes open on macOS and xdg-open on Linux when UseShellExecute=true. On macOS, the Arguments property isn't passed correctly, so in the above example, .Net Core actually creates a process with arguments

open pwsh -c 1+1;read-host

open treats -c 1+1;read-host as arguments to itself, not pwsh, so it fails (since -c is not a valid parameter to open) and outputs help text. Looking at the help text for open, the correct format that should be used is:

open pwsh --args -c 1:1;read-host

Probably something similar for the Linux code path, but I only tested this on macOS.

@karelz
Copy link
Member

karelz commented Jul 9, 2018

That sounds fairly straightforward. @wtgodbe what do you think?
@SteveL-MSFT is it blocking PowerShell scenarios?

@wtgodbe
Copy link
Member

wtgodbe commented Jul 9, 2018

Seems reasonable. Looking into code paths now.

To start with, looks like this is the start of the bad path:
https://github.com/dotnet/corefx/blob/4e27e0ad415dda38c2c2add84f0315a6f56406b3/src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs#L298-L302

@wtgodbe
Copy link
Member

wtgodbe commented Jul 9, 2018

@wtgodbe wtgodbe self-assigned this Jul 9, 2018
@SteveL-MSFT
Copy link
Contributor Author

@karelz this is not blocking PowerShell Core. Since enabling UseShellExecute on non-Windows is new capability, to workaround this, if the user passed Arguments, we simply set UseShellExecute to false. not ideal, but sufficient for now. The main scenario we wanted to enable is supporting start-process https://bing.com to work on non-Windows as it currently works on Windows.

@wtgodbe
Copy link
Member

wtgodbe commented Jul 12, 2018

@karelz @SteveL-MSFT do either of you guys know how to best test a fix with powershell? I have a fix prepared, but I don't know how to get powershell to target my local build of CoreFx.

@SteveL-MSFT
Copy link
Contributor Author

@wtgodbe I believe you can just overwrite the corefx assembly in $PSHOME

@wtgodbe
Copy link
Member

wtgodbe commented Jul 13, 2018

Actually in testing without my change, I get a different result than you did. My output is:

2
/bin/sh: read-host: command not found

However, running read-host on its own works as expected.

@wtgodbe
Copy link
Member

wtgodbe commented Jul 13, 2018

I'm also having trouble testing with a local version of CoreFx - after copying in CoreFx libraries, I'm getting errors like:

Unhandled Exception: System.TypeInitializationException: The type initializer for 'System.Management.Automation.PSVersionInfo' threw an exception. ---> System.IO.FileNotFoundException: /usr/local/microsoft/powershell/6.0.2/System.Management.Automation.dll
at System.Diagnostics.FileVersionInfo.GetVersionInfo(String fileName)
at System.Management.Automation.PSVersionInfo..cctor()
--- End of inner exception stack trace ---
at System.Management.Automation.PSVersionInfo.get_GitCommitId()
at System.Management.Automation.Tracing.SysLogProvider.Log(PSEventId eventId, PSChannel channel, PSTask task, PSOpcode opcode, PSLevel level, PSKeyword keyword, Object[] args)
at System.Management.Automation.Tracing.PSSysLogProvider.WriteEvent(PSEventId id, PSChannel channel, PSOpcode opcode, PSLevel level, PSTask task, PSKeyword keyword, Object[] args)
at System.Management.Automation.Tracing.PSEtwLog.LogOperationalInformation(PSEventId id, PSOpcode opcode, PSTask task, PSKeyword keyword, Object[] args)
at Microsoft.PowerShell.UnmanagedPSEntry.Start(String consoleFilePath, String[] args, Int32 argc)
at Microsoft.PowerShell.ManagedPSEntry.Main(String[] args)

@SteveL-MSFT
Copy link
Contributor Author

@wtgodbe that exception is complaining it can't find SMA.dll at that location. Did you only overwrite the single assembly?

@wtgodbe
Copy link
Member

wtgodbe commented Jul 13, 2018

I did, in that case I get:

The shell cannot be started. A failure occurred during initialization:
Could not load file or assembly 'System.ComponentModel.Primitives, Version=4.2.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'. The system cannot find the file specified.

If I follow the chain & copy over the other dependencies (System.ComponentModel.Primitives and System.Runtime), I get:

FailFast: The type initializer for 'Microsoft.PowerShell.ApplicationInsightsTelemetry' threw an exception.

at System.Environment.FailFast(System.String, System.Exception)
at System.Environment.FailFast(System.String, System.Exception)
at Microsoft.PowerShell.UnmanagedPSEntry.Start(System.String, System.String[], Int32)
at Microsoft.PowerShell.ManagedPSEntry.Main(System.String[])

@SteveL-MSFT
Copy link
Contributor Author

@wtgodbe looking at your ShellExecute branch, it appears you branched off master which is way ahead of 2.1.x release branch which is what PSCore6.1 is currently built against. I think to test your fix against PSCore6.1, you should apply your change to the 2.1.x release branch and build off that.

I built a private off your fork using release/2.1.2 and cherry-picked your change. Was able to copy System.Diagnostics.Process.dll over the one in $PSHOME and get PSCore6.1 running.

However, it appears that open on macOS doesn't do what I thought it was going to do. What open does given a filename is start the app associated with that file and pass what follows --args to that application. So if I manually run:

open $PSHOME/pwsh --args -c 1+1

I expected a new instance of PSCore6.1 to start and run 1+1 outputting 2. However, instead PSCore6.1 starts in a new terminal tab which tells me that the application associated with the pwsh executable is the terminal and the terminal is receiving the remaining --args, not pwsh. So it seems that the best option here might be to now allow passing args if UseShellExecute=true (at least on macOS, haven't tested any of this on Linux).

@SteveL-MSFT
Copy link
Contributor Author

SteveL-MSFT commented Jul 14, 2018

The alternative is to allow passing args, but document that on non-Windows (not a fan of this inconsistency), the args get passed the default app (on Windows, it does get passed to the executable at least in the pwsh test case). The downside to supporting that is the end user needs to know exactly what the default app is. For example, if they open a URL, then the command line args passed to Chrome may not work with Firefox. In which case, it would be better for them to invoke Chrome or Firefox directly.

@tmds
Copy link
Member

tmds commented Dec 5, 2018

The original issue reported here should be fixed by dotnet/corefx#33052.

@danmoseley
Copy link
Member

@tmds @SteveL-MSFT can we close this then? If there is more to do then another issue might be clearest

@SteveL-MSFT
Copy link
Contributor Author

If this is fixed in 3.0, we can close this. Not sure when we can move PowerShell Core 6 to 3.0.

@danmoseley
Copy link
Member

fixed then.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants