Include filename when CreateProcess fails with file not found #7394

marler8997 · 2020-02-09T04:31:46Z

Print a more helpful error message when spawnProcess fails on windows. Instead of:

Failed to spawn new process (The system cannot find the file specified.)

You now get this:

Failed to spawn process 'SOMEPROGRAM' (The system cannot find the file specified.)

dlang-bot · 2020-02-09T04:31:48Z

Thanks for your pull request and interest in making D better, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	20572	enhancement	spawnProcess to print process filename on error

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + phobos#7394"

thewilsonator

Looks good. Could do with some unittests.

marler8997 · 2020-02-09T04:36:15Z

unittests for the peelApplicationName function or for the CreateProcess part as well?

CyberShadow

More detailed error messages are nice but I think this is not a good way to do it. We don't know if the "not found" error was generated due to the path given to CreateProcess not existing - it could be due to some DLL not being found or the system not finding some other resource. I suggest to instead reword the message to something like "Failed to spawn process " ~ exePathHere ~ ": " ~ e.msg

thewilsonator · 2020-02-09T04:39:20Z

Both.

CyberShadow · 2020-02-09T04:39:45Z

std/process.d

+private auto peelApplicationName(inout(char)[] commandLine)
+{
+    import std.string : indexOf;
+    if (commandLine.length == 0) return commandLine;
+    char endChar = ' ';
+    if (commandLine[0] == '"')
+    {
+        commandLine = commandLine[1..$];
+        endChar = '"';
+    }
+    const end = commandLine.indexOf(endChar);
+    return commandLine[0 .. (end == -1) ? $ : end];
+}


Why is this function at all necessary? std.process functions already take the command as an array of arguments, and its first element will be the executable name. This function isn't even correct, it would have to call or precisely replicate the logic of CommandLineToArgvW, which it doesn't.

spawnProcessImpl actually takes a single string, since that's what CreateProcessW takes. And I think the logic is correct here. Take a look at the documentation for CreateProcess. It finds the application name based on the first whitespace characters, unless it starts with a " character. And windows filenames cannot contain quotes so you don't need to handle escapes in that instance.

https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw

Here's the applicable documentation on it:

The lpApplicationName parameter can be NULL. In that case, the module name must be the first white space–delimited token in the lpCommandLine string. If you are using a long file name that contains a space, use quoted strings to indicate where the file name ends and the arguments begin....

spawnProcessImpl actually takes a single string

This is a very bad reason to 1) undo the work that's done in the same module, and 2) duplicate the work of the operating system.

And I think the logic is correct here.

No. Sorry, it's not even close. For one thing, Windows ignores whitespace at the end of filenames. For another, tabs can also be used to separate arguments. There are likely many other subtle differences in behavior which would be pointless to try to match.

Sorry, the way you are approaching this is not good at all. Please rework this submission.

marler8997 · 2020-02-09T04:44:44Z

More detailed error messages are nice but I think this is not a good way to do it. We don't know if the "not found" error was generated due to the path given to CreateProcess not existing - it could be due to some DLL not being found or the system not finding some other resource. I suggest to instead reword the message to something like "Failed to spawn process " ~ exePathHere ~ ": " ~ e.msg

I'm not sure if that's correct. If you are missing a DLL you might get a different error code. I'll do some experiments...

CyberShadow · 2020-02-09T04:47:01Z

I'll do some experiments...

Don't waste your time. You cannot predict the exact behavior of all past and future implementations of the API. You also cannot know for sure the total set of conditions that create that error, unless you look at the implementation, which you 1) can't, and even if you could, would 2) still apply to only the version you looked at.

marler8997 · 2020-02-09T04:48:04Z

Don't waste your time. You cannot predict the exact behavior of all past and future implementations of the API. You also cannot know for sure the total set of conditions that create that error, unless you look at the implementation, which you 1) can't, and even if you could, would 2) still apply to only the version you looked at.

