remove dbus for windows build and change audio dep logic
ClosedPublic

Authored by brute4s99 on Jun 7 2019, 11:36 PM.

Details

Summary

These audio dependencies aren't used by KNotifications on Win32.
Similarly, DBus is not used on Windows.

Diff Detail

Repository
R289 KNotifications
Branch
arcpatch-D21660
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13753
Build 13771: arc lint + arc unit
brute4s99 created this revision.Jun 7 2019, 11:36 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 7 2019, 11:36 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
brute4s99 requested review of this revision.Jun 7 2019, 11:36 PM
brute4s99 retitled this revision from simplify conditions to simplify conditions and remove audio dependency for win32.Jun 7 2019, 11:38 PM
brute4s99 edited the summary of this revision. (Show Details)
brute4s99 added a reviewer: broulik.
nicolasfella added inline comments.
CMakeLists.txt
42

We don't need DBus on Windows, do we?

apol added a subscriber: apol.Jun 10 2019, 11:57 AM

I'd say you're not simplifying it, you're changing the logic. This needs to be reflected in the commit message.

CMakeLists.txt
91–92

indentation is wrong.

brute4s99 updated this revision to Diff 59551.Jun 10 2019, 7:23 PM

fixed indentation

brute4s99 retitled this revision from simplify conditions and remove audio dependency for win32 to change audio dep logic wrt win32.Jun 10 2019, 7:24 PM
brute4s99 marked an inline comment as done.Jun 18 2019, 9:10 AM
brute4s99 added inline comments.
CMakeLists.txt
42

we don't, I guess, but dbus-daemon.exe still runs in the background so I can't say.
The functionality doesn't seem to have been affected by removing this in my build, though.

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.

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.

brute4s99 marked 2 inline comments as done.Jul 5 2019, 3:44 PM

may I land this?

nicolasfella requested changes to this revision.Jul 9 2019, 3:09 PM

This needs rebasing since I recently changed the audio logic for Android

CMakeLists.txt
38

This seems to be entirely urelated to audio?

This revision now requires changes to proceed.Jul 9 2019, 3:09 PM
brute4s99 edited the summary of this revision. (Show Details)Jul 9 2019, 3:24 PM
brute4s99 added inline comments.Jul 9 2019, 3:28 PM
CMakeLists.txt
38

I was trying to simplify code there by removing NOT. Should I remove this change?

nicolasfella added inline comments.Jul 9 2019, 3:37 PM
CMakeLists.txt
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

brute4s99 added inline comments.Jul 9 2019, 4:56 PM
CMakeLists.txt
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

nicolasfella accepted this revision.Jul 9 2019, 4:56 PM
This revision is now accepted and ready to land.Jul 9 2019, 4:56 PM
brute4s99 added inline comments.Jul 11 2019, 10:31 AM
CMakeLists.txt
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. (:

nicolasfella added inline comments.Jul 11 2019, 11:00 AM
CMakeLists.txt
38

But this is KNotifications, not KDE Connect. So we need DBus for KNotifications on Windows?

brute4s99 added inline comments.Jul 12 2019, 11:04 AM
CMakeLists.txt
38

ah, yeah we could bypass DBus on Windows in KNotifications

brute4s99 updated this revision to Diff 61639.Jul 12 2019, 11:06 AM

no more dbus on Windows build of KNotifications

brute4s99 retitled this revision from change audio dep logic wrt win32 to remove dbus and change audio dep logic.Jul 12 2019, 11:07 AM
brute4s99 edited the summary of this revision. (Show Details)
brute4s99 retitled this revision from remove dbus and change audio dep logic to remove dbus for windows build and change audio dep logic.Jul 12 2019, 11:25 AM
nicolasfella added inline comments.Jul 12 2019, 12:05 PM
CMakeLists.txt
42

Move it down to the audio logic, the if check is the same

nicolasfella requested changes to this revision.Jul 12 2019, 12:06 PM
This revision now requires changes to proceed.Jul 12 2019, 12:06 PM
brute4s99 marked 8 inline comments as done.Jul 17 2019, 12:41 PM
nicolasfella accepted this revision.Jul 17 2019, 7:48 PM
This revision is now accepted and ready to land.Jul 17 2019, 7:48 PM
This revision was automatically updated to reflect the committed changes.

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

sorry about that. I'll find a better solution that doesn't break builds 🙈

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.

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.

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.

There is a fix for this issue, see https://phabricator.kde.org/D25350

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.

+1 for no dbus on Windows!

Considering that D-Bus doesn't really belong on Windows,

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.

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;
}

No, not really, I fixed that long ago, we auto-detect this and use the KDE_FORK_SLAVES code paths for that

static bool forkSlaves()

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.

That is bad, then this must be fixed.
That should happen on Windows, too.

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.