Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de>
Created attachment 138244 [details] [review] travis-ci: Add cross building support for mingw 64 bit compiler
Created attachment 138246 [details] [review] travis-ci: Add cross building support for mingw 64 bit compiler - remove obsolate include
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).
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
(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", ^
(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.
Created attachment 138260 [details] [review] Mingw 64bit compile fix Bug:
(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 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 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 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
(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
Created attachment 138283 [details] [review] travis-ci: Add cross building support for mingw 64 bit compiler - minor fixes
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
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 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 on attachment 138284 [details] [review] dbus-transport-socket: Correctly print DBusSocket with DBUS_SOCKET_FORMAT Review of attachment 138284 [details] [review]: ----------------------------------------------------------------- Go ahead
Comment on attachment 138285 [details] [review] sysdeps-win: Print word-size-dependent offset correctly Review of attachment 138285 [details] [review]: ----------------------------------------------------------------- Yes please
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.