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 656914 - Load GIO_EXTRA_MODULES first, and ignore duplicates
Load GIO_EXTRA_MODULES first, and ignore duplicates
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
: 656059 (view as bug list)
Depends on:
Blocks: 656906
 
 
Reported: 2011-08-19 14:37 UTC by Stef Walter
Modified: 2011-08-27 23:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
giomodule: When loading GIO_EXTRA_MODULES skip duplicates (5.19 KB, patch)
2011-08-19 14:37 UTC, Stef Walter
reviewed Details | Review
giomodule: When loading GIO_EXTRA_MODULES skip duplicates (8.03 KB, patch)
2011-08-24 11:02 UTC, Stef Walter
none Details | Review
After discussing with Alex on IRC, exposed the duplicate blocking behavior (13.36 KB, patch)
2011-08-24 11:49 UTC, Stef Walter
none Details | Review

Description Stef Walter 2011-08-19 14:37:05 UTC
So that projects can load test in-tree modules properly, it should be possible to load modules from other directories, and have those override the installed modules from the same project.

The GIO_EXTRA_MODULES environment variable (see #523039) already exists, but it loads after the built in modules, and then the installed and uninstalled modules conflict.

As discussed IRC, a decent way to do this seems to be: 

 a) look at GIO_EXTRA_MODULES directories first and
 b) don't load modules with the same basename
Comment 1 Stef Walter 2011-08-19 14:37:25 UTC
Created attachment 194233 [details] [review]
giomodule: When loading GIO_EXTRA_MODULES skip duplicates

* Load modules from paths listed in GIO_EXTRA_MODULES environment
   variable first.
 * Ignore duplicate modules based on module basename.
 * Document behavior.
Comment 2 Dan Winship 2011-08-19 14:42:53 UTC
see bug 656059 for an alternate approach
Comment 3 Stef Walter 2011-08-19 14:51:20 UTC
Aha. Yes, that seems like a good approach too. The g_io_module_test method Dan proposes seems slightly less likely to surprise someone. Either way is fine with me.

Alex, which one should we merge?
Comment 4 Matthias Clasen 2011-08-19 14:56:41 UTC
I'm slightly in favour of the env var approach, since it can be used outside of tests too, and is more in line with what we do elsewhere (gdk-pixbuf loaders, immodules, etc)
Comment 5 Dan Winship 2011-08-23 23:33:32 UTC
Comment on attachment 194233 [details] [review]
giomodule: When loading GIO_EXTRA_MODULES skip duplicates

>* Load modules from paths listed in GIO_EXTRA_MODULES environment
>   variable first.
> * Ignore duplicate modules based on module basename.

fix indent on first line

>+ *  The GIO_EXTRA_MODULES environment variables can be used to specify

<envar>GIO_EXTRA_MODULES</envar> (likewise with PATH later on)

also, no "s" on "variable"

>+is_duplicate_module_name (const gchar *basename,
>+                          GHashTable *module_names)

align the "*"s

>+  /* No duplicate checking */
>+  if (!module_names)
>+    return FALSE;

I was going to complain about the comment style (comments should say things that are true at the point where the comment appears, so that comment either needs an "if" in it, or else it should be inside the if body rather than before it) but then decided that you should probably just remove the comments, because they're obvious anyway.

>+ * an extension point it implementes is used with e.g.

fix the pre-existing typo while you're moving it
Comment 6 Alexander Larsson 2011-08-24 07:05:46 UTC
Yeah, i also favour the env var approach.
Comment 7 Alexander Larsson 2011-08-24 07:11:06 UTC
Review of attachment 194233 [details] [review]:

It might be interesting to somehow expose the duplication-avoiding function in the API too, so other apps using gio modules can do the same thing.

::: gio/giomodule.c
@@ +285,3 @@
+
+  /* Duplicate checking, but this is the first one */
+  g_hash_table_insert (module_names, g_strdup (basename), "PRESENT");

"PRESENT" is kinda weird. I tend to use the same value as the key, GINT_TO_POINTER(1).
Comment 8 Stef Walter 2011-08-24 11:02:36 UTC
Created attachment 194565 [details] [review]
giomodule: When loading GIO_EXTRA_MODULES skip duplicates

* Load modules from paths listed in GIO_EXTRA_MODULES environment
   variable first.
 * Ignore duplicate modules based on module basename.
 * Add g_io_modules_scan_all_in_directory_full() function, which allows
   callers to specify flags to ignore duplicate base names when scanning.
 * Document behavior.
Comment 9 Stef Walter 2011-08-24 11:05:17 UTC
Updated for Dan's and Alex's comments. I hope this solves your use case Alex. It allows non-gio callers to get in on the 'no duplicate base name' action if desired.

git bz, seems to do the screwy thing with removing whitespace when attaching commits to bugs... Will send Colin a patch.
Comment 10 Alexander Larsson 2011-08-24 11:13:45 UTC
Thats not right, duplicate name skipping should only be between each single instance of use of modules. For instance gio modules used to load plugins to some app should not be affected by basenames of core gio extensions.
Comment 11 Stef Walter 2011-08-24 11:49:33 UTC
Created attachment 194571 [details] [review]
After discussing with Alex on IRC, exposed the duplicate blocking behavior

and exposed it as a 'scope' optionally used when loading modules.

giomodule: When loading GIO_EXTRA_MODULES skip duplicates

* Load modules from paths listed in GIO_EXTRA_MODULES environment
   variable first.
 * Ignore duplicate modules based on module basename.
 * Add the concept of GIOModuleScope which allows other callers to
   skip duplicate loaded modules, or block specific modules based on
   basename.
 * Document behavior.
Comment 12 Stef Walter 2011-08-26 13:28:11 UTC
Checked with Alex on IRC. Added some missing Since:, and then merged.
Comment 13 Dan Winship 2011-08-27 23:06:56 UTC
*** Bug 656059 has been marked as a duplicate of this bug. ***