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 13754
Build 13772: 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
44

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
44

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
40

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
40

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
40

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
40

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
40

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
40

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
40

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
44

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 🙈