Bug 61637

Created attachment 75712 [details] [review]
Fixed CMake linux builds; they require rt library.

Created attachment 75713 [details] [review]
CMake linux fixes when using meinproc4 doc generator.

Comment on attachment 75712 [details] [review]
Fixed CMake linux builds; they require rt library.

Review of attachment 75712 [details] [review]:
-----------------------------------------------------------------

::: cmake/CMakeLists.txt
@@ +494,5 @@
>  endif  (DBUS_BUILD_TESTS)
>  
> +if (UNIX)
> +    set(DBUS_LIBRARIES dbus-1 rt)
> +    set(DBUS_INTERNAL_LIBRARIES dbus-internal rt)

No, this should never be necessary. Instead, use something like:

    target_link_libraries(dbus-1 ${CMAKE_THREAD_LIBS_INIT} rt)

for the non-WIN32 case in dbus/CMakeLists.txt.

(Strictly speaking, -lrt might be required, optional or broken, depending on platform... but I don't particularly care about CMake working on non-Linux Unix, which should use Autotools instead.)

Comment on attachment 75713 [details] [review]
CMake linux fixes when using meinproc4 doc generator.

Review of attachment 75713 [details] [review]:
-----------------------------------------------------------------

Seems reasonable.

(In reply to comment #3)
> Comment on attachment 75712 [details] [review] [review]
> Fixed CMake linux builds; they require rt library.
> 
> Review of attachment 75712 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: cmake/CMakeLists.txt
> @@ +494,5 @@
> >  endif  (DBUS_BUILD_TESTS)
> >  
> > +if (UNIX)
> > +    set(DBUS_LIBRARIES dbus-1 rt)
> > +    set(DBUS_INTERNAL_LIBRARIES dbus-internal rt)
> 
> No, this should never be necessary. Instead, use something like:
> 
>     target_link_libraries(dbus-1 ${CMAKE_THREAD_LIBS_INIT} rt)
> 
> for the non-WIN32 case in dbus/CMakeLists.txt.
> 
This do not fix all cases: all targets in bus subdir using DBUS_INTERNAL_LIBRARIES needs rt too. 

I can add the following stuff to bus/CMakeLists.txt

IF (NOT WIN32)
    set(DBUS_INTERNAL_LIBRARIES "${DBUS_INTERNAL_LIBRARIES} rt")
endif ()

(In reply to comment #5)
> >     target_link_libraries(dbus-1 ${CMAKE_THREAD_LIBS_INIT} rt)
>
> This do not fix all cases: all targets in bus subdir using
> DBUS_INTERNAL_LIBRARIES needs rt too. 

Sorry, yeah, I meant "what I said, and then the same again for dbus-internal".

The general design principle should be that the libraries used by any given static or shared library are an implementation detail of that static or shared library: if dbus-daemon directly uses dbus-internal and Expat, and dbus-internal uses pthreads and librt, it should be enough to say "link dbus-daemon against dbus-internal and Expat" and the build system should be clever enough to work out that it also needs pthreads and librt.

This appears to be true for pthreads - dbus-daemon does not explicitly link to ${CMAKE_THREAD_LIBS_INIT}, but it still works - so presumably CMake is clever enough to make this happen.

In the Autotools stack, it's libtool that is responsible for getting this right.

Created attachment 75898 [details] [review]
Fix cmake linux build: dbus-1 and dbus-internal require to link to rt library

(In reply to comment #6)

> This appears to be true for pthreads - dbus-daemon does not explicitly link
> to ${CMAKE_THREAD_LIBS_INIT}, but it still works - so presumably CMake is
> clever enough to make this happen.

yes, works as expected

Comment on attachment 75898 [details] [review]
Fix cmake linux build: dbus-1 and dbus-internal require to link to rt library

Review of attachment 75898 [details] [review]:
-----------------------------------------------------------------

Thanks, ship it.

committed to master

Created attachment 77120 [details]
Build log

With the change, still fails to build on openSUSE. Attached the fail part

(In reply to comment #11)
> Created attachment 77120 [details]
> Build log
> 
> With the change, still fails to build on openSUSE. Attached the fail part

 libtool: link: gcc -Wall -Wextra -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wcast-align -Wno-address -Wfloat-equal -Wdeclaration-after-statement -Wno-unused-label -Wno-missing-field-initializers -Wno-unused-parameter -Wno-sign-compare -Wno-pointer-sign -Wno-type-limits -fno-common -fno-strict-aliasing -fmessage-length=0 -grecord-gcc-switches -O2 -Wall -D_FORTIFY_SOURCE=2 -fstack-protector -funwind-tables -fasynchronous-unwind-tables -g -fno-strict-aliasing -fPIC -fpie -fstack-protector -pie -o dbus-daemon-launch-helper activation-helper-bin.o config-loader-expat.o config-parser-common.o config-parser-trivial.o desktop-file.o utils.o activation-helper.o  ../dbus/.libs/libdbus-internal.a -lsystemd-login -lsystemd-daemon -lexpat -lpthread

The build log shows that this is an autotools issue, that dbus-daemon-launch-helper could not be build and that there is no -rt library added to the link line.