Re: Review Request 129590: KAuth: Make D-Bus dependency optional.

[David Faure]

On lundi 17 juillet 2017 11:21:29 CEST Ralf Habacker wrote:
> -target_link_libraries(KF5Auth PRIVATE Qt5::Widgets Qt5::DBus)
> +target_link_libraries(KF5Auth PRIVATE Qt5::Widgets ${DBUS_LIBRARY})

If this means KAuth doesn't use DBus (on some platforms), then the proper fix 
would be to only link to DBus on the platforms where this is needed.

A cmake option isn't needed, if there's no code that needs to be disabled.

Assuming the Windows and OSX implementations don't use DBus
   (this must be the case, otherwise your patch would break things)
then the proper logic would be

if (UNIX)
  require DBus
  link to DBus
  enable autotests that require DBus
endif()

-- 
David Faure, fa...@kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5

Re: Review Request 129590: KAuth: Make D-Bus dependency optional.

[Ralf Habacker]

> On Jan. 8, 2017, 6:53 nachm., David Faure wrote:
> > autotests/CMakeLists.txt, line 5
> > 
> >
> > Better split this into two, to avoid confusing people.

I've update the patch, but do not have access rights to upload the path, so I 
appended it here:

commit ec98addbfbba62310e050046078380c10192b829
Author: Ralf Habacker 
Date:   Mon Jul 17 11:16:39 2017 +0200

Make building with QtDBus optional

This patch adds an ENABLE_DBUS CMake option defaulting to ON,
which can be used to make KAuth not depend on DBus.
This is useful for some apps that are running on Windows or Mac OSX.

see https://www.mail-archive.com/kde-frameworks-devel@kde.org/msg34246.html

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 13a6129..c53ff15 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -11,8 +11,16 @@ feature_summary(WHAT REQUIRED_PACKAGES_NOT_FOUND 
FATAL_ON_MISSING_REQUIRED_PACKA
 
 set(CMAKE_MODULE_PATH ${ECM_MODULE_PATH} ${ECM_KDE_MODULE_DIR})
 
+option(ENABLE_DBUS "Enable D-Bus functionality" ON)
+add_feature_info(D-Bus ENABLE_DBUS "Enable D-Bus functionality")
+
+if(ENABLE_DBUS)
+set(DBUS_PACKAGE DBus)
+set(DBUS_LIBRARY Qt::DBus)
+endif()
+
 set(REQUIRED_QT_VERSION 5.6.0)
-find_package(Qt5 ${REQUIRED_QT_VERSION} CONFIG REQUIRED Widgets DBus)
+find_package(Qt5 ${REQUIRED_QT_VERSION} CONFIG REQUIRED Widgets 
${DBUS_PACKAGE})
 include(KDEInstallDirs)
 include(KDEFrameworkCompilerSettings NO_POLICY_SCOPE)
 include(KDECMakeSettings)
diff --git a/autotests/CMakeLists.txt b/autotests/CMakeLists.txt
index b53d760..fe07742 100644
--- a/autotests/CMakeLists.txt
+++ b/autotests/CMakeLists.txt
@@ -5,6 +5,10 @@ if(NOT Qt5Test_FOUND)
 message(STATUS "Qt5Test not found, autotests will not be built.")
 return()
 endif()
+if(NOT ENABLE_DBUS)
+message(STATUS "Autotests will not be built, because DBus was disabled.")
+return()
+endif()
 
 qt5_add_dbus_adaptor(kauth_dbus_adaptor_tests_SRCS
  ../src/backends/dbus/org.kde.kf5auth.xml
@@ -39,7 +43,7 @@ target_include_directories(kauth_tests_static
 ${CMAKE_CURRENT_SOURCE_DIR}/../src
 )
 
-target_link_libraries(kauth_tests_static PUBLIC Qt5::DBus KF5::CoreAddons)
+target_link_libraries(kauth_tests_static PUBLIC ${DBUS_LIBRARY} 
KF5::CoreAddons)
 
 ### next target ###
 
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt
index 662a0dd..5754ceb 100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -31,7 +31,7 @@ add_library(KF5::Auth ALIAS KF5Auth)
 target_include_directories(KF5Auth INTERFACE 
"$")
 
 target_link_libraries(KF5Auth PUBLIC Qt5::Core KF5::CoreAddons)  # for KJob
-target_link_libraries(KF5Auth PRIVATE Qt5::Widgets Qt5::DBus)
+target_link_libraries(KF5Auth PRIVATE Qt5::Widgets ${DBUS_LIBRARY})
 set_target_properties(KF5Auth PROPERTIES VERSION   ${KAUTH_VERSION_STRING}
  SOVERSION ${KAUTH_SOVERSION}
  EXPORT_NAME Auth


- Ralf


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129590/#review101885
---


On Nov. 30, 2016, 10:59 vorm., Gleb Popov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129590/
> ---
> 
> (Updated Nov. 30, 2016, 10:59 vorm.)
> 
> 
> Review request for KDE Frameworks and kdewin.
> 
> 
> Repository: kauth
> 
> 
> Description
> ---
> 
> This adds an `ENABLE_DBUS` CMake option defaulting to `ON`, which can be used 
> to make KAuth not depend on DBus. This is useful for some apps that are 
> running on Windows.
> 
> https://www.mail-archive.com/kde-frameworks-devel@kde.org/msg34246.html
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt b8b7ba7 
>   autotests/CMakeLists.txt b53d760 
>   src/CMakeLists.txt 1b6930d 
> 
> Diff: https://git.reviewboard.kde.org/r/129590/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gleb Popov
> 
>

Re: Review Request 129590: KAuth: Make D-Bus dependency optional.

[David Faure]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129590/#review101885
---



Looks good, just one comment.


autotests/CMakeLists.txt (line 5)


Better split this into two, to avoid confusing people.


- David Faure


On Nov. 30, 2016, 9:59 a.m., Gleb Popov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129590/
> ---
> 
> (Updated Nov. 30, 2016, 9:59 a.m.)
> 
> 
> Review request for KDE Frameworks and kdewin.
> 
> 
> Repository: kauth
> 
> 
> Description
> ---
> 
> This adds an `ENABLE_DBUS` CMake option defaulting to `ON`, which can be used 
> to make KAuth not depend on DBus. This is useful for some apps that are 
> running on Windows.
> 
> https://www.mail-archive.com/kde-frameworks-devel@kde.org/msg34246.html
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt b8b7ba7 
>   autotests/CMakeLists.txt b53d760 
>   src/CMakeLists.txt 1b6930d 
> 
> Diff: https://git.reviewboard.kde.org/r/129590/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Gleb Popov
> 
>

Review Request 129590: KAuth: Make D-Bus dependency optional.

[Gleb Popov]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129590/
---

Review request for KDE Frameworks and kdewin.


Repository: kauth


Description
---

This adds an `ENABLE_DBUS` CMake option defaulting to `ON`, which can be used 
to make KAuth not depend on DBus. This is useful for some apps that are running 
on Windows.

https://www.mail-archive.com/kde-frameworks-devel@kde.org/msg34246.html


Diffs
-

  CMakeLists.txt b8b7ba7 
  autotests/CMakeLists.txt b53d760 
  src/CMakeLists.txt 1b6930d 

Diff: https://git.reviewboard.kde.org/r/129590/diff/


Testing
---


Thanks,

Gleb Popov