Bug 105662

Summary: travis-ci: Add cross building support for mingw 64 bit compiler
Product: dbus Reporter: Ralf Habacker <ralf.habacker>
Component: coreAssignee: D-Bus Maintainers <dbus>
Status: RESOLVED FIXED QA Contact: D-Bus Maintainers <dbus>
Severity: normal    
Priority: medium CC: ralf.habacker
Version: unspecifiedKeywords: patch
Hardware: All   
OS: All   
Whiteboard: review+
i915 platform: i915 features:
Attachments: travis-ci: Add cross building support for mingw 64 bit compiler
travis-ci: Add cross building support for mingw 64 bit compiler
travis-ci: Add cross building support for mingw 64 bit compiler
Mingw 64bit compile fix
travis-ci: Add cross building support for mingw 64 bit compiler
dbus-transport-socket: Correctly print DBusSocket with DBUS_SOCKET_FORMAT
sysdeps-win: Print word-size-dependent offset correctly

Description Ralf Habacker 2018-03-21 13:44:35 UTC
Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de>
Comment 1 Ralf Habacker 2018-03-21 13:44:38 UTC
Created attachment 138244 [details] [review]
travis-ci: Add cross building support for mingw 64 bit compiler
Comment 2 Ralf Habacker 2018-03-21 14:01:18 UTC
Created attachment 138246 [details] [review]
travis-ci: Add cross building support for mingw 64 bit compiler

- remove obsolate include
Comment 3 Simon McVittie 2018-03-21 14:15:56 UTC
Comment on attachment 138244 [details] [review]
travis-ci: Add cross building support for mingw 64 bit compiler

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

::: .travis.yml
@@ +39,5 @@
> +  - ci_host=mingw32 ci_variant=debug
> +  - ci_host=mingw32 ci_buildsys=cmake
> +  - ci_host=mingw64
> +  - ci_host=mingw64 ci_variant=debug
> +  - ci_host=mingw64 ci_buildsys=cmake

I'm a bit concerned that adding three more builds is going to make Travis-CI even slower.

Perhaps we could combine some of these builds, for instance

- ci_host=mingw32
- ci_host=mingw64 ci_variant=debug
- ci_host=mingw64 ci_buildsys=cmake
- ci_host=mingw32 ci_buildsys=cmake ci_variant=debug    (maybe)

or something like that? That way we test 32- and 64-bit, we test production and debug builds, and we test CMake and Autotools, but we don't do the full combinatorial explosion.

::: cmake/x86_64-w64-mingw32.cmake
@@ +9,5 @@
> +set(CMAKE_CROSSCOMPILING TRUE)
> +set(WIN32 TRUE)
> +set(MINGW TRUE)
> +
> +include(CMakeForceCompiler)

As with Bug #105636, can this include() be removed now that you're not using the cmake_force_*_compiler macros?

::: tools/ci-build.sh
@@ +94,4 @@
>  make="make -j${ci_parallel} V=1 VERBOSE=1"
>  
>  case "$ci_host" in
> +    (mingw32)

There seems to be a lot of duplication between the 32- and 64-bit builds: a lot of the differences are just because we're cross-compiling and using MSYS libraries, not because of the word size.

I think it would make this script simpler if we used the GNU tuple as the value of ci_host (when not "native"), so ci_host=i686-w64-mingw32 or ci_host=x86_64-w64-mingw32 instead of mingw32 or mingw64? That's already what the documentation in tools/ci-install.sh says it should be - I'm not sure why I used plain "mingw" in the actual implementation.

Then we could use

case "$ci_host" in
    (*-w64-mingw32)
        ...

and

mirror=http://repo.msys2.org/mingw/${ci_host%%-*}

(the %%-* modifier removes the longest possible suffix matching the pattern -*, i.e. everything after the i686 or x86_64)

and later on

wget ${mirror}/mingw-w64-${ci_host%%-*}-${pkg}-any.pkg.tar.xz

You'd still need to use

if [ "${ci_host%%-*}" = i686 ]; then
    mingw="$(pwd)/mingw32"
else
    mingw="$(pwd)/mingw64"
fi

to set ${mingw}, but I think that's the only place where we want that form?

@@ +225,3 @@
>                  set _ "$@"
>                  set "$@" --build="$(build-aux/config.guess)"
>                  set "$@" --host=i686-w64-mingw32

... and then we could use --host="${ci_host}" here to cover both ways, which seems nicer.

@@ +305,5 @@
>                  ci_test=no
>                  ;;
> +            (mingw64)
> +                set _ "$@"
> +                set "$@" -D CMAKE_TOOLCHAIN_FILE="${srcdir}/cmake/x86_64-w64-mingw32.cmake"

Similarly we could use "${srcdir}/cmake/${ci_host}.cmake" here.

