|
Description
Simon McVittie
2013-09-02 16:20:01 UTC
Created attachment 85071 [details] [review] 1/6] Allow dbus-daemon --nofork on Windows On Windows, the dbus-daemon is not able to fork (daemonize). If someone explicitly requests forking, it should fail, but if someone explicitly requests *not* forking, there seems no harm in allowing it. A few of the regression tests specifically require a dbus-daemon that will not fork, so allowing this option on Windows means those tests don't need an extra OS condition. Created attachment 85072 [details] [review] 2/6] _dbus_become_daemon: don't pretend it worked This function is meaningless (and possibly unimplementable) on Windows. We shouldn't call it; if we do, it should raise an error. Created attachment 85073 [details] [review] 3/6] corrupt test: close connection before releasing GSocket GSocket takes responsibility for closing the fd, and there doesn't seem to be any way to tell it not to. When this test is adapted to run under DBusLoop as an alternative to dbus-glib, that becomes a problem, because DBusLoop/DBusSocketSetEpoll do not tolerate that. Work around it. Created attachment 85074 [details] [review] 4/6] Define DBUS_COMPILATION externally for all tests that use internal stuff It might as well go in the AM_CPPFLAGS rather than in the source code. Created attachment 85076 [details] [review] 5/6] Tests: allow dbus-glib to be replaced with use of libdbus-internal We only use dbus-glib for its main loop; within dbus, DBusLoop is available as an alternative, although it isn't thread-safe and isn't public API. For tests that otherwise only use libdbus public API, it's desirable to be able to avoid DBusLoop, so we can run them against an installed libdbus as an integration test. However, if we don't have dbus-glib, we're going to have to use an in-tree main loop, which might as well be DBusLoop. The major disadvantage of using dbus-glib is that it isn't safe to link both dbus-1 and dbus-internal at the same time. This is awkward for a future test case that wants to use _dbus_getsid() in dbus-daemon.c, but only on Windows (fd.o #54445). If we use the same API wrapper around both dbus-glib and DBusLoop, we can compile that test against dbus-glib or against DBusLoop, depending on the platform. Created attachment 85077 [details] [review] 6/6] test/dbus-daemon, test/dbus-daemon-eavesdrop: allow external dbus-daemon It's easier to automate these tests if they launch their own dbus-daemon, but easier to debug them if they don't: you can launch a dbus-daemon separately, under gdb. However, tests that need a specially-configured dbus-daemon will have to be skipped. *** Bug 68496 has been marked as a duplicate of this bug. *** *** Bug 68498 has been marked as a duplicate of this bug. *** Created attachment 85082 [details] [review] 8/8] dbus-sysdeps-win.h: do not redefine _inline if not already defined mingw-w64 3.0~svn5915 defines it to __inline, provoking a redefinition warning. It looks as though this was probably just a workaround for a broken mingw version of wspiapi.h anyway? --- Vaguely related. This + attachment 85080 [details] [review] from Bug #68610 get me a version of dbus that compiles successfully under mingw-w64, although dbus-daemon crashes under wine. Comment on attachment 85071 [details] [review] 1/6] Allow dbus-daemon --nofork on Windows looks good Comment on attachment 85072 [details] [review] 2/6] _dbus_become_daemon: don't pretend it worked Review of attachment 85072 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 85073 [details] [review] 3/6] corrupt test: close connection before releasing GSocket Review of attachment 85073 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 85074 [details] [review] 4/6] Define DBUS_COMPILATION externally for all tests that use internal stuff Review of attachment 85074 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 85076 [details] [review] 5/6] Tests: allow dbus-glib to be replaced with use of libdbus-internal Review of attachment 85076 [details] [review]: ----------------------------------------------------------------- looks good BTW: using dbus-glib has also the disadvantage of a dependency lock - dbus is required for building dbus-glib, which is required to build dbus test apps. (In reply to comment #14) > BTW: using dbus-glib has also the disadvantage of a dependency lock - dbus > is required for building dbus-glib, which is required to build dbus test > apps. Yes, although sacrificing full test coverage/generality for a "bootstrap" build is pretty common. Thanks, patches 1-5 merged. There are a couple more unreviewed, and I'm about to add a couple of new ones. Created attachment 85109 [details] [review] run-with-tmp-session-bus.sh: create a unique temporary file per process This makes the regression tests OK to run in parallel. Created attachment 85110 [details] [review] Add CPPFLAGS to "shared if possible" test binaries (In reply to comment #17) > Created attachment 85110 [details] [review] > Add CPPFLAGS to "shared if possible" test binaries ... for which I forgot to quote this long commit message: In principle we ought to define DBUS_STATIC_BUILD in anything that's using libdbus-internal.la (to avoid linking failures on statically-linked mingw builds), and DBUS_TEST_USE_INTERNAL in any test that's using the non-dbus-glib code paths of test-utils.[ch] (to avoid the GLib requirement, although in practice, everything "shared if possible" requires GLib at the moment anyway). Comment on attachment 85077 [details] [review] 6/6] test/dbus-daemon, test/dbus-daemon-eavesdrop: allow external dbus-daemon Review of attachment 85077 [details] [review]: ----------------------------------------------------------------- look good ::: test/dbus-daemon.c @@ +200,5 @@ > + "unusally-configured dbus-daemon"); > + f->skip = TRUE; > + return; > + } > + You may print out the used config file to allow users to run related daemon by hand. Comment on attachment 85082 [details] [review] 8/8] dbus-sysdeps-win.h: do not redefine _inline if not already defined Review of attachment 85082 [details] [review]: ----------------------------------------------------------------- This has been introduced in 2008 by this commit: http://cgit.freedesktop.org/dbus/dbus/commit/?id=452ff68a2d81beb55c43713bc226a51b7c3d38a7 "... check for wspiapi.h presence which isn't available (and unneeded) in mingw32" Comment on attachment 85109 [details] [review] run-with-tmp-session-bus.sh: create a unique temporary file per process Review of attachment 85109 [details] [review]: ----------------------------------------------------------------- looks good Comment on attachment 85110 [details] [review] Add CPPFLAGS to "shared if possible" test binaries Review of attachment 85110 [details] [review]: ----------------------------------------------------------------- looks good (In reply to comment #20) > 8/8] dbus-sysdeps-win.h: do not redefine _inline if not already defined > This has been introduced in 2008 by this commit: > http://cgit.freedesktop.org/dbus/dbus/commit/ > ?id=452ff68a2d81beb55c43713bc226a51b7c3d38a7 > > "... check for wspiapi.h presence which isn't available (and unneeded) in > mingw32" I see. So the patch submitter probably didn't actually use the __GNUC__ case, since at the time, mingw32 didn't have that header? But now I get into that case (with mingw-w64), and it warns (because "_inline" was already defined to "inline", and we're redefining it as ""). If defining _inline is a workaround for something that used it without defining it first (which I assume it must have been), then making it conditional seems right; or we could delete it altogether, if you'd prefer? (In reply to comment #23) > (In reply to comment #20) > > 8/8] dbus-sysdeps-win.h: do not redefine _inline if not already defined > > > This has been introduced in 2008 by this commit: > > http://cgit.freedesktop.org/dbus/dbus/commit/ > > ?id=452ff68a2d81beb55c43713bc226a51b7c3d38a7 > > > > "... check for wspiapi.h presence which isn't available (and unneeded) in > > mingw32" > > I see. So the patch submitter probably didn't actually use the __GNUC__ > case, since at the time, mingw32 didn't have that header? But now I get into > that case (with mingw-w64), and it warns (because "_inline" was already > defined to "inline", and we're redefining it as ""). > > If defining _inline is a workaround for something that used it without > defining it first (which I assume it must have been), then making it > conditional seems right; or we could delete it altogether, if you'd prefer? I think we can remove this because: 1. the related bug is mentioned at http://sourceforge.net/p/mingw/bugs/1636/ and seems to affect w2k only which is not supported by dbus. 2. mingw-w64 uses __CRT_INLINE, _inline is never used. (In reply to comment #24) > I think we can remove this because: > > 1. the related bug is mentioned at http://sourceforge.net/p/mingw/bugs/1636/ > and seems to affect w2k only which is not supported by dbus. > > 2. mingw-w64 uses __CRT_INLINE, _inline is never used. Thanks for researching this. I'll check that we define WINVER to at least 0x501, and if so, do a new patch that deletes the inclusion of wspiapi.h. (In reply to comment #19) > You may print out the used config file to allow users to run related daemon > by hand. If the dbus-daemon has a weird configuration, it'll be OK for the test that assumes a weird configuration, but not for the other tests (which don't)... I'd rather keep it simple. Created attachment 85240 [details] [review] dbus-sysdeps-win: don't include wspiapi.h This block provoked a warning on mingw-w64 because we were redefining _inline. According to Ralf's research, it was introduced in 452ff68a: Windows 2000 doesn't have getaddrinfo and related functions in ws2tcpip.h, but does have a shim implementation in wspiapi.h. At the time of 452ff68a, mingw32 didn't have wspiapi.h, so it's unclear why there was a __GNUC__ code path here. The "#define _inline" on that code path looks likely to be some sort of workaround for a faulty version of wspiapi.h? Current mingw-w64 does have wspiapi.h, so we enter the __GNUC__ code path and get the redefinition. dbus no longer supports Windows 2000, so we no longer need wspiapi.h at all, and can rely on XP or later. (Ralf's policy is to only support versions of Windows that are still supported by Microsoft, and Windows 2000 reached the end of its life-cycle in 2010.) (In reply to comment #27) > dbus-sysdeps-win: don't include wspiapi.h Or if you'd prefer to just remove the "#define _inline", that would be fine too. Comment on attachment 85109 [details] [review] run-with-tmp-session-bus.sh: create a unique temporary file per process Committed to master, thanks. I might also pull this into 1.6 since being unable to run "make -j4 check" is pretty annoying. Comment on attachment 85110 [details] [review] Add CPPFLAGS to "shared if possible" test binaries committed Created attachment 85604 [details] [review] [PATCH] Remove DBUS_COMPILATION from test source code This is a complemental patch of "4/6] Define DBUS_COMPILATION externally for all tests that use internal stuff". Which left a DBUS_COMPILATION in test/break-loader.c Comment on attachment 85082 [details] [review] 8/8] dbus-sysdeps-win.h: do not redefine _inline if not already defined Review of attachment 85082 [details] [review]: ----------------------------------------------------------------- looks good. Comment on attachment 85604 [details] [review] [PATCH] Remove DBUS_COMPILATION from test source code (In reply to comment #31) > [PATCH] Remove DBUS_COMPILATION from test source code > > This is a complemental patch of "4/6] Define DBUS_COMPILATION externally for > all tests that use internal stuff". Which left a DBUS_COMPILATION in > test/break-loader.c Applied, thanks (In reply to comment #32) > Comment on attachment 85082 [details] [review] > 8/8] dbus-sysdeps-win.h: do not redefine _inline if not already defined > > looks good. Thanks, but I'm not going to apply this if Ralf objects. We can either take this or Attachment #85240 [details]. (In reply to comment #27) > Created attachment 85240 [details] [review] [review] > dbus-sysdeps-win: don't include wspiapi.h > looks good (In reply to comment #34) > (In reply to comment #32) > > Comment on attachment 85082 [details] [review] [review] > > 8/8] dbus-sysdeps-win.h: do not redefine _inline if not already defined > > > > looks good. > > Thanks, but I'm not going to apply this if Ralf objects. We can either take > this or Attachment #85240 [details]. Both patches looks good and are working as expected. Because w2k isn't supported, as simon stated, #85240 fits better and should be choosed. (In reply to comment #36) > (In reply to comment #34) > > (In reply to comment #32) > > > Comment on attachment 85082 [details] [review] [review] [review] > > > 8/8] dbus-sysdeps-win.h: do not redefine _inline if not already defined > > > > > > looks good. > > > > Thanks, but I'm not going to apply this if Ralf objects. We can either take > > this or Attachment #85240 [details]. > > Both patches looks good and are working as expected. Because w2k isn't > supported, as simon stated, #85240 fits better and should be choosed. Just for the record: http://www.freedesktop.org/wiki/Software/dbus/#index6h1 mentioned windows xp as lowest supported windows os. Comment on attachment 85240 [details] [review] dbus-sysdeps-win: don't include wspiapi.h applied, thanks Comment on attachment 85082 [details] [review] 8/8] dbus-sysdeps-win.h: do not redefine _inline if not already defined rejected, Attachment #85240 [details] was applied instead (In reply to comment #6) > Created attachment 85077 [details] [review] > 6/6] test/dbus-daemon, test/dbus-daemon-eavesdrop: allow external > dbus-daemon > > It's easier to automate these tests if they launch their own > dbus-daemon, but easier to debug them if they don't: you can launch > a dbus-daemon separately, under gdb. However, tests that need a > specially-configured dbus-daemon will have to be skipped. This is all that's left here. I'd like to apply this so I can gdb the dbus-daemon more easily, but if the other maintainers don't want this, we can ignore it and close this bug RESOLVED FIXED. Comment on attachment 85077 [details] [review] 6/6] test/dbus-daemon, test/dbus-daemon-eavesdrop: allow external dbus-daemon Review of attachment 85077 [details] [review]: ----------------------------------------------------------------- looks good (In reply to comment #41) > Comment on attachment 85077 [details] [review] [review] > 6/6] test/dbus-daemon, test/dbus-daemon-eavesdrop: allow external > dbus-daemon > > Review of attachment 85077 [details] [review] [review]: > ----------------------------------------------------------------- > > looks good (In reply to comment #41) > Comment on attachment 85077 [details] [review] [review] > 6/6] test/dbus-daemon, test/dbus-daemon-eavesdrop: allow external > dbus-daemon > > Review of attachment 85077 [details] [review] [review]: > ----------------------------------------------------------------- > > looks good applied to master, will be available in 1.7.12 |
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.