Bug 68506 - glib support for cmake build system
Summary: glib support for cmake build system
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Ralf Habacker
QA Contact:
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks: 41252 54445 68855
  Show dependency treegraph
 
Reported: 2013-08-24 13:04 UTC by Ralf Habacker
Modified: 2014-01-07 19:14 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Add glib support to cmake build system (10.91 KB, patch)
2013-08-24 13:04 UTC, Ralf Habacker
Details | Splinter Review
Add glib support to cmake build system (update) (10.91 KB, patch)
2013-08-27 12:43 UTC, Ralf Habacker
Details | Splinter Review
Add glib support to cmake build system (update 2) (10.88 KB, patch)
2013-08-27 20:52 UTC, Ralf Habacker
Details | Splinter Review
fix test-dbus-daemon running issue caused by hardcoded listen addres (2.32 KB, patch)
2013-10-10 07:47 UTC, Ralf Habacker
Details | Splinter Review
fix test-dbus-daemon running issue caused by hardcoded listen address (with autotools support) (2.90 KB, patch)
2013-10-10 12:05 UTC, Ralf Habacker
Details | Splinter Review
Add glib support to cmake build system (update 3) (9.55 KB, patch)
2013-10-14 15:50 UTC, Ralf Habacker
Details | Splinter Review
Skip unix only syslog test (975 bytes, patch)
2013-10-14 19:07 UTC, Ralf Habacker
Details | Splinter Review
Add glib support to cmake buildsystem (update 4) (9.20 KB, patch)
2014-01-06 15:34 UTC, Ralf Habacker
Details | Splinter Review
Add glib support to cmake build system (update 5) (11.25 KB, patch)
2014-01-06 20:47 UTC, Ralf Habacker
Details | Splinter Review
Add glib support to cmake build system (update 6) (11.15 KB, patch)
2014-01-07 06:08 UTC, Ralf Habacker
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Habacker 2013-08-24 13:04:38 UTC
Created attachment 84561 [details] [review]
Add glib support to cmake build system

The appended patch adds compiling dbus with glib support.
Comment 1 Simon McVittie 2013-08-27 09:39:54 UTC
Comment on attachment 84561 [details] [review]
Add glib support to cmake build system

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

::: cmake/CMakeLists.txt
@@ +101,4 @@
>  
>  find_package(EXPAT)
>  find_package(X11)
> +find_package(GLib2)

Just to check: this is a non-mandatory dependency, right?

(If it doesn't find GLib, the configure-equivalent shouldn't fail, just unset GLIB2_FOUND.)

