Differential D28169

[krununittest] Skip the test if ktelnetservice5{,.desktop} isn't found
AbandonedPublic
Actions

Authored by ahmadsamir on Mar 20 2020, 6:52 PM.

Details

Reviewers
dfaure
sitter
feverfew
ngraham
Group Reviewers
Frameworks
Summary

This reverts commit 14b7f2c7ee72b; the added unit test failed on the
CI system because hasSchemeHandler() couldn't find the ktelnetservice5.desktop
file in /usr/share/applications, as KIO isn't installed on the CI system.

However, if we have a service and it claims to support opening urls with
certain protocols/schemes, then we should let it do that.

Besides the original condition seems flawed as it meant that even if
isProtocolInSupportedList() returned true, we'd still take the
KIOFuse path.

CCBUG: 418258

Test Plan

krununittest passes even if /usr/bin/ktelnetservice5 isn't found, which
is the case on the CI system as KIO isn't installed there.

Diff Detail

Repository
R241 KIO
Branch
l-krun-ssh-2 (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24019
Build 24037: arc lint + arc unit
ahmadsamir created this revision.Mar 20 2020, 6:52 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMar 20 2020, 6:52 PM
ahmadsamir requested review of this revision.Mar 20 2020, 6:52 PM
dfaure requested changes to this revision.Mar 20 2020, 8:21 PM
dfaure added inline comments.
src/core/desktopexecparser.cpp
350 ↗(On Diff #78120)

Doesn't this break the special case in the comment below?
VLC claims to support smb:// but if we have a password, we need to be able to pass it, so we want to use KIOFuse?

And then if you fix that, we're back to a simple revert of the last commit, no?

This revision now requires changes to proceed.Mar 20 2020, 8:21 PM
ahmadsamir added inline comments.Mar 20 2020, 9:31 PM
src/core/desktopexecparser.cpp
350 ↗(On Diff #78120)

Indeed.

However I still think the condition is flawed:

  • vlc.desktop says it supports smb:// so "supported" is true, !supported is false, we go the branch of ||, userName is not empty but password is empty, go to KIOFuse
  • but what if both the userName and password _aren't_ empty, vlc is already out of the picture

IIUC, vlc has native smb support which will beat any FUSE equivalent, right?

I'll think about it some more.... :/

dfaure added inline comments.Mar 21 2020, 9:34 AM
src/core/desktopexecparser.cpp
350 ↗(On Diff #78120)

The point is that KIO has no way to pass the password to VLC directly (passing it on the command line would be very insecure, other users could see it).
So it's not like "VLC is out of the picture", it's rather "let's use FUSE (which will get the password) and pass the path to VLC" instead of "let's call VLC directly, but it will fail for lack of password".

At least that's the intent of this code, which seems somewhat logical to me, and isn't the heart of the matter in this commit which is supposed to fix the CI regression :-)

Shouldn't we just skip tests if ktelnetservice wasn't found?

ahmadsamir updated this revision to Diff 78152.Mar 21 2020, 11:48 AM
ahmadsamir retitled this revision from [DesktopExecParser] Let the service open protocols is claims to support to [krununittest] Skip the test if ktelnetservice5{,.desktop} isn't found.
ahmadsamir edited the test plan for this revision. (Show Details)

Just fix unittest

dfaure added a comment.Mar 21 2020, 12:01 PM

Ah, we had tried already to make it work uninstalled.

The actual way to do that would be to copy that desktop file into a local ApplicationsLocation. I'll do that, I just did something similar in KParts.

ahmadsamir added a comment.Mar 21 2020, 12:26 PM
In D28169#631809, @dfaure wrote:

Ah, we had tried already to make it work uninstalled.

The actual way to do that would be to copy that desktop file into a local ApplicationsLocation. I'll do that, I just did something similar in KParts.

That ApplicationsLocation will be on the CI system?

dfaure added a comment.Mar 21 2020, 12:31 PM
In D28169#631841, @ahmadsamir wrote:
In D28169#631809, @dfaure wrote:

Ah, we had tried already to make it work uninstalled.

The actual way to do that would be to copy that desktop file into a local ApplicationsLocation. I'll do that, I just did something similar in KParts.

That ApplicationsLocation will be on the CI system?

Yes, in ~/.qttest/share/applications
See https://commits.kde.org/kio/b60aae15eaf97bc921fb7ad3a2068448f303055f
But it still fails, I'm trying to debug why.

Can't reproduce it locally even after uninstaling kio and all ktelnetservice*.desktop from /usr, so the problem is something else...

dfaure added a comment.Mar 21 2020, 12:36 PM
This comment was removed by dfaure.
dfaure added a comment.Mar 21 2020, 2:06 PM

OK, it's fixed now, after 3 commits :-)
You can drop this change, it's better that CI actually tests the code.
Thanks for your help though (with this, and with everything else - very much appreciated)

ahmadsamir abandoned this revision.Mar 21 2020, 2:52 PM
In D28169#631885, @dfaure wrote:

OK, it's fixed now, after 3 commits :-)
You can drop this change, it's better that CI actually tests the code.

[..]
Yesterday, I did try tweaking XDG_DATA_DIRS to make it find the .desktop file without it being in /usr/share/applications, but I didn't think that it'd take time for ksyscoca to pick it up (or even that ksycoca was involved, though in retrospect that makes sense).

However, a couple of points that are still bothering me (consider them food for thought):

  • I still think the workaround for vlc should be tightened, with e.g. checking for .desktop file name
  • Currently the code calls hasSchemeHandler() twice, once from KRun::init() and then in resultingArguments(); also for KMimeTypeTrader::self()->preferredService(), it's called from KRun schemeService() and from hasSchemeHandler() (the latter is used twice so, preferredService() is called three times :D)
dfaure added a comment.Mar 21 2020, 3:53 PM
In D28169#631893, @ahmadsamir wrote:

Yesterday, I did try tweaking XDG_DATA_DIRS to make it find the .desktop file without it being in /usr/share/applications

It's more portable to use QStandardPaths test mode and copy the file where QStandardPaths will expect it.
XDG_DATA_DIRS wouldn't work on macOS or Windows.

but I didn't think that it'd take time for ksyscoca to pick it up (or even that ksycoca was involved, though in retrospect that makes sense).

KMimeTypeTrader is all about querying ksycoca.

  • I still think the workaround for vlc should be tightened, with e.g. checking for .desktop file name

I don't think this is VLC specific at all.
xine or mplayer or other non-KIO-based apps would have the exact same problem. They can't get hold of the password.
Anyhow, to be discussed with the FUSE people if I'm missing something. As you probably know, I'm not involved in the FUSE stuff.

  • Currently the code calls hasSchemeHandler() twice, once from KRun::init() and then in resultingArguments(); also for KMimeTypeTrader::self()->preferredService(), it's called from KRun schemeService() and from hasSchemeHandler() (the latter is used twice so, preferredService() is called three times :D)

Interesting point.
Checking hasSchemeHandler in both KRun and inside the implementation ProcessLauncherJob makes sense to me. It's two different layers of public API; passing information between the two would make for horrible API.
In KRun it allows us to say ok, we don't need to find the mimetype, go straight to running the associated service.
In KProcessRunner (used by ProcessLauncherJob), it's indeed a bit stupid to look up the associated service when we *are* running this service. Oh, especially since KIO::DesktopExecParser::supportedProtocols already grabs those protocols from the MimeType... So supported is true. Then what? why? Debugging further.... OK I see. The earlier comment I deleted is true again. ssh://root@ has username, so this makes the fuse request, fails, and then blindly sets useKioexec = true for no good reason. I'll fix that logic. And then we can remove the unneeded hasSchemeHandler() hack.

Thanks for making me dig further ;)

Revision Contents
Changeset List

PathPackages
Mautotests/krununittest.cpp (12 lines)

Diff 78156

View Options

autotests/krununittest.cpp

Loading...