run_program() fixes #2748
run_program() fixes #2748
Conversation
argv is supposed to be Unix-style and in Unix-style argv[0] is program name. Let's try to have it that way.
argv is supposed to be Unix-style and in Unix-style argv[0] is program name. Let's try to have it that way. Also clean up a comment that is not relevant to Windows code path.
On Windows, run_program() uses argv[0] as program name. argv is Unix-style and POSIX says argv[0] should be just filename without path. If someone sets argv[0] to only filename instead of full path then run_program() may fail to launch the program. Since the file parameter should have full path to the program use that as program name when constructing the command line. The launched program may see different argv[0] than programmer wanted but this way run_program() can search %PATH% for the program should that be necessary.
Client path was quoted because on Windows run_program() concatenates arguments to a single string. Without quoting any path with a space would cause problems interpreting the command line. What run_program() does with its parameters is really an implementation detail of it and users of run_program() shouldn't need to know or care about it. Moving quoting into run_program() also removes the need to duplicate quoting everywhere run_program() is used. The program name or path to it needs to be valid filesystem name and it's always the first in argv so quoting it in run_program() is trivial. run_program() doesn't know which other arguments may contain paths and quoting arbitrary arguments is harder so keep quoted_data_dir for now.
|
This PR fixes #2657 |
I think that's a little premature. @JuhaSointusalo specifically referenced a quite separate conversation on boinc_alpha about screensavers and graphics apps. Let's leave #2657 open again (good idea to re-open it) until we're all sure we're all on the same page. |
|
@JuhaSointusalo, Is this ready for review? |
|
Is this ready for review?
Yes.
|
|
@davidpanderson, @TheAspens, could you please review this? |
|
Just a kind reminder, @davidpanderson, @TheAspens |
|
@CharlieFenton, could you please double check? I believe this particular PR fixes the issue presented in #2713 and have better implementation than #2657. Thanks |
|
I have removed myself as a reviewer as this is an area that I do have expertise in. However, should we have a meeting to discuss the differences between this pull request and #2742 ? |
|
If we should have a meeting on this, and all the related pull requests, who all needs to be there? @davidpanderson , @JuhaSointusalo as the key developers - who else? |
|
I have been involved with this issue since it first appeared here as #2029. I did record in #2713 that I had tested a Windows build from that code, and that it successfully addressed all the issues I was aware of. I'm happy to perform similar tests on the alternative formulation, or to join a meeting, if required, but for the time being I think we're OK |
|
@AenBleidd Sorry, I now did a quick review of the commits in #2713 and see that they are independent of the changes in this PR. So I withdraw my objection. However, I have neither the time nor the inclination to wend my way through this long convoluted string of PRs. I believe that we agree that PR #2657 should be closed without merging; as far as I can see there were no commits in that PR. I defer to @JuhaSointusalo and others to decide what should be done. |
|
What problem does this PR fix, and how? |
|
Quick recap. There was a problem with GPU detection when client was started without using path to it but searched from
In #2713 I changed the function to use Turns out the code that launches graphics apps and screensavers exploited the previously mentioned implementation detail and passed in rubbish That's the history part. On to this PR. Passing in rubbish parameters just because you can get away with it today is plain and simply bad programming so this PR fixes the other users of Since there was a request to compare this PR and #2742. #2742 was opened because David didn't understand the changes I did, didn't read the commit messages, didn't bother ask me to explain the changes and instead though it would be a good idea to revert the changes I had intentionally done. This PR lets |
lib: change
run_program()to usefileparameter as program name on Windowsmgr,scr: fix
argvs in calls torun_program()client: remove quoting client path in launching GPU detection
See commit messages for details.
Fixes launching graphics apps with Manager and screensaver as reported on boinc_alpha.