After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 671499 - Don't set G_MESSAGES_DEBUG unconditionally
Don't set G_MESSAGES_DEBUG unconditionally
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: libfolks
git master
Other All
: Normal critical
: Unset
Assigned To: folks-maint
folks-maint
Depends on:
Blocks:
 
 
Reported: 2012-03-06 21:28 UTC by Philip Withnall
Modified: 2012-03-17 14:47 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
debug: Remove FOLKS_DEBUG (10.79 KB, patch)
2012-03-06 23:33 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
folks: Don't set G_MESSAGES_DEBUG unconditionally (1.58 KB, patch)
2012-03-17 08:54 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Philip Withnall 2012-03-06 21:28: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.)
Comment 1 Philip Withnall 2012-03-06 21:30:10 UTC
(See this commit for an example of stupidity: http://git.gnome.org/browse/folks/commit/?id=c34cd4b9f07b22f3813c3f26051521cb6338ea5e)
Comment 2 Jasper St. Pierre (not reading bugmail) 2012-03-06 23:33:28 UTC
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.
Comment 3 Philip Withnall 2012-03-07 09:26:43 UTC
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.
Comment 4 Jasper St. Pierre (not reading bugmail) 2012-03-17 08:54:20 UTC
Created attachment 209993 [details] [review]
folks: Don't set G_MESSAGES_DEBUG unconditionally

This better?
Comment 5 Philip Withnall 2012-03-17 14:23:34 UTC
Review of attachment 209993 [details] [review]:

Looks good to me. Please commit to master. Thanks!
Comment 6 Jasper St. Pierre (not reading bugmail) 2012-03-17 14:47:46 UTC
Attachment 209993 [details] pushed as 31e8034 - folks: Don't set G_MESSAGES_DEBUG unconditionally