Bug 41252 - CMake should build and run "modular tests" if GLib, dbus-glib are available
Summary: CMake should build and run "modular tests" if GLib, dbus-glib are available
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.4.x
Hardware: Other All
: medium enhancement
Assignee: Havoc Pennington
QA Contact: John (J5) Palmieri
URL:
Whiteboard: review+
Keywords: patch
Depends on: 68506 73495
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-27 03:55 UTC by Simon McVittie
Modified: 2014-09-07 09:39 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Skip unix only syslog test (975 bytes, patch)
2013-10-14 19:36 UTC, Ralf Habacker
Details | Splinter Review
Add add_test_executable and add_service_executable macros (12.27 KB, patch)
2014-01-07 06:15 UTC, Ralf Habacker
Details | Splinter Review
Add run_tests target (1.05 KB, patch)
2014-01-07 06:16 UTC, Ralf Habacker
Details | Splinter Review
Do not block tests on abort (940 bytes, patch)
2014-01-07 06:16 UTC, Ralf Habacker
Details | Splinter Review
Create session.conf anf system.conf for test data (2.36 KB, patch)
2014-01-07 06:17 UTC, Ralf Habacker
Details | Splinter Review
define TEST_BUS_LAUNCH_BINARY (1.31 KB, patch)
2014-01-07 06:17 UTC, Ralf Habacker
Details | Splinter Review
cleanup project tags (1.57 KB, patch)
2014-01-07 06:18 UTC, Ralf Habacker
Details | Splinter Review
Use macros for test and helper executable targets (13.28 KB, patch)
2014-01-08 00:11 UTC, Ralf Habacker
Details | Splinter Review
Use macros for test and helper executable targets (update 1) (13.33 KB, patch)
2014-01-08 00:16 UTC, Ralf Habacker
Details | Splinter Review
Do not block tests on abort (965 bytes, patch)
2014-01-08 00:16 UTC, Ralf Habacker
Details | Splinter Review
Use cmake build in executable suffix (918 bytes, patch)
2014-01-08 01:32 UTC, Ralf Habacker
Details | Splinter Review
define TEST_BUS_LAUNCH_BINARY for cmake to keep in sync with autotools (1.34 KB, patch)
2014-01-08 01:32 UTC, Ralf Habacker
Details | Splinter Review
Create session.conf and system.conf for test data from *.in files (1.51 KB, patch)
2014-01-08 01:49 UTC, Ralf Habacker
Details | Splinter Review
Add cmake target_run_tests (1.11 KB, patch)
2014-01-08 01:50 UTC, Ralf Habacker
Details | Splinter Review
Use macros for test and helper executable targets (update 2) (14.11 KB, patch)
2014-01-08 23:41 UTC, Ralf Habacker
Details | Splinter Review
tests: don't block and wait for a debugger on abort (969 bytes, patch)
2014-01-09 15:00 UTC, Simon McVittie
Details | Splinter Review
Give cmake users some hints/requirements about how to run test applications (2.34 KB, patch)
2014-01-14 19:17 UTC, Ralf Habacker
Details | Splinter Review
CMake warning-- (813 bytes, patch)
2014-01-14 20:00 UTC, Ralf Habacker
Details | Splinter Review
Fix windows doc for running tests (1.42 KB, patch)
2014-01-14 23:46 UTC, Ralf Habacker
Details | Splinter Review
Give cmake users some hints/requirements about how to run test applications (2.53 KB, patch)
2014-01-17 11:53 UTC, Ralf Habacker
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2011-09-27 03:55:44 UTC
test/Makefile.am now builds the following "modular tests":

* test-corrupt, test-dbus-daemon, test-loopback, test-marshal,
  test-relay: these use GLib/GObject/GIO, dbus-glib and the public libdbus API

* test-refs, test-syslog: these use GLib/GObject/GIO and libdbus-internal

The CMake build system should run these too. I'm likely to write all future tests in this style, since it's more maintainable, does not add extra goo to the library (so it can be enabled by default), and in the case of tests that use only the public libdbus API, can even be used to test an installed libdbus.

