|
Description
Marvin Schmidt
2011-09-08 04:23:49 UTC
You haven't actually attached a patch, but the abstract-namespace check is pointless in dbus-glib anyway: the result isn't used for anything. My configureac branch fixes this and various other obsolete things: thank you for reminding me to get it into Bugzilla. Created attachment 51134 [details] [review] [01/14] Remove EXPANDED_LOCALSTATEDIR etc., no longer needed Created attachment 51135 [details] [review] [02/14] Use standard autotools @abs_top_builddir@ to replace TEST_SERVICE_BINARY etc. Created attachment 51136 [details] [review] [03/14] Remove unused support for "verbose mode" Created attachment 51137 [details] [review] [04/14] Remove remnants of DBUS_DISABLE_ASSERT Created attachment 51138 [details] [review] [05/14] Remove remnants of DBUS_DISABLE_CHECKS Created attachment 51139 [details] [review] [06/14] Move AC_ARG_ENABLE for checks/assertions closer to where it takes effect The command line is parsed early on anyway. Created attachment 51140 [details] [review] [07/14] Remove --with-socket-dir, use /tmp for its only use test-profile.c is not run by default anyway, and it hard-codes the use of Unix sockets which isn't portable off Unix. If you have Unix sockets but not /tmp, then your platform has worse problems than inability to run all dbus-glib tests. Created attachment 51141 [details] [review] [08/14] Remove check for abstract sockets, not relevant to dbus-glib In passing, this fixes this bug as filed. Created attachment 51142 [details] [review] [09/14] Turn _dbus_glib_test into a standalone test case, since it only tests public API Created attachment 51144 [details] [review] [10/14] Use more modern assertions in errors test Created attachment 51145 [details] [review] [11/14] _dbus_gvalue_utils_test: don't call private API dbus_g_type_specialized_init should be, and is, sufficient. Created attachment 51146 [details] [review] [12/14] Move _dbus_gvalue_utils_test into a separate binary, it only uses public API Created attachment 51147 [details] [review] [13/14] Flatten _dbus_gmain_test into _dbus_gvalue_test It actually only tests _dbus_gtype_from_signature and related things, and has nothing to do with main loop integration. Created attachment 51148 [details] [review] [14/14] Remove declaration of _dbus_gutils_test, which does not exist Created attachment 53106 [details] [review] [8/14 v2] Remove check for abstract sockets, not relevant to dbus-glib --- v2: also remove from Android.mk, added by Bug #42532. Created attachment 53107 [details] [review] [9/14 v2] Turn _dbus_glib_test into a standalone test case, since it only tests public API --- The deleted code was trivially modified (a comment change) in master. Created attachment 53108 [details] [review] [13/14 v2] Flatten _dbus_gmain_test into _dbus_gvalue_test It actually only tests _dbus_gtype_from_signature and related things, and has nothing to do with main loop integration. --- The deleted code was trivially modified (a comment change) in master. (Cannot be merged until Bug #40151 is; rebases cleanly onto that branch.) Comment on attachment 51134 [details] [review] [01/14] Remove EXPANDED_LOCALSTATEDIR etc., no longer needed Review of attachment 51134 [details] [review]: ----------------------------------------------------------------- Looks good; nothing uses this anymore in the source tree. Comment on attachment 51135 [details] [review] [02/14] Use standard autotools @abs_top_builddir@ to replace TEST_SERVICE_BINARY etc. Review of attachment 51135 [details] [review]: ----------------------------------------------------------------- Looks good; passes 'make distclean; make distcheck' too. Comment on attachment 51136 [details] [review] [03/14] Remove unused support for "verbose mode" Review of attachment 51136 [details] [review]: ----------------------------------------------------------------- Looks good. Comment on attachment 51137 [details] [review] [04/14] Remove remnants of DBUS_DISABLE_ASSERT Review of attachment 51137 [details] [review]: ----------------------------------------------------------------- Looks good. Comment on attachment 51138 [details] [review] [05/14] Remove remnants of DBUS_DISABLE_CHECKS Review of attachment 51138 [details] [review]: ----------------------------------------------------------------- also looks good. Comment on attachment 51139 [details] [review] [06/14] Move AC_ARG_ENABLE for checks/assertions closer to where it takes effect Review of attachment 51139 [details] [review]: ----------------------------------------------------------------- Looks good. Comment on attachment 51140 [details] [review] [07/14] Remove --with-socket-dir, use /tmp for its only use Review of attachment 51140 [details] [review]: ----------------------------------------------------------------- Looks good; the testcase appears to pass when run manually except that it dies in dbus_shutdown() as it gets disconnected from the bus for some reason. But the tests that use the socket in /tmp pass, which for the purposes of this patch is all that matters. Comment on attachment 53106 [details] [review] [8/14 v2] Remove check for abstract sockets, not relevant to dbus-glib Review of attachment 53106 [details] [review]: ----------------------------------------------------------------- test-profile appears to use HAVE_ABSTRACT_SOCKETS in a couple of places; so if we really do want to remove this bit, we'd better update test-profile.c too. So NAK for now. Comment on attachment 53107 [details] [review] [9/14 v2] Turn _dbus_glib_test into a standalone test case, since it only tests public API Review of attachment 53107 [details] [review]: ----------------------------------------------------------------- Looks good. Comment on attachment 51144 [details] [review] [10/14] Use more modern assertions in errors test Review of attachment 51144 [details] [review]: ----------------------------------------------------------------- Looks good. Comment on attachment 51145 [details] [review] [11/14] _dbus_gvalue_utils_test: don't call private API Review of attachment 51145 [details] [review]: ----------------------------------------------------------------- Looks good. Comment on attachment 51146 [details] [review] [12/14] Move _dbus_gvalue_utils_test into a separate binary, it only uses public API Review of attachment 51146 [details] [review]: ----------------------------------------------------------------- specialized-types.c: In function ‘test_specialized_hash_2’: specialized-types.c:94:17: warning: variable ‘realval’ set but not used [-Wunused-but-set-variable] specialized-types.c: In function ‘test_ao_slist’: specialized-types.c:263:18: warning: unused variable ‘obj’ [-Wunused-variable] specialized-types.c:276:18: warning: unused variable ‘obj’ [-Wunused-variable] specialized-types.c:290:18: warning: unused variable ‘obj’ [-Wunused-variable] If you don't care about those, then this patch looks good. Test passes, anyway. Comment on attachment 51146 [details] [review] [12/14] Move _dbus_gvalue_utils_test into a separate binary, it only uses public API Review of attachment 51146 [details] [review]: ----------------------------------------------------------------- Hmm, disregard the compiler warnings, the code looks good. The compiler apparently doesn't think some of the glib macros (like G_IS_OBJECT or G_VALUE_HOLDS) possibly due to optimization. (gcc version 4.7.2 20120921 (Red Hat 4.7.2-2)). Patch is good. Comment on attachment 53108 [details] [review] [13/14 v2] Flatten _dbus_gmain_test into _dbus_gvalue_test Review of attachment 53108 [details] [review]: ----------------------------------------------------------------- Looks good; though bonus points for g_assert_cmpstr() conversion at the same time if you're an overachiever. Comment on attachment 51148 [details] [review] [14/14] Remove declaration of _dbus_gutils_test, which does not exist Review of attachment 51148 [details] [review]: ----------------------------------------------------------------- Looks good. Created attachment 70262 [details] [review] [v3] Remove check for abstract sockets, not relevant to dbus-glib test-profile.c was its only use, and all it was used for was to listen on an abstract or path-based socket, matching what D-Bus would do, when obtaining comparative performance figures for D-Bus vs. plain Unix sockets. test-profile.c isn't normally run, only works on Unix, and the check for abstract sockets was broken on current glibc (fd.o #29895), leading to us using the non-abstract code path anyway, so this clearly wasn't very important. I'm tempted to delete test-profile.c entirely, but until then, let's just make it use path-based sockets. --- I merged the rest for 0.102, thanks. Created attachment 70263 [details] [review] test-profile: actually initialize GLib, if required It's alarming how often the phrase "I'm amazed this ever worked" comes up while maintaining dbus-glib. Created attachment 70264 [details] [review] test-profile: exit a bit more gracefully We close the connection to ourselves, resulting in libdbus shooting us in the head unless we ask it not to. Nice to see how thoroughly this code has been tested... For future reference, it can be tested like this: DBUS_TOP_BUILDDIR=$(pwd) ./test/core/run-test.sh profile (or substitute the real absolute top build directory, if out-of-tree). (In reply to comment #33) > bonus points for g_assert_cmpstr() conversion at the same > time if you're an overachiever. I think applying any patches to dbus-glib at all is more involvement than it deserves :-P I wouldn't object to a patch to do this, but I don't think it's important. We can improve the assertions if any of them start failing. Regarding the new patches here, I would also accept a patch that deleted test-profile.c entirely. It's not clear to me whether it's actually any use. If anyone cared about optimizing libdbus' (de)serialization layer, it might possibly be useful as an indication of the fastest it could possibly be, but such people can easily retrieve it from git history. Comment on attachment 70262 [details] [review] [v3] Remove check for abstract sockets, not relevant to dbus-glib Review of attachment 70262 [details] [review]: ----------------------------------------------------------------- Looks good. *** Bug 56300 has been marked as a duplicate of this bug. *** (In reply to comment #36) > Created attachment 70263 [details] [review] [review] > test-profile: actually initialize GLib, if required > > It's alarming how often the phrase "I'm amazed this ever worked" comes > up while maintaining dbus-glib. Ran into this and the next one while testing, and I"d filed bug 56300 about it, which you've helpfully fixed now. Same for the shutdown issue, so 56300 is duped here. Comment on attachment 70263 [details] [review] test-profile: actually initialize GLib, if required Review of attachment 70263 [details] [review]: ----------------------------------------------------------------- Looks good. Comment on attachment 70264 [details] [review] test-profile: exit a bit more gracefully Review of attachment 70264 [details] [review]: ----------------------------------------------------------------- Looks good. All fixed in git for 0.102, thanks. |
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.