D29170: Detect executables without +x permission in $PATH to improve error message

[Ahmad Samir]

ahmadsamir added a comment.


  In D29170#657334 <https://phabricator.kde.org/D29170#657334>, @dfaure wrote:
  
  > Resolve relative executables using the directory of the .desktop file referring to them
  >
  > Not useful for /usr/share/applications stuff, but useful for custom setups
  >  where a custom desktop file refers to a local executable.
  >
  > Re-reading the thread https://lists.freedesktop.org/archives/xdg/2011-April/011883.html
  >  I'm wondering if this is the right thing to do though.
  >
  > After all, on the command line "foo" doesn't start a local executable called foo,
  >  only ./foo does that. I was always working under that assumption for Exec= as well
  >  (the fact that the current dir isn't part of the search path). Undecided.
  
  
  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.
  
  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  ./, (I can't remember exactly but it I think I read somewhere that's for security reasons).
  
  That also means that the original code trying to find the executable in the current dir never really worked, 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.

INLINE COMMENTS

> desktopexecparser.cpp:465
> +                }
> +                if (QFile::exists(exePath)) {
> +                    execlist[0] = exePath;

I think we should check for isExecutable() here too (this matches the behaviour of QStandardPaths::findExecutable()).

> kprocessrunner.cpp:53
> +    const QFileInfo fi(executable);
> +    if (fi.exists() && !fi.isExecutable())
> +        return executable;

Coding style: braces around if block.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D29170

To: dfaure, ahmadsamir
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200426/64dda588/attachment.html>