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.
@@ -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?
@@ -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.
@@ -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
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