The dbus-glib dependency for these tests is a circular dependency, which isn't ideal. It's only used for the main loop integration, not for the object mapping, so Bug #31515 could fix this (if Thiago doesn't veto it).
Comment 1 Ralf Habacker 2013-10-10 14:42:02 UTC
(In reply to comment #0)
> test/Makefile.am now builds the following "modular tests":
> 
> * test-corrupt, test-dbus-daemon, test-loopback, test-marshal,
>   test-relay: these use GLib/GObject/GIO, dbus-glib and the public libdbus
> API
These test are mostly running on a cross compiled dbus, although without using dbus-glib 

xxx@yyy:~/src/dbus-cmake-cross-build> bin/test-corrupt.exe 
/corrupt/tcp: ..........OK
/corrupt/byte-order/tcp: ..........OK

xxx@yyy:~/src/dbus-cmake-cross-build> DBUS_SESSION_BUS_ADDRESS= DBUS_TEST_DATA=z:$PWD/test/data DBUS_TEST_DAEMON=bin/dbus-daemon.exe bin/test-dbus-daemon.exe 
/creds: Failed to start message bus: Failed to read directory "z:/home/ralf.habacker/src/dbus-cmake-cross-build/test/data/valid-config-files\session.d": Pfad nicht gefunden.

xxx@yyy:~/src/dbus-cmake-cross-build> bin/test-loopback.exe 
/connect/tcp: .OK
/connect/nonce-tcp: .OK
/message/tcp: ......OK
/message/nonce-tcp: ......OK
/message/bad-guid: ........OK

xxx@yyy:~/src/dbus-cmake-cross-build> bin/test-marshal.exe 
/demarshal/le: OK
/demarshal/be: OK
/demarshal/needed/le: OK
/demarshal/needed/be: OK

xxx@yyy:~/src/dbus-cmake-cross-build> bin/test-relay.exe
/connect: ..OK
/relay: .........OK
/limit: ..OK

xxx@yyy:~/src/dbus-cmake-cross-build> bin/test-refs.exe 
/refs/connection: OK
/refs/message: OK
/refs/pending-call: OK
/refs/server: OK

> 
> * test-refs, test-syslog: these use GLib/GObject/GIO and libdbus-internal
> 
xxx@yyy:~/src/dbus-cmake-cross-build> bin/test-refs.exe 
/refs/connection: OK
/refs/message: OK
/refs/pending-call: OK
/refs/server: OK

xxx@yyy:~/src/dbus-cmake-cross-build> bin/test-syslog.exe 
/syslog: GLib-Message: Not implemented: g_test_trap_fork

(test-syslog.exe:8): GLib-ERROR **: g_test_trap_ assertion with no trapped test

abnormal program termination

> The CMake build system should run these too. I'm likely to write all future
> tests in this style, since it's more maintainable, does not add extra goo to
> the library (so it can be enabled by default), and in the case of tests that
> use only the public libdbus API, can even be used to test an installed
> libdbus.
>
Comment 2 Simon McVittie 2013-10-10 16:23:43 UTC
(In reply to comment #1)
> These test are mostly running on a cross compiled dbus, although without
> using dbus-glib 

\o/

> xxx@yyy:~/src/dbus-cmake-cross-build> DBUS_SESSION_BUS_ADDRESS=
> DBUS_TEST_DATA=z:$PWD/test/data DBUS_TEST_DAEMON=bin/dbus-daemon.exe
> bin/test-dbus-daemon.exe 
> /creds: Failed to start message bus: Failed to read directory
> "z:/home/ralf.habacker/src/dbus-cmake-cross-build/test/data/valid-config-
> files\session.d": Pfad nicht gefunden.

The CMake build system might just need to create an empty session.d?

> xxx@yyy:~/src/dbus-cmake-cross-build> bin/test-syslog.exe 
> /syslog: GLib-Message: Not implemented: g_test_trap_fork

I think I must have misunderstood g_test_trap_fork(). Everything from the first "if (g_test_trap_fork" down to the comment "manual test (this is the best we can do on Windows" can be #ifdef G_OS_WIN32.

For the "manual test" part, the manual test is to check that "regression test for _dbus_system_log(): 42" does something that would be appropriate for an info message, and "regression test for _dbus_system_log(): 666" does something that would be appropriate for a security message. I think they go to the Windows event log or something? This might be something that's only practical on real Windows, not in Wine.
Comment 3 Ralf Habacker 2013-10-14 19:36:23 UTC
Created attachment 87622 [details] [review]
Skip unix only syslog test
Comment 4 Ralf Habacker 2013-10-14 19:49:24 UTC
(In reply to comment #2)
> (In reply to comment #1)
> > These test are mostly running on a cross compiled dbus, although without
> > using dbus-glib 
> 
> \o/
> 
> > xxx@yyy:~/src/dbus-cmake-cross-build> DBUS_SESSION_BUS_ADDRESS=
> > DBUS_TEST_DATA=z:$PWD/test/data DBUS_TEST_DAEMON=bin/dbus-daemon.exe
> > bin/test-dbus-daemon.exe 
> > /creds: Failed to start message bus: Failed to read directory
> > "z:/home/ralf.habacker/src/dbus-cmake-cross-build/test/data/valid-config-
> > files\session.d": Pfad nicht gefunden.
> 
> The CMake build system might just need to create an empty session.d?

fixed in basic glib support #68506

> 
> > xxx@yyy:~/src/dbus-cmake-cross-build> bin/test-syslog.exe 
> > /syslog: GLib-Message: Not implemented: g_test_trap_fork
> 
> I think I must have misunderstood g_test_trap_fork(). Everything from the
> first "if (g_test_trap_fork" down to the comment "manual test (this is the
> best we can do on Windows" can be #ifdef G_OS_WIN32.
> 
> For the "manual test" part, the manual test is to check that "regression
> test for _dbus_system_log(): 42" does something that would be appropriate
> for an info message, and "regression test for _dbus_system_log(): 666" does
> something that would be appropriate for a security message. I think they go
> to the Windows event log 

no, it uses currently OutputDebugString http://msdn.microsoft.com/en-us/library/windows/desktop/aa363362%28v=vs.85%29.aspx

> This might be something that's only practical on real Windows, not in Wine.

Such message could be displayed with dbgview on real windows or on wine.
Comment 5 Ralf Habacker 2013-10-14 19:55:42 UTC
(In reply to comment #4)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > These test are mostly running on a cross compiled dbus, although without
> > > using dbus-glib 
> > 
> > \o/
> > 
> > > xxx@yyy:~/src/dbus-cmake-cross-build> DBUS_SESSION_BUS_ADDRESS=
> > > DBUS_TEST_DATA=z:$PWD/test/data DBUS_TEST_DAEMON=bin/dbus-daemon.exe
> > > bin/test-dbus-daemon.exe 
> > > /creds: Failed to start message bus: Failed to read directory
> > > "z:/home/ralf.habacker/src/dbus-cmake-cross-build/test/data/valid-config-
> > > files\session.d": Pfad nicht gefunden.
> > 
> > The CMake build system might just need to create an empty session.d?
> 
> fixed in basic glib support #68506
> 
> > 
> > > xxx@yyy:~/src/dbus-cmake-cross-build> bin/test-syslog.exe 
> > > /syslog: GLib-Message: Not implemented: g_test_trap_fork
> > 
> > I think I must have misunderstood g_test_trap_fork(). Everything from the
> > first "if (g_test_trap_fork" down to the comment "manual test (this is the
> > best we can do on Windows" can be #ifdef G_OS_WIN32.
> > 
> > For the "manual test" part, the manual test is to check that "regression
> > test for _dbus_system_log(): 42" does something that would be appropriate
> > for an info message, and "regression test for _dbus_system_log(): 666" does
> > something that would be appropriate for a security message. I think they go
> > to the Windows event log 
> 
> no, it uses currently OutputDebugString
> http://msdn.microsoft.com/en-us/library/windows/desktop/aa363362%28v=vs.
> 85%29.aspx
> 
> > This might be something that's only practical on real Windows, not in Wine.
> 
> Such message could be displayed with dbgview on real windows or on wine.

Windows event log only makes sense when running dbus-daemon as service or when started on boot time. In that case message could be added with http://msdn.microsoft.com/en-us/library/windows/desktop/aa363750%28v=vs.85%29.aspx
Comment 6 Simon McVittie 2014-01-06 19:23:10 UTC
Comment on attachment 87622 [details] [review]
Skip unix only syslog test

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

::: test/internals/syslog.c
@@ +51,4 @@
>  test_syslog (Fixture *f,
>      gconstpointer data)
>  {
> +#ifndef G_OS_WIN32

I'd have said #ifdef G_OS_UNIX, but this is fine too. Please merge after I've finished releasing 1.7.10.
Comment 7 Ralf Habacker 2014-01-06 20:39:41 UTC
(In reply to comment #6)
> Comment on attachment 87622 [details] [review] [review]
> Skip unix only syslog test
> 
> Review of attachment 87622 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: test/internals/syslog.c
> @@ +51,4 @@
> >  test_syslog (Fixture *f,
> >      gconstpointer data)
> >  {
> > +#ifndef G_OS_WIN32
> 
> I'd have said #ifdef G_OS_UNIX, but this is fine too. Please merge after
> I've finished releasing 1.7.10.

committed
Comment 8 Ralf Habacker 2014-01-07 06:15:59 UTC
Created attachment 91577 [details] [review]
Add add_test_executable and add_service_executable macros
Comment 9 Ralf Habacker 2014-01-07 06:16:25 UTC
Created attachment 91578 [details] [review]
Add run_tests target
Comment 10 Ralf Habacker 2014-01-07 06:16:54 UTC
Created attachment 91579 [details] [review]
Do not block tests on abort
Comment 11 Ralf Habacker 2014-01-07 06:17:28 UTC
Created attachment 91580 [details] [review]
Create session.conf anf system.conf for test data
Comment 12 Ralf Habacker 2014-01-07 06:17:59 UTC
Created attachment 91581 [details] [review]
define TEST_BUS_LAUNCH_BINARY
Comment 13 Ralf Habacker 2014-01-07 06:18:27 UTC
Created attachment 91582 [details] [review]
cleanup project tags
Comment 14 Simon McVittie 2014-01-07 11:48:00 UTC
Comment on attachment 91577 [details] [review]
Add add_test_executable and add_service_executable macros

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

::: cmake/modules/Macros.cmake
@@ +14,5 @@
> +macro(add_test_executable _target _source)
> +    add_executable(${_target} ${_source})
> +    target_link_libraries(${_target} ${ARGN})
> +    # require binfmt
> +    set(_env "DBUS_TEST_DAEMON=${CMAKE_BINARY_DIR}/bin/dbus-daemon.exe")

.exe is wrong for a non-Windows host architecture, use ${EXEEXT}; but this seems to be redundant with another environment addition later.

(In principle it should be possible to run these tests on native Linux when not cross-compiling, right?)

@@ +20,5 @@
> +    list(APPEND _env "DBUS_BLOCK_ON_ABORT=1")
> +    list(APPEND _env "DBUS_FATAL_WARNINGS=1")
> +    if (CMAKE_CROSSCOMPILING)
> +        set(PREFIX "z:")
> +        set(EXT ".exe")

The top-level CMakeLists.txt uses ${EXEEXT}, which is the standard naming under Autotools. If there isn't a standard name in CMake, it'll make life easier if we steal the Autotools one.

"We're cross-compiling" is the wrong test for whether to use ${EXEEXT}; the right test is "the host architecture is Windows" (in the Autotools terminology where you compile on the build architecture to produce binaries for the host architecture).

@@ +22,5 @@
> +    if (CMAKE_CROSSCOMPILING)
> +        set(PREFIX "z:")
> +        set(EXT ".exe")
> +        # run with wine
> +        add_test(${_target} wine ${PREFIX}${EXECUTABLE_OUTPUT_PATH}/${_target}${EXT})

If you're relying on binfmt_misc to be able to run the dbus-daemon, is there any reason not to rely on it for the tests themselves too, avoiding this code path? That's what I do for the subset of tests that work under Wine in Automake.

@@ +28,5 @@
> +        set(PREFIX)
> +        set(EXT)
> +        add_test(NAME ${_target} COMMAND $<TARGET_FILE:${_target}>)
> +    endif()
> +    list(APPEND _env "DBUS_TEST_DAEMON=${CMAKE_BINARY_DIR}/bin/dbus-daemon${EXT}")

This is redundant with the other line that sets DBUS_TEST_DAEMON. This version looks more correct.

@@ +34,5 @@
> +    list(APPEND _env "DBUS_TEST_HOMEDIR=${PREFIX}${CMAKE_BINARY_DIR}/dbus")
> +    set_tests_properties(${_target} PROPERTIES ENVIRONMENT "${_env}")
> +endmacro(add_test_executable)
> +
> +macro(add_service_executable _target _source)

add_helper_executable or add_test_helper, perhaps?
Comment 15 Simon McVittie 2014-01-07 11:48:31 UTC
Comment on attachment 91578 [details] [review]
Add run_tests target

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

Looks good, after fixing the previous patch.
Comment 16 Simon McVittie 2014-01-07 11:49:41 UTC
Comment on attachment 91579 [details] [review]
Do not block tests on abort

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

::: cmake/modules/Macros.cmake
@@ +17,4 @@
>      # require binfmt
>      set(_env "DBUS_TEST_DAEMON=${CMAKE_BINARY_DIR}/bin/dbus-daemon.exe")
>      list(APPEND _env "DBUS_SESSION_BUS_ADDRESS=")
> +    #list(APPEND _env "DBUS_BLOCK_ON_ABORT=1")

Don't comment out code; either leave it in, or delete it.

It's probably a good idea to have the tests default to just crashing rather than blocking and waiting for a debugger, though. I might make the equivalent change in Autotools-land.
Comment 17 Simon McVittie 2014-01-07 11:55:38 UTC
Comment on attachment 91580 [details] [review]
Create session.conf anf system.conf for test data

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

::: cmake/CMakeLists.txt
@@ +469,5 @@
>      set(TEST_SOCKET_DIR ${DBUS_SESSION_SOCKET_DIR} )
>      set(TEST_LAUNCH_HELPER_BINARY ${EXECUTABLE_OUTPUT_PATH}/dbus-daemon-launch-helper-test)
> +    if (WIN32 OR CROSS_COMPILING)
> +        set (TEST_LISTEN "autolaunch:scope=*install-path")
> +        set (TEST_CONNECT_ADDRESS "autolaunch:scope=*install-path")

This is incorrect if you're cross-compiling from one Unix to another.

Am I right in thinking that UNIX and WIN32 are defined according to what Autotools calls the host architecture, i.e. the architecture for which we're building binaries?

If so, then the equivalent of the code in configure.ac is:

if (UNIX)
    TEST_LISTEN=unix:tmpdir=${TEST_SOCKET_DIR}
else()
    TEST_LISTEN="tcp:host=localhost"
endif()

(or you could use if (WIN32)/else/endif if you prefer the conditional to be that way round).

If you want to use autolaunch: for a Windows host, you can use that instead of TCP if you want, but please make the same change in configure.ac (near line 1651).

TEST_CONNECT_ADDRESS doesn't seem to be used for anything? All the tests should be able to pick up the right connectable address from dbus-daemon --print-address, hopefully?
Comment 18 Simon McVittie 2014-01-07 11:56:39 UTC
Comment on attachment 91581 [details] [review]
define TEST_BUS_LAUNCH_BINARY

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

::: cmake/CMakeLists.txt
@@ +468,4 @@
>      set(DBUS_TEST_DATA ${CMAKE_BINARY_DIR}/test/data)
>      set(TEST_SOCKET_DIR ${DBUS_SESSION_SOCKET_DIR} )
>      set(TEST_LAUNCH_HELPER_BINARY ${EXECUTABLE_OUTPUT_PATH}/dbus-daemon-launch-helper-test)
> +    set(TEST_BUS_LAUNCH_BINARY ${EXECUTABLE_OUTPUT_PATH}/dbus-launch)

Strictly speaking, this should end with ${EXEEXT}; but we only use it on Unix anyway, so this patch is OK.
Comment 19 Simon McVittie 2014-01-07 11:57:01 UTC
Comment on attachment 91582 [details] [review]
cleanup project tags

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

If you say so.
Comment 20 Ralf Habacker 2014-01-08 00:11:11 UTC
Created attachment 91626 [details] [review]
Use macros for test and helper executable targets

fixed issues
Comment 21 Ralf Habacker 2014-01-08 00:16:30 UTC
Created attachment 91627 [details] [review]
Use macros for test and helper executable targets (update 1)

Do not remove block tests on abort
Comment 22 Ralf Habacker 2014-01-08 00:16:59 UTC
Created attachment 91628 [details] [review]
Do not block tests on abort
Comment 23 Ralf Habacker 2014-01-08 01:32:19 UTC
Created attachment 91629 [details] [review]
Use cmake build in executable suffix
Comment 24 Ralf Habacker 2014-01-08 01:32:56 UTC
Created attachment 91630 [details] [review]
define TEST_BUS_LAUNCH_BINARY for cmake to keep in sync with autotools
Comment 25 Ralf Habacker 2014-01-08 01:49:43 UTC
Created attachment 91632 [details] [review]
Create session.conf and system.conf for test data from *.in files

There are some remaining issues with the TEST-LISTEN address. Some note: 

- test-dbus-daemon* is not able to run a daemon wrapped by wine, which means that the following line from patch 91627         

set(_env "DBUS_TEST_DAEMON=${WINE_EXECUTABLE} ${PREFIX}${CMAKE_BINARY_DIR}/bin/dbus-daemon${EXEEXT}")

do not work. 

As alternative i thought to set DBUS_TEST_DAEMON_ADDRESS to a build path related autolaunch scope (autolaunch:scope=*install-path), which starts the daemon and fetches the address from it. Unfortunally autolaunch support in libdbus runs the daemon with --session and not with the required config file.

Looks that I have to implement something like run-with-tmp-session-bus.sh or to extend test-dbus-daemon* to support a wrapper, e.g 

set(_env "DBUS_TEST_WRAPPER=${WINE_EXECUTABLE}")
list(APPEND _env "DBUS_TEST_DAEMON=${PREFIX}${CMAKE_BINARY_DIR}/bin/dbus-daemon${EXEEXT}")

Need to think more about.
Comment 26 Ralf Habacker 2014-01-08 01:50:04 UTC
Comment on attachment 91582 [details] [review]
cleanup project tags

committed to master
Comment 27 Ralf Habacker 2014-01-08 01:50:51 UTC
Created attachment 91633 [details] [review]
Add cmake target_run_tests
Comment 28 Ralf Habacker 2014-01-08 01:51:19 UTC
(In reply to comment #27)
> Created attachment 91633 [details] [review] [review]
> Add cmake target_run_tests

commit messages extended
Comment 29 Simon McVittie 2014-01-08 11:36:24 UTC
(In reply to comment #25)
> - test-dbus-daemon* is not able to run a daemon wrapped by wine

Right, it expects DBUS_TEST_DAEMON to be a single executable, not a shell command line. It does the equivalent of this shell script:

    "$DBUS_TEST_DAEMON" --nofork ...

whereas your syntax would only work if it did the equivalent of this shell script:

    $DBUS_TEST_DAEMON --nofork ...

I don't want this code to have to compose, escape and execute /bin/sh command lines if I can possibly avoid it.

> set(_env "DBUS_TEST_DAEMON=${WINE_EXECUTABLE}
> ${PREFIX}${CMAKE_BINARY_DIR}/bin/dbus-daemon${EXEEXT}")

Can't you just use

set(_env "DBUS_TEST_DAEMON=${CMAKE_BINARY_DIR}/bin/dbus-daemon${EXEEXT}")

and when cross-compiling and cross-testing on Linux, rely on binfmt_misc to inspect dbus-daemon.exe, notice that it's a Windows executable and transparently run it under Wine? AIUI you're relying on that behaviour already, somewhere else (and the equivalent setup in Autotools also relies on it).
Comment 30 Ralf Habacker 2014-01-08 23:41:50 UTC
Created attachment 91718 [details] [review]
Use macros for test and helper executable targets (update 2)

Completed running cross compiled test applications using binfmt_misc kernel support.
Comment 31 Ralf Habacker 2014-01-08 23:42:51 UTC
(In reply to comment #29)
> Can't you just use
> 
> set(_env "DBUS_TEST_DAEMON=${CMAKE_BINARY_DIR}/bin/dbus-daemon${EXEEXT}")
> 
> and when cross-compiling and cross-testing on Linux, rely on binfmt_misc to
> inspect dbus-daemon.exe, notice that it's a Windows executable and
> transparently run it under Wine? AIUI you're relying on that behaviour
> already, somewhere else (and the equivalent setup in Autotools also relies
> on it)

good hint, works well with updated patch set
Comment 32 Simon McVittie 2014-01-09 14:59:36 UTC
Comment on attachment 91628 [details] [review]
Do not block tests on abort

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

OK, or you can squash it into the patch that introduced this line (currently Attachment #91718 [details], I think).
Comment 33 Simon McVittie 2014-01-09 15:00:34 UTC
Created attachment 91759 [details] [review]
tests: don't block and wait for a debugger on abort

In general, I think developers running the tests would expect
them to terminate rather than hanging. Developers who want to debug
such an abort by attaching a debugger to a live process can still set
DBUS_BLOCK_ON_ABORT in the environment.

---

Here's the equivalent of Attachment #91628 [details], but for Autotools.
Comment 34 Simon McVittie 2014-01-09 15:01:31 UTC
Comment on attachment 91629 [details] [review]
Use cmake build in executable suffix

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

::: cmake/CMakeLists.txt
@@ +353,5 @@
>  
>  # test binary names
>  if (WIN32)
> +    # follow Automake's naming convention so we can share .in files
> +    set (EXEEXT ${CMAKE_EXECUTABLE_SUFFIX})

Oh, is this a built-in thing already? Nice. review+
Comment 35 Simon McVittie 2014-01-09 15:01:59 UTC
Comment on attachment 91630 [details] [review]
define TEST_BUS_LAUNCH_BINARY for cmake to keep in sync with autotools

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

Sure.
Comment 36 Simon McVittie 2014-01-09 15:02:28 UTC
Comment on attachment 91632 [details] [review]
Create session.conf and system.conf for test data from *.in files

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

Seems reasonable.
Comment 37 Simon McVittie 2014-01-09 15:03:56 UTC
Comment on attachment 91633 [details] [review]
Add cmake target_run_tests

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

OK. If there's some more conventional name for this target ("check" in Autotools, "test" in some other build systems like Perl's), renaming it to that, or making the conventionally-named target depend on it, would also be nice.
Comment 38 Simon McVittie 2014-01-09 15:06:30 UTC
Comment on attachment 91718 [details] [review]
Use macros for test and helper executable targets (update 2)

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

> When cross compiling for Windows on Linux we are using wine to run test
> executables. Because wine is mandatory for running test, it's presence is
> verified on cmake run. The configuring process will be aborted if wine
> has not been found.

An interesting idea, but I don't think this is necessary. In general, tests are not expected to pass when cross-compiling; if they do happen to work, that's great, but I don't think it's appropriate to abort the build for this.

(The normal way to test cross-built binaries, if at all, is to build the tests on the build platform, copy the executables onto the host platform and run them there.)
Comment 39 Simon McVittie 2014-01-09 15:12:19 UTC
Comment on attachment 91718 [details] [review]
Use macros for test and helper executable targets (update 2)

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

::: cmake/bus/CMakeLists.txt
@@ +106,5 @@
>  endif (DBUS_SERVICE)
>  
>  if (DBUS_ENABLE_EMBEDDED_TESTS)
> +	set(SOURCES ${BUS_SOURCES} ${BUS_DIR}/test-main.c)
> +	add_test_executable(bus-test "${SOURCES}"  ${DBUS_INTERNAL_LIBRARIES} ${XML_LIBRARY})

The add_test_executable and add_helper_executable bits are fine.

::: cmake/modules/Macros.cmake
@@ +1,1 @@
> +if (DBUS_BUILD_TESTS AND CMAKE_CROSSCOMPILING AND CMAKE_SYSTEM_NAME STREQUAL "Windows")

This is going to break the build if someone planned to cross-compile the tests on Unix, then copy them onto a Windows machine for testing.

@@ +7,5 @@
> +    endif()
> +    if (NOT CMAKE_HOST_SYSTEM_NAME STREQUAL "Linux")
> +        message(FATAL_ERROR "ERROR: Running cross compiled test applications is not supported on this os !!!")
> +    endif()
> +    if (NOT EXISTS /proc/sys/fs/binfmt_misc/DOSWin)

I think this is way outside the scope of the dbus build system.

Debian's wine packages automatically set up similar system integration, via the binfmt-support package, so dbus doesn't need to do anything here.

systemd has a similar (although currently more limited) mechanism for registering binfmt_misc hooks; I would expect that other Linux distributions' packaging will either pick up binfmt-support from Debian, or use systemd's version, to get this to happen automatically.

@@ +11,5 @@
> +    if (NOT EXISTS /proc/sys/fs/binfmt_misc/DOSWin)
> +        FILE(WRITE ${CMAKE_BINARY_DIR}/run-as-root
> +            "echo :DOSWin:M::MZ::${WINE_EXECUTABLE}: >/proc/sys/fs/binfmt_misc/register")
> +        FILE(INSTALL ${CMAKE_BINARY_DIR}/run-as-root DESTINATION /tmp PERMISSIONS OWNER_EXECUTE OWNER_READ)
> +        message(FATAL_ERROR "binfmt_misc needs to be configured to run wine executables.\nPlease run\n      sudo /tmp/run-as-root\n")

If this was written to /tmp it would be a trivial symlink attack, so it's a good thing you didn't do that :-)
Comment 40 Simon McVittie 2014-01-09 15:19:46 UTC
(In reply to comment #39)
> I think this is way outside the scope of the dbus build system.

It would be fine to have a new section in README.win describing how to run the D-Bus tests under Wine when cross-compiling on Linux, though. It could say something like:

    Some distributions' Wine packages set it up as an interpreter for Windows
    executables automatically. If yours doesn't, ...
Comment 41 Ralf Habacker 2014-01-09 21:56:19 UTC
(In reply to comment #33)
> Created attachment 91759 [details] [review] [review]
> tests: don't block and wait for a debugger on abort
> 
> In general, I think developers running the tests would expect
> them to terminate rather than hanging. Developers who want to debug
> such an abort by attaching a debugger to a live process can still set
> DBUS_BLOCK_ON_ABORT in the environment.
> 
> ---
> 
> Here's the equivalent of Attachment #91628 [details], but for Autotools.

looks good
Comment 42 Ralf Habacker 2014-01-09 23:37:09 UTC
Comment on attachment 91629 [details] [review]
Use cmake build in executable suffix

committed to master
Comment 43 Ralf Habacker 2014-01-09 23:42:31 UTC
Comment on attachment 91630 [details] [review]
define TEST_BUS_LAUNCH_BINARY for cmake to keep in sync with autotools

committed to master with a remove of a related todo

diff --git a/cmake/test/CMakeLists.txt b/cmake/test/CMakeLists.txt
index b3e0390..e29a499 100644
--- a/cmake/test/CMakeLists.txt
+++ b/cmake/test/CMakeLists.txt
@@ -194,7 +194,3 @@ FOREACH(FILE ${FILES})
         MESSAGE("FROM: ${FILE}\nTO: ${TARGET}\n")
     ENDIF (CONFIG_VERBOSE)
 ENDFOREACH(FILE)
-
-# todo: for installation the TEST_..._BINARY variables must reflect the 
-#       installation dir or has to be defined relative
-#
Comment 44 Ralf Habacker 2014-01-09 23:49:45 UTC
Comment on attachment 91632 [details] [review]
Create session.conf and system.conf for test data from *.in files

committed to master
Comment 45 Ralf Habacker 2014-01-10 00:09:40 UTC
Comment on attachment 91633 [details] [review]
Add cmake target_run_tests

renamed to 'check' as present in autotools and committed to master
Comment 46 Ralf Habacker 2014-01-10 00:30:36 UTC
Comment on attachment 91718 [details] [review]
Use macros for test and helper executable targets (update 2)

Committed the reviewed part to master with a modified commit message 

"Use macros for test and helper executable targets on cmake build system.
    
The new macros add_test_executables and add helper_executables provides a
platform independent way for specifing dbus test and service applications.
    
On native Windows and Linux/UNIX systems the test applications are
directly runable.
    
When cross compiling for Windows on Linux test applications could be
executed on the Linux host system with the help of wine and activated
binfmt_misc support for wine."
    
Will provide an additional patch for the remaining issues.
Comment 47 Ralf Habacker 2014-01-10 00:37:45 UTC
Comment on attachment 91759 [details] [review]
tests: don't block and wait for a debugger on abort

merged with the cmake related fix and committed to master
Comment 48 Ralf Habacker 2014-01-14 19:17:23 UTC
Created attachment 92082 [details] [review]
Give cmake users some hints/requirements about how to run test applications
Comment 49 Ralf Habacker 2014-01-14 20:00:15 UTC
Created attachment 92088 [details] [review]
CMake warning--
Comment 50 Ralf Habacker 2014-01-14 23:46:59 UTC
Created attachment 92103 [details] [review]
Fix windows doc for running tests
Comment 51 Simon McVittie 2014-01-16 12:49:11 UTC
Comment on attachment 92082 [details] [review]
Give cmake users some hints/requirements about how to run test applications

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

This seems unnecessary to me, but if you want to add it, add it. Fine for 1.8.0 since it only touches the non-default build system.

::: cmake/modules/Macros.cmake
@@ +1,4 @@
> +if(DBUS_BUILD_TESTS AND CMAKE_CROSSCOMPILING AND CMAKE_SYSTEM_NAME STREQUAL "Windows")
> +    if(CMAKE_HOST_SYSTEM_NAME STREQUAL "Linux")
> +        find_file(WINE_EXECUTABLE wine_)
> +        if(EXISTS /proc/sys/fs/binfmt_misc/DOSWin)

Does a major distribution call it DOSWin, or is that just your own convention?

If we're going to pick a random name, it might as well be "/proc/sys/fs/binfmt_misc/wine" as used by Debian/Ubuntu.

If DOSWin is the equivalent name in SUSE or Red Hat or something, keeping it as-is is fine.
Comment 52 Simon McVittie 2014-01-16 12:49:30 UTC
Comment on attachment 92088 [details] [review]
CMake warning--

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

Sure, please do.
Comment 53 Simon McVittie 2014-01-16 12:50:51 UTC
Comment on attachment 92103 [details] [review]
Fix windows doc for running tests

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

This requires your patches from Bug #73495, I think. After that, it seems fine.
Comment 54 Ralf Habacker 2014-01-17 11:10:18 UTC
Comment on attachment 92088 [details] [review]
CMake warning--

committed
Comment 55 Ralf Habacker 2014-01-17 11:53:04 UTC
Created attachment 92281 [details] [review]
Give cmake users some hints/requirements about how to run test applications

with host limited search for common known binfmt_misc keys used for wine system
Comment 56 Ralf Habacker 2014-01-20 22:03:03 UTC
(In reply to comment #55)
> Created attachment 92281 [details] [review] [review]
> Give cmake users some hints/requirements about how to run test applications
> 
> with host limited search for common known binfmt_misc keys used for wine
> system

As the base patch has been reviewed and the remaining issues has been solved, i assume there is not problem to commit to 1.8

Should I merge 1.8 then into master or cherry-pick this commmit ?
Comment 57 Simon McVittie 2014-01-21 11:54:21 UTC
(In reply to comment #56)
> As the base patch has been reviewed and the remaining issues has been
> solved, i assume there is not problem to commit to 1.8

Oh, sorry, I missed merging these before 1.8.

I'd normally say "you've missed the boat for 1.8, it can wait til 1.9 now" - but this is only documentation and the non-default build system, and I did review it already, so yeah, please commit to dbus-1.8 and merge into master.
Comment 58 Ralf Habacker 2014-01-21 20:12:28 UTC
Comment on attachment 92281 [details] [review]
Give cmake users some hints/requirements about how to run test applications

committed to dbus-1.8 and master
Comment 59 Simon McVittie 2014-04-28 14:20:37 UTC
Comment on attachment 92103 [details] [review]
Fix windows doc for running tests

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

This doesn't seem to have been applied. Please apply it if you still want it - it looks fine to me, but as a non-Windows user I'm not going to test it :-)
Comment 60 Ralf Habacker 2014-09-07 09:38:59 UTC
(In reply to comment #59)
> Comment on attachment 92103 [details] [review] [review]
> Fix windows doc for running tests
> 
> Review of attachment 92103 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> This doesn't seem to have been applied. Please apply it if you still want it
> - it looks fine to me, but as a non-Windows user I'm not going to test it :-)

oversaw this somehow, committed.


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.