::: cmake/test/CMakeLists.txt
@@ +84,5 @@
> +    include_directories(
> +        ${GLIB2_INCLUDE_DIR}
> +        ${GOBJECT_INCLUDE_DIR}
> +        ${DBUSGLIB_INCLUDE_DIR}
> +        ${DBUSGLIB_INCLUDE_DIR}/..

What's "${DBUSGLIB_INCLUDE_DIR}/.." doing here? The Autotools build system doesn't need that.
Comment 2 Ralf Habacker 2013-08-27 12:42:38 UTC
(In reply to comment #1)
> Comment on attachment 84561 [details] [review] [review]
> Add glib support to cmake build system
> 
> Review of attachment 84561 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: cmake/CMakeLists.txt
> @@ +101,4 @@
> >  
> >  find_package(EXPAT)
> >  find_package(X11)
> > +find_package(GLib2)
> 
> Just to check: this is a non-mandatory dependency, right?
> 
> (If it doesn't find GLib, the configure-equivalent shouldn't fail, just
> unset GLIB2_FOUND.)
This is already optional. Making it mandatory requires to add the term REQUIRED see http://cmake.org/cmake/help/v2.8.8/cmake.html#command:find_package

> 
> ::: cmake/test/CMakeLists.txt
> @@ +84,5 @@
> > +    include_directories(
> > +        ${GLIB2_INCLUDE_DIR}
> > +        ${GOBJECT_INCLUDE_DIR}
> > +        ${DBUSGLIB_INCLUDE_DIR}
> > +        ${DBUSGLIB_INCLUDE_DIR}/..
> 
> What's "${DBUSGLIB_INCLUDE_DIR}/.." doing here? The Autotools build system
> doesn't need that.
Has been there because of a design issue in the related FindDBUSGLIB.cmake, will provide a fix.
Comment 3 Ralf Habacker 2013-08-27 12:43:56 UTC
Created attachment 84707 [details] [review]
Add glib support to cmake build system (update)

fixed ${DBUSGLIB_INCLUDE_DIR]/.. issue
Comment 4 Simon McVittie 2013-08-27 14:39:59 UTC
Comment on attachment 84707 [details] [review]
Add glib support to cmake build system (update)

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

::: cmake/modules/FindDBusGLib.cmake
@@ +22,5 @@
> +
> +find_path(DBUSGLIB_INCLUDE_DIR
> +          NAMES dbus-glib.h
> +          HINTS ${PC_LibDBUSGLIB_INCLUDEDIR}
> +          PATH_SUFFIXES dbus-1.0/dbus)

Would "NAMES dbus/dbus-glib.h ... PATH_SUFFIXES dbus-1.0" work? That'd seem more appropriate: you're meant to #include <dbus/dbus-glib.h>.
Comment 5 Ralf Habacker 2013-08-27 20:52:04 UTC
Created attachment 84751 [details] [review]
Add glib support to cmake build system (update 2)

Somehow loaded the old patch again, fixed now
Comment 6 Ralf Habacker 2013-08-28 17:18:03 UTC
(In reply to comment #5)
> Created attachment 84751 [details] [review] [review]
> Add glib support to cmake build system (update 2)
> 
> Somehow loaded the old patch again, fixed now

cross compiling with autotools fails with: 

Making all in .
make[3]: Entering directory `/home/ralf/src/dbus-autotools-cross-build/test'
  CCLD     test-dbus-daemon.exe
dbus-daemon.o:/home/ralf/src/dbus-autotools-cross-build/test/../../dbus/test/dbus-daemon.c:423: undefined reference to `_dbus_getsid'
collect2: error: ld returned 1 exit status
make[3]: *** [test-dbus-daemon.exe] Fehler 1

I tried to use libdbus-internal.la, but got then the following error:

  CCLD     test-dbus-daemon.exe
/usr/i686-w64-mingw32/sys-root/mingw/lib/libdbus-1.dll.a(d000084.o):(.text+0x0): multiple definition of `dbus_error_free'
../dbus/.libs/libdbus-internal.a(libdbus_internal_la-dbus-errors.o):/home/ralf/src/dbus-autotools-cross-build/dbus/../../dbus/dbus/dbus-errors.c:212: first defined here
/usr/i686-w64-mingw32/sys-root/mingw/lib/libdbus-1.dll.a(d000087.o):(.text+0x0): multiple definition of `dbus_error_is_set'
../dbus/.libs/libdbus-internal.a(libdbus_internal_la-dbus-errors.o):/home/ralf/src/dbus-autotools-cross-build/dbus/../../dbus/dbus/dbus-errors.c:330: first defined here
/usr/i686-w64-mingw32/sys-root/mingw/lib/libdbus-1.dll.a(d000086.o):(.text+0x0): multiple definition of `dbus_error_init'
../dbus/.libs/libdbus-internal.a(libdbus_internal_la-dbus-errors.o):/home/ralf/src/dbus-autotools-cross-build/dbus/../../dbus/dbus/dbus-errors.c:189: first defined here
collect2: error: ld returned 1 exit status
make[3]: *** [test-dbus-daemon.exe] Fehler 1
Comment 7 Simon McVittie 2013-08-28 17:46:22 UTC
(In reply to comment #6)
> dbus-daemon.c:423: undefined reference to `_dbus_getsid'

That's the test you added for #54445, I suspect.

The problem here is that each test executable can only be linked against the internal library or against the public shared library, never both. I'll have to think about this.
Comment 8 Ralf Habacker 2013-09-27 18:46:59 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > dbus-daemon.c:423: undefined reference to `_dbus_getsid'
> 
> That's the test you added for #54445, I suspect.
> 
> The problem here is that each test executable can only be linked against the
> internal library or against the public shared library, never both. I'll have
> to think about this.

As the dependency problem has been solved with 87df259d8c4aae3d188a15d7976c0f63141119e8 I took the chance to run some glib depending test cases with cmake.  

cd dbus-cross-build-root
DBUS_TEST_DAEMON=bin/dbus-daemon.exe DBUS_TEST_DATA=z:$PWD/test/data DBUS_SESSION_BUS_ADDRESS=autolaunch wine bin/test-dbus-daemon.exe
/creds: ** Message: bin/dbus-daemon.exe --config-file=z:/home/ralf/src/dbus-cross-cmake-build/test/data/valid-config-files/session.conf
** Message: ProcessID of this process is 8
OK
/echo/session: ** Message: bin/dbus-daemon.exe --config-file=z:/home/ralf/src/dbus-cross-cmake-build/test/data/valid-config-files/session.conf
OK
/echo/limited: ** Message: bin/dbus-daemon.exe --config-file=z:/home/ralf/src/dbus-cross-cmake-build/test/data/valid-config-files/incoming-limit.conf
Failed to start message bus: Unknown address type 'unix'
Comment 9 Simon McVittie 2013-09-30 11:23:22 UTC
(In reply to comment #8)
> Failed to start message bus: Unknown address type 'unix'

Ah, yes, that test uses a specific config file which hard-codes unix:tmpdir=/tmp. Either this test should be #ifdef G_OS_UNIX, or the config file it uses (incoming-limit.conf) should become a .in file with @DBUS_SESSION_BUS_LISTEN_ADDRESS@ substituted (like bus/session.conf.in).
Comment 10 Simon McVittie 2013-09-30 11:41:21 UTC
(In reply to comment #8)
> DBUS_SESSION_BUS_ADDRESS=autolaunch

FYI, that isn't a valid address (it's meant to be "autolaunch:"), although for a test that starts its own dbus-daemon, it doesn't matter.
Comment 11 Ralf Habacker 2013-10-10 07:47:32 UTC
Created attachment 87374 [details] [review]
fix test-dbus-daemon running issue caused by hardcoded listen addres

Other config files already use the variable TEST_LISTEN, so i guess this should be used here too.
Comment 12 Ralf Habacker 2013-10-10 07:50:47 UTC
(In reply to comment #10)
> (In reply to comment #8)
> > DBUS_SESSION_BUS_ADDRESS=autolaunch
> 
> FYI, that isn't a valid address (it's meant to be "autolaunch:"), although
> for a test that starts its own dbus-daemon, it doesn't matter.

yes, it is better to unset. With patch #87374 and enabled binfmt .exe handler 
I get the following results running test-dbus-daemon 

xxxx@yyyy:~/src/dbus-cross-cmake-build> DBUS_SESSION_BUS_ADDRESS= DBUS_TEST_DAEMON=bin/dbus-daemon.exe DBUS_TEST_DATA=z:$PWD/test/data wine bin/test-dbus-daemon.exe 
/creds: ** Message: starting dbus daemon: bin/dbus-daemon.exe --config-file=z:/home/ralf/src/dbus-cross-cmake-build/test/data/valid-config-files/session.conf

** Message: ProcessID of this process is 8
OK
/echo/session: ** Message: starting dbus daemon: bin/dbus-daemon.exe --config-file=z:/home/ralf/src/dbus-cross-cmake-build/test/data/valid-config-files/session.conf

OK
/echo/limited: ** Message: starting dbus daemon: bin/dbus-daemon.exe --config-file=z:/home/ralf/src/dbus-cross-cmake-build/test/data/valid-config-files/incoming-limit.conf

OK
Comment 13 Simon McVittie 2013-10-10 11:15:03 UTC
Comment on attachment 87374 [details] [review]
fix test-dbus-daemon running issue caused by hardcoded listen addres

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

Good idea, but you still need to generate incoming-listen.conf from incoming-listen.conf.in.

In Autotools-land, the easiest way is to put it in AC_CONFIG_FILES. Please do a `make distcheck` to ensure that it works correctly in a clean source tree.

In CMake, it looks as though the FOREACH(FILE_TYPE *.conf.in *.service.in) block already handles that.
Comment 14 Ralf Habacker 2013-10-10 12:05:24 UTC
Created attachment 87384 [details] [review]
fix test-dbus-daemon running issue caused by hardcoded listen address (with autotools support)

did run "make distcheck" without any problems
Comment 15 Ralf Habacker 2013-10-10 12:05:53 UTC
(In reply to comment #13)
> In CMake, it looks as though the FOREACH(FILE_TYPE *.conf.in *.service.in)
> block already handles that.
yes
Comment 16 Simon McVittie 2013-10-10 12:55:01 UTC
(In reply to comment #14)
> Created attachment 87384 [details] [review]
> fix test-dbus-daemon running issue caused by hardcoded listen address (with
> autotools support)
> 
> did run "make distcheck" without any problems

Great, please apply. Does this also resolve Bug #41252?

If you've tested them, please apply the patches from Bug #66453 too.
Comment 17 Ralf Habacker 2013-10-10 13:42:14 UTC
Comment on attachment 87384 [details] [review]
fix test-dbus-daemon running issue caused by hardcoded listen address (with autotools support)

committed
Comment 18 Colin Walters 2013-10-10 15:52:28 UTC
This commit breaks the build of gnome-continuous:

cp: cannot stat '../../test/data/valid-config-files/incoming-limit.conf': No such file or directory
make[3]: *** [all-local] Error 1
make[3]: *** Waiting for unfinished jobs....
mv -f .deps/libdbus_testutils_internal_la-test-utils.Tpo .deps/libdbus_testutils_internal_la-test-utils.Plo
make[3]: Leaving directory `/ostbuild/source/dbus-bootstrap/_build/test'
make[2]: *** [all-recursive] Error 1
make[2]: Leaving directory `/ostbuild/source/dbus-bootstrap/_build/test'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/ostbuild/source/dbus-bootstrap/_build'
make: *** [all] Error 2
ob: pid 5925 exited with code 2
Build of dbus-bootstrap failed

Possibly a missing AC_SUBST?  Investigating.
Comment 19 Colin Walters 2013-10-10 15:57:38 UTC
(In reply to comment #18)

> Possibly a missing AC_SUBST?  Investigating.

Pushed a fix:
http://cgit.freedesktop.org/dbus/dbus/commit/?id=5618696768805abfbc3741f60817584451648795
Comment 20 Simon McVittie 2013-10-10 16:17:56 UTC
(In reply to comment #19)
> Pushed a fix:
> http://cgit.freedesktop.org/dbus/dbus/commit/
> ?id=5618696768805abfbc3741f60817584451648795

Pushed a fix for the fix: 880209788. If we're going to have a review procedure (which we do, and which I think we should), please follow it; and if you're pushing a "trivial" change, at least re-read it yourself and be 110% sure that it's right :-)
Comment 21 Ralf Habacker 2013-10-14 15:50:19 UTC
Created attachment 87608 [details] [review]
Add glib support to cmake build system (update 3)

new:
- create test/data/valid-config-files/session.d
- do not find external dbus-glib package
Comment 22 Ralf Habacker 2013-10-14 19:07:59 UTC
Created attachment 87621 [details] [review]
Skip unix only syslog test
Comment 23 Ralf Habacker 2013-10-14 19:35:24 UTC
Comment on attachment 87621 [details] [review]
Skip unix only syslog test

moved to bug #41252
Comment 24 Ralf Habacker 2013-10-15 16:31:43 UTC
(In reply to comment #21)
> Created attachment 87608 [details] [review] [review]
> Add glib support to cmake build system (update 3)
> 
> new:
> - create test/data/valid-config-files/session.d
> - do not find external dbus-glib package

Any time for review ?
Comment 25 Ralf Habacker 2014-01-06 15:34:37 UTC
Created attachment 91542 [details] [review]
Add glib support to cmake buildsystem (update 4)

- do not use dbusglib
Comment 26 Ralf Habacker 2014-01-06 16:08:13 UTC
Simon, any chance to get this in 1.8 ?
Comment 27 Simon McVittie 2014-01-06 19:21:04 UTC
Comment on attachment 91542 [details] [review]
Add glib support to cmake buildsystem (update 4)

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

We can't distribute files under a license that isn't clearly specified.

Other than that, this looks good. Do the tests pass? (On which platforms?)

::: cmake/modules/FindGLib2.cmake
@@ +8,5 @@
> +# Copyright (c) 2008 Laurent Montel, <montel@kde.org>
> +# Copyright (c) 2013 Ralf Habacker, <ralf.habacker@freenet.de>
> +#
> +# Redistribution and use is allowed according to the terms of the BSD license.
> +# For details see the accompanying COPYING-CMAKE-SCRIPTS file.

No such file.

There are several distinct licenses referred to as "the BSD license", so I'd have to see the exact license text to know whether this is something we can distribute.

::: cmake/modules/FindGObject.cmake
@@ +9,5 @@
> +# Copyright (c) 2011, Raphael Kubo da Costa <kubito@gmail.com>
> +# Copyright (c) 2006, Tim Beaulen <tbscope@gmail.com>
> +#
> +# Redistribution and use is allowed according to the terms of the BSD license.
> +# For details see the accompanying COPYING-CMAKE-SCRIPTS file.

Likewise.
Comment 28 Simon McVittie 2014-01-06 19:22:09 UTC
(In reply to comment #26)
> Simon, any chance to get this in 1.8 ?

It's missed the boat for 1.7.10 (1.8 rc1) unless there's something else critically wrong with that release, but it could go in 1.8 if finished.
Comment 29 Ralf Habacker 2014-01-06 20:47:10 UTC
Created attachment 91559 [details] [review]
Add glib support to cmake build system (update 5)

add license file
Comment 30 Ralf Habacker 2014-01-06 20:54:10 UTC
(In reply to comment #27)
> Comment on attachment 91542 [details] [review] [review]
> Add glib support to cmake buildsystem (update 4)
> 
> Review of attachment 91542 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> We can't distribute files under a license that isn't clearly specified.
see updated patch. 
> 
> Other than that, this looks good. Do the tests pass? 
running test cases needs some additional build system fixes which at the end have a running "make test" target.
Would you like to see the related patched appended to this bug or https://bugs.freedesktop.org/show_bug.cgi?id=41252

(On which platforms?)
The tests are tested on wine with mingw32.
Comment 31 Ralf Habacker 2014-01-07 06:08:15 UTC
Created attachment 91576 [details] [review]
Add glib support to cmake build system (update 6)

- Remove remaining dbusglib references
Comment 32 Ralf Habacker 2014-01-07 06:20:33 UTC
(In reply to comment #30)
> > Other than that, this looks good. Do the tests pass? 
> running test cases needs some additional build system fixes which at the end
> have a running "make test" target.
> Would you like to see the related patched appended to this bug or
> https://bugs.freedesktop.org/show_bug.cgi?id=41252
This bug is already added as dependency of 41252 and some related problems are already raised there, so i added the additional patches to bug 41252.
Comment 33 Simon McVittie 2014-01-07 11:39:06 UTC
Comment on attachment 91576 [details] [review]
Add glib support to cmake build system (update 6)

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

Looks fine to me, thanks!
Comment 34 Ralf Habacker 2014-01-07 19:13:52 UTC
Comment on attachment 91576 [details] [review]
Add glib support to cmake build system (update 6)

committed to master


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.