True, however I think if we can rule out the case of a missing dll, then it's pretty safe to assume this error code will only apply when the application name is missing. If we are wrong, then we can fix the error message later to be more ambiguous.

CyberShadow · 2020-02-09T04:48:44Z

The error message I proposed is not any less unclear and does not make unwarranted assumptions. Why do you insist on keeping your wording?

marler8997 · 2020-02-09T04:51:42Z

Why do you insist on keeping your wording?

I'm not insisting on it. I'm suggesting we try to find out whether or not we can make this assumption. If we can't, then you're wording is better.

CyberShadow · 2020-02-09T04:52:17Z

We cannot make such assumptions anywhere in Phobos for the reasons I already mentioned above.

CyberShadow · 2020-02-09T04:56:39Z

std/process.d

+private auto peelApplicationName(inout(char)[] commandLine)
+{
+    import std.string : indexOf;
+    if (commandLine.length == 0) return commandLine;
+    char endChar = ' ';
+    if (commandLine[0] == '"')
+    {
+        commandLine = commandLine[1..$];
+        endChar = '"';
+    }
+    const end = commandLine.indexOf(endChar);
+    return commandLine[0 .. (end == -1) ? $ : end];
+}


spawnProcessImpl actually takes a single string

This is a very bad reason to 1) undo the work that's done in the same module, and 2) duplicate the work of the operating system.

And I think the logic is correct here.

No. Sorry, it's not even close. For one thing, Windows ignores whitespace at the end of filenames. For another, tabs can also be used to separate arguments. There are likely many other subtle differences in behavior which would be pointless to try to match.

Sorry, the way you are approaching this is not good at all. Please rework this submission.

marler8997 · 2020-02-09T05:00:20Z

@CyberShadow could you describe what way you'd like me to implement pulling the application name from the commandLine string?

CyberShadow · 2020-02-09T05:05:51Z

@CyberShadow could you describe what way you'd like me to implement pulling the application name from the commandLine string?

I suggest changing the first argument of the Windows version of spawnProcessImpl to a struct type or tuple which contains both the command-line string (for CreateProcess) and a text description of the action being performed (to be displayed in case of error). At the spawnProcess call site, make it reference args[0]; at the spawnShell call site, make it explicitly say something like "Failed to run shell command " ~ command.

marler8997 · 2020-02-09T05:13:00Z

Making it print the full shell command is a bit more than I was planning on doing and I'm not sure if we want to print the full shell command...

CyberShadow · 2020-02-09T05:13:59Z

Anything works as long as the error is not misleadingly inaccurate.

std/process.d

CyberShadow · 2020-02-09T05:25:34Z

std/process.d

@@ -682,6 +685,12 @@ private Pid spawnProcessImpl(scope const(char[])[] args,
    }
 }

+private struct SpawnContext


There is no need to add a struct type if you're going to add a new argument. The reason I suggested using a struct was because a newly added argument would be unused in the POSIX version. The POSIX version doesn't need this informaiton, so there is no need to change its signature at all; but, if you're going to add a new argument, there is no need to introduce a struct.

If the signatures between windows and posix don't match then we'll need to have 2 different calls to these functions inside version statements. Is that what you want?

This is not true. They already don't match in master.

didn't compile when they had different signatures, I've pushed the change with the versions. Not sure what you want to do here.

didn't compile when they had different signatures

Please take a moment to have a closer look at the signatures of the Windows and POSIX versions of spawnProcessImpl, as they are in master right now (before your changes).

Not sure what you want to do here.

