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
compatibility consideration for UseShellExecute with Process.Start() #23877
Comments
If a Unix oriented developer was writing code, she could well expect it to use Which do we want to optimize for? And would we want another boolean to get the opposite? We should double check whether we are currently following Mono behavior (our intent was we follow Mono). Of course we need not do so. |
I agree, that an option for launching
https://msdn.microsoft.com/en-us/library/system.diagnostics.processstartinfo.useshellexecute.aspx :
The documentation does however specify the behavieour:
https://github.com/mairaw/docs/blob/master/xml/System.Diagnostics/Process.xml:
PS: Sorry for the huge size of this post. |
I agree that this should be fixed, because on Windows full .Net Framework (which I would consider to be the reference behaviour) |
I'd suggest |
|
That's all I was getting at, simply that it was an opinion rather than something documented. |
The documentation I've linked doesn't explicitly forbid that Additionally, using pipes (as in @wfurt's example) is also something that on Windows does not work via That said, if |
Yeah - IMO, System.Diagnostics.Process should never have had a "UseShellExecute" even on Windows (let alone making it the default.) Why create an api whose very shape screams "This is for calling CreateProcessA/W" and then at the last minute, jump to a completely different api at a completely different layering level? We can't go back and fix that blunder but let's not double down on it. |
@danmosemsft I didn't check corefx behaviour but Mono tries to mimic .net Windows behaviour using /usr/bin/open on macos and xdg-open (+ few fallbacks) on Linux. Mono never directly calls /bin/sh as that's not portable. In general, my recommendation would be try not to touch/improve this API but design a new one which is not Windows-centric and lives in a namespace which makes sense for new developers coming to the platform |
FWIW, dotnet/corefx@75f34a5 (which changes the behavior of UseShellExecute to use Note in that dotnet-templating PR how the intent is to run the standard Linux "postActions": [
{
"condition": "(OS != \"Windows_NT\")",
"description": "Make scripts executable",
"manualInstructions": [{ "text": "Run 'chmod +x *.sh'" }],
"actionId": "cb9a6cf3-4f5c-4860-b9d2-03a574959774",
"args": {
"+x": "*.sh"
},
"continueOnError": true
}
] This is supposed to result in the command
This is happening with the released version of .Net Core 2.1.300, and it has broken a lot of templates that were relying on the The behavior of UseShellExecute not using the Linux shell was a major point of surprise for me, and by major point of surprise, I mean my thought was "Why in the world would you ever do it that way? That makes no sense!" I realize that cross-platform compatibility is important, but this really needs to be documented so that Linux developers will know that UseShellExecute doesn't actually mean what it says. And there needs to be some flag that actually means "Use /bin/sh", because Linux developers need that for a lot of scenarios. |
See dotnet/core#1857 for a use-case broken by the change to |
If I understand correctly, the main problem with existing behavior is that it broke code that worked on Unix on .NET Core 1.0-2.0 by changing to have different (but to some other people desirable, and consistent with Mono) behavior. Such code now needs to be written to explicitly find and launch the installed shell. And ideally we should have anticipated this break. I agree with @marek-safar that hypothetically changing the behavior again would just cause more breaks and problems. It seems to me the way forward if we want to do anything is to add a way to invoke something with the actual shell as suggested by @TSlivede here - that is meaningful on both Windows and Unix. I will close this, then, and if anyone is motivated, they can open such an API proposal. cc @wtgodbe, @krwq as area owners feel free to reopen if you suggest otherwise. |
I think there is still a compatibility issue here that needs addressing. @TSlivede said:
Consider this snippet on Linux: using System;
using System.Diagnostics;
namespace CreateProcess
{
class Program
{
static void Main(string[] args)
{
String cmd = args[0];
var startInfo = new ProcessStartInfo(cmd);
startInfo.UseShellExecute = true;
startInfo.CreateNoWindow = true;
using (Process px = Process.Start(startInfo))
{
px.WaitForExit();
Console.WriteLine("cmd returned {0}", px.ExitCode);
}
}
}
} If we run it as:
So dotnet/corefx#23705 (or something else?) breaks executing of applications via Edit: Ah, dotnet/corefx#24017 might be the culprit here. The original change dotnet/corefx#23705 used the mono approach of trying to execute a process directly before falling back to using |
@danmosemsft said:
I agree, that another incompatible change to this API should not be made. However, at least "executing of applications via Sadly, I see no option, how "executing of applications [...] through a fully qualified path" could be fixed without breaking the current behaviour: A valid executable could still be opened in a reasonable way by I think, that to support "executing of applications [...] through a fully qualified path" dotnet would need to
This would change the behaviour for script files (they are currently most likely opened in an editor and would then be executed. Therefore this is an incompatible change - however the new behaviour is closer to windows full .NET and does not convert a previously working call to this API to a call that produces an error, so maybe this should still be changed? Anyway, all this is messy. I'd recommend to deprecate this API and provide a new clean one with explicit options, that only do exactly one thing each - not one flag that enables search in As @marek-safar suggested, the naming of such an API shouldn't be windows-centric and should make sense to all developers. Maybe:
|
I like what @TSlivede is proposing.
👍 we also need to detect file:// uri
Maybe simplify this check to: regular file with execute bit set for user, group or other. |
One possible problem with the simple "execute bit set" check is that when common Windows filesystems (NTFS and FAT32) are mounted on Linux, they often end up mounted with 777 permissions: read, write and execute permissions for user, group, and other. This varies from distro to distro, but on the Linux Mint system I'm on right now (where I didn't change the default permission settings for mounting NTFS), I have an external hard disk with an NTFS partition, and its files are all showing up with read/write/execute bits set for all users. So the simple "regular file with execute bit set" check may produce some false positives if the file is on an NTFS-mounted filesystem, and possibly in other circumstances as well. I think what @TSlivede proposed (executable bit set AND is a valid executable / a script with |
Alternatively, instead of trying to figure out if it is a valid executable up front, we could fall back from execve returning ENOEXEC. |
No API changes. When UseShellExecute is set, this would happen: Current behavior:
Proposed behavior:
|
@tmds @TSlivede I like some parts of the proposition (naming to be discussed) and I think we should not do too much magic in System.Process - it always leads to problems - I don't believe there is "one solution suits everyone" here. IMO UseShellExecute should be always doing 'sh -c' or something similar and not Simple solution could be perhaps something similar to what @TSlivede has proposed:
and perhaps:
(this sounds more like a job for an external nuget package though which could also implement "DWIM" logic - personally not a fun of heuristics in the framework but got no problem with them as separate entities) OpenInTerminal - IMO we should not have it - IMO it is separate problem |
@krwq As I already explained above, I beleave, that I can fully agree to this:
and could understand if the dotnet core team decides to remove all heuristics and |
I actually also don't really see the need for that feature. I just included this suggestion, because it was requested here and because |
Windows behavior for UseShellExecute is:
On Linux, by doing |
@tmds I agree that on Windows openning xlsx opens Excel - I do not say this is bad but as a counterargument please note that on Windows if you open command prompt and type One option perhaps could be adding something like: |
Let's focus on behavior of the Process class.
Even if UseShellExecute sounds to a Linux user as if it will use a shell program, we shouldn't make that the implementation. For compatibility/consistency reasons, we need to try to behave the same as Windows. |
Yes, but that doesn't mean
|
I know, that in the *NIX context "shell" basically always means command-line-shell. But AFAIK .NET was originally developed for Windows, so when the term shell is used in the API it almost certainly means shell as in Windows Shell - which is AFAIK closely related to the graphical userinterface of Windows and not to |
I think URLs and piping is something which convinces me more toward the proposed solution. I think it would be useful to compile a table of all scenarios we can think of which are expected to work on Windows and then from there figure out how to handle that on Linux so that we get maximum compat. It would be useful to also add stuff which is not working on Windows but currently does (or previously did) on Linux and decide what do we want to do with that going forward. |
However I don't think implementing all that within dotnet core is a good Idea... |
@TSlivede I think abandoning completely won't work - we should cover at least most of the use cases (likely something really close to what you have proposed) and propose better alternative once we know what that is. |
If by this you mean "the most used use cases" - ok, this shouldn't be too hard. Partially reverting dotnet/corefx#24017 (try to execute directly before using If by most cases you mean something like 80% of all different use cases - I don't think that's possible, because |
More strange things about
I think we should not try to emulate these strange things on *NIX... |
@TSlivede I meant used use cases 😄 I'll see if we got some data about it available from nuget/github. I agree on what you just wrote (unless we got someone requesting any of that and has a valid scenario). EDIT: unfortunately we only got usage counts but not details on how it was used... |
* Clarify that ProcessStartInfo.UseShellExecute does not mean bash The property name is very confusing on unix-like platforms. Anyone familiar with unix-like platforms will probably assume it means /bin/bash or /bin/sh. Clarify that it does not mean that shell and that it refers to the conecpt of shell on Windows, which is closer in meaning to "desktop". For an example of this tripping up users, see: dotnet/core#1857. For more on the cross-platform inconsistencies of UseShellExecute, see https://github.com/dotnet/corefx/issues/24704 * Update ProcessStartInfo.xml
I think we can close this one with the changes made in dotnet/corefx#33052. |
Thank you a lot for fixing this @tmds! |
Explicitly set UseShellExecute to true, because in .NetFramework the default value is true but in .NetCore the default value changed to false See: dotnet/winforms#1520 (comment) and https://github.com/dotnet/corefx/issues/24704
This is fresh start of topic related to dotnet/corefx#23705 and #23537
At least on Unix "UseShell" has standard meaning: shell is used to execute requested command.
There are many examples - like Python's popen() or simply difference between execv() and system().
On Windows his has different meaning and implies to open file with default application.
Unix implementation was recently updated to match Windows behavior.
However that possibly creates backward compatibility problem. 2.0 has been released with using /bin/sh to match traditional Unix experience.
Change in behavior can break existing usage of Process.Star() - and it did even in trivial case for corefx tests.
/bin/sh is pretty much guarantied to exist on any normal Unix system but new helpers may not.
Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName ="ls"}) works with 2.0 but it does not work with current master branch on my system.
Further more, Unix users would expect normal shell behavior. Following examples will succeed when running with dotnet 2.0
One can pass in fragments of shell code with 2.0.
another common use case is use if command pipeline and have shell handling it:
In this case shell will run each command in separate process and it will redirect output from cat to stdin of grep. This is pretty typical use.
Last part is use of .profile and shell variables. When used with shell, for example "echo $HOME" would expand to home directory. When used without shell, it would be passed in as string.
The text was updated successfully, but these errors were encountered: