|
Description
Simon McVittie
2011-09-27 03:55:44 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. > (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. Created attachment 87622 [details] [review] Skip unix only syslog test (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. (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 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. (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 Created attachment 91577 [details] [review] Add add_test_executable and add_service_executable macros Created attachment 91578 [details] [review] Add run_tests target Created attachment 91579 [details] [review] Do not block tests on abort Created attachment 91580 [details] [review] Create session.conf anf system.conf for test data Created attachment 91581 [details] [review] define TEST_BUS_LAUNCH_BINARY Created attachment 91582 [details] [review] cleanup project tags 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 on attachment 91578 [details] [review] Add run_tests target Review of attachment 91578 [details] [review]: ----------------------------------------------------------------- Looks good, after fixing the previous patch. 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 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 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 on attachment 91582 [details] [review] cleanup project tags Review of attachment 91582 [details] [review]: ----------------------------------------------------------------- If you say so. Created attachment 91626 [details] [review] Use macros for test and helper executable targets fixed issues Created attachment 91627 [details] [review] Use macros for test and helper executable targets (update 1) Do not remove block tests on abort Created attachment 91628 [details] [review] Do not block tests on abort Created attachment 91629 [details] [review] Use cmake build in executable suffix Created attachment 91630 [details] [review] define TEST_BUS_LAUNCH_BINARY for cmake to keep in sync with autotools 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 on attachment 91582 [details] [review] cleanup project tags committed to master Created attachment 91633 [details] [review] Add cmake target_run_tests (In reply to comment #27) > Created attachment 91633 [details] [review] [review] > Add cmake target_run_tests commit messages extended (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). 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. (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 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). 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 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 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 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 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 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 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 :-) (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, ... (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 on attachment 91629 [details] [review] Use cmake build in executable suffix committed to master 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 on attachment 91632 [details] [review] Create session.conf and system.conf for test data from *.in files committed to master Comment on attachment 91633 [details] [review] Add cmake target_run_tests renamed to 'check' as present in autotools and committed to master 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 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 Created attachment 92082 [details] [review] Give cmake users some hints/requirements about how to run test applications Created attachment 92088 [details] [review] CMake warning-- Created attachment 92103 [details] [review] Fix windows doc for running tests 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 on attachment 92088 [details] [review] CMake warning-- Review of attachment 92088 [details] [review]: ----------------------------------------------------------------- Sure, please do. 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 on attachment 92088 [details] [review] CMake warning-- committed 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 (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 ? (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 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 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 :-) (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.