GNOME Bugzilla – Bug 683256
caribou unnecessarily requires pygobject 3.3.3 or up to build
Last modified: 2012-09-05 09:32:15 UTC
Created attachment 223268 [details] [review] Be compatible with pygobject from before and after the fix for bug #676746. Commit 82794781ceff849c98e8db15f9ada9da3b1db472 ("make_schema: s/print/print_/ according to pygobject change") made caribou build with pygobjects with the fix for bug 676746 but not with older ones (see https://bugs.gentoo.org/show_bug.cgi?id=433692 ). This is not all that hard to work around, with a patch like the one I'm about to attach (the same one as on that gentoo bug, just as a git patch against master). Should you prefer to just require a recent pygobject at build time may I suggest you document the requirement in README and/or NEWS, and possibly change the getattr(setting.gvariant, "print_")(False) to just setting.gvariant.print_(False)? The change to pygobject was made so you wouldn't need a weird getattr to call functions like this one :)
(In reply to comment #0) > Created an attachment (id=223268) [details] [review] > Be compatible with pygobject from before and after the fix for bug #676746. > > Commit 82794781ceff849c98e8db15f9ada9da3b1db472 ("make_schema: s/print/print_/ > according to pygobject change") made caribou build with pygobjects with the fix > for bug 676746 but not with older ones (see > https://bugs.gentoo.org/show_bug.cgi?id=433692 ). This is not all that hard to > work around, with a patch like the one I'm about to attach (the same one as on > that gentoo bug, just as a git patch against master). It seems that pygobject still requires a lot of workarounds. > Should you prefer to just require a recent pygobject at build time may I > suggest you document the requirement in README and/or NEWS, and possibly change > the getattr(setting.gvariant, "print_")(False) to just > setting.gvariant.print_(False)? The change to pygobject was made so you > wouldn't need a weird getattr to call functions like this one :) I agree that if pygobject requirement increased README and/or NEWS should be updated. Anyway, taking into account that there are ways to avoid that, I prefer the solution you proposed. Some comments about that patch soon.
Review of attachment 223268 [details] [review]: ::: tools/make_schema.py @@ +59,3 @@ + printfunc = getattr(setting.gvariant, 'print_', None) + if printfunc is None: + printfunc = getattr(setting.gvariant, 'print') The only missing thing here is explain why you are doing this. Not sure if this workaround is really common using pygobject, but I feel that in any case, it could be good to add a little comment explaining what is happening there.
Created attachment 223280 [details] [review] Add comment requested in review You're quite right: it's especially useful to document which pygobject versions are involved here so this can eventually be removed. How's this?
Review of attachment 223268 [details] [review]: Changing status of the patch, as the issue I found was solved on next patch.
Review of attachment 223280 [details] [review]: Seems ok to me.
I have sent a mail to Eitan, to check if my review "is good enough" to get this kind of patches approved to be committed. I would prefer to wait a little for his answer before committing it. Thanks for your patches.
(In reply to comment #6) > I have sent a mail to Eitan, to check if my review "is good enough" to get this > kind of patches approved to be committed. I would prefer to wait a little for > his answer before committing it. > > Thanks for your patches. I have talked with Eitan, and he is ok. Please commit those changes (if not, please tell me and I will commit them). Thanks for the patches.
I'm pretty sure I can't push changes, so please commit them if they're ok :)
(In reply to comment #8) > I'm pretty sure I can't push changes, so please commit them if they're ok :) Just committed. Closing the bug. Thanks for the patches.
(In reply to comment #0) > Commit 82794781ceff849c98e8db15f9ada9da3b1db472 ("make_schema: s/print/print_/ > according to pygobject change") made caribou build with pygobjects with the fix > for bug 676746 but not with older ones (see > https://bugs.gentoo.org/show_bug.cgi?id=433692 ). This is not all that hard to > work around, with a patch like the one I'm about to attach (the same one as on > that gentoo bug, just as a git patch against master). FWIW, I have just made a new Caribou release, that includes this changes, in the case that you want to avoid a gentoo specific patch.