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 684282 - Add support static link of GIO modules
Add support static link of GIO modules
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks: 791100 791372
 
 
Reported: 2012-09-18 10:16 UTC by Andoni Morales
Modified: 2018-01-04 16:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add support for loading modules statically linked (4.88 KB, patch)
2012-09-18 10:16 UTC, Andoni Morales
reviewed Details | Review
Initial support for static gio modules in glib-networking (1.36 KB, patch)
2012-09-18 10:17 UTC, Andoni Morales
reviewed Details | Review
Sample application (820 bytes, application/x-xz)
2012-09-18 10:18 UTC, Andoni Morales
  Details
Add support loading static modules (5.69 KB, patch)
2012-09-19 09:34 UTC, Andoni Morales
none Details | Review
Add static version of the library (3.40 KB, patch)
2012-09-27 22:35 UTC, Nicolas Dufresne (ndufresne)
none Details | Review
gio: Add support for static modules (3.72 KB, patch)
2014-02-26 08:58 UTC, Olivier Crête
none Details | Review
GIOModule: Use unique names for load/unload symbols (3.27 KB, patch)
2017-12-01 00:58 UTC, Xavier Claessens
none Details | Review
GTypeModule: Allow registering static types (3.84 KB, patch)
2017-12-01 00:58 UTC, Xavier Claessens
none Details | Review
GTypeModule: Allow registering static types (5.16 KB, patch)
2017-12-01 03:22 UTC, Xavier Claessens
none Details | Review
GIOModule: Use unique names for load/unload symbols (9.56 KB, patch)
2017-12-01 20:05 UTC, Xavier Claessens
none Details | Review
GTypeModule: Allow registering static types (5.16 KB, patch)
2017-12-01 20:06 UTC, Xavier Claessens
none Details | Review
GIOModule: Use unique names for load/unload symbols (9.56 KB, patch)
2017-12-01 20:55 UTC, Xavier Claessens
none Details | Review
GTypeModule: Allow registering static types (5.70 KB, patch)
2017-12-01 20:55 UTC, Xavier Claessens
none Details | Review
GIOModule: Use unique names for load/unload symbols (10.04 KB, patch)
2018-01-04 16:05 UTC, Xavier Claessens
committed Details | Review
GTypeModule: Allow registering static types (6.05 KB, patch)
2018-01-04 16:05 UTC, Xavier Claessens
committed Details | Review

Description Andoni Morales 2012-09-18 10:16:53 UTC
Created attachment 224597 [details] [review]
Add support for loading modules statically linked

As part of the porting work for GStreamer to Android, one of the requirements is to provide a wrapper shared library containing all the GStreamer dependencies (including glib), that developers can link easily with their application without having to deal with a large number of shared libraries.
For this, we would like to be able to include GIO modules in this library too, and be able to load them statically instead of using the dynamic loading path.

The following patch adds support for loading modules that are statically linked, requiring this module to define the load/unload/query functions suffixed with the module name to avoid duplicated symbols in modules eg:
 * g_io_module_load_gnutls
 * g_io_module_unload_gnutls
 * g_io_module_query_gnutls

Attached is a path for glib-networking, that should be compiled with CFLAGS=-DGLIB_NETWORKING_BUILD_STATIC for this purporse.

To reproduce and demonstrate what's being done for Android I have attached a test application with the following files:
 * glib_android.c: defines JNI_OnLoad, which is called when a library is loaded. The gio modules should be registered there.
 * test-tls.c: test application that calls the init function.

To compile it run:
  PREFIX=/path/to/prefix make

After compiling it, you will see:
 * libglib_android.so: shared library with glib_android.o, glib, it's dependencies and the TLS gio module.
 * test-tls: a sample app that links to libglib_android.so and calls the init function.

Run it with:
  make run

