GNOME Bugzilla – Bug 673918
Port to newer libgee
Last modified: 2013-02-11 06:11:38 UTC
Looking at jhbuild modulesets, libgee 0.8 is the supported version for GNOME 3.6, and it's not nice that one needs to install libgee 0.6 from packages just to build folks and dependencies. So I went on and moved folks to the newer libgee. It results in a code simplification also, and it's nicer to use. (Patches are also ready for gnome-contacts, which I tested, and for gnome-shell. I'll attach them if this is taken)
Created attachment 211849 [details] [review] Port to libgee 0.8 libgee 0.8 is the default for GNOME 3.6, and it is mostly compatible with previous versions. Changes are in the area of hash containers, which are greatly simplified due to the Hashable interface. Soname has been bumped to indicate ABI break.
This is an API break as well as an ABI break. I don’t feel too good about breaking API again yet; I think people still hate me from the last time. Travis, what do you think (about breaking API, not about hating me)? Giovanni, would it be possible to rework this patch so it doesn’t break API (or ABI)? I guess you’d have to remove uses of the Hashable interface. (Don’t actually rework the patch yet, though, until we’ve decided whether to break folks’ API.)
(In reply to comment #2) > This is an API break as well as an ABI break. I don’t feel too good about > breaking API again yet; I think people still hate me from the last time. > Travis, what do you think (about breaking API, not about hating me)? I really don't want to do it :) I'll check out the changes soon and maybe talk to the maintainer to see if they can't manage some backwards compatibility. It's just really unfortunate that they aren't done breaking their API, since it affects us. I guess we knew that as a risk going in though. :-/
Having taken a quick look at the changes between libgee 0.6 and 0.8, we should be able to port to 0.8 without breaking folks’ API. We’d just end up not being able to use the Hashable interface, but that’s not too much of a problem. This would, however, cause an implicit API break for anybody using the Gee data structures returned by folks. I suspect that these breaks would be minor, however. Porting to 0.8 has the advantage that we can make use of the fix for bug #663337.
Philip, Travis, What's the status on this issue? Is this something we need to push forward probably?
(In reply to comment #5) > What's the status on this issue? Is this something we need to push forward > probably? This still needs doing, and it probably needs doing sooner rather than later. Patches welcome! :-D
Created attachment 217332 [details] [review] updated the previous patch and got it all to build. I've updated the patch from before so it applies cleanly to master and builds. I'll check that make check still works shortly. Does this need to go into 0.7.2? It does break the API a bit, but does clean up the code quite a bit also.
(In reply to comment #7) > Created an attachment (id=217332) [details] [review] > updated the previous patch and got it all to build. > > I've updated the patch from before so it applies cleanly to master and builds. > I'll check that make check still works shortly. Does this need to go into > 0.7.2? It does break the API a bit, but does clean up the code quite a bit > also. It's worth the effort to avoid API breaks if at all possible. Philip, you said that should be possible earlier - do you still think that's true?
(In reply to comment #8) > Philip, you said that should be possible earlier - do you still think that's > true? I think it’s definitely worth playing around with. The only reason I can think of which might prevent us doing it is if HashSet (et al) require a generic type which implements Hashable. I don’t know if this is the case.
A quick test here shows it may be possible. We will need to change all our GLib.HashFunc<weak void*> methods to Gee.HashDataFunc however from what I can tell here. Documentation for gee 0.8 seems to be hard to find, probably partly because gee 0.6 is in package gee-1.0 which confuses things quite a bit. Anyway, I'll try changing our hash functions to that Gee.HashDataFunc here and see what that does for us. If it ends up being an api break I suggest we go with the original patch that makes AbstractFieldDetails subclass Hashable, because if we are breaking api we might as well end up with cleaner code.
In order to use Gee 0.8 we need to pass static hash functions, but AbstractFieldDetails hash and equal methods are not able to be static since they use this.value. Alternatively we could use Hashable interface like the original patch does. Philip, am I missing something? Do you see another way to get this in without an api change?
(In reply to comment #11) > Philip, am I missing something? Do you see another way to get this in without > an api change? You could try adding in AbstractFieldDetails.[hash|equal]_static() which would take the value to compare in as a parameter, and are static. If that doesn’t work then I guess we have to break API, and might as well go the whole hog and use Hashable everywhere.
Philip, Locally I've tried this: public static uint hash_static (AbstractFieldDetails<T> value) { HashFunc hash_func = direct_hash; if (typeof (T) == typeof (string)) hash_func = str_hash; return hash_func (value); } but get an error on the typeof (T), maybe we should just use (Gee.HashDataFunc) str_hash for the hashes of AbstractFieldDetails<string> subclasses and other hash methods for other needs?
Hmm, using (Gee.HashDataFunc) str_hash gives some warning about "copying delegates is discouraged" then fails to build.
(In reply to comment #13) > Philip, > > Locally I've tried this: *snip* > but get an error on the typeof (T), maybe we should just use (Gee.HashDataFunc) > str_hash for the hashes of AbstractFieldDetails<string> subclasses and other > hash methods for other needs? Did you try this? public static uint hash_static<T> (AbstractFieldDetails<T> value) … (In reply to comment #14) > Hmm, using (Gee.HashDataFunc) str_hash gives some warning about "copying > delegates is discouraged" then fails to build. Casting to (unowned Gee.HashDataFunc) might work.
Created attachment 218043 [details] [review] Redone to no longer need any api breaks. Philip, With this patch all the unit tests that pass on master also pass. Am I correct in stating that this doesn't change the existing API so doesn't need soname bump, etc.?
Review of attachment 218043 [details] [review]: Looking good; just a few questions. :-) ::: backends/eds/lib/edsf-persona.vala @@ +492,3 @@ new HashMultiMap<string, ImFieldDetails> (null, null, + (Gee.HashDataFunc) AbstractFieldDetails<string>.hash_static, + (Gee.EqualDataFunc) AbstractFieldDetails<string>.equal_static); Is it not possible to pass “(Gee.HashDataFunc) ImFieldDetails.hash_static” instead of falling back to AbstractFieldDetails? ::: folks/abstract-field-details.vala @@ +277,3 @@ + public static bool equal_static<T> (AbstractFieldDetails<T> left, + AbstractFieldDetails<T> right) This line should be indented four spaces further than the “public” only. The function itself needs a documentation comment. Separately from the documentation comment (so that it doesn’t appear in the documentation), there should probably also be a comment linking to this bug report and mentioning that [hash|equal]_static() are work-arounds to avoid breaking API, and can be removed when we next break API. @@ +415,3 @@ } + + public static uint hash_static<T> (AbstractFieldDetails<T>? value) Should the value parameter really be nullable?
I made the value parameter nullable because it is null in some unit tests. Otherwise the folks unit tests were failing. I'll check on the other, using ImFieldDetails.hash_static etc.
If I try to use the subclasses for hash_static and equal_static I get stuff like this: edsf-persona.vala:493.30-493.55: error: The name `hash_static' does not exist in the context of `Folks.ImFieldDetails' (Gee.HashDataFunc) ImFieldDetails.hash_static, I have tried to make the AbstractDetails<T> hash_static virtual, but it can't be virtual and static according to the valac errors I get. I'll add documentation to the hash_static and equal_static methods saying to remove them once we break api at some point. With that and the indentation is it ready to merge?
(In reply to comment #18) > I made the value parameter nullable because it is null in some unit tests. > Otherwise the folks unit tests were failing. I think those unit tests should be fixed to not use null instead. (In reply to comment #19) > If I try to use the subclasses for hash_static and equal_static I get stuff > like this: > > edsf-persona.vala:493.30-493.55: error: The name `hash_static' does not exist > in the context of `Folks.ImFieldDetails' > (Gee.HashDataFunc) ImFieldDetails.hash_static, Hum. I thought Vala allowed inheritance of static members (it’s certainly what the documentation says). Perhaps it’s worth asking about in #vala. If it’s not allowed in Vala then http://live.gnome.org/Vala/Tutorial needs updating. > I'll add documentation to the hash_static and equal_static methods saying to > remove them once we break api at some point. With that and the indentation is > it ready to merge? Yup, pending the outcome of the above comment. Thanks for working on this. :-)
Created attachment 218126 [details] [review] Redone to no longer accept null values in hash_static Turns out one of the main problems that was giving null values to hash_static was casting the hash and equal delegates. Together with Evan I rewrote the static methods to use the Gee.HashDataFunc and Gee.EqualDataFunc signatures to avoid casting. Now all the folks unit tests pass and null values are not accepted by the hash_static function.
*** Bug 661102 has been marked as a duplicate of this bug. ***
Review of attachment 218126 [details] [review]: Looks good to me. Please commit with the minor changes below, and don’t forget to update the NEWS file! ::: folks/abstract-field-details.vala @@ +278,2 @@ /** + * Same as equal, but static, so we can use libgee 0.8 without an api break. s/equal/{@link AbstractFieldDetails.equal}/ @@ +294,3 @@ + { + GLib.return_val_if_fail (left != null, 0); + GLib.return_val_if_fail (right != null, 0); s/0/false/
Hi, talking with the libgee maintainer, Its not clear that 0.8 will be ready for GNOME 3.6 (see tracker bug #679587 for the remaining problems), so I wonder if this patch can be stored in a branch until we can be sure that 0.8 will be released for GNOME 3.6 Thanks
I guess this is related to these changes. I get this when building folks master: /home/cassidy/gnome/folks/backends/libsocialweb/sw-backend.vala: In function 'folks_backends_sw_backend_real_unprepare_co': /home/cassidy/gnome/folks/backends/libsocialweb/sw-backend.vala:146:5: warning: passing argument 1 of 'gee_abstract_map_get_values' from incompatible pointer type [enabled by default] In file included from ../../folks/folks.h:11:0, from sw-backend.c:27: /usr/include/gee-0.8/gee.h:1128:16: note: expected 'struct GeeAbstractMap *' but argument is of type 'struct GeeMap *' /home/cassidy/gnome/folks/backends/libsocialweb/sw-backend.vala: In function 'folks_backends_sw_backend_constructor': /home/cassidy/gnome/folks/backends/libsocialweb/sw-backend.vala:67:2: error: too few arguments to function 'gee_hash_map_new' In file included from ../../folks/folks.h:11:0, from sw-backend.c:27: /usr/include/gee-0.8/gee.h:1233:13: note: declared here configure did pass, so that's either a bug in the code or it doesn't check for the right gee version.
GNOME 3.6 isn't set to release until September from what I can see. In any case if gee 0.8 isn't going to be out by then I'll revert these two commits and reopen this bug. I'll likely need to fix up the patch again when 0.8 does come out, but I'll keep these commits on a branch until then. I'll fix this later today.
It seems this made folks/empathy depend on unreleased version of libgee. With 0.7.2 they doesn't compile.
So, at the moment which libgee version should Folks and Empathy be able to build with? 0.7.2 or 0.6.4?
So I just checkd with our beloved release manager and he said we should stuck with libgee 0.6.x for now. Jeremy: could you please revert the 0.8 patches from Folks ?
Guillaume, Ok, reverted those patches from folks until after the libgee 0.8 release happens (which should be before gnome 3.6, but we'll see)
(In reply to comment #30) > Ok, reverted those patches from folks until after the libgee 0.8 release > happens (which should be before gnome 3.6, but we'll see) Maciej has promised 0.8 will be released for GNOME 3.6 (see https://bugzilla.gnome.org/show_bug.cgi?id=680152#c8), so I guess we should get this patch updated and committed ASAP so it gets some testing. Jeremy, would you mind updating the patch please?
Yep, will do
(In reply to comment #32) > Yep, will do Actually, I just checked with the release team, and we decided it’ll be safer to stick with 0.6 for GNOME 3.6. We can port to 0.8 at the start of the next cycle. Basic reasoning: • There are lots of modules in GNOME which currently use libgee 0.6. • There’s no way all of them will be ported in time. • It seems unnecessarily painful to require both 0.6 and 0.8 to be packaged by distros for GNOME 3.6. • There is currently only one module (gxml) which uses libgee 0.8, and it isn’t essential for GNOME 3.6 (sorry Richard!). • Porting folks to 0.8 would require many of the users of folks to also port to 0.8, which would be Un-fun. Therefore, this is punted to GNOME 3.8. Sorry for the confusion and mess that this bug has turned into.
I spent a bit of time trying to port folks to libgee 0.8 again, but the signature of HashDataFunc and EqualDataFunc is different than it was. The new HashDataFunc has a particular bug https://bugzilla.gnome.org/show_bug.cgi?id=687143 which makes an ugly workaround needed for the porting. I suggest we wait for that to get fixed in libgee itself so folks code doesn't need to be filled with ugly workaround code like this: this._urls = new HashSet<UrlFieldDetails> ( (v) => { return AbstractFieldDetails.hash_static (v as AbstractFieldDetails); }, AbstractFieldDetails.equal_static);
Hi Jeremy, is there any new on this? Is there a bug open about the bug in libgee?
From what I see, currently, folks is the main package that needs porting to gee-1.0. Further down in the dependency chain I see those packages in my build bot: * Empathy: requires gee indirectly through folks.. * gnome-contacts: explicitly requires libgee and indirectly through folks The rest of GNOME 3.7.x seems to be ready for gee 0.8.
Looks like we've got the same problem with HashMultiMap (bug 693356) as we did for HashSet (bug 687143). I've got a work-around for now, but the rebasing is otherwise moving along.
Created attachment 235534 [details] [review] Port to Gee 0.8 (>= 0.8.4) Patch for branch: http://cgit.collabora.com/git/user/treitter/folks.git/log/?h=bgo673918-port-to-gee-0.8 All the tests work (save for telepathy/init and eds/create-remove-stores, but those fail on master in the same way currently anyhow). It depends upon libgee 0.8.4, which should be released in a few days from now. You can test it by installing the libgee 0.8 branch from git and changing its version in configure.ac to 0.8.4
Review of attachment 235534 [details] [review]: Looks mostly fine to me, with a few comments below. I guess this can be committed with these changes, even before libgee 0.8.4 is released. Thanks for taking the time to push this, Travis! ::: NEWS @@ +4,2 @@ Dependencies: +• libgee ≥ 0.8 s/0.8/0.8.4/ @@ +7,1 @@ Major changes: Might want to mention this under ‘Major changes’ as well. ::: backends/eds/lib/edsf-persona.vala @@ +1172,3 @@ this._roles = new HashSet<RoleFieldDetails> ( + AbstractFieldDetails<string>.hash_static, + AbstractFieldDetails<string>.equal_static); Shouldn’t this be AbstractFieldDetails<Role>? @@ +2039,3 @@ this._postal_addresses = new HashSet<PostalAddressFieldDetails> ( + AbstractFieldDetails<string>.hash_static, + AbstractFieldDetails<string>.equal_static); Shouldn’t this be AbstractFieldDetails<PostalAddress> (and again below)? ::: folks/abstract-field-details.vala @@ +278,3 @@ /** + * Same as {@link AbstractFieldDetails.equal}, but static, + * so we can use libgee 0.8 without an api break. s/api/API/ @@ +281,3 @@ + * + * See [[https://bugzilla.gnome.org/show_bug.cgi?id=673918|673918]]" + * This can and should be removed next time we break the api. s/api/API/. What’s the ‘"’ for? @@ +435,3 @@ + /** + * Same as {@link AbstractFieldDetails.hash}, but static, + * so we can use libgee 0.8 without an api break. s/api/API/ (and again below). ::: folks/individual.vala @@ +2235,3 @@ this._roles = new HashSet<RoleFieldDetails> ( + AbstractFieldDetails<string>.hash_static, + AbstractFieldDetails<string>.equal_static); Shouldn’t this be AbstractFieldDetails<Role> (and again below)? @@ +2313,3 @@ this._postal_addresses = new HashSet<PostalAddressFieldDetails> ( + AbstractFieldDetails<string>.hash_static, + AbstractFieldDetails<string>.equal_static); Shouldn’t this be AbstractFieldDetails<PostalAddress> (and again below)?
Changes made and pushed. Thanks! commit 01cc61aca9247a43f5132331a155f00d363e5e7d Author: Travis Reitter <travis.reitter@collabora.co.uk> Date: Wed Feb 6 21:33:24 2013 -0800 Port folks to libgee 0.8. Added hash_static and equal_static to AbstractFieldDetails. Use AbstractFieldDetails hash_static and equal_static where needed. Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=673918 NEWS | 3 + backends/eds/Makefile.am | 2 +- backends/eds/lib/Makefile.am | 2 +- backends/eds/lib/edsf-persona.vala | 93 ++++++++++++++-------- backends/eds/lib/folks-eds-uninstalled.pc.in | 2 +- backends/eds/lib/folks-eds.deps | 2 +- backends/eds/lib/folks-eds.pc.in | 2 +- backends/key-file/Makefile.am | 2 +- backends/key-file/kf-persona.vala | 18 ++--- backends/libsocialweb/Makefile.am | 2 +- backends/libsocialweb/lib/Makefile.am | 4 +- .../lib/folks-libsocialweb-uninstalled.pc.in | 2 +- backends/libsocialweb/lib/folks-libsocialweb.pc.in | 2 +- backends/libsocialweb/lib/swf-persona.vala | 12 +-- backends/ofono/Makefile.am | 2 +- backends/telepathy/Makefile.am | 2 +- backends/telepathy/lib/Makefile.am | 2 +- .../lib/folks-telepathy-uninstalled.pc.in | 2 +- backends/telepathy/lib/folks-telepathy.pc.in | 2 +- .../telepathy/lib/tpf-persona-store-cache.vala | 12 +-- backends/telepathy/lib/tpf-persona.vala | 28 +++---- backends/tracker/Makefile.am | 2 +- backends/tracker/lib/Makefile.am | 4 +- .../tracker/lib/folks-tracker-uninstalled.pc.in | 2 +- backends/tracker/lib/folks-tracker.pc.in | 2 +- backends/tracker/lib/trf-persona-store.vala | 5 +- backends/tracker/lib/trf-persona.vala | 53 ++++++------ configure.ac | 18 +---- docs/Makefile.am | 8 +- folks/Makefile.am | 4 +- folks/abstract-field-details.vala | 48 +++++++++++ folks/backend-store.vala | 9 +-- folks/debug.vala | 2 +- folks/folks-uninstalled.pc.in | 2 +- folks/folks.deps | 2 +- folks/folks.pc.in | 2 +- folks/individual-aggregator.vala | 10 +-- folks/individual.vala | 83 +++++++++---------- folks/potential-match.vala | 2 +- tests/eds/Makefile.am | 2 +- tests/eds/add-persona.vala | 20 ++--- tests/eds/email-details.vala | 3 +- tests/eds/individual-retrieval.vala | 3 +- tests/eds/link-personas.vala | 24 +++--- tests/eds/linkable-properties.vala | 4 +- tests/eds/phone-details.vala | 3 +- tests/eds/set-emails.vala | 4 +- tests/eds/set-im-addresses.vala | 5 +- tests/eds/set-phones.vala | 4 +- tests/eds/set-roles.vala | 4 +- tests/folks/Makefile.am | 2 +- tests/folks/aggregation.vala | 2 +- tests/folks/backend-loading.vala | 12 +-- tests/key-file/Makefile.am | 2 +- tests/key-file/individual-retrieval.vala | 3 +- tests/lib/Makefile.am | 2 +- tests/lib/eds/Makefile.am | 2 +- tests/lib/libsocialweb/Makefile.am | 2 +- tests/lib/telepathy/contactlist/Makefile.am | 2 +- tests/lib/tracker/Makefile.am | 2 +- tests/libsocialweb/Makefile.am | 2 +- tests/telepathy/Makefile.am | 2 +- tests/telepathy/individual-properties.vala | 12 +-- tests/telepathy/individual-retrieval.vala | 7 +- tests/tracker/Makefile.am | 2 +- tests/tracker/add-persona.vala | 29 ++++--- tests/tracker/duplicated-emails.vala | 8 +- tests/tracker/duplicated-phones.vala | 8 +- tests/tracker/link-personas.vala | 8 +- tests/tracker/match-email-addresses.vala | 8 +- tests/tracker/match-im-addresses.vala | 8 +- tests/tracker/match-known-emails.vala | 8 +- tests/tracker/match-phone-number.vala | 8 +- tests/tracker/remove-persona.vala | 4 +- tests/tracker/set-duplicate-email.vala | 8 +- tests/tracker/set-emails.vala | 4 +- tests/tracker/set-im-addresses.vala | 5 +- tests/tracker/set-notes.vala | 4 +- tests/tracker/set-phones.vala | 4 +- tests/tracker/set-postal-addresses.vala | 4 +- tests/tracker/set-roles.vala | 6 +- tests/tracker/set-urls.vala | 4 +- tools/Makefile.am | 2 +- tools/inspect/Makefile.am | 2 +- tools/inspect/inspect.vala | 2 +- 85 files changed, 385 insertions(+), 337 deletions(-)