Bug 88966 - fix for running cmake cross compiled test executables without binfmt support
Summary: fix for running cmake cross compiled test executables without binfmt support
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other Windows (All)
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 93976
  Show dependency treegraph
 
Reported: 2015-02-04 13:24 UTC by Ralf Habacker
Modified: 2016-02-08 22:16 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Add running cross compiled application without the (3.73 KB, patch)
2015-02-04 13:24 UTC, Ralf Habacker
Details | Splinter Review
Do not require binfmt support for running cross compiled test applications (update 1) (3.82 KB, patch)
2015-11-10 10:25 UTC, Ralf Habacker
Details | Splinter Review
Do not require binfmt support for running cross compiled test applications. (3.89 KB, patch)
2015-12-07 11:50 UTC, Ralf Habacker
Details | Splinter Review
Do not require binfmt support for running cross compiled test applications. (3.30 KB, patch)
2015-12-29 21:21 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 2015-02-04 13:24:20 UTC
Created attachment 113158 [details] [review]
Add running cross compiled application without the

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.
Comment 1 Simon McVittie 2015-02-05 12:57:47 UTC
(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 2 Simon McVittie 2015-02-05 13:02:31 UTC
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.
Comment 3 Ralf Habacker 2015-11-10 10:25:09 UTC
Created attachment 119535 [details] [review]
Do not require binfmt support for running cross compiled test applications (update 1)

rebased to dbus-1.10
Comment 4 Ralf Habacker 2015-11-10 10:43:04 UTC
(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.
Comment 5 Ralf Habacker 2015-11-29 09:17:05 UTC
(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 ?
Comment 6 Ralf Habacker 2015-12-07 11:50:26 UTC
Created attachment 120385 [details] [review]
Do not require binfmt support for running cross compiled test applications.

Bug:
Comment 7 Ralf Habacker 2015-12-07 11:50:49 UTC
Comment on attachment 119535 [details] [review]
Do not require binfmt support for running cross compiled test applications (update 1)

obsolate
Comment 8 Ralf Habacker 2015-12-07 11:51:33 UTC
(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.
Comment 9 Ralf Habacker 2015-12-07 12:30:25 UTC
(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.
Comment 10 Ralf Habacker 2015-12-18 22:28:08 UTC
(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.
Comment 11 Ralf Habacker 2015-12-29 21:21:22 UTC
Created attachment 120731 [details] [review]
Do not require binfmt support for running cross compiled test applications.

Bug:
Comment 12 Ralf Habacker 2015-12-29 21:23:49 UTC
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.
Comment 13 Ralf Habacker 2016-02-03 07:58:06 UTC
okay for commit to master ? I'm blocked to work on bug 93976, which needs to patch the same area.
Comment 14 Simon McVittie 2016-02-08 18:01:08 UTC
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.)
Comment 15 Simon McVittie 2016-02-08 18:04:00 UTC
(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.
Comment 16 Ralf Habacker 2016-02-08 22:16:25 UTC
(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.


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.