Improving UseShellExecute implementation on Process.Start #23705

maryamariyan · 2017-08-31T14:39:52Z

Fixes both #19956 and #22299

ghost · 2017-08-31T15:51:49Z

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Linux.cs

+
+        private string GetPathToProgram(string program)
+        {
+            var path = Environment.GetEnvironmentVariable("PATH");


Please review the corefx guidelines for "var" usage - these don't fit.

The guidelines are at https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/coding-style.md 😃

oops I didnt realize you guys will get notified of my PR just yet :) I need to cleanup all this now

When you create a PR, folks will notice fairly quickly even if they aren't assigned it. So generally you throw up a PR either when you think you're done, or when you need feedback on what you have so far 😄

danmoseley · 2017-08-31T15:56:48Z

src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs

-                Assert.Equal(42, p.ExitCode);
-            }
-        }
+        //[Fact]


Since you commented out tests -- are you looking for feedback on the approach, before you write tests?

I have the failing tests commented out for now, since I'm toggling between different os as I make fixes (win, osx and linux). I will keep them commented out until all get fixes.

actually some of them are failing because I don't know if it is ok to change current behavior of Process.Start just yet or not. I'll explain in more detail in a bit when I have a good summary to share.

danmoseley · 2017-08-31T15:57:33Z

src/System.Diagnostics.Process/src/System/Diagnostics/Process.OSX.cs

 using System.Collections.Generic;
 using System.ComponentModel;
+using System.Globalization;


I do'nt think you need all those. I believe VS has a refactoring to remove unnecessary using statements.

danmoseley · 2017-08-31T15:58:39Z

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Linux.cs

