Unix: make UseShellExecute execute executables #33052

tmds · 2018-10-25T19:43:50Z

On Windows, UseShellExecute executes executables. This gives it the same behavior on Unix.

This implements improvements discussed in https://github.com/dotnet/corefx/issues/24704.

CC @TSlivede @krwq @wtgodbe @rmunn @danmosemsft @omajid @marek-safar @wfurt @stephentoub

On Windows, UseShellExecute executes executables. This gives it the same behavior on Unix.

TSlivede · 2018-10-28T15:37:57Z

On Windows, UseShellExecute executes executables.

This is only true if ProcessStartInfo.Verb is null, "", or "open".
As I mentioned here, UseShellExecute can do all sorts of things, depending on the value of Verb. (If UseShellExecute is false, then Verb is ignored AFAIK). If Verb has a value that Windows doesn't understand, an Exception is thrown.

This gives it the same behavior on Unix.

To behave the same as on Windows:
If UseShellExecute is true, we should check the value of Verb.

If it is null, "", or "open", the code that tries to execute or open files (currently lines 308..336 in Process.Unix.cs in your PR) is used.
If it is "runas", "edit", "print" or some other known typical value for Verb, we could throw a "not (yet?) implemented" or "platform not supported" exception. (Or maybe we don't test this...?)
If it is something else, we throw the same exception that is used on Windows for unknown values of Verb.

krwq · 2018-10-29T23:01:36Z

It would be useful to add some test cases for this

tmds · 2018-11-06T12:34:27Z

This is ready for review.

TSlivede · 2018-11-06T20:34:34Z

First of all: Thanks for implementing this!

Sorry, that I didn't notice this earlier, but on Windows Verb is matched case insensitive, so "Open" or even "oPeN" should also be accepted...

Just nitpicking: I do see advantages of moving the check of Verb into its own method (the constants "", open, etc. can be moved into a private field, so there are no "magic constants" within the code). However I think this reduces code readability and more importantly basically does a double negation of the logic behind Verb IMHO:
Instead of selecting the correct action based on the value of Verb and throwing an error for unknown values, the current implementation executes the "open"-Code if no exception was thrown because !IsValidVerb(startInfo.Verb) was false.

It's also hard to add Code for other values of Verb in the current implementation.

I've added a code suggestion as a review.

TSlivede

The suggestions below only make sense together, not one comment alone.

Reasons for this suggestion in my comment.

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

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

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

tmds · 2018-11-07T22:00:15Z

I'm going to extend this PR with the WorkingDirectory behavior for UseShellExecute (and add a test for it):

When UseShellExecute is true, the WorkingDirectory property specifies the location of the executable. If WorkingDirectory is an empty string, the current directory is understood to contain the executable.

tmds · 2018-11-08T12:53:39Z

Unrelated CI fail, Linux arm64 Release Build, Ubuntu.1604.Arm64.Open-arm64:

System.Net.NameResolution.Tests.GetHostByNameTest
 DnsObsoleteBeginEndGetHostByName_EmptyString_ReturnsHostName
 DnsObsoleteGetHostByName_EmptyString_ReturnsHostName
System.Net.NameResolution.Tests.GetHostEntryTest
 Dns_GetHostEntryAsync_HostString_Ok(hostName: \"\")
 Dns_GetHostEntry_HostString_Ok(hostName: \"\")

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

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

krwq · 2018-11-21T18:38:34Z

@tmds lgtm - I think the verb check might have been correct in the first version (negated logic got me confused) - sorry for confusion

krwq

LGTM after reverting verb check to !string.IsNullOrEmpty(verb)

tmds · 2018-11-22T13:50:25Z

Looking at the build logs, it's not clear why some things failed.
One clear error message:

09:13:14 Fatal error in IL Linker
09:13:14 System.InvalidOperationException: Operation is not valid due to the current state of the object.
09:13:14    at Mono.Collections.Generic.Collection`1.Enumerator.CheckState()
09:13:14    at Mono.Collections.Generic.Collection`1.Enumerator.MoveNext()
09:13:14    at Mono.Linker.TypeReferenceExtensions.GetMethods(TypeReference type)+MoveNext()
09:13:14    at Mono.Linker.Steps.TypeMapStep.TryMatchMethod(TypeReference type, MethodDefinition method)
09:13:14    at Mono.Linker.Steps.TypeMapStep.GetBaseMethodInTypeHierarchy(TypeDefinition type, MethodDefinition method)
09:13:14    at Mono.Linker.Steps.TypeMapStep.GetBaseMethodInTypeHierarchy(MethodDefinition method)
09:13:14    at Mono.Linker.Steps.TypeMapStep.MapVirtualBaseMethod(MethodDefinition method)
09:13:14    at Mono.Linker.Steps.TypeMapStep.MapVirtualMethod(MethodDefinition method)
09:13:14    at Mono.Linker.Steps.TypeMapStep.MapVirtualMethods(TypeDefinition type)
09:13:14    at Mono.Linker.Steps.TypeMapStep.MapType(TypeDefinition type)
09:13:14    at Mono.Linker.Steps.TypeMapStep.ProcessAssembly(AssemblyDefinition assembly)
09:13:14    at Mono.Linker.Steps.BaseStep.Process(LinkContext context)
09:13:14    at Mono.Linker.Pipeline.Process(LinkContext context)
09:13:14    at Mono.Linker.Driver.Run(ILogger customLogger)
09:13:14    at Mono.Linker.Driver.Execute(String[] args, ILogger customLogger)
09:13:14   System.Linq -> /mnt/j/workspace/dotnet_corefx/master/linux-

@dotnet-bot Test Linux arm64 Release Build please
@dotnet-bot Test Linux x64 Release Build please

LGTM after reverting verb check to !string.IsNullOrEmpty(verb)

The verb != string.Empty that is in the PR now should be good. ProcessStartInfo.Verb returns string.Empty instead of null.

danmoseley · 2018-11-22T15:31:48Z

@sbomer for the bug in the linker. I assume it doesn't repro when building the tree locally with the same flags.

stephentoub · 2018-11-22T15:46:58Z

The linker issue is https://github.com/dotnet/corefx/issues/30714

tmds · 2018-11-27T09:43:14Z

@stephentoub @danmosemsft is this good to merge?

stephentoub · 2018-11-27T11:20:44Z

I'll take a look today. Thanks.

src/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs

stephentoub · 2018-11-28T13:13:22Z

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

+                }
+                else
+                {
+                    if ((throwOnNoExec == false)


Nit: !throwOnNoExec

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

stephentoub · 2018-11-28T13:26:56Z

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

@@ -1732,5 +1836,8 @@ private SecureString AsSecureString(string str)

            return secureString;
        }
+
+        [DllImport("libc")]
+        private static extern int chmod(string path, int mode);


We already define this in ProcessTests.Unix.cs. If memory serves, it was moved out of the shared file to avoid AOT / UWP issues.

I moved it back to the Unix.cs file and split the WriteScriptFile method in a .Windows.cs and a .Unix.cs version.

tmds · 2018-11-29T08:49:39Z

All tests failed on Windows.10.Nano.Amd64.Open - x64 - Debug

@dotnet-bot test Windows x64 Debug Build please

tmds · 2018-12-04T13:31:55Z

Is this good to merge?

krwq · 2018-12-04T18:21:05Z

@tmds we need 1 more sign-off. cc: @stephentoub @wtgodbe

stephentoub · 2018-12-05T15:59:21Z

Thanks, @tmds.

tmds · 2018-12-05T16:01:31Z

Thank you too and @krwq for reviewing, and @TSlivede for many useful inputs on Windows behavior.

* Unix: make UseShellExecute execute executables On Windows, UseShellExecute executes executables. This gives it the same behavior on Unix. * Add cross-platform tests * fix Windows test build failure * Add test for non-executable file with x-bit * Skip tests on unsupported platforms * Unix: only allow specific Verbs * Limit Unix tests to run on Linux * Fix test for verb null * Improve verb check * test: make working directory a temp location * test: refactor WriteScriptFile * test: refactor temp dir creation * test: fix build failure * Condense verb check * Find the executable in the ProcessStartInfo.WorkingDirectory * Don't pass Arguments when opening a document * PR Feedback * Fix verb check * PR feedback

* Unix: make UseShellExecute execute executables On Windows, UseShellExecute executes executables. This gives it the same behavior on Unix. * Add cross-platform tests * fix Windows test build failure * Add test for non-executable file with x-bit * Skip tests on unsupported platforms * Unix: only allow specific Verbs * Limit Unix tests to run on Linux * Fix test for verb null * Improve verb check * test: make working directory a temp location * test: refactor WriteScriptFile * test: refactor temp dir creation * test: fix build failure * Condense verb check * Find the executable in the ProcessStartInfo.WorkingDirectory * Don't pass Arguments when opening a document * PR Feedback * Fix verb check * PR feedback Commit migrated from dotnet/corefx@95619c8

Unix: make UseShellExecute execute executables

5afbdf1

On Windows, UseShellExecute executes executables. This gives it the same behavior on Unix.

tmds changed the title ~~Unix: make UseShellExecute execute executables~~ WIP Unix: make UseShellExecute execute executables Oct 25, 2018

tmds added 7 commits November 5, 2018 20:35

Add cross-platform tests

5585d7a

fix Windows test build failure

d20d976

Add test for non-executable file with x-bit

4de225b

Skip tests on unsupported platforms

417ebd1

Unix: only allow specific Verbs

fa37504

Limit Unix tests to run on Linux

56341fe

Fix test for verb null

a11365c

tmds changed the title ~~WIP Unix: make UseShellExecute execute executables~~ Unix: make UseShellExecute execute executables Nov 6, 2018

TSlivede suggested changes Nov 6, 2018

View reviewed changes

krwq reviewed Nov 6, 2018

View reviewed changes

src/System.Diagnostics.Process/tests/ProcessTests.cs Outdated Show resolved Hide resolved

krwq reviewed Nov 6, 2018

View reviewed changes

src/System.Diagnostics.Process/tests/ProcessTests.cs Show resolved Hide resolved

krwq reviewed Nov 6, 2018

View reviewed changes

src/System.Diagnostics.Process/tests/ProcessTests.cs Outdated Show resolved Hide resolved

krwq reviewed Nov 6, 2018

View reviewed changes

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs Outdated Show resolved Hide resolved

tmds added 4 commits November 7, 2018 10:28

Improve verb check

15d5e29

test: make working directory a temp location

76b092a

test: refactor WriteScriptFile

84b28b0

test: refactor temp dir creation

9773b1c

TSlivede reviewed Nov 7, 2018

View reviewed changes

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs Outdated Show resolved Hide resolved

test: fix build failure

b995115

stephentoub reviewed Nov 7, 2018

View reviewed changes

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs Show resolved Hide resolved

Condense verb check

946b7d2

Find the executable in the ProcessStartInfo.WorkingDirectory

39bfa71

krwq reviewed Nov 20, 2018

View reviewed changes

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs Show resolved Hide resolved

krwq reviewed Nov 20, 2018

View reviewed changes

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs Show resolved Hide resolved

krwq reviewed Nov 20, 2018

View reviewed changes

src/System.Diagnostics.Process/tests/ProcessTests.Unix.cs Outdated Show resolved Hide resolved

tmds added 2 commits November 21, 2018 16:50

PR Feedback

fe1f74a

Fix verb check

fa1c0cd

krwq approved these changes Nov 21, 2018

View reviewed changes

stephentoub reviewed Nov 28, 2018

View reviewed changes

src/Common/src/Interop/Unix/System.Native/Interop.ForkAndExecProcess.cs Outdated Show resolved Hide resolved

stephentoub reviewed Nov 28, 2018

View reviewed changes

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs Outdated Show resolved Hide resolved

stephentoub reviewed Nov 28, 2018

View reviewed changes

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs Outdated Show resolved Hide resolved

stephentoub reviewed Nov 28, 2018

View reviewed changes

src/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs Show resolved Hide resolved

stephentoub reviewed Nov 28, 2018

View reviewed changes

PR feedback

86ef599

krwq approved these changes Dec 4, 2018

View reviewed changes

stephentoub approved these changes Dec 5, 2018

View reviewed changes

stephentoub merged commit 95619c8 into dotnet:master Dec 5, 2018

karelz added this to the 3.0 milestone Dec 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unix: make UseShellExecute execute executables #33052

Unix: make UseShellExecute execute executables #33052

tmds commented Oct 25, 2018

TSlivede commented Oct 28, 2018 •

edited

krwq commented Oct 29, 2018

tmds commented Nov 6, 2018

TSlivede commented Nov 6, 2018

TSlivede left a comment

tmds commented Nov 7, 2018

tmds commented Nov 8, 2018

krwq commented Nov 21, 2018 •

edited

krwq left a comment

tmds commented Nov 22, 2018

danmoseley commented Nov 22, 2018

stephentoub commented Nov 22, 2018

tmds commented Nov 27, 2018

stephentoub commented Nov 27, 2018

stephentoub Nov 28, 2018

stephentoub Nov 28, 2018

tmds Nov 28, 2018

tmds commented Nov 29, 2018

tmds commented Dec 4, 2018

krwq commented Dec 4, 2018 •

edited

stephentoub commented Dec 5, 2018

tmds commented Dec 5, 2018

Unix: make UseShellExecute execute executables #33052

Unix: make UseShellExecute execute executables #33052

Conversation

tmds commented Oct 25, 2018

TSlivede commented Oct 28, 2018 • edited

krwq commented Oct 29, 2018

tmds commented Nov 6, 2018

TSlivede commented Nov 6, 2018

TSlivede left a comment

Choose a reason for hiding this comment

tmds commented Nov 7, 2018

tmds commented Nov 8, 2018

krwq commented Nov 21, 2018 • edited

krwq left a comment

Choose a reason for hiding this comment

tmds commented Nov 22, 2018

danmoseley commented Nov 22, 2018

stephentoub commented Nov 22, 2018

tmds commented Nov 27, 2018

stephentoub commented Nov 27, 2018

stephentoub Nov 28, 2018

Choose a reason for hiding this comment

stephentoub Nov 28, 2018

Choose a reason for hiding this comment

tmds Nov 28, 2018

Choose a reason for hiding this comment

tmds commented Nov 29, 2018

tmds commented Dec 4, 2018

krwq commented Dec 4, 2018 • edited

stephentoub commented Dec 5, 2018

tmds commented Dec 5, 2018

TSlivede commented Oct 28, 2018 •

edited

krwq commented Nov 21, 2018 •

edited

krwq commented Dec 4, 2018 •

edited