Would actually putting in a little effort be too much to ask? :(

I'm reading alot of hostility coming from you right now and I have no idea why? Why are you being so aggressive and insulting me?

I'm looking at master and they have the same signatures.

https://github.com/dlang/phobos/blob/master/std/process.d#L355
https://github.com/dlang/phobos/blob/master/std/process.d#L695

I'm looking at master and they have the same signatures.

Please, look a little closer...

One of them takes an array, the other takes a string.

My suggestion was to replace the array in the Windows version with a struct. Then, no changes to the POSIX signature or additional version blocks would be required.

I'm reading alot of hostility coming from you right now and I have no idea why? Why are you being so aggressive and insulting me?

Sorry, I feel like you are abusing my time by not spending a little more of your time to research this issue and submit a good contribution.

If you like, let's close this PR and I will submit one which addresses this issue in a manner I consider to be optimal.

BTW, you still need to file an issue in Bugzilla for the changelog.

I don't care much how this is implemented. I can see that you care very much, so feel free to let me know what you want. There's no need for saying things like "would actually putting in a little effort be too much to ask". I'm willing to do what you want, and I don't think I've given you any reason to think otherwise or any reason to think that I'm not willing to put in work. I've already completely re-written this thing 3 times in the last 40 minutes based on your suggestions but I can't read your mind to know what you want.

I don't see why you think reading my mind is required. I wrote:

I suggest changing the first argument of the Windows version of spawnProcessImpl to a struct type or tuple

I don't see how that's unclear.

CyberShadow · 2020-02-09T05:27:45Z

std/process.d

+        if (context.arg0)
+            return newFromLastError("Failed to spawn process '" ~ cast(string)context.arg0 ~ "'");
+        if (context.shell)
+            return newFromLastError("Failed to run shell command '" ~ cast(string)context.shell ~ "'");


This logic should be at the call site, not here. Either preformat the string there, or pass a scope delegate which does so on demand (which is less wasteful).

marler8997 · 2020-02-09T05:55:09Z

Now I see what you wanted. Sorry I didn't notice that extra set of square brackets on the first argument of spawnProcessImpl.

CyberShadow · 2020-02-09T05:57:16Z

Still needs Bugzilla issue + mention of its ID in commit message.

CyberShadow · 2020-02-09T06:01:39Z

Sorry I didn't notice that extra set of square brackets on the first argument of spawnProcessImpl.

It couldn't be any other way because the POSIX program execution primitive (exec) takes an array of strings (argc/argv).

marler8997 · 2020-02-09T06:03:25Z

It couldn't be any other way because the POSIX program execution primitive (exec) takes an array of strings (argc/argv).

Yes I'm keenly aware of this, I just didn't put 2 and 2 together. I didn't get what you were wanting and I'm sorry for the frustration.

marler8997 · 2020-02-09T06:05:02Z

Thanks for taking the time @CyberShadow, I like your solution much better :)

marler8997 · 2020-02-09T06:07:56Z

Looking back at your comments I don't know why I wasn't getting it. I think my brain turned off a little when I started sensing the hostility. I've heard that people's brains can stop working when they feel attacked, I think that may be what happened here.

CyberShadow · 2020-02-09T06:19:45Z

I guess I could have been less antagonizing. Sorry. Thanks for dealing with me and for the contribution :)

Fix issue 20572

marler8997 · 2020-05-16T18:55:26Z

Cleaning up old PRs that aren't going anywhere.

CyberShadow · 2020-05-16T21:28:23Z

Oops, what happened here? I thought this was ready for merging.

marler8997 · 2020-05-16T23:28:32Z

Yeah it was ready, just no one merged it...so it gets the axe as part of my PR spring cleaning :)

Based on dlang#7394 and as discussed in dlang#7726.

marler8997 requested review from CyberShadow, kyllingstad and schveiguy as code owners February 9, 2020 04:31

thewilsonator reviewed Feb 9, 2020

View reviewed changes

marler8997 force-pushed the createProcessFile branch from d0af305 to 20bf4c6 Compare February 9, 2020 04:34

CyberShadow requested changes Feb 9, 2020

View reviewed changes

CyberShadow reviewed Feb 9, 2020

View reviewed changes

CyberShadow requested changes Feb 9, 2020

View reviewed changes

