GNOME Bugzilla – Bug 671499
Don't set G_MESSAGES_DEBUG unconditionally
Last modified: 2012-03-17 14:47:48 UTC
It leaks to processes which link to libfolks (like gnome-shell), which is bad. We should either set it conditionally on FOLKS_DEBUG, or just abolish FOLKS_DEBUG altogether and use G_MESSAGES_DEBUG by itself. (Note that I haven't checked what else in folks depends on FOLKS_DEBUG, so there might be complications to this.)
(See this commit for an example of stupidity: http://git.gnome.org/browse/folks/commit/?id=c34cd4b9f07b22f3813c3f26051521cb6338ea5e)
Created attachment 209119 [details] [review] debug: Remove FOLKS_DEBUG Since glib now suppresses all g_debug messages by default, controlled by the G_MESSAGES_DEBUG environment variable, we don't need our own thing.
Review of attachment 209119 [details] [review]: Thanks for working on this. Folks can't break API, and there should be no need for API to be broken, so a few bits of the patch need changing. ::: folks/backend-store.vala @@ -139,3 @@ - var debug_no_colour = Environment.get_variable ("FOLKS_DEBUG_NO_COLOUR"); - this._debug = - Debug.dup_with_flags (Environment.get_variable ("FOLKS_DEBUG"), As suggested below, in order to not break API, why not just change this Environmen.get_variable() call to use G_MESSAGES_DEBUG rather than FOLKS_DEBUG? ::: folks/debug.vala @@ -98,3 @@ - * @since 0.5.1 - */ - public bool debug_output_enabled Removing this is an API break. Why not keep it and just have backend-store.vala read G_MESSAGES_DEBUG rather than FOLKS_DEBUG? @@ -230,3 @@ - */ - public static Debug dup_with_flags (string? debug_flags, - bool colour_enabled) Removing this is also an API break. ::: folks/individual-aggregator.vala @@ -860,3 @@ /* Debug output. */ - if (this._debug.debug_output_enabled == true) - { Removing this check will add some extra overhead on a fairly critical path in folks. I haven't profiled this (so I could well be wrong), but it would be nice to keep it conditionalised if possible.
Created attachment 209993 [details] [review] folks: Don't set G_MESSAGES_DEBUG unconditionally This better?
Review of attachment 209993 [details] [review]: Looks good to me. Please commit to master. Thanks!
Attachment 209993 [details] pushed as 31e8034 - folks: Don't set G_MESSAGES_DEBUG unconditionally