Bug 68506

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.

(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.

Created attachment 84707 [details] [review]
Add glib support to cmake build system (update)

fixed ${DBUSGLIB_INCLUDE_DIR]/.. issue

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>.

Created attachment 84751 [details] [review]
Add glib support to cmake build system (update 2)

Somehow loaded the old patch again, fixed now

(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

(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.

(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'

(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).

(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.

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.

(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 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.

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

(In reply to comment #13)
> In CMake, it looks as though the FOREACH(FILE_TYPE *.conf.in *.service.in)
> block already handles that.
yes

(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 on attachment 87384 [details] [review]
fix test-dbus-daemon running issue caused by hardcoded listen address (with autotools support)

committed

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.

(In reply to comment #18)

> Possibly a missing AC_SUBST?  Investigating.

Pushed a fix:
http://cgit.freedesktop.org/dbus/dbus/commit/?id=5618696768805abfbc3741f60817584451648795

(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 :-)

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

Created attachment 87621 [details] [review]
Skip unix only syslog test

Comment on attachment 87621 [details] [review]
Skip unix only syslog test

moved to bug #41252

(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 ?

Created attachment 91542 [details] [review]
Add glib support to cmake buildsystem (update 4)

- do not use dbusglib

Simon, any chance to get this in 1.8 ?

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.

(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.

Created attachment 91559 [details] [review]
Add glib support to cmake build system (update 5)

add license file

(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.

Created attachment 91576 [details] [review]
Add glib support to cmake build system (update 6)

- Remove remaining dbusglib references

(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 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 on attachment 91576 [details] [review]
Add glib support to cmake build system (update 6)

committed to master