Bug 88966

(In reply to Ralf Habacker from comment #0)
> To run cross compiled test applications cmake build system requires binfmt
> support, which is not available on remote build systems like opensuse build
> service. The appended patch fixes this issue.

I'm really not sure how useful it is to be running "make check" on cross-compiled binaries... testing them on the target platform, sure, but testing via emulation on a platform that is not the one where they're actually meant to run seems rather fragile (if a test fails, is it because the code is broken, or because the emulation is incomplete?), and this is quite a lot of machinery to make it work.

Comment on attachment 113158 [details] [review]
Add running cross compiled application without the

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

Reviewing anyway, on the basis that you might convince me...

::: cmake/modules/Macros.cmake
@@ +52,5 @@
> +        if(HAVE_BINFMT_WINE_SUPPORT)
> +            add_test(NAME ${_target} COMMAND $<TARGET_FILE:${_target}>)
> +        else()
> +            add_test(NAME ${_target} COMMAND ${WINE_EXECUTABLE} $<TARGET_FILE:${_target}>)
> +            list(APPEND _env "DBUS_TEST_DAEMON_WRAPPER=/usr/bin/wine")

${WINE_EXECUTABLE}, surely?

::: test/test-utils-glib.c
@@ +95,4 @@
>    GError *error = NULL;
>    GString *address;
>    gint address_fd;
> +  gchar *argv[] = {

This could still be "const gchar *argv[]" (a mutable array of constant strings) if you use my suggestion below.

@@ +156,5 @@
>      }
>  
> +  if (g_getenv ("DBUS_TEST_DAEMON_WRAPPER") != NULL)
> +    {
> +      argv[0] = g_strdup (g_getenv ("DBUS_TEST_DAEMON_WRAPPER"));

This is leaked. I'd prefer to assign it to a separate gchar * which is freed on exit from the function, then copy that pointer to argv[0], so we don't have to keep track of which elements of argv should/shouldn't be freed.

Created attachment 119535 [details] [review]
Do not require binfmt support for running cross compiled test applications (update 1)

rebased to dbus-1.10

(In reply to Simon McVittie from comment #2)
> 
> ${WINE_EXECUTABLE}, surely?

yes, it is the wine executable on the host side.

> > +      argv[0] = g_strdup (g_getenv ("DBUS_TEST_DAEMON_WRAPPER"));
> 
> This is leaked. I'd prefer to assign it to a separate gchar * which is freed
> on exit from the function, then copy that pointer to argv[0], so we don't
> have to keep track of which elements of argv should/shouldn't be freed.

fixed by using g_ptr_array_add in dbus-1.10 branch, which free's memory by default.

(In reply to Ralf Habacker from comment #4)

> fixed by using g_ptr_array_add in dbus-1.10 branch, which free's memory by
> default.
The related commit is http://cgit.freedesktop.org/dbus/dbus/commit/?h=dbus-1.10&id=4eddd1bf52dd00dfc651f3aa3e2411d9d438ef51. Any open issue with this patch or is it ready for inclusion ?

Created attachment 120385 [details] [review]
Do not require binfmt support for running cross compiled test applications.

Bug:

Comment on attachment 119535 [details] [review]
Do not require binfmt support for running cross compiled test applications (update 1)

obsolate

(In reply to Ralf Habacker from comment #6)
> Created attachment 120385 [details] [review] [review]
> Do not require binfmt support for running cross compiled test applications.
> 
rebased on master.

(In reply to Simon McVittie from comment #1)
> (In reply to Ralf Habacker from comment #0)
> > To run cross compiled test applications cmake build system requires binfmt
> > support, which is not available on remote build systems like opensuse build
> > service. The appended patch fixes this issue.
> 
> I'm really not sure how useful it is to be running "make check" on
> cross-compiled binaries

running the tests helped to find a hidden tcp socket bug mentioned at bug 92721 comment 61

... testing them on the target platform, sure, but
> testing via emulation on a platform that is not the one where they're
> actually meant to run seems rather fragile (if a test fails, is it because
> the code is broken, or because the emulation is incomplete?),

as mentioned above: The recent state of wine let run the complete test suite not only without any issue, but moreover can help to find dbus implementation bugs because of subtil timing differences not detected on native linux.

(In reply to Simon McVittie from comment #1)
> (In reply to Ralf Habacker from comment #0)
> > To run cross compiled test applications cmake build system requires binfmt
> > support, which is not available on remote build systems like opensuse build
> > service. The appended patch fixes this issue.
>
> I'm really not sure how useful it is to be running "make check" on
> cross-compiled binaries... testing them on the target platform, sure, but
> testing via emulation on a platform that is not the one where they're
> actually meant to run seems rather fragile (if a test fails, is it because
> the code is broken, or because the emulation is incomplete?)
this is easy: dbus prints out error messages and wine also like 'fixme:.....' or 'warn':...
> and this is quite a lot of machinery to make it work.
This patch helps to avoid the need of additional system configuration, it makes the wine binfmt support optional ;-)

Also I do not see that there is a public windows machine available as part of dbus CI to run dbus test bus cases. The only public available CI system I have access to is the opensuse build system (obs), which could be used to run cross compiled dbus test cases on wine, which is better than nothing.

Created attachment 120731 [details] [review]
Do not require binfmt support for running cross compiled test applications.

Bug:

Comment on attachment 120731 [details] [review]
Do not require binfmt support for running cross compiled test applications.

- do not add wine as wrapper when spawning dbus-daemon.exe from an application already running on wine.

okay for commit to master ? I'm blocked to work on bug 93976, which needs to patch the same area.

Comment on attachment 120731 [details] [review]
Do not require binfmt support for running cross compiled test applications.

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

A couple of lines here look as though they were meant to be somewhere else, but other than that the commit looks fine, and there's nothing actively wrong with those lines.

::: cmake/modules/Macros.cmake
@@ +52,5 @@
> +        if(HAVE_BINFMT_WINE_SUPPORT)
> +            add_test(NAME ${_target} COMMAND $<TARGET_FILE:${_target}> --tap)
> +        else()
> +            add_test(NAME ${_target} COMMAND ${WINE_EXECUTABLE} ${PREFIX}$<TARGET_FILE:${_target}> --tap)
> +            list(APPEND _env "DBUS_TEST_DAEMON_WRAPPER=${WINE_EXECUTABLE}")

This line looks as though it was meant to be in a separate branch? Nothing here uses DBUS_TEST_DAEMON_WRAPPER.

@@ +64,4 @@
>      list(APPEND _env "DBUS_FATAL_WARNINGS=1")
>      list(APPEND _env "DBUS_TEST_DATA=${PREFIX}${CMAKE_BINARY_DIR}/test/data")
>      list(APPEND _env "DBUS_TEST_HOMEDIR=${PREFIX}${CMAKE_BINARY_DIR}/dbus")
> +    list(APPEND _env "DBUS_TEST_EXEC=${PREFIX}${CMAKE_BINARY_DIR}/bin")

This is really an orthogonal change? It's nothing to do with Wine. (It looks OK though.)

(In reply to Simon McVittie from comment #14)
> > +            list(APPEND _env "DBUS_TEST_DAEMON_WRAPPER=${WINE_EXECUTABLE}")
> 
> This line looks as though it was meant to be in a separate branch? Nothing
> here uses DBUS_TEST_DAEMON_WRAPPER.

Or perhaps you've lost the part of your initial commit where test-utils-glib.c respected that variable (maybe into a different commit?), and it needs to come back into this commit.

(In reply to Simon McVittie from comment #14)
> > +            list(APPEND _env "DBUS_TEST_DAEMON_WRAPPER=${WINE_EXECUTABLE}")
> 
> This line looks as though it was meant to be in a separate branch? Nothing
> here uses DBUS_TEST_DAEMON_WRAPPER.
Thanks for pointing it out, which is a left over from previous patch version.
> 
> @@ +64,4 @@
> >      list(APPEND _env "DBUS_FATAL_WARNINGS=1")
> >      list(APPEND _env "DBUS_TEST_DATA=${PREFIX}${CMAKE_BINARY_DIR}/test/data")
> >      list(APPEND _env "DBUS_TEST_HOMEDIR=${PREFIX}${CMAKE_BINARY_DIR}/dbus")
> > +    list(APPEND _env "DBUS_TEST_EXEC=${PREFIX}${CMAKE_BINARY_DIR}/bin")
> 
> This is really an orthogonal change? It's nothing to do with Wine. 
Agreed, This would only be required when test-cases are running outside the build root. 

I'm going to submitted the patch without these changes. Thanks for reviewing.