Note: Glib should be compiled with --enable-static and CC='gcc -fPIC -DPIC'
Comment 1 Andoni Morales 2012-09-18 10:17:52 UTC
Created attachment 224598 [details] [review]
Initial support for static gio modules in glib-networking
Comment 2 Andoni Morales 2012-09-18 10:18:19 UTC
Created attachment 224599 [details]
Sample application
Comment 3 Colin Walters 2012-09-18 13:16:51 UTC
Review of attachment 224598 [details] [review]:

You should use a configure argument rather than a magic define.  --enable-gio-static-module or something.
Comment 4 Colin Walters 2012-09-18 13:23:17 UTC
Review of attachment 224597 [details] [review]:

::: gio/giomodule.c
@@ +283,3 @@
   GIOModule *module = G_IO_MODULE (gmodule);
 
+  if (module->is_static) {

Coding style is wrong - GLib is GNU style.  Braces on a new line, space between identifer and paren.

@@ +353,3 @@
+  module = g_object_new (G_IO_TYPE_MODULE, NULL);
+  module->filename = g_strdup_printf("libgio%s.so", name);
+  module->load_func_name = g_strdup_printf("g_io_module_load_%s", name);

Why not just compute these names when the load function is called?  I can't think of a reason we need to malloc and hold on to them.

::: gio/giomodule.h
@@ +65,3 @@
                                                                     GIOModuleScope    *scope);
 
+gboolean              g_io_module_load_static_module              (const char *name);

Who uses this API?  The application?  A consuming library? So something like GStreamer would call g_io_module_load_static_module ("gnutls") ?

In any case when adding new public API, you need to use GLIB_AVAILABLE_IN_2_36, document it, and add it to gio/gio.symbols.
Comment 5 Colin Walters 2012-09-18 13:34:16 UTC
At a much higher level - what about solving this problem by adding an API like g_io_module_add_resource_path() ?

