Created attachment 224598 [details] [review]
Initial support for static gio modules in glib-networking

Created attachment 224599 [details]
Sample application

Review of attachment 224598 [details] [review]:

You should use a configure argument rather than a magic define.  --enable-gio-static-module or something.

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.

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.

(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)...

(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.

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

(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.

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 :)

(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.

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.

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.

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);

(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 ?

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

(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.

> 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).

(In reply to comment #18)

That sounds like a good solution. I'll try with your recommendation as soon as possible

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);
}

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

(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 ?

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 !

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?

(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.

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().

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.

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.

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.

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.

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.

Review of attachment 364709 [details] [review]:

I think there is a GI annotation to say that's it's nullable.

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.

(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 :)

I think it's (allow-none), and this has been added to the doc automatically years ago.

allow-none is deprecated: https://wiki.gnome.org/Projects/GObjectIntrospection/Annotations/

Ok then, somebody failed at telling anyone over the Gnome ML, or it was really quite ....

(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?)

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 */

(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).

(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?

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.

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.

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.

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.

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.

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.

Voilà, I have now actually tested this and it works with a few changes in glib-networking.

See bug #791100, there is a patch to adapt gnutls plugin to support static linking.

My opinion is that Xavier's solution is just better, we should obsolete the other.

I have the patch needed for cerbero to use this new mechanism ready too.

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.)

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.
```

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.

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.

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.

(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)
{
}

Review of attachment 366305 [details] [review]:

This looks good to me, thanks.

Review of attachment 366306 [details] [review]:

Yes.

(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).

Merged, thanks !