+            var lookupLocation = path.Split(":");
+            foreach (var loc in lookupLocation)
+            {
+                string[] dirs = Directory.GetFiles(loc, program);


instead of dirs maybe files ? and instead of loc maybe dir

maryamariyan · 2017-09-01T05:10:28Z

src/System.Diagnostics.Process/tests/ProcessTests.Linux.cs

+        [Fact]
+        public void UseShellExecuteIsFalse_ThrowsWithOrWithoutMyChanges()
+        {
+            // The test below fails on linux with or without my changes 


Observation 1: the commented out test below fails on linux with or without my changes.
Notice that UseShellExecute is false.

Question: is this ok to fail?

maryamariyan · 2017-09-01T05:10:46Z

src/System.Diagnostics.Process/tests/ProcessTests.OSX.cs

+        [Fact]
+        public void UseShellExecuteIsFalse_ThrowsWithOrWithoutMyChanges()
+        {
+            // The test below fails on osx with or without my changes 


Observation 2: the commented out test below fails on osx with or without my changes.
Notice that UseShellExecute is false.

Question: is this ok to fail?

maryamariyan · 2017-09-01T05:13:32Z

src/System.Diagnostics.Process/tests/ProcessTests.OSX.cs

+            using (var p = Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName = "exit", Arguments = "42" }))
+            {
+                Assert.True(p.WaitForExit(WaitInMS));
+                Assert.Equal(42, p.ExitCode); // Assert.Equal Failure with after adding my changes: Expected: 42, Actual: 1


Observation 3: this Assert.Equals fails on osx only after I submit my changes.
Notice that UseShellExecute is true.

Question: do I need to fix this to keep the old behavior?

maryamariyan · 2017-09-01T05:13:51Z

src/System.Diagnostics.Process/tests/ProcessTests.Linux.cs

+            using (var p = Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName = "exit", Arguments = "42" }))
+            {
+                Assert.True(p.WaitForExit(WaitInMS));
+//                Assert.Equal(42, p.ExitCode); // Assert.Equal Failure with after adding my changes: Expected: 42, Actual: 1


Observation 4: this Assert.Equals fails on linux only after I submit my changes.
Notice that UseShellExecute is true.

Question: do I need to fix this to keep the old behavior?

I don't think this should work. If it did work, UseShellExecute would have completely different effects on Linux and Windows. (see my comment)
PS: This also applies to your other Questions from four hours ago.

danmoseley · 2017-09-05T20:27:37Z

src/System.Diagnostics.Process/tests/ProcessTests.Linux.cs

+        [OuterLoop("Launches default application")]
+        public void NewXdg_TestWithFilename_ShouldUseOpenWithDefaultApp()
+        {
+            string fileToOpen = Path.Combine(Environment.CurrentDirectory, "..", "..", "..", "..", "PATENTS.TXT");


Better to have a txt file in the tests\ directory and have this in your test csproj

<Content Include="foo.txt"> <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory> </Content>

Making sure we are coherent with mono implementation Added unit tests per linux and osx

maryamariyan · 2017-09-08T05:13:24Z

src/System.Diagnostics.Process/tests/ProcessTests.Linux.cs

+                    {
+                        if (px != null)
+                        {
+                            Assert.Equal(program, px.ProcessName);


In this test I mostly care to test which process we ended up using to open the input file. therefore I could remove the extra assertions that check if px exited and the exitcode value assertion as well

maryamariyan · 2017-09-08T05:15:03Z

src/System.Diagnostics.Process/tests/ProcessTests.Linux.cs

+                    Assert.Equal("xdg-open", p.ProcessName);
+                    p.WaitForExit();
+                    Assert.True(p.HasExited);
+                    Assert.Equal(2, p.ExitCode);


In this test I wanted to show that when no file exists the xdg-open process will return the expected exitcode it uses when file doesnt exist. so the most important assertion here is Assert.Equal(2, p.ExitCode) and the rest could be removed if needed

maryamariyan · 2017-09-08T05:15:27Z

src/System.Diagnostics.Process/tests/ProcessTests.OSX.cs

+using System.Threading;
+using System.Security;
+using Xunit;
+using Xunit.NetCore.Extensions;


need to check if I can remove unnecessary usings from here. also, there is room to cleanup the osx tests.

yes, you can always remove unnecessary usings. VS should do it for you in a couple clicks.

Surely. But I have VS installed on windows and VSCode on the other machine.

When I'm using VS on windows, the "remove and sort" feature is not available for me on this file. I think it is because the intellisense for this file is not available, probably because in csproj it is not included for windows.

VSCode also has limited functionality I think. It doesn't have the feature to remove and sort. I think there is limited intellisense functionality available in VSCode in general. I may be wrong, just my observation so far.

Ah right. I wouldn't bother with it then. we don't do it consistently and it's easy for us to do it periodically.

@maryamariyan It's definitely not obvious at first, but if you go into the "Configuration Manager" menu in VS, you can switch the build configuration for each project. You can select the OSX configuration for this project and then do refactorings.

Not that you need to rush to do that or anything, just a tip for the future 😄

Thanks @mellinoe :)

maryamariyan · 2017-09-08T05:24:08Z

src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs

        [Fact]
-        public void TestUseShellExecute_Unix_Succeeds()
+        public void ProcessStart_UseShellExecuteTrue_OpenNano_OpensNano()


this fails on osx. I need to investigate

danmoseley · 2017-09-10T01:40:43Z

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Linux.cs

+        private string GetPathToProgram(string program)
+        {
+            string path = Environment.GetEnvironmentVariable("PATH");
+            string[] dirs = path.Split(":");


If for some reason PATH is not set in this shell, this will throw NRE.

it looks like Mono falls back to the current directory.
https://github.com/mono/mono/blob/master/mono/eglib/gpath.c#L236

You could eliminate all this code and just call ResolvePath() if you don't mind a slightly different resolution algorithm. I think that's fine.

yes, I just noticed ResolvePath() is also calling path to get path. I'm going to extract that logic into a private method and reuse that private method both in ResolvePath() and in here. Thanks

maryamariyan · 2017-09-12T02:01:35Z

@danmosemsft my plan is to let the build run once with all [OuterLoop] commented out.
I think this should fail on OSX tests that pop up browsers I think.
Once the build is finished, then I'll update with another commit to uncomment all the [OuterLoop] lines.

danmoseley · 2017-09-12T02:02:17Z

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs

+                if (!startInfo.UseShellExecute)
+                {
+                    // Could not find the file
+                    Debug.Assert(string.IsNullOrEmpty(filename));


If filename was null/empty (and USE=false), then it would go into this block. so why assert? Debug.Assert should only be to catch mistakes by us (you). In this case it seems like it's for bad user input, and we should throw somewhere.

This assert is against filename.

filename is retrieved from ResolvePath() on line 250. This happens when startInfo.FileName is a URL.

I added a comment on the summary of this method to explain this.
Also, it's not expected to reach this line in any way when filename is not null. In that case we would have thrown already on line 264

danmoseley · 2017-09-12T02:03:15Z

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs

+                    out childPid,
+                    out stdinFd, out stdoutFd, out stderrFd);
+
+                Debug.Assert(result == 0);


If result != 0 does that mean you made a mistake in your code? If not, Debug.Assert is probably not the right choice. You should either ignore it (if appropriate) or throw, or some other effect visible in retail.

it is not expected to reach here and result != 0 so I think the assert is proper here.

danmoseley · 2017-09-12T02:05:28Z

src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs

+            Win32Exception e = Assert.Throws<Win32Exception>(() => Process.Start(new ProcessStartInfo { UseShellExecute = false, FileName = "exit", Arguments = "42" }));
+        }
+
+        [Fact]


this test and the one below, do they match Windows behavior? If so they shoud lbe in ProcessTests.cs (ie., not Unix specific file). This will help make certain the behavior stays the same betwen Unix and windows.

and if they don't match Windows behavior, hopefully we thought that through, and a comment here explaining why we chose to diverge would be good

danmoseley · 2017-09-12T02:10:35Z

src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs

+                    px.Kill();
+                    px.WaitForExit();
+                    Assert.True(px.HasExited);
+                    Assert.Equal(137, px.ExitCode); // 137 means the process was killed


137 = Interop.Sys.Signals.SIGKILL + 128 btw... I think the shell adds 128 to signals as the exit code.

ah, BASH man page The return value of a simple command is its exit status, or 128+n if the command is terminated by signal n.

danmoseley · 2017-09-12T02:10:51Z

src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs

+        // [OuterLoop("Opens program")]
+        public void ProcessStart_OpenFileOnLinux_UsesSpecifiedProgram(string programToOpenWith)
+        {
+


danmoseley · 2017-09-12T02:13:08Z

src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs

+            }
+            else
+            {
+                Console.WriteLine($"Program specified to open file with {programToOpenWith} is not installed on this machine.");


you could possibly log Environment.GetenvironmentVariable("PATH") if that might help.

danmoseley · 2017-09-12T02:13:57Z

src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs

+            using (var px = Process.Start(programToOpenWith, fileToOpen))
+            {
+                Console.WriteLine($"in OSX, {nameof(programToOpenWith)} is {programToOpenWith}, while {nameof(px.ProcessName)} is {px.ProcessName}.");
+                // Assert.Equal(programToOpenWith, px.ProcessName); // on OSX, process name is dotnet for some reason


you were going to open an issue? if so you could put the URL of hte issue in the comment

created #23972 to track this.

danmoseley · 2017-09-12T02:15:50Z

src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs

+
+        [Fact, PlatformSpecific(TestPlatforms.OSX)]
+        // [OuterLoop("Opens program")]
+        public void ProcessStart_UseShellExecuteTrue_TryOpenFileThatDoesntExist_ReturnsExitCode1()


where is the Linux version of this test? also, why is this OSX specific? If it matches Windows behavior, it should be a test for all platforms?

There is another test similar to this for Linux, but since the return code is different for linux I split it to two different tests. (the exit code is coming from xdg-open and open).
I'll add a comment to explain why I split them to two tests

danmoseley

LGTM aside from comments. Thanks!

maryamariyan · 2017-09-12T16:38:53Z

src/System.Diagnostics.Process/tests/ProcessTests.cs

+        {
+            using (var px = Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName = Path.GetTempPath() }))
+            {
+                Assert.Null(px); // Not sure why px returned is null. but the call does not throw and opens folder successfully.


Added test for #23969

joperezr · 2017-09-12T17:10:51Z

Looking at the two test failures seems like you need to make some tests platform specific. netcoreapp-nano and uap-RS2 are throwing: System.PlatformNotSupportedException : UseShellExecute is not supported on this platform.

joperezr

Other than the comments by the rest of the folks, this LGTM once CI is green.

maryamariyan · 2017-09-12T17:29:29Z

@dotnet-bot test outerloop please

wfurt · 2017-10-17T04:53:59Z

It seems like this change will break backward compatibility.
/bin/sh basically exists on any unix. New helpers may or may not.
The other problem is that shell normally handles pipes, shell variable expansions and (dangerous) embedded subcommands (like "echo cat /etc/os-release")
Running shell will also typically process .profile and possibly .bashrc (or /etc/bashrc)
There may be additional variables set or PATH variable can be modified.

Secondary note is that "use shell" has established meaning on Unix.
I think we are breaking that.
Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName ="ls"})
used to work but it does not any more.
@janvorli, @stephentoub ot @tmds may have some more thoughts.

TSlivede · 2017-10-17T18:30:39Z

@wfurt I agree, that "shellExecute" sounds like command-line-shell (e.g. /bin/sh). So yes, "UseShellExecute" might be an unlucky name for the given purpose.
The reason for this name is the Win32-API function ShellExecute - "shell" typically doesn't mean command-line-shell within the Windows-API.

That said, using /bin/sh for "UseShellExecute" should never have been implemented, because that's not what "UseShellExecute" is about, See https://github.com/dotnet/corefx/issues/19956#issuecomment-325704971 and the following comments.

Process.Start(new ProcessStartInfo { UseShellExecute = true, FileName ="ls"})
used to work but it does not any more.

I didn't notice that before but yes, this case should be fixed if it doesn't work currently. (Because Windows .Net does also respect the Path-Environment-Variable when using "UseShellExecute") It should not be fixed by using /bin/sh but by searching FileName in the PATH-Environment-Variable before passing it to the helper application. Additionally, we might want to ensure that if FileName is an executable, it is executed without "helper application", because ShellExecute on windows allows executing of application-binaries and some of the "helper applications" seem to not really support that.

danmoseley · 2017-10-17T19:15:37Z

We attempted to match Mono behavior. It's a good point that the original 'sh' behavior we had would be useful for some folks and maybe we should have a way to choose that. Also we can discuss whether UseShellExecute=true on Unix should use sh or xdg-open. In the Windows world, Shell Execute means more or less "here's a file please give it to the correct program". Maybe we need ExecuteWithShell=true|false also. @wfurt will open an issue to discuss this...

stephentoub · 2017-10-17T20:56:04Z

because that's not what "UseShellExecute" is about

Says who?

wfurt · 2017-10-17T22:16:36Z

I created #24704 with some more examples of 2.0 behavior. Who ever cares about Process.Start() with shell should jump on that thread.

Improving UseShellExecute implementation on Process.Start Commit migrated from dotnet/corefx@e670fd3

dnfclas added the cla-already-signed label Aug 31, 2017

maryamariyan changed the title ~~Temp unix fix~~ Attempt to improve UseShellExecute implementation on Process.Start Aug 31, 2017

ghost reviewed Aug 31, 2017

View reviewed changes

danmoseley reviewed Aug 31, 2017

View reviewed changes

maryamariyan force-pushed the temp-unix-fix branch from bd05de2 to 4660ea6 Compare August 31, 2017 17:07

karelz added the area-System.Diagnostics.Process label Aug 31, 2017

karelz assigned maryamariyan Aug 31, 2017

maryamariyan changed the title ~~Attempt to improve UseShellExecute implementation on Process.Start~~ [WIP] Improving UseShellExecute implementation on Process.Start Aug 31, 2017

maryamariyan changed the title ~~[WIP] Improving UseShellExecute implementation on Process.Start~~ [WIP] Don't Review Yet. Improving UseShellExecute implementation on Process.Start Aug 31, 2017

maryamariyan force-pushed the temp-unix-fix branch 2 times, most recently from d88796a to dc2d88f Compare September 1, 2017 05:07

maryamariyan commented Sep 1, 2017

View reviewed changes

maryamariyan force-pushed the temp-unix-fix branch from dc2d88f to 625a87b Compare September 1, 2017 21:09

danmoseley reviewed Sep 5, 2017

View reviewed changes

maryamariyan force-pushed the temp-unix-fix branch 4 times, most recently from f4c01d8 to 30b1771 Compare September 8, 2017 05:05

Improving unix implementation for UseShellExecute

75f34a5

Making sure we are coherent with mono implementation Added unit tests per linux and osx

maryamariyan force-pushed the temp-unix-fix branch from 30b1771 to 75f34a5 Compare September 8, 2017 05:07

maryamariyan commented Sep 8, 2017

View reviewed changes

danmoseley reviewed Sep 10, 2017

View reviewed changes

fixing failure on linux

fc03dc4

danmoseley reviewed Sep 12, 2017

View reviewed changes

danmoseley approved these changes Sep 12, 2017

View reviewed changes

Applied second round code review

3682f8a

maryamariyan commented Sep 12, 2017

View reviewed changes

joperezr approved these changes Sep 12, 2017

View reviewed changes

uncommenting OuterLoops and fixing conditional test

3a5c6c3

maryamariyan merged commit e670fd3 into dotnet:master Sep 12, 2017

maryamariyan deleted the temp-unix-fix branch September 12, 2017 18:40

maryamariyan mentioned this pull request Sep 13, 2017

Fixing assertion failure happening on some of the unix machines #24017

Merged

karelz added this to the 2.1.0 milestone Oct 11, 2017

paavohuhtala mentioned this pull request Jun 24, 2018

Show a message when no games are detected OpenSAGE/OpenSAGE#128

Merged

wfurt mentioned this pull request Jan 31, 2020

compatibility consideration for UseShellExecute with Process.Start() dotnet/runtime#23877

Closed

teinarss mentioned this pull request Feb 28, 2020

[WIP] As a player in a multiplayer game I should be able to click on a url in a message to open that url in my browser. OpenRA/OpenRA#17532

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#23705 from maryamariyan/temp-unix-fix

f5e67ea

Improving UseShellExecute implementation on Process.Start Commit migrated from dotnet/corefx@e670fd3

Improving UseShellExecute implementation on Process.Start #23705

Improving UseShellExecute implementation on Process.Start #23705

Conversation

maryamariyan commented Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryamariyan Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

maryamariyan Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

maryamariyan Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

maryamariyan Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

TSlivede Sep 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryamariyan Sep 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryamariyan Sep 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryamariyan commented Sep 12, 2017

Choose a reason for hiding this comment

maryamariyan Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joperezr commented Sep 12, 2017

joperezr left a comment

Choose a reason for hiding this comment

maryamariyan commented Sep 12, 2017

wfurt commented Oct 17, 2017

TSlivede commented Oct 17, 2017 • edited Loading

danmoseley commented Oct 17, 2017

stephentoub commented Oct 17, 2017

wfurt commented Oct 17, 2017

maryamariyan commented Aug 31, 2017 •

edited

Loading

maryamariyan Sep 1, 2017 •

edited

Loading

maryamariyan Sep 1, 2017 •

edited

Loading

maryamariyan Sep 1, 2017 •

edited

Loading

maryamariyan Sep 1, 2017 •

edited

Loading

TSlivede Sep 1, 2017 •

edited

Loading

maryamariyan Sep 8, 2017 •

edited

Loading

maryamariyan Sep 10, 2017 •

edited

Loading

maryamariyan Sep 12, 2017 •

edited

Loading

TSlivede commented Oct 17, 2017 •

edited

Loading