Basically teach GIO how to look up modules using GResource.
Comment 6 Matthias Clasen 2012-09-18 23:09:19 UTC
(In reply to comment #5)

> Basically teach GIO how to look up modules using GResource.

Isn't that a little backwards ?
Resources are data thats pulled out of elf sections.
Modules contain elf sections (holding executable code)...
Comment 7 Colin Walters 2012-09-19 02:26:16 UTC
(In reply to comment #6)
> (In reply to comment #5)
> 
> > Basically teach GIO how to look up modules using GResource.
> 
> Isn't that a little backwards ?
> Resources are data thats pulled out of elf sections.
> Modules contain elf sections (holding executable code)...

There's no circular dependency involved - it is recursive, but then so are e.g. tarballs (files that contain filesystem trees); there's no problem with that.
Comment 8 Andoni Morales 2012-09-19 09:34:50 UTC
Created attachment 224722 [details] [review]
Add support loading static modules

A per comments:
  * Fixed indentation.
  * Resolve name functions.
  * Added new symbol to gio.symbols
  * Added documentation
  * Added GLIB_AVAILABLE_IN_2_34
Comment 9 Andoni Morales 2012-09-19 10:30:36 UTC
(In reply to comment #4)

> Who uses this API?  The application?  A consuming library? So something like
> GStreamer would call g_io_module_load_static_module ("gnutls") ?

This should be used by the application during its initialization. Developers should know before hand the modules required by the application, link them statically and load them manually using this API.

GStreamer also uses a configure option for static plugins similar to what gio modules should use like glib-networkig, where the register function of the plugin is renamed to gst_plugin_mp3dec_register using a helper macro.(http://cgit.freedesktop.org/gstreamer-sdk/gstreamer/tree/gst/gstplugin.h#n308)
Applications linking statically GStreamer and glib should than register plugins manually during the init step, as well as static GIO modules.
Comment 10 Andoni Morales 2012-09-19 11:10:20 UTC
Regarding registering/loading static modules. The approach in gstreamer of calling directly gst_plugin_foo_register instead of gst_plugin_register_static("foo") helps at link time, since otherwise you need to explicetly link with --whole-archive the gio module. so maybe plugins should define something similar, a g_io_module_gnutls_load_static function that calls itself g_io_module_register_static_module("gnutls") to help linking.
Let me rework this a little bit :)
Comment 11 Matthias Clasen 2012-09-19 11:38:58 UTC
(In reply to comment #7)
 
> There's no circular dependency involved - it is recursive, but then so are e.g.
> tarballs (files that contain filesystem trees); there's no problem with that.

I'm not saying there's a problem per se. Just that it seems a totally unnecessary extra roundtrip.
Comment 12 Matthias Clasen 2012-09-19 11:41:40 UTC
I don't like the new api. It doesn't make much sense to me.
If you want to see something that might be acceptable, look for gtks builtin immodules.
Comment 13 Sebastian Dröge (slomo) 2012-09-19 11:54:43 UTC
There's a big difference between builtin immodules and static modules though... you don't want to change GTK's code and rebuild GTK just to add a new static module, like the TLS module from glib-networking.

The approach for immodules does not really work here.
Comment 14 Colin Walters 2012-09-19 19:25:11 UTC
Review of attachment 224722 [details] [review]:

The coding style is still inconsistent:

unload_func_name = g_strdup_printf("g_io_module_unload_%s", gmodule->name);
module->library = g_module_open (NULL, G_MODULE_BIND_LAZY | G_MODULE_BIND_LOCAL);

The first one should be 

unload_func_name = g_strdup_printf ("g_io_module_unload_%s", gmodule->name);
Comment 15 Matthias Clasen 2012-09-20 17:42:48 UTC
(In reply to comment #13)
> There's a big difference between builtin immodules and static modules though...
> you don't want to change GTK's code and rebuild GTK just to add a new static
> module, like the TLS module from glib-networking.
> 
> The approach for immodules does not really work here.

So where does the static module reside then ? In the application code ?
Comment 16 Alexander Larsson 2012-09-20 18:05:15 UTC
I don't understand this approach at all.

You create a faked dynamic module with a string, this string is then used to create another string by prepending g_io_module_load_ to it, then the dynamic loader is used to find this symbol which needs to be a public exported function.

Then you add a manual call to g_io_module_load_static_module("gnutls") which with will trigger this sequence of calls in order to eventually reach g_io_module_load_gnutls() which will then just call g_tls_backend_gnutls_register() (unloading will never happen anyway).

Why not just call g_tls_backend_gnutls_register() directly?

The only useful thing that the sequence above does is call _g_io_modules_ensure_extension_points_registered (), its possible we should export this. Then any app that wants to statically link the tls giomodule can just link to it and call:

 g_io_modules_ensure_extension_points_registered ();
 g_io_module_load_gnutls (); <- Well, this should probably be renamed
Comment 17 Andoni Morales 2012-09-25 09:23:40 UTC
(In reply to comment #16)
> I don't understand this approach at all.
> 
> You create a faked dynamic module with a string, this string is then used to
> create another string by prepending g_io_module_load_ to it, then the dynamic
> loader is used to find this symbol which needs to be a public exported
> function.
> 
> Then you add a manual call to g_io_module_load_static_module("gnutls") which
> with will trigger this sequence of calls in order to eventually reach
> g_io_module_load_gnutls() which will then just call
> g_tls_backend_gnutls_register() (unloading will never happen anyway).
> 
> Why not just call g_tls_backend_gnutls_register() directly?

The whole point of creating this fake dynamic module is that g_tls_backend_gnutls_register requires a GIOModule as first paramater and this module is reused in several places. Another reason to have a public API to load static modules is that the register function can be named differently on each module as it's private to the module. So it's much easier iterating over a list of modules names and calling g_io_module_load_static_module() than looking at the name of the register function.
Unfortunately I don't know this code that much, so I am completely open to a different implementation.
Comment 18 Alexander Larsson 2012-09-25 12:00:09 UTC
> The whole point of creating this fake dynamic module is that
> g_tls_backend_gnutls_register requires a GIOModule as first paramater
> and this module is reused in several places.

That is not inherent to registering a GIO extension though. Its purely due to the types being created as dynamic types that are to be dlopened. Since you're not actually loading it with dlopen you should register the plugin types as regular static types. This should be no more than a few lines of code changes when building i.e. glib-networking as a static library rather than as a dynamically loaded module.

>Another reason to have a public API to load static modules is that the register 
> function can be named differently on each module as it's private to the module.

I don't think the public api in the statically linked glib-networking library should be "g_tls_backend_gnutls_register()", which is why i said "Well, this should probably be renamed" in the example. Each static library you link to that includes gio plugins needs a nicely namespaced initialization function that initiates the right types and registers them as implementing the right extension points.

For instance, something like:

void
g_gnutls_tls_init (void)
{
  g_io_modules_ensure_extension_points_registered ();
  g_io_extension_point_implement (G_TLS_BACKEND_EXTENSION_POINT_NAME,
				  g_tls_backend_gnutls_get_type(),
				  "gnutls",
				  0);
}

Note how there is no need to register any GTypeModule, because g_tls_backend_gnutls_get_type() returns an ordinary static GType just like any other GType you link to at linktime (or those in your app).
Comment 19 Andoni Morales 2012-09-25 13:31:35 UTC
(In reply to comment #18)

That sounds like a good solution. I'll try with your recommendation as soon as possible
Comment 20 Nicolas Dufresne (ndufresne) 2012-09-27 20:50:22 UTC
I've looked at this today, and I'm thinking we don't event need to add those _init() method. All we have to do is expose the _get_type(), which is as simple as installing the .h when a static build is being done. Maybe I'm over reducing, let me know what you think.

An example assuming you are linking with the static extensions:

void
my_func()
{
  extern GType g_tls_backend_gnutls_get_type (void);
  g_io_modules_ensure_extension_points_registered ();
  g_io_extension_point_implement (G_TLS_BACKEND_EXTENSION_POINT_NAME,
                  g_tls_backend_gnutls_get_type(),
                  "gnutls",
                  0);
}
Comment 21 Sebastian Dröge (slomo) 2012-09-27 21:20:44 UTC
And what my_func() in your example does is something that should be exported by a function with a common signature and name, as in comment #18. This sounds like the way to go IMHO
Comment 22 Nicolas Dufresne (ndufresne) 2012-09-27 21:36:44 UTC
(In reply to comment #21)
> And what my_func() in your example does is something that should be exported by
> a function with a common signature and name, as in comment #18. This sounds
> like the way to go IMHO

The example is just something that can currently be done using the static libraries (demo) (aside that the static library shall not contain the object from the -module.c file). The question is shall we really create an API and expose it (glib-networking does not have this at all at the moment). Or do we leave it to the users to do extern ?
Comment 23 Nicolas Dufresne (ndufresne) 2012-09-27 22:35:08 UTC
Created attachment 225297 [details] [review]
Add static version of the library

Just to give some guidance, this patch show how to get rid of the offending symbols for static build. Instead of doing two build and use ifdef all over, we can simply exclude the object file that contains the offending symbols and create a second static library (with -static suffix).

Next step would be to define the _init() method, add and install appropriate header files (we could simply export the class headers). Also, ensure that the library and header does not get installed when option --disable-static is set. Probably provide .pc files to ease linking. Have fun !
Comment 24 Alexander Larsson 2012-09-28 07:26:00 UTC
I'm somewhat worried about the details of that example.
glibresolver.c has for example has:

#ifdef GLIBPROXY_MODULE
static void
g_libproxy_resolver_class_finalize (GLibProxyResolverClass *klass)
{
}

G_DEFINE_DYNAMIC_TYPE_EXTENDED (GLibProxyResolver,
				g_libproxy_resolver,
				G_TYPE_OBJECT, 0,
				G_IMPLEMENT_INTERFACE_DYNAMIC (G_TYPE_PROXY_RESOLVER,
							       g_libproxy_resolver_iface_init))
#else
G_DEFINE_TYPE_EXTENDED (GLibProxyResolver,
			g_libproxy_resolver,
			G_TYPE_OBJECT, 0,
			G_IMPLEMENT_INTERFACE (G_TYPE_PROXY_RESOLVER,
					       g_libproxy_resolver_iface_init))
#endif


Which is correct, in that if you compile as a module you get a dynamic type and otherwise a static type. And, you didn't add -DGLIBPROXY_MODULE to the -static CFLAGS.

So, it *seems* right. However, are we sure that the object file from the first build that uses glibproxyresolver.c isn't reused for the next library? Does automake automatically rename the object file so that we're guaranteeing it to be built twice?
Comment 25 Colin Walters 2012-09-28 12:47:45 UTC
(In reply to comment #24)
> 
> So, it *seems* right. However, are we sure that the object file from the first
> build that uses glibproxyresolver.c isn't reused for the next library? Does
> automake automatically rename the object file so that we're guaranteeing it to
> be built twice?

You need to use AM_PROG_CC_C_O for that.
Comment 26 Olivier Crête 2014-02-26 08:58:06 UTC
Created attachment 270355 [details] [review]
gio: Add support for static modules

Andoni's patch doesn't actually creates a link to the library but just dlopens the existing program, this means that with more aggressive linking flags, the TLS module is not linked in, this updated/simplified patch just ignore all of the plugin loading code and just calls the load function directly from g_io_module_load_static_module().
Comment 27 Sebastian Dröge (slomo) 2014-03-07 11:33:07 UTC
I think we should just go with what Nicolas said in comment 20, together with a generic function that registers the extension point implementation and gets all the stuff together.
Comment 28 Olivier Crête 2014-06-25 23:55:02 UTC
The problem with the just exporting the _get_type() function is modules which like the gnutls module actually export 2 types (plain gnutls and pkcs11 in this case). That means users of the static modules would need to know they need to register both.
Comment 29 Xavier Claessens 2017-12-01 00:58:50 UTC
Created attachment 364708 [details] [review]
GIOModule: Use unique names for load/unload symbols

GIO modules should include their name into their exported symbols to
make them unique. This avoids symbol clash when building modules
statically.

extract_name() function is copied from GStreamer which recently
switched to the same symbol naming scheme.
Comment 30 Xavier Claessens 2017-12-01 00:58:59 UTC
Created attachment 364709 [details] [review]
GTypeModule: Allow registering static types

This makes easier to write a module that can be both dynamic and static.
It will allow to statically build modules from glib-networking, for
example.
Comment 31 Xavier Claessens 2017-12-01 01:02:00 UTC
I've been thinking a bit about this issue, now that GStreamer fixed their plugins to be able to build them only once for dynamic and static.

Those 2 patches are untested yet, but it borrows the same idea than what has been done in GStreamer. The only change needed in glib-networking to make it support static build is renaming their _load() and _unload() functions.
Comment 32 Nicolas Dufresne (ndufresne) 2017-12-01 02:54:55 UTC
Review of attachment 364709 [details] [review]:

I think there is a GI annotation to say that's it's nullable.
Comment 33 Xavier Claessens 2017-12-01 03:22:24 UTC
Created attachment 364713 [details] [review]
GTypeModule: Allow registering static types

This makes easier to write a module that can be both dynamic and static.
It will allow to statically build modules from glib-networking, for
example.
Comment 34 Xavier Claessens 2017-12-01 03:23:55 UTC
(In reply to Nicolas Dufresne (stormer) from comment #32)
> Review of attachment 364709 [details] [review] [review]:
> 
> I think there is a GI annotation to say that's it's nullable.

I don't see in which case that API could be used in another language, but added (nullable) just in case we finally get a gtkdoc that use those :)
Comment 35 Nicolas Dufresne (ndufresne) 2017-12-01 03:33:08 UTC
I think it's (allow-none), and this has been added to the doc automatically years ago.
Comment 36 Xavier Claessens 2017-12-01 03:39:51 UTC
allow-none is deprecated: https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations/
Comment 37 Nicolas Dufresne (ndufresne) 2017-12-01 14:28:14 UTC
Ok then, somebody failed at telling anyone over the Gnome ML, or it was really quite ....
Comment 38 Philip Withnall 2017-12-01 15:22:44 UTC
(In reply to Nicolas Dufresne (stormer) from comment #37)
> Ok then, somebody failed at telling anyone over the Gnome ML, or it was
> really quite ....

It’s the same as any other API deprecation ((allow-none) still works, it’s just deprecated). i.e. Not announced on a mailing list, but all the documentation has was updated quite a while ago. (Unless you’ve found some documentation which is not up to date?)
Comment 39 Philip Withnall 2017-12-01 17:26:10 UTC
Review of attachment 364708 [details] [review]:

::: gio/giomodule.c
@@ +292,3 @@
 
+static gchar *
+extract_name (const char *filename)

This needs a comment explaining what the function does.

@@ +303,3 @@
+    if (bname[i] == '-')
+      bname[i] = '_';
+  }

Please reformat in GLib’s coding style:

for (…)
  {
    if (…)
      bname[i] = …;
  }

@@ +316,3 @@
+    len = dot - bname - prefix_len;
+  else
+    len = g_utf8_strlen (bname + prefix_len, -1);

The TRUE branch of the if statement computes `len` in bytes; the FALSE branch computes it in unichars.

@@ +325,3 @@
+
+static gboolean
+get_symbols (GIOModule *module)

Perhaps call it `load_symbols` rather than `get_symbols`. `get` implies to me that it should return the symbols rather than setting them on module->[un]load.

@@ +334,3 @@
+  name = extract_name (module->filename);
+  load_symname = g_strconcat("g_io_", name, "_load", NULL);
+  unload_symname = g_strconcat("g_io_", name, "_unload", NULL);

Missing a space before the `(` on these two lines.

@@ +345,3 @@
+  if (!ret)
+    {
+      // Fallback to old names

Please use /* C-style comments */
Comment 40 Philip Withnall 2017-12-01 17:31:39 UTC
(In reply to Xavier Claessens from comment #34)
> (In reply to Nicolas Dufresne (stormer) from comment #32)
> > Review of attachment 364709 [details] [review] [review] [review]:
> > 
> > I think there is a GI annotation to say that's it's nullable.
> 
> I don't see in which case that API could be used in another language, but
> added (nullable) just in case we finally get a gtkdoc that use those :)

gtk-doc has supported (nullable) since 2014 (commit 36b2f2ba5de7cd4dd1bb6d116361b9b9aaef4f4e).
Comment 41 Philip Withnall 2017-12-01 17:33:13 UTC
(In reply to Xavier Claessens from comment #33)
> Created attachment 364713 [details] [review] [review]
> GTypeModule: Allow registering static types
> 
> This makes easier to write a module that can be both dynamic and static.
> It will allow to statically build modules from glib-networking, for
> example.

Do your two patches supersede all the previous patches on this bug, or are they meant to be used together? How do you propose the nullability changes in this patch are used by developers?
Comment 42 Xavier Claessens 2017-12-01 18:22:18 UTC
Other patches are not needed if we go that way.

The idea is that gnutls module will rename its g_io_module_load() function to g_io_gnutls_load(), and when an app static link on libgiognutls.a it will call g_io_gnutls_load(NULL); to register the extension point.
Comment 43 Xavier Claessens 2017-12-01 20:05:59 UTC
Created attachment 364780 [details] [review]
GIOModule: Use unique names for load/unload symbols

GIO modules should include their name into their exported symbols to
make them unique. This avoids symbol clash when building modules
statically.

extract_name() function is copied from GStreamer which recently
switched to the same symbol naming scheme.
Comment 44 Xavier Claessens 2017-12-01 20:06:05 UTC
Created attachment 364781 [details] [review]
GTypeModule: Allow registering static types

This makes easier to write a module that can be both dynamic and static.
It will allow to statically build modules from glib-networking, for
example.
Comment 45 Xavier Claessens 2017-12-01 20:07:47 UTC
Review comments fixed. Also included doc changes, and allow renaming g_io_module_query() too otherwise we still get symbol clash. That one is a bit harder because it's used in a gio-querymodule tool which thus need to share extract_name() code.
Comment 46 Xavier Claessens 2017-12-01 20:55:29 UTC
Created attachment 364782 [details] [review]
GIOModule: Use unique names for load/unload symbols

GIO modules should include their name into their exported symbols to
make them unique. This avoids symbol clash when building modules
statically.

extract_name() function is copied from GStreamer which recently
switched to the same symbol naming scheme.
Comment 47 Xavier Claessens 2017-12-01 20:55:35 UTC
Created attachment 364783 [details] [review]
GTypeModule: Allow registering static types

This makes easier to write a module that can be both dynamic and static.
It will allow to statically build modules from glib-networking, for
example.
Comment 48 Xavier Claessens 2017-12-01 20:56:25 UTC
Voilà, I have now actually tested this and it works with a few changes in glib-networking.
Comment 49 Xavier Claessens 2017-12-01 21:17:41 UTC
See bug #791100, there is a patch to adapt gnutls plugin to support static linking.
Comment 50 Nicolas Dufresne (ndufresne) 2017-12-06 19:12:20 UTC
My opinion is that Xavier's solution is just better, we should obsolete the other.
Comment 51 Xavier Claessens 2017-12-06 19:23:03 UTC
I have the patch needed for cerbero to use this new mechanism ready too.
Comment 52 Philip Withnall 2018-01-04 12:13:20 UTC
Review of attachment 364782 [details] [review]:

A few more changes needed.

::: gio/giomodule-priv.c
@@ +41,3 @@
+  const gchar *dot;
+  gsize prefix_len, len;
+  int i;

s/int/gsize/, since it’s an array index (and also doesn’t need to be signed).

@@ +57,3 @@
+    prefix_len = 0; /* use whole name (minus suffix) as plugin name */
+
+  dot = g_utf8_strchr (bname, -1, '.');

Nitpick: Not sure you need to use g_utf8_strchr() here instead of normal strchr(), since `.` is an ASCII character. It’s not harmful, but it could be a bit confusing (since it immediately makes anyone reading the code start to think in terms of unichars, rather than bytes).

@@ +58,3 @@
+
+  dot = g_utf8_strchr (bname, -1, '.');
+  if (dot)

Nitpick: `if (dot != NULL)`

::: gio/giomodule.h
@@ +118,3 @@
+ * and everything after first dot stripped off (e.g. libgiognutls.so -> gnutls).
+ * Renaming those symbols avoids symbol name clash when building modules
+ * statically.

Please mention that the old names will continue to work (but can’t be built statically), and that `-` is converted to `_`. There are also a few typos and formatting problems here. How about something like:

```
Since 2.56, this function should be named `g_io_<modulename>_load`, where `modulename` is the plugin’s filename with the `lib` or `libgio` prefix and everything after the first dot removed, and with `-` replaced with `_` throughout. For example, `libgiognutls-helper.so` becomes `gnutls_helper`. Using the new symbol names avoids name clashes when building modules statically. The old symbol names continue to be supported, but cannot be used for static builds.
```

@@ +136,3 @@
+ * and everything after first dot stripped off (e.g. libgiognutls.so -> gnutls).
+ * Renaming those symbols avoids symbol name clash when building modules
+ * statically.

(Similar changes as above needed.)

@@ +172,3 @@
+ * and everything after first dot stripped off (e.g. libgiognutls.so -> gnutls).
+ * Renaming those symbols avoids symbol name clash when building modules
+ * statically.

(Similar changes as above needed.)
Comment 53 Philip Withnall 2018-01-04 12:20:00 UTC
Review of attachment 364783 [details] [review]:

Looks reasonable, but please add something like the following (basically what you wrote in comment #42) to the commit message to make the usage clear:

```
A module can rename its g_io_module_load() function to g_io_<modulename>_load(), and then an application which links statically against that module can call g_io_<modulename>_load(NULL) to register types and extension points from the module. If a module is loaded dynamically, its load() function will continue to be called with a non-NULL GIOModule instance.
```
Comment 54 Emmanuele Bassi (:ebassi) 2018-01-04 15:12:01 UTC
Review of attachment 364782 [details] [review]:

::: gio/giomodule.h
@@ +118,3 @@
+ * and everything after first dot stripped off (e.g. libgiognutls.so -> gnutls).
+ * Renaming those symbols avoids symbol name clash when building modules
+ * statically.

We could add a kernel-style macro, like:

```
G_IO_MODULE (modulename)
```

that does the expansion for us, and saves us some headaches.
Comment 55 Xavier Claessens 2018-01-04 16:05:12 UTC
Created attachment 366305 [details] [review]
GIOModule: Use unique names for load/unload symbols

GIO modules should include their name into their exported symbols to
make them unique. This avoids symbol clash when building modules
statically.

extract_name() function is copied from GStreamer which recently
switched to the same symbol naming scheme.
Comment 56 Xavier Claessens 2018-01-04 16:05:19 UTC
Created attachment 366306 [details] [review]
GTypeModule: Allow registering static types

This makes easier to write a module that can be both dynamic and static.
It will allow to statically build modules from glib-networking, for
example.

A module can rename its g_io_module_load() function to
g_io_<modulename>_load(), and then an application which links statically
against that module can call g_io_<modulename>_load(NULL) to register
types and extension points from the module. If a module is loaded
dynamically, its load() function will continue to be called with a
non-NULL GIOModule instance.
Comment 57 Xavier Claessens 2018-01-04 16:12:15 UTC
(In reply to Emmanuele Bassi (:ebassi) from comment #54)
> We could add a kernel-style macro, like:
> 
> ```
> G_IO_MODULE (modulename)
> ```
> 
> that does the expansion for us, and saves us some headaches.

In practice the name is straightforward, see the corresponding commit in glib-networking: bug #791100 attachment #364789 [details].

Also, you would need 3 macros not just one. Not sure it adds clarity, but I'm not against if you think it's really important.
G_IO_MODULE_LOAD(gnutls)
{
}

G_IO_MODULE_UNLOAD(gnutls)
{
}

G_IO_MODULE_QUERY(gnutls)
{
}
Comment 58 Philip Withnall 2018-01-04 16:13:05 UTC
Review of attachment 366305 [details] [review]:

This looks good to me, thanks.
Comment 59 Philip Withnall 2018-01-04 16:13:27 UTC
Review of attachment 366306 [details] [review]:

Yes.
Comment 60 Philip Withnall 2018-01-04 16:14:24 UTC
(In reply to Xavier Claessens from comment #57)
> (In reply to Emmanuele Bassi (:ebassi) from comment #54)
> > We could add a kernel-style macro, like:
> > 
> > ```
> > G_IO_MODULE (modulename)
> > ```
> > 
> > that does the expansion for us, and saves us some headaches.
> 
> In practice the name is straightforward, see the corresponding commit in
> glib-networking: bug #791100 attachment #364789 [details].
> 
> Also, you would need 3 macros not just one. Not sure it adds clarity, but
> I'm not against if you think it's really important.
> G_IO_MODULE_LOAD(gnutls)
> {
> }
> 
> G_IO_MODULE_UNLOAD(gnutls)
> {
> }
> 
> G_IO_MODULE_QUERY(gnutls)
> {
> }

I agree with Xavier; I don’t see the benefits of adding macros for the names, given that the macros would be added at the same time as introducing the name change (so we couldn’t use them to, for example, transparently update old names in existing code).
Comment 61 Xavier Claessens 2018-01-04 16:17:09 UTC
Merged, thanks !