<table><tr><td style="">ahmadsamir added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D29170">View Revision</a></tr></table><br /><div><div><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D29170#657689" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D29170#657689</a>, <a href="https://phabricator.kde.org/p/dfaure/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@dfaure</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><blockquote style="border-left: 3px solid #8C98B8;
color: #6B748C;
font-style: italic;
margin: 4px 0 12px 0;
padding: 8px 12px;
background-color: #F8F9FC;">
<div style="font-style: normal;
padding-bottom: 4px;">In <a href="https://phabricator.kde.org/D29170#657450" style="background-color: #e7e7e7;
border-color: #e7e7e7;
border-radius: 3px;
padding: 0 4px;
font-weight: bold;
color: black;text-decoration: none;">D29170#657450</a>, <a href="https://phabricator.kde.org/p/ahmadsamir/" style="
border-color: #f1f7ff;
color: #19558d;
background-color: #f1f7ff;
border: 1px solid transparent;
border-radius: 3px;
font-weight: bold;
padding: 0 4px;">@ahmadsamir</a> wrote:</div>
<div style="margin: 0;
padding: 0;
border: 0;
color: rgb(107, 116, 140);"><p>I don't think you need to go out of your way to support custom setups, after all it's quite simple for the user to edit the .desktop file and specify the path to the executable. Even from the xdg thread, they were talking about installing a virtual machine guest stuff, which is something the user only has to do once, whereas, from my POV at least, a .desktop file is more for things your run frequently/on a regular basis.</p></div>
</blockquote>
<p>One use case could be running software on a USB key. The desktop file there doesn't know the full path, but a relative one.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>It would simplify the code, and would be consistent with how a shell would run a program; indeed a binary in the current dir has to be explicitly prefixed with ./</p></blockquote>
<p>I'm not sure if now you're advocating to support "./foo" or no support for relative executables at all.</p></div>
</blockquote>
<p>The latter, i.e. not handling the relative path.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>That also means that the original code trying to find the executable in the current dir never really worked</p></blockquote>
<p>You keep reading it that way, and I keep saying that code was assuming "realExecutable" is an absolute path. As my (new) comment in the code says, it actually is one, if the program was found (and is executable).<br />
What happened with not-found-therefore-still-relative executable names in there was purely accidental (and fixed by this commit).</p></blockquote>
<p>Yep, resultingArguments() would resolve/get the full path.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>because "current dir" is most likely where the _KIO executable_ exists (I tested with qt-creator and QDir::current() is indeed where the compiled binary exists). So not that useful.</p></blockquote>
<p>Worse, if you start "dolphin /tmp" from your $HOME (using konsole), then QDir::curren() is $HOME, completely unrelated to what dolphin is showing.</p></blockquote>
<p>Current dir is indeed irrelevant for a .desktop file, it only makes sense from the CLI or for actual executables looking for .so files.</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p>I got no reply on k-f-d but Albert Astals Cid on IRC was of the opinion of "stick to the spec, don't support relative executables in any way", which is a fair point.<br />
If you agree too I can just revert to not supporting relative paths at all. It all came from what I thought was a request from you in the first review, and that old request on xdg.</p></blockquote>
<p>Part of the issue is, if that feature is implemented here but doesn't get adapted by other DE's/file managers too, 3rd parties won't use it as they usually want a one-size-fits-all solution (which doesn't exist....), and from what I see with a shell script is much more robust for those 3rd parties.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D29170#inline-166791">View Inline</a><span style="color: #4b4d51; font-weight: bold;">dfaure</span> wrote in <span style="color: #4b4d51; font-weight: bold;">desktopexecparser.cpp:465</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">But if we do that, a non-executable file gets ignored here and we don't get the warning that this commit is all about.</p>
<p style="padding: 0; margin: 8px;">It would all be much simpler [for this custom purpose] if findExecutable didn't check the executable bit ;-) Then we'd have one full path to check ourselves. Instead we have to duplicate the PATH lookup. I'd like to avoid having to also duplicate the current-dir-of-desktop-file lookup and the libexec lookup.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p style="padding: 0; margin: 8px;">But if we do that, a non-executable file gets ignored here and we don't get the warning that this commit is all about.</p></blockquote>
<p style="padding: 0; margin: 8px;">Righto. The isExecutable() check is done in KProcessRunner.</p>
<p style="padding: 0; margin: 8px;">And I echo that request, findExecutable() would be slightly more useful if it reports that it found a file with that name but it's not executable; the QStandardPaths API doesn't offer any other way of searching PATH, so the only method it has should report such a case. I am not sure if upstream would take a patch for that...</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R241 KIO</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D29170">https://phabricator.kde.org/D29170</a></div></div><br /><div><strong>To: </strong>dfaure, ahmadsamir<br /><strong>Cc: </strong>kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns<br /></div>