Do not look for kioslave binary in applicationDirPath on *nix (#386859)
ClosedPublic

Authored by cullmann on Nov 14 2017, 2:43 AM.

Details

Summary

src/core/slave.cpp (Slave::createSlave): Do not look for the kioslave
binary in QCoreApplication::applicationDirPath() on Q_OS_UNIX. In
distribution packages, this ends up looking for the binary in /usr/bin,
which is where the legacy kdelibs 3 installed its kioslave binary. As a
result, we end up invoking the kdelibs 3 kioslave binary with our KF5
KIO slave and crashing it due to the mixed Qt/KDE libraries.

BUG: 386859

Diff Detail

Repository
R241 KIO
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kkofler created this revision.Nov 14 2017, 2:43 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptNov 14 2017, 2:43 AM
kfunk requested changes to this revision.Nov 14 2017, 11:18 AM
kfunk added a subscriber: kfunk.

Just chiming in here, without having a final solution for your problem: This search path has been added for a reason -- to find a kioslave.exe next to the application binary in application bundles on macOS/Windows.

See:
https://git.reviewboard.kde.org/r/125778/

CC'ing Christoph.

This revision now requires changes to proceed.Nov 14 2017, 11:18 AM

Doesn't the QStandardPaths call below find it?

Yeah, that was done to be able to provide win/linux install bundles.
Actually would it make not more sense to suffix the kioslave with "5" or something like that like we did for the libs to avoids clashs?

Doesn't the QStandardPaths call below find it?

No, as that will use searchPaths.

kkofler updated this revision to Diff 22338.Nov 14 2017, 1:06 PM
kkofler retitled this revision from Do not look for kioslave binary in applicationDirPath (#386859) to Do not look for kioslave binary in applicationDirPath on *nix (#386859).
kkofler edited the summary of this revision. (Show Details)

So, since this line makes sense on Windows/Mac, let's only get rid of it on Q_OS_UNIX then.

I can live with that ;=)
But still I think kioslave5 as name would make this not needed and be more in line with kded5/...

Yeah, that was done to be able to provide win/linux install bundles.

A GNU/Linux install bundle should have a reparented FHS-like tree, possibly wrapped in an image file (Flatpak/Snap/AppImage). In such a tree, the libexec binaries will never be in $prefix/bin (the applicationDirPath), but always somewhere under $prefix/libexec or $prefix/lib.

Windows and macOS are different and I believe you when you say that looking in the applicationDirPath makes sense there.

And the historical reason the name was not suffixed is because the binary was moved to a private libexec subdirectory to prevent conflicts. This worked fine as long as /usr/bin did not end up in the search path (in particular, for the whole 4.x series).

I can live with that patch, thought I still think we should just rename the helper, but perhaps that breaks other things if people relied on the name.

dfaure added a subscriber: dfaure.Dec 2 2017, 4:20 PM

Isn't it time to uninstall kdelibs3? :-)

Otherwise yes I'm ok with you renaming the helper executable, its use is very self-contained. Only KIO can call it, it takes a path to a socket created by KIO...

Ping? This has been stuck for over a year now.

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptJan 3 2019, 6:42 PM
cullmann commandeered this revision.Aug 17 2019, 3:11 PM
cullmann edited reviewers, added: kkofler; removed: cullmann.
cullmann updated this revision to Diff 63931.Aug 17 2019, 3:27 PM

Instead of changing the search paths, alter the kioslave binary name to "kioslave5" to avoid clashs with old variants.

Could somebody try this?
Unfortunately, neither with or without this patch I can get all kio unit tests running here properly :(

dfaure accepted this revision.Aug 24 2019, 8:59 AM

Seems to work for me.

But it's weird that the KIO unittests don't work for you... we should fix that.

Make sure to edit the commit log so it doesn't look like this RR's description anymore, if needed.

Ok, Kevin, ok with this variant, too?

If you ask me (there are 2 Kevins on this bug), I think this will work. I think it's kinda overkill (it pretty much defeats the point of having version-specific libexecdirs if we end up renaming the binaries anyway), but if it works (which should be the case), I'm OK with it.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 25 2019, 12:20 PM
This revision was automatically updated to reflect the committed changes.