D21660: remove dbus for windows build and change audio dep logic

[Ralf Habacker]

habacker added a comment.


  In D21660#565205 , @cullmann wrote:
  
  > 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.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: cullmann, habacker, aspotashev, bcooksley, apol, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Christoph Cullmann]

cullmann added a comment.


  That is bad, then this must be fixed.
  That should happen on Windows, too.
  Just like it works nicely on macOS.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: cullmann, habacker, aspotashev, bcooksley, apol, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Ralf Habacker]

habacker added a comment.


  In D21660#565113 , @cullmann wrote:
  
  > 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 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;

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: cullmann, habacker, aspotashev, bcooksley, apol, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Christoph Cullmann]

cullmann added a comment.


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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: cullmann, habacker, aspotashev, bcooksley, apol, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Ralf Habacker]

habacker added a comment.


  In D21660#565110 , @cullmann wrote:
  
  > 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.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: cullmann, habacker, aspotashev, bcooksley, apol, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Christoph Cullmann]

cullmann added a comment.


  kio doesn't require dbus for the normal slave stuff.
  It can be that it requires it for the kcookie or auth stuff.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: cullmann, habacker, aspotashev, bcooksley, apol, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Ralf Habacker]

habacker added a comment.


  In D21660#563560 , @bcooksley 
wrote:
  
  > 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)

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: cullmann, habacker, aspotashev, bcooksley, apol, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Christoph Cullmann]

cullmann added a comment.


  +1 for no dbus on Windows!

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: cullmann, habacker, aspotashev, bcooksley, apol, nicolasfella, 
kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Ben Cooksley]

bcooksley added a comment.


  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.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: habacker, aspotashev, bcooksley, apol, nicolasfella, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Ralf Habacker]

habacker added a comment.


  In D21660#563425 , @bcooksley 
wrote:
  
  > 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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: habacker, aspotashev, bcooksley, apol, nicolasfella, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Ben Cooksley]

bcooksley added a comment.


  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.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: habacker, aspotashev, bcooksley, apol, nicolasfella, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Ralf Habacker]

habacker added a comment.


  In D21660#481421 , @bcooksley 
wrote:
  
  > 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.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: habacker, aspotashev, bcooksley, apol, nicolasfella, kde-frameworks-devel, 
LeGast00n, GB_2, michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Piyush Aggarwal]

brute4s99 added a comment.


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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: aspotashev, bcooksley, apol, nicolasfella, kde-frameworks-devel, LeGast00n, 
sbergeron, michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Alexander Potashev]

aspotashev added a comment.


  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

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: aspotashev, bcooksley, apol, nicolasfella, kde-frameworks-devel, LeGast00n, 
sbergeron, michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Piyush Aggarwal]

This revision was automatically updated to reflect the committed changes.
Closed by commit R289:be466180db21: remove dbus for windows build and change 
audio dep logic (authored by brute4s99).

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21660?vs=61716&id=61968

REVISION DETAIL
  https://phabricator.kde.org/D21660

AFFECTED FILES
  CMakeLists.txt
  src/knotificationmanager.cpp

To: brute4s99, broulik, nicolasfella
Cc: bcooksley, apol, nicolasfella, kde-frameworks-devel, LeGast00n, sbergeron, 
michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Piyush Aggarwal]

brute4s99 added a comment.


  landing it

REPOSITORY
  R289 KNotifications

BRANCH
  arcpatch-D21660_1

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: bcooksley, apol, nicolasfella, kde-frameworks-devel, LeGast00n, sbergeron, 
michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Nicolas Fella]

nicolasfella accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R289 KNotifications

BRANCH
  arcpatch-D21660_1

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: bcooksley, apol, nicolasfella, kde-frameworks-devel, LeGast00n, sbergeron, 
michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Piyush Aggarwal]

brute4s99 marked 8 inline comments as done.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: bcooksley, apol, nicolasfella, kde-frameworks-devel, LeGast00n, sbergeron, 
michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Piyush Aggarwal]

brute4s99 updated this revision to Diff 61716.
brute4s99 added a comment.


  updated

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D21660?vs=61639&id=61716

BRANCH
  arcpatch-D21660_1

REVISION DETAIL
  https://phabricator.kde.org/D21660

AFFECTED FILES
  CMakeLists.txt
  src/knotificationmanager.cpp

To: brute4s99, broulik, nicolasfella
Cc: bcooksley, apol, nicolasfella, kde-frameworks-devel, LeGast00n, sbergeron, 
michaelh, ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Nicolas Fella]

nicolasfella requested changes to this revision.
This revision now requires changes to proceed.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: bcooksley, apol, nicolasfella, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Nicolas Fella]

nicolasfella added inline comments.

INLINE COMMENTS

> CMakeLists.txt:40
> +if (NOT ANDROID)
> +find_package(Qt5 ${REQUIRED_QT_VERSION} CONFIG REQUIRED DBus)
> +else ()

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

REPOSITORY
  R289 KNotifications

BRANCH
  arcpatch-D21660_1

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: bcooksley, apol, nicolasfella, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns

D21660: remove dbus for windows build and change audio dep logic

[Piyush Aggarwal]

brute4s99 retitled this revision from "remove dbus and change audio dep logic" 
to "remove dbus for windows build and change audio dep logic".

REPOSITORY
  R289 KNotifications

BRANCH
  arcpatch-D21660_1

REVISION DETAIL
  https://phabricator.kde.org/D21660

To: brute4s99, broulik, nicolasfella
Cc: bcooksley, apol, nicolasfella, kde-frameworks-devel, LeGast00n, michaelh, 
ngraham, bruns