::: tools/ci-install.sh
@@ +87,5 @@
> +                $sudo dpkg --add-architecture i386
> +                ;;
> +            (mingw64)
> +                $sudo dpkg --add-architecture amd64
> +                ;;

Travis-CI machines are amd64 anyway, so we only need to add the i386 architecture to get wine:i386. We should be able to install wine:amd64 already.

@@ +105,5 @@
> +                $sudo apt-get -qq -y install \
> +                    binutils-mingw-w64-x86-64\
> +                    g++-mingw-w64-x86-64 \
> +                    wine:amd64 \
> +                    ${NULL}

This is one of the few places where you'd still need separate cases for i686-w64-mingw32 and x86_64-w64-mingw32 (and unfortunately you can't even use ${ci_host%%-*} here because it's x86-64 not x86_64 in the package name).
Comment 4 Ralf Habacker 2018-03-21 15:37:21 UTC
Created attachment 138251 [details] [review]
travis-ci: Add cross building support for mingw 64 bit compiler

- renamed mingwxx to *-w64-mingw
- reduced number of builds
Comment 5 Ralf Habacker 2018-03-21 15:59:22 UTC
(In reply to Ralf Habacker from comment #4)
> Created attachment 138251 [details] [review] [review]
> travis-ci: Add cross building support for mingw 64 bit compiler
> 
> - renamed mingwxx to *-w64-mingw
> - reduced number of builds

ci_host=x86_64-w64-mingw32 ci_variant=debug returns

../../dbus/dbus-transport-socket.c: In function ‘socket_handle_watch’:
../../dbus/dbus-transport-socket.c:1028:9: error: format ‘%Iu’ expects argument of type ‘size_t’, but argument 6 has type ‘int’ [-Werror=format=]
         _dbus_verbose ("asked to handle watch %p on fd %" DBUS_SOCKET_FORMAT " that we don't recognize\n",
         ^
Comment 6 Simon McVittie 2018-03-21 18:51:53 UTC
(In reply to Ralf Habacker from comment #5)
> ci_host=x86_64-w64-mingw32 ci_variant=debug returns
> 
> ../../dbus/dbus-transport-socket.c: In function ‘socket_handle_watch’:
> ../../dbus/dbus-transport-socket.c:1028:9: error: format ‘%Iu’ expects
> argument of type ‘size_t’, but argument 6 has type ‘int’ [-Werror=format=]

The CI has found a pre-existing bug here. Presumably nobody previously enabled verbose mode for 64-bit Windows.

        _dbus_verbose ("asked to handle watch %p on fd %" DBUS_SOCKET_FORMAT " that we don't recognize\n",
                       watch, dbus_watch_get_socket (watch));

dbus_watch_get_socket() returns an int, but this _dbus_verbose() call should really be using _dbus_watch_get_socket() to get the DBusSocket instead.
Comment 7 Ralf Habacker 2018-03-21 20:41:48 UTC
Created attachment 138260 [details] [review]
Mingw 64bit compile fix

Bug:
Comment 8 Ralf Habacker 2018-03-22 07:33:47 UTC
(In reply to Ralf Habacker from comment #7)
> Created attachment 138260 [details] [review] [review]
> Mingw 64bit compile fix

with this patch all variants are building see https://travis-ci.org/rhabacker/dbus/builds/356548791
Comment 9 Simon McVittie 2018-03-22 13:15:51 UTC
Comment on attachment 138260 [details] [review]
Mingw 64bit compile fix

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

::: dbus/dbus-transport-socket.c
@@ +1026,4 @@
>                         flags);
>        else
>          _dbus_verbose ("asked to handle watch %p on fd %" DBUS_SOCKET_FORMAT " that we don't recognize\n",
> +                       watch, _dbus_socket_printable (_dbus_watch_get_socket (watch)));

This part should be a separate commit, something like:

dbus-transport-socket: Correctly print DBusSocket with DBUS_SOCKET_FORMAT

Previously, on 64-bit Windows we were passing a 32-bit int where the format string expects a 64-bit SOCKET.
Comment 10 Simon McVittie 2018-03-22 13:22:40 UTC
Comment on attachment 138260 [details] [review]
Mingw 64bit compile fix

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

::: dbus/dbus-sysdeps-win.c
@@ +2662,4 @@
>              DPRINTF ("%3d %s", i++, pSymbol->Name);
>          }
>        else
> +        DPRINTF ("%3d 0x%" OFFSET_FORMAT, i++, sf.AddrPC.Offset);

If AddrPC.Offset is the same size as a pointer, I think you can use "%3d 0x%Ix" (with a capital i) on all Windows platforms? https://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx says the I size modifier is correct for ptrdiff_t and size_t.

Or if "%Ix" isn't suitable for some reason, adding OFFSET_FORMAT looks OK. The commit message should be more like this:

"""
sysdeps-win: Print word-size-dependent offset correctly

AddrPC.Offset is the same size as a pointer, but previously we printed it as though it was the same size as a long, which is 32 bits on 64-bit Windows.
"""

(Ideally, change that suggested commit message to mention what the type of AddrPC.Offset is - I'd guess size_t, ptrdiff_t or [u]intptr_t.)
Comment 11 Simon McVittie 2018-03-22 13:26:44 UTC
Comment on attachment 138251 [details] [review]
travis-ci: Add cross building support for mingw 64 bit compiler

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

Looks good apart from the missing ";;"

::: tools/ci-install.sh
@@ +86,5 @@
> +            (i686-w64-mingw32)
> +                $sudo dpkg --add-architecture i386
> +                ;;
> +            (x86_64-w64-mingw32)
> +                $sudo dpkg --add-architecture amd64

As I said in my previous review, you don't need to add the amd64 architecture, because Travis-CI is an amd64 system already. But if it works, I suppose it's harmless.

@@ +105,5 @@
> +                $sudo apt-get -qq -y install \
> +                    binutils-mingw-w64-x86-64\
> +                    g++-mingw-w64-x86-64 \
> +                    wine:amd64 \
> +                    ${NULL}

This should have a trailing ";;" like the other case
Comment 12 Ralf Habacker 2018-03-22 14:01:59 UTC
(In reply to Simon McVittie from comment #10)
> Comment on attachment 138260 [details] [review] [review]
> Mingw 64bit compile fix
> 
> Review of attachment 138260 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: dbus/dbus-sysdeps-win.c
> @@ +2662,4 @@
> >              DPRINTF ("%3d %s", i++, pSymbol->Name);
> >          }
> >        else
> > +        DPRINTF ("%3d 0x%" OFFSET_FORMAT, i++, sf.AddrPC.Offset);
> 
> If AddrPC.Offset is the same size as a pointer, I think you can use "%3d
> 0x%Ix" (with a capital i) on all Windows platforms?
> https://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx says the I size
> modifier is correct for ptrdiff_t and size_t.
> 
> Or if "%Ix" isn't suitable for some reason, adding OFFSET_FORMAT looks OK.
> The commit message should be more like this:
> 
> """
> sysdeps-win: Print word-size-dependent offset correctly
> 
> AddrPC.Offset is the same size as a pointer, but previously we printed it as
> though it was the same size as a long, which is 32 bits on 64-bit Windows.
> """
> 
> (Ideally, change that suggested commit message to mention what the type of
> AddrPC.Offset is - I'd guess size_t, ptrdiff_t or [u]intptr_t.)

at least with mingw header it it 

#ifdef __i386__
  sf.AddrPC.Offset    = context.Eip --> DWORD
#elif defined(_M_X64)
  sf.AddrPC.Offset    = context.Rip; -> DWORD64
#elif defined(_M_IA64)
  sf.AddrPC.Offset    = context.StIIP -> ULONGLONG
Comment 13 Ralf Habacker 2018-03-22 14:10:31 UTC
Created attachment 138283 [details] [review]
travis-ci: Add cross building support for mingw 64 bit compiler

- minor fixes
Comment 14 Ralf Habacker 2018-03-22 14:11:02 UTC
Created attachment 138284 [details] [review]
dbus-transport-socket: Correctly print DBusSocket with DBUS_SOCKET_FORMAT

Previously, on 64-bit Windows we were passing a 32-bit int where the
format string expects a 64-bit SOCKET.

- separated patch
Comment 15 Ralf Habacker 2018-03-22 14:11:27 UTC
Created attachment 138285 [details] [review]
sysdeps-win: Print word-size-dependent offset correctly

AddrPC.Offset is the same size as a pointer, but previously
we printed it as though it was the same size as a long,
which is 32 bits on 64-bit Windows.

- separated patch
Comment 16 Simon McVittie 2018-03-22 19:27:08 UTC
Comment on attachment 138283 [details] [review]
travis-ci: Add cross building support for mingw 64 bit compiler

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

Thanks, go ahead. You might want to apply the compilation fixes first (I'll review them next).
Comment 17 Simon McVittie 2018-03-22 19:27:56 UTC
Comment on attachment 138284 [details] [review]
dbus-transport-socket: Correctly print DBusSocket with DBUS_SOCKET_FORMAT

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

Go ahead
Comment 18 Simon McVittie 2018-03-22 19:28:23 UTC
Comment on attachment 138285 [details] [review]
sysdeps-win: Print word-size-dependent offset correctly

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

Yes please
Comment 19 Simon McVittie 2018-04-09 12:36:26 UTC
Ralf seems to have applied all these. Fixed in git for 1.13.4

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.