GNOME Bugzilla – Bug 687441
ABI break in master: g_menu_attribute_hash_iter_get_type, g_menu_link_hash_iter_get_type removed
Last modified: 2012-11-19 21:17:29 UTC
The commit "build: Prototype GType accessors for private classes" moved the linkage of g_menu_link_hash_iter_get_type and g_menu_attribute_hash_iter_get_type from external (the default linkage for a definition with no prior declaration) to static. This causes abicheck.sh to fail - the symbols have disappeared from expected-abi. Both symbols were already visible as ABI in GIO 2.34.1: % objdump -Tx /usr/lib/x86_64-linux-gnu/libgio-2.0.so.0|grep 'g_menu.*hash_iter' 00000000000a4dd0 g DF .text 0000000000000090 Base g_menu_link_hash_iter_get_type 00000000000a4aa0 g DF .text 0000000000000090 Base g_menu_attribute_hash_iter_get_type Possible resolutions are: * make them external again * declare that they were never meant to be ABI, remove them from gio.symbols, and note their removal visibly enough that distributions that track individual symbols in libraries' ABIs (e.g. Debian) know that their removal is considered to be OK
Making them external again would look like this: diff --git a/gio/gmenumodel.c b/gio/gmenumodel.c index f9cd826..34285e5 100644 --- a/gio/gmenumodel.c +++ b/gio/gmenumodel.c @@ -195,7 +195,9 @@ typedef struct typedef GMenuLinkIterClass GMenuLinkHashIterClass; -static GType g_menu_link_hash_iter_get_type (void); +/* Strictly speaking, this is (unintentionally) ABI, but don't use it + * outside GIO. */ +GType g_menu_link_hash_iter_get_type (void); G_DEFINE_TYPE (GMenuLinkHashIter, g_menu_link_hash_iter, G_TYPE_MENU_LINK_ITER) @@ -251,7 +253,9 @@ typedef struct typedef GMenuAttributeIterClass GMenuAttributeHashIterClass; -static GType g_menu_attribute_hash_iter_get_type (void); +/* Strictly speaking, this is (unintentionally) ABI, but don't use it + * outside GIO. */ +GType g_menu_attribute_hash_iter_get_type (void); G_DEFINE_TYPE (GMenuAttributeHashIter, g_menu_attribute_hash_iter, G_TYPE_MENU_ATTRIBUTE_ITER)
and the other option is this, plus some NEWS: diff --git a/gio/gio.symbols b/gio/gio.symbols index b2a95b2..b1808f2 100644 --- a/gio/gio.symbols +++ b/gio/gio.symbols @@ -1680,7 +1680,6 @@ g_menu_append g_menu_append_item g_menu_append_section g_menu_append_submenu -g_menu_attribute_hash_iter_get_type g_menu_attribute_iter_get_name g_menu_attribute_iter_get_next g_menu_attribute_iter_get_type @@ -1709,7 +1708,6 @@ g_menu_item_set_label g_menu_item_set_link g_menu_item_set_section g_menu_item_set_submenu -g_menu_link_hash_iter_get_type g_menu_link_iter_get_name g_menu_link_iter_get_next g_menu_link_iter_get_type
I'd prefer to make them public. But this is *exactly* the kind of fuckup that the new CFLAGS warnings are intended to prevent. Patch incoming.
Actually...we should switch glib to actually *require* a symbol to be listed in the file before it's exported, rather than having it be a manually run check. This is what I did for gobject-introspection: http://git.gnome.org/browse/gobject-introspection/commit/?id=3ee1c9f984a71903a8eeef693c6c187291f25a0b
Created attachment 227897 [details] [review] Bring back a couple of private-but-extern symbols These both existed in 2.34.1, but are not exposed in headers, and were meant to be private. Making them static (in commit 84475e43) was technically an ABI break, and in particular it causes abicheck.sh to fail. Bug: https://bugzilla.gnome.org/show_bug.cgi?id=687441
(In reply to comment #4) > Actually...we should switch glib to actually *require* a symbol to be listed in > the file before it's exported, rather than having it be a manually run check. ... or use versioned symbols, like telepathy-glib does, if you want to be really fancy. In telepathy-glib, releases (defined as anything with version x.y.z, as opposed to random snapshots from git with version number x.y.z.1) are not allowed to have symbols matching /^tp_/ that aren't part of a version. Random snapshots *are* allowed to have such symbols, and they're assigned to a special TP_x.y.z.1_UNRELEASED version to ensure that binaries built against a non-release (whose ABI is not guaranteed) fail loudly until they are relinked. In telepathy-glib we also have a stricter policy about symbol names: all extern-but-G_GNUC_INTERNAL symbols match /^_tp/, all public symbols match /^tp_/. I'm not sure whether that's feasible/desirable for GLib or not?
Review of attachment 227897 [details] [review]: Looks correct. Thanks for catching this!
Created attachment 227901 [details] [review] Predeclare more things in tests
Created attachment 227902 [details] [review] Fix more void prototypes in tests
Review of attachment 227901 [details] [review]: Yep, looks good.
Review of attachment 227902 [details] [review]: Looks good.
(In reply to comment #6) > (In reply to comment #4) > > Actually...we should switch glib to actually *require* a symbol to be listed in > > the file before it's exported, rather than having it be a manually run check. > > ... or use versioned symbols, like telepathy-glib does, if you want to be > really fancy. That one has been discussed and rejected as basically not worth the pain by others...I wouldn't be totally opposed to it myself, but...something to keep in mind I guess. > In telepathy-glib, releases (defined as anything with version x.y.z, as opposed > to random snapshots from git with version number x.y.z.1) are not allowed to > have symbols matching /^tp_/ that aren't part of a version. Random snapshots > *are* allowed to have such symbols, and they're assigned to a special > TP_x.y.z.1_UNRELEASED version to ensure that binaries built against a > non-release (whose ABI is not guaranteed) fail loudly until they are relinked. Ah. That is pretty nifty. > In telepathy-glib we also have a stricter policy about symbol names: all > extern-but-G_GNUC_INTERNAL symbols match /^_tp/, all public symbols match > /^tp_/. I'm not sure whether that's feasible/desirable for GLib or not? Well...I would personally like that, but I believe Ryan much prefers using G_GNUC_INTERNAL functions that start with g_, not _g_. You can pretty easily recognize code he's written if you see G_GNUC_INTERNAL, just like davidz code has the (%s, %d) stuff appended to every GError.
Created attachment 227909 [details] [review] Fix more warning-addition fallout I'm normally a big fan of small atomic commits, but I also want to get things done this afternoon...
Review of attachment 227909 [details] [review]: Only one minor comment, accepted-commit-now regardless. ::: tests/onceinit.c @@ +113,3 @@ char *argv[]) { + G_GNUC_UNUSED GThread *threads[N_THREADS]; If it's not used, just delete it?
Pushed. (In reply to comment #14) > If it's not used, just delete it? 16:17 < walters> eww 16:17 < walters> looks like it busy loops until the counter reaches max, rather than doing g_thread_join () 16:18 < smcv> *shrug* 16:18 < walters> well, whatever, let's not change test semantics now, just commit and get make check going again
I'd have preferred if we just broke the ABI. This sort of thing is extremely harmless -- the most harm it causes is alarm bells in people's checker tools. It doesn't actually break anyone's code.
We discussed this on IRC and most (Colin as an exception) agreed that we should just hide the symbols, so I'm going to do that.