marler8997 force-pushed the createProcessFile branch 6 times, most recently from 4600280 to 948da23 Compare February 9, 2020 05:26

CyberShadow reviewed Feb 9, 2020

View reviewed changes

marler8997 force-pushed the createProcessFile branch 2 times, most recently from bcdd2ca to a9d6497 Compare February 9, 2020 05:33

marler8997 force-pushed the createProcessFile branch 2 times, most recently from 468b9fd to 9cf9f01 Compare February 9, 2020 05:54

CyberShadow approved these changes Feb 9, 2020

View reviewed changes

marler8997 force-pushed the createProcessFile branch from 9cf9f01 to e1b40b5 Compare February 9, 2020 05:57

marler8997 force-pushed the createProcessFile branch from e1b40b5 to db06a52 Compare February 9, 2020 06:02

dlang-bot added the Severity:Enhancement label Feb 9, 2020

Include filename when CreateProcess fails with file not found

57d1188

Fix issue 20572

marler8997 force-pushed the createProcessFile branch from db06a52 to 57d1188 Compare February 9, 2020 06:36

marler8997 closed this May 16, 2020

CyberShadow added the Review:Phantom Zone Has value/information for future work, but closed for now label May 16, 2020

CyberShadow mentioned this pull request Dec 14, 2020

[stable] std.process: Include program name in exception msg if it cannot be found #7726

Merged

CyberShadow added a commit to CyberShadow/phobos that referenced this pull request Dec 17, 2020

std.process: Simplify error message generation on Windows

2659166

Based on dlang#7394 and as discussed in dlang#7726.

CyberShadow added a commit to CyberShadow/phobos that referenced this pull request Dec 17, 2020

std.process: Simplify error message generation on Windows

bdecfa3

Based on dlang#7394 and as discussed in dlang#7726.

CyberShadow added a commit to CyberShadow/phobos that referenced this pull request Dec 17, 2020

std.process: Simplify error message generation on Windows

928f992

Based on dlang#7394 and as discussed in dlang#7726.

CyberShadow added a commit to CyberShadow/phobos that referenced this pull request Dec 17, 2020

std.process: Simplify error message generation on Windows

05ab297

Based on dlang#7394 and as discussed in dlang#7726.

Include filename when CreateProcess fails with file not found #7394

Include filename when CreateProcess fails with file not found #7394

Conversation

marler8997 commented Feb 9, 2020 • edited Loading

dlang-bot commented Feb 9, 2020 • edited Loading

Bugzilla references

Testing this PR locally

thewilsonator left a comment

Choose a reason for hiding this comment

marler8997 commented Feb 9, 2020

CyberShadow left a comment

Choose a reason for hiding this comment

thewilsonator commented Feb 9, 2020

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

marler8997 commented Feb 9, 2020

CyberShadow commented Feb 9, 2020

marler8997 commented Feb 9, 2020

CyberShadow commented Feb 9, 2020

marler8997 commented Feb 9, 2020

CyberShadow commented Feb 9, 2020

Choose a reason for hiding this comment

marler8997 commented Feb 9, 2020

CyberShadow commented Feb 9, 2020

marler8997 commented Feb 9, 2020

CyberShadow commented Feb 9, 2020

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

CyberShadow Feb 9, 2020 • 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

marler8997 commented Feb 9, 2020

CyberShadow commented Feb 9, 2020

CyberShadow commented Feb 9, 2020

marler8997 commented Feb 9, 2020

marler8997 commented Feb 9, 2020

marler8997 commented Feb 9, 2020

CyberShadow commented Feb 9, 2020 • edited Loading

marler8997 commented May 16, 2020

CyberShadow commented May 16, 2020

marler8997 commented May 16, 2020

marler8997 commented Feb 9, 2020 •

edited

Loading

dlang-bot commented Feb 9, 2020 •

edited

Loading

CyberShadow Feb 9, 2020 •

edited

Loading

CyberShadow commented Feb 9, 2020 •

edited

Loading