Bug 40711 - fix trivia in the build system, including unnecessary and wrong abstract-socket check
Summary: fix trivia in the build system, including unnecessary and wrong abstract-sock...
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: GLib (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: John (J5) Palmieri
URL: http://cgit.freedesktop.org/~smcv/dbu...
Whiteboard:
Keywords: patch
: 56300 (view as bug list)
Depends on: 40151
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-08 04:23 UTC by Marvin Schmidt
Modified: 2012-12-04 17:43 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
[01/14] Remove EXPANDED_LOCALSTATEDIR etc., no longer needed (2.45 KB, patch)
2011-09-13 08:47 UTC, Simon McVittie
Details | Splinter Review
[02/14] Use standard autotools @abs_top_builddir@ to replace TEST_SERVICE_BINARY etc. (3.88 KB, patch)
2011-09-13 08:47 UTC, Simon McVittie
Details | Splinter Review
[03/14] Remove unused support for "verbose mode" (3.71 KB, patch)
2011-09-13 08:48 UTC, Simon McVittie
Details | Splinter Review
[04/14] Remove remnants of DBUS_DISABLE_ASSERT (1.16 KB, patch)
2011-09-13 08:48 UTC, Simon McVittie
Details | Splinter Review
[05/14] Remove remnants of DBUS_DISABLE_CHECKS (747 bytes, patch)
2011-09-13 08:48 UTC, Simon McVittie
Details | Splinter Review
[06/14] Move AC_ARG_ENABLE for checks/assertions closer to where it takes effect (2.34 KB, patch)
2011-09-13 08:49 UTC, Simon McVittie
Details | Splinter Review
[07/14] Remove --with-socket-dir, use /tmp for its only use (1.89 KB, patch)
2011-09-13 08:49 UTC, Simon McVittie
Details | Splinter Review
[08/14] Remove check for abstract sockets, not relevant to dbus-glib (2.38 KB, patch)
2011-09-13 08:50 UTC, Simon McVittie
Details | Splinter Review
[09/14] Turn _dbus_glib_test into a standalone test case, since it only tests public API (6.04 KB, patch)
2011-09-13 08:51 UTC, Simon McVittie
Details | Splinter Review
[10/14] Use more modern assertions in errors test (902 bytes, patch)
2011-09-13 08:51 UTC, Simon McVittie
Details | Splinter Review
[11/14] _dbus_gvalue_utils_test: don't call private API (833 bytes, patch)
2011-09-13 08:51 UTC, Simon McVittie
Details | Splinter Review
[12/14] Move _dbus_gvalue_utils_test into a separate binary, it only uses public API (28.44 KB, patch)
2011-09-13 08:52 UTC, Simon McVittie
Details | Splinter Review
[13/14] Flatten _dbus_gmain_test into _dbus_gvalue_test (4.57 KB, patch)
2011-09-13 08:52 UTC, Simon McVittie
Details | Splinter Review
[14/14] Remove declaration of _dbus_gutils_test, which does not exist (775 bytes, patch)
2011-09-13 08:53 UTC, Simon McVittie
Details | Splinter Review
[8/14 v2] Remove check for abstract sockets, not relevant to dbus-glib (2.90 KB, patch)
2011-11-03 05:51 UTC, Simon McVittie
Details | Splinter Review
[9/14 v2] Turn _dbus_glib_test into a standalone test case, since it only tests public API (6.04 KB, patch)
2011-11-03 05:53 UTC, Simon McVittie
Details | Splinter Review
[13/14 v2] Flatten _dbus_gmain_test into _dbus_gvalue_test (4.57 KB, patch)
2011-11-03 05:55 UTC, Simon McVittie
Details | Splinter Review
[v3] Remove check for abstract sockets, not relevant to dbus-glib (5.30 KB, patch)
2012-11-19 16:14 UTC, Simon McVittie
Details | Splinter Review
test-profile: actually initialize GLib, if required (964 bytes, patch)
2012-11-19 16:14 UTC, Simon McVittie
Details | Splinter Review
test-profile: exit a bit more gracefully (1.73 KB, patch)
2012-11-19 16:14 UTC, Simon McVittie
Details | Splinter Review

Description Marvin Schmidt 2011-09-08 04:23:49 UTC
The same problem was reported against core in https://bugs.freedesktop.org/show_bug.cgi?id=29895

Attaching identical patch to fix the issue
Comment 1 Simon McVittie 2011-09-13 08:45:56 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.
Comment 2 Simon McVittie 2011-09-13 08:47:04 UTC
Created attachment 51134 [details] [review]
[01/14] Remove EXPANDED_LOCALSTATEDIR etc., no longer needed
Comment 3 Simon McVittie 2011-09-13 08:47:26 UTC
Created attachment 51135 [details] [review]
[02/14] Use standard autotools @abs_top_builddir@ to replace  TEST_SERVICE_BINARY etc.
Comment 4 Simon McVittie 2011-09-13 08:48:02 UTC
Created attachment 51136 [details] [review]
[03/14] Remove unused support for "verbose mode"
Comment 5 Simon McVittie 2011-09-13 08:48:29 UTC
Created attachment 51137 [details] [review]
[04/14] Remove remnants of DBUS_DISABLE_ASSERT
Comment 6 Simon McVittie 2011-09-13 08:48:54 UTC
Created attachment 51138 [details] [review]
[05/14] Remove remnants of DBUS_DISABLE_CHECKS
Comment 7 Simon McVittie 2011-09-13 08:49:21 UTC
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.
Comment 8 Simon McVittie 2011-09-13 08:49:47 UTC
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.
Comment 9 Simon McVittie 2011-09-13 08:50:23 UTC
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.
Comment 10 Simon McVittie 2011-09-13 08:51:05 UTC
Created attachment 51142 [details] [review]
[09/14] Turn _dbus_glib_test into a standalone test case,  since it only tests public API
Comment 11 Simon McVittie 2011-09-13 08:51:29 UTC
Created attachment 51144 [details] [review]
[10/14] Use more modern assertions in errors test
Comment 12 Simon McVittie 2011-09-13 08:51:52 UTC
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.
Comment 13 Simon McVittie 2011-09-13 08:52:19 UTC
Created attachment 51146 [details] [review]
[12/14] Move _dbus_gvalue_utils_test into a separate binary,  it only uses public API
Comment 14 Simon McVittie 2011-09-13 08:52:56 UTC
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.
Comment 15 Simon McVittie 2011-09-13 08:53:15 UTC
Created attachment 51148 [details] [review]
[14/14] Remove declaration of _dbus_gutils_test, which does  not exist
Comment 16 Simon McVittie 2011-11-03 05:51:49 UTC
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.
Comment 17 Simon McVittie 2011-11-03 05:53:56 UTC
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.
Comment 18 Simon McVittie 2011-11-03 05:55:05 UTC
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.
Comment 19 Simon McVittie 2012-04-16 04:27:44 UTC
(Cannot be merged until Bug #40151 is; rebases cleanly onto that branch.)
Comment 20 Dan Williams 2012-10-22 21:39:36 UTC
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 21 Dan Williams 2012-10-22 21:52:41 UTC
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 22 Dan Williams 2012-10-22 21:57:15 UTC
Comment on attachment 51136 [details] [review]
[03/14] Remove unused support for "verbose mode"

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

Looks good.
Comment 23 Dan Williams 2012-10-22 21:58:42 UTC
Comment on attachment 51137 [details] [review]
[04/14] Remove remnants of DBUS_DISABLE_ASSERT

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

Looks good.
Comment 24 Dan Williams 2012-10-22 21:59:49 UTC
Comment on attachment 51138 [details] [review]
[05/14] Remove remnants of DBUS_DISABLE_CHECKS

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

also looks good.
Comment 25 Dan Williams 2012-10-22 22:02:34 UTC
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 26 Dan Williams 2012-10-22 22:25:59 UTC
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 27 Dan Williams 2012-10-22 22:28:50 UTC
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 28 Dan Williams 2012-10-22 22:31:10 UTC
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 29 Dan Williams 2012-10-22 22:32:46 UTC
Comment on attachment 51144 [details] [review]
[10/14] Use more modern assertions in errors test

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

Looks good.
Comment 30 Dan Williams 2012-10-22 22:34:13 UTC
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 31 Dan Williams 2012-10-22 22:38:08 UTC
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 32 Dan Williams 2012-10-22 22:40:59 UTC
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 33 Dan Williams 2012-10-22 22:43:44 UTC
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 34 Dan Williams 2012-10-22 22:44:20 UTC
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.
Comment 35 Simon McVittie 2012-11-19 16:14:07 UTC
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.
Comment 36 Simon McVittie 2012-11-19 16:14:27 UTC
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.
Comment 37 Simon McVittie 2012-11-19 16:14:50 UTC
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).
Comment 38 Simon McVittie 2012-11-19 16:19:58 UTC
(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 39 Dan Williams 2012-12-04 17:39:06 UTC
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.
Comment 40 Dan Williams 2012-12-04 17:40:08 UTC
*** Bug 56300 has been marked as a duplicate of this bug. ***
Comment 41 Dan Williams 2012-12-04 17:41:14 UTC
(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 42 Dan Williams 2012-12-04 17:41:33 UTC
Comment on attachment 70263 [details] [review]
test-profile: actually initialize GLib, if required

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

Looks good.
Comment 43 Dan Williams 2012-12-04 17:42:18 UTC
Comment on attachment 70264 [details] [review]
test-profile: exit a bit more gracefully

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

Looks good.
Comment 44 Simon McVittie 2012-12-04 17:43:46 UTC
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.