These audio dependencies aren't used by KNotifications on Win32.
Similarly, DBus is not used on Windows.
Details
Diff Detail
- Repository
- R289 KNotifications
- Branch
- arcpatch-D21660_1
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 13875 Build 13893: arc lint + arc unit
CMakeLists.txt | ||
---|---|---|
40 | We don't need DBus on Windows, do we? |
I'd say you're not simplifying it, you're changing the logic. This needs to be reflected in the commit message.
CMakeLists.txt | ||
---|---|---|
93–94 | indentation is wrong. |
CMakeLists.txt | ||
---|---|---|
40 | we don't, I guess, but dbus-daemon.exe still runs in the background so I can't say. |
With regards to Windows, please note that any unit test which depends on calls that involve D-Bus on the CI system will likely lead to that test hanging because dbus-daemon is not launched by the CI system.
Where possible D-Bus should be avoided on Windows.
Thanks for taking a look, @bcooksley! We already use dbus-daemon.exe for a lot of talking with the notifications and (I think many) other features, so we can't really avoid it just now.
This needs rebasing since I recently changed the audio logic for Android
CMakeLists.txt | ||
---|---|---|
37–38 | This seems to be entirely urelated to audio? |
CMakeLists.txt | ||
---|---|---|
37–38 | I was trying to simplify code there by removing NOT. Should I remove this change? |
CMakeLists.txt | ||
---|---|---|
37–38 | Yes. It is neither related to Windows nor audio. What you can to is to add Windows to the condition to not look for DBus on Windows, but please do that in a separate patch. Or move the find call for DBus down to the auto checks inside the if |
CMakeLists.txt | ||
---|---|---|
37–38 | I'm looking into the DBus problem, we currently use DBus for the system tray icon on Windows. Once that is resolved, I should be able to completely remove DBus from the Windows build |
CMakeLists.txt | ||
---|---|---|
37–38 | Update: nope. I think it's still use for the dbus interfaces. I confirmed by ending the dbus-daemon.exe process by hand to see it. Turns out, dbus is quite important for KDE Connect regardless of the OS. (: |
CMakeLists.txt | ||
---|---|---|
37–38 | But this is KNotifications, not KDE Connect. So we need DBus for KNotifications on Windows? |
CMakeLists.txt | ||
---|---|---|
37–38 | ah, yeah we could bypass DBus on Windows in KNotifications |
CMakeLists.txt | ||
---|---|---|
40 | Move it down to the audio logic, the if check is the same |
This broke some public APIs on Windows: since Qt5DBus is not being "imported" by CMake, a group of .cpp files is not being compiled, see e.g. https://cgit.kde.org/knotifications.git/tree/src/CMakeLists.txt#n25 . Thus e.g. class KStatusNotifierItem implementation becomes unavailable at linking time, even though CMake configuration installs the respective header files.
As a consequence, we have linking errors in applications that rely on KStatusNotifierItem: https://phabricator.kde.org/T11275
There is no need to start dbus-daemon by CI. dbus-daemon is started by the dbus client library by default, if not running for the given install path.
If tests launch D-Bus Daemon, then they will cause the CI system to jam as CTest will wait indefinitely for dbus-daemon to exit.
D-Bus should not be used under any circumstances on Windows.
Considering that D-Bus doesn't really belong on Windows, and has been known to cause security software (such as anti-malware packages) to generate false positives in the past, we really should avoid D-Bus completely if at all possible on Windows. For standalone applications there isn't really a reason to have it around.
KDE uses dbus for many internal services. On a recent linux system I see the following KDE services in dbus
org.kde.ActivityManager
org.kde.JobViewServer
org.kde.kappmenu
org.kde.kcookiejar5
org.kde.kdeconnectd
org.kde.kded5
org.kde.keyboard
org.kde.kglobalaccel
org.kde.kiod5
org.kde.kioexecd
org.kde.klauncher5
org.kde.klipper
org.kde.knotify
org.kde.kpasswdserver
org.kde.krunner
org.kde.kssld5
org.kde.kuiserver
org.kde.kwalletd5
org.kde.Solid.PowerManagement
org.kde.Solid.PowerManagement.PolicyAgent
org.kde.StatusNotifierWatcher
Some of them are Linux only, but some are required on Windows (for example kio)
and has been known to cause security software (such as anti-malware packages) to generate false positives in the past,
This has not been reported to the official dbus issue tracker (https://gitlab.freedesktop.org/groups/dbus/-/issues?scope=all&utf8=%E2%9C%93&state=opened&search=virus) . Can you point me to related reports ?
For standalone applications there isn't really a reason to have it around.
From what I know does kio depends on dbus, which mean you are saying that standalone applications could not use kio and my be others (see above)
kio doesn't require dbus for the normal slave stuff.
It can be that it requires it for the kcookie or auth stuff.
klauncher is used to launch kio slaves. In the main() function of klauncher there is a check if a DBus session bus is running (https://lxr.kde.org/source/frameworks/kinit/src/klauncher/klauncher_main.cpp#0187) . If not, klauncher is terminated. This looks like a hard requirement for DBus if kio slaves are used.
No, not really, I fixed that long ago, we auto-detect this and use the KDE_FORK_SLAVES code paths for that
see in slave.cpp
static bool forkSlaves() { // In such case we start the slave via QProcess. // It's possible to force this by setting the env. variable // KDE_FORK_SLAVES, Clearcase seems to require this. if (bForkSlaves.load() == -1) { bool fork = qEnvironmentVariableIsSet("KDE_FORK_SLAVES"); // no dbus? => fork slaves as we can't talk to klauncher if (!fork) { fork = !QDBusConnection::sessionBus().interface(); } #ifdef Q_OS_UNIX if (!fork) { // check the UID of klauncher QDBusReply<uint> reply = QDBusConnection::sessionBus().interface()->serviceUid(klauncher()->service()); // if reply is not valid, fork, most likely klauncher can not be run or is not installed // fallback: if there's an klauncher process owned by a different user: still fork if (!reply.isValid() || getuid() != reply) { fork = true; } } #endif bForkSlaves.testAndSetRelaxed(-1, fork ? 1 : 0); } return bForkSlaves.load() == 1; }
but this function is only called on Linux (https://github.com/KDE/kio/blob/a8de3cb2032aba3dc86b1c7a7e745ca000f21bf4/src/core/slave.cpp#L458), so my statement is still true.
#ifdef Q_OS_UNIX if (forkSlaves() == 1) { ... QProcess::startDetached(kioslave, args); return slave; } #endif
Later on there is the code, called on Windows, which requires klauncher
QString errorStr; QDBusReply<int> reply = klauncher()->requestSlave(protocol, url.host(), slaveAddress.toString(), errorStr); if (!reply.isValid()) { error_text = i18n("Cannot talk to klauncher: %1", klauncher()->lastError().message()); error = KIO::ERR_CANNOT_LAUNCH_PROCESS; delete slave; return 0;
That is bad, then this must be fixed.
That should happen on Windows, too.
Just like it works nicely on macOS.
BTW: Does starting slaves on Linux kills started slave after slave timeout occurs, which is performed by klauncher by default ?
In case you are running a KDE app using kioslave on a removal disc, after closing the main app, you will have a problem ejecting the removal disc because of still running kioslaves.