Created attachment 149907 [details] [review]
Support loading C plugins from the source directory

Add a "load-from-sources" property to the engine,
add a set_load_from_sources vfunc to the loader and
use it in the C loader.

Note that we need to use the ".libs" directory because
the module loader constructs its name using g_module_build_path()

Review of attachment 149907 [details] [review]:

How do you plan to use this new property?
The typical usage I'd see and document would be:

> #ifdef ENABLE_UNINSTALLED_BUILD
>   peas_engine_set_load_from_source (engine, TRUE);
> #endif

So I'd like to see such a method added to the engine for that. What do you think?

Also, in your patch it looks like there is no way for the new loaders to inherit that load-from-source property value: you set it for every available loaders, but you don't set it on a new loader instantiation, so if, say, the C loader is instanciated after you set the property on the engine, it won't actually be able to load the plugins from source unless you set the property once again.

::: libpeas/peas-engine.c
@@ +319,3 @@
+      g_hash_table_foreach (engine->priv->loaders,
+			    (GHFunc) loader_set_load_from_sources,
+			    engine);

IMHO this should be moved to peas_engine_set_load_from_source()

(In reply to comment #2)
> Review of attachment 149907 [details] [review]:
> 
> How do you plan to use this new property?
> The typical usage I'd see and document would be:
> 
> > #ifdef ENABLE_UNINSTALLED_BUILD
> >   peas_engine_set_load_from_source (engine, TRUE);
> > #endif
> 
> So I'd like to see such a method added to the engine for that. What do you
> think?

You can't do that. It needs to be a run-time property, because I'm not interested in installing a different version of libpeas when I want to run a single-program from its source tree.

This could cause SELinux failures for applications that aren't run from the source tree, as they look for folders they shouldn't.

> Also, in your patch it looks like there is no way for the new loaders to
> inherit that load-from-source property value: you set it for every available
> loaders, but you don't set it on a new loader instantiation, so if, say, the C
> loader is instanciated after you set the property on the engine, it won't
> actually be able to load the plugins from source unless you set the property
> once again.

Nod, should be fixed.

> ::: libpeas/peas-engine.c
> @@ +319,3 @@
> +      g_hash_table_foreach (engine->priv->loaders,
> +                (GHFunc) loader_set_load_from_sources,
> +                engine);
> 
> IMHO this should be moved to peas_engine_set_load_from_source()

OK.

(In reply to comment #3)
> (In reply to comment #2)
> > Review of attachment 149907 [details] [review] [details]:
> > 
> > How do you plan to use this new property?
> > The typical usage I'd see and document would be:
> > 
> > > #ifdef ENABLE_UNINSTALLED_BUILD
> > >   peas_engine_set_load_from_source (engine, TRUE);
> > > #endif
> > 
> > So I'd like to see such a method added to the engine for that. What do you
> > think?
> 
> You can't do that. It needs to be a run-time property, because I'm not
> interested in installing a different version of libpeas when I want to run a
> single-program from its source tree.

Well actually I meant that sample of code would be used in the application, not in the library, so the library would have the property all the time, just that it would not be set by the app when it's installed in /usr, let's say.

Do you have another idea on how an application should set this property when required?

> This could cause SELinux failures for applications that aren't run from the
> source tree, as they look for folders they shouldn't.

I don't know anything about selinux, really :-(

Created attachment 150295 [details] [review]
Support loading C plugins from the source directory

By letting g_module_open() handle trying to load the libtool
archives if available.

Created attachment 150297 [details] [review]
Support loading C plugins from the source directory

By letting g_module_open() handle trying to load the libtool
archives if available.

g_module_build_path seems to actually have an incoherent behaviour given how you specify the module name. It seems to assume the module_name is a filename if it begins with 'lib' and so it's not necessary to append .so in that case.

This is the gmodule-dl.c implementation of g_module_build_path()

> static gchar*
> _g_module_build_path (const gchar *directory,
>                       const gchar *module_name)
> {
>   if (directory && *directory) {
>     if (strncmp (module_name, "lib", 3) == 0)
>       return g_strconcat (directory, "/", module_name, NULL);
>     else
>       return g_strconcat (directory, "/lib",
>                           module_name, "." G_MODULE_SUFFIX,
>                           NULL);
>   } else if (strncmp (module_name, "lib", 3) == 0)
>     return g_strdup (module_name);
>   else
>     return g_strconcat ("lib", module_name, "." G_MODULE_SUFFIX, NULL);
> }

So it looks like having the soname beginning with 'lib' in the .totem-plugin file should just fix the issue without further pain, instead of not specifying the 'lib' prefix as it is usually the case.

The remaining issue would then be to not pass the .so suffix to peas_object_new when creating loader modules:

-  loader_basename = g_strdup_printf ("lib%sloader.%s", loader_id, G_MODULE_SUFFIX);
+  loader_basename = g_strdup_printf ("lib%sloader", loader_id, G_MODULE_SUFFIX);

Now, we could also drop our use of g_module_build_path and just use g_build_path instead. This would allow one not to use the 'lib' prefix for their modules filenames (I don't know if libtool requires it, but I know other software on linux that don't use a 'lib' prefix for their modules and work just fine), and would avoid having the weird case when you don't specify the lib prefix so g_module prepends lib and appends '.so'. Now we would have to know how it behaves under other systems.

Thinking about it, the loader stuff is useless as it's private api to the lib which is much likely to be installed anyway.

(In reply to comment #7)
> g_module_build_path seems to actually have an incoherent behaviour given how
> you specify the module name. It seems to assume the module_name is a filename
> if it begins with 'lib' and so it's not necessary to append .so in that case.
> 
> This is the gmodule-dl.c implementation of g_module_build_path()
> 
> > static gchar*
> > _g_module_build_path (const gchar *directory,
> >                       const gchar *module_name)
> > {
> >   if (directory && *directory) {
> >     if (strncmp (module_name, "lib", 3) == 0)
> >       return g_strconcat (directory, "/", module_name, NULL);
> >     else
> >       return g_strconcat (directory, "/lib",
> >                           module_name, "." G_MODULE_SUFFIX,
> >                           NULL);
> >   } else if (strncmp (module_name, "lib", 3) == 0)
> >     return g_strdup (module_name);
> >   else
> >     return g_strconcat ("lib", module_name, "." G_MODULE_SUFFIX, NULL);
> > }
> 
> So it looks like having the soname beginning with 'lib' in the .totem-plugin
> file should just fix the issue without further pain, instead of not specifying
> the 'lib' prefix as it is usually the case.

Except that would break portability to Windows, and other platforms where a prefix isn't used for shared libraries.

> The remaining issue would then be to not pass the .so suffix to peas_object_new
> when creating loader modules:
> 
> -  loader_basename = g_strdup_printf ("lib%sloader.%s", loader_id,
> G_MODULE_SUFFIX);
> +  loader_basename = g_strdup_printf ("lib%sloader", loader_id,
> G_MODULE_SUFFIX);
> 
> Now, we could also drop our use of g_module_build_path and just use
> g_build_path instead. This would allow one not to use the 'lib' prefix for
> their modules filenames (I don't know if libtool requires it, but I know other
> software on linux that don't use a 'lib' prefix for their modules and work just
> fine), and would avoid having the weird case when you don't specify the lib
> prefix so g_module prepends lib and appends '.so'. Now we would have to know
> how it behaves under other systems.

Or you could use the code I submitted above which will just strip the G_MODULE_SUFFIX, thus require the backend to try and load the .la file (for uninstalled tests), and then the module with the G_MODULE_SUFFIX if not available.

That would also work on Windows, and the other implementations of the module loading available in glib/gmodule.

I've been doing a bit of research to try and understand the implication of everything...

(In reply to comment #9)
> Except that would break portability to Windows, and other platforms where a
> prefix isn't used for shared libraries.

Considering Windows/mingw, libtool will just generate libhelloworld.dll on windows too, and g_module_build_path will prepend a "lib" and append a ".dll" too if those are not present in the path. Actually, all the gmodule backends behave this way, apart from os2 (it doesn't prepend anything) and win32/cygwin (it prepends 'cyg' instead of 'lib', supposedly to avoid conflicting names).
So indeed, it looks like using "libhelloworld" in the .totem-plugin file will actually break on (and only on) cygwin. Special casing sucks.

The incriminated libtool bit that replaces the lib prefix:
cygwin*)
soname_spec='`echo ${libname} | sed -e 's/^lib/cyg/'``echo ${release} | $SED -e 's/[[.]]/-/g'`${versuffix}${shared_ext}'

But thinking of it, plugins are not simple shared libraries "with the lib prefix", they are not forced to have the "lib" prefix at all, as libtool's -module argument withdraws this requirement, so we could actually have helloworld.so instead of libhelloworld.so... This is what gtk+ immodules do fwiw, despite the 'lib' prefix seems to be used pretty much everywhere else in the gtk+ ecosystem...

> Or you could use the code I submitted above which will just strip the
> G_MODULE_SUFFIX, thus require the backend to try and load the .la file (for
> uninstalled tests), and then the module with the G_MODULE_SUFFIX if not
> available.

Actually it will try the submitted name first, then the name + .so/.dll, then the name + .la, the doc seems to be wrong if I read the code correctly. So it'd be indeed even less costly that what you expected.

> That would also work on Windows, and the other implementations of the module
> loading available in glib/gmodule.

I must admit I saw the latest developments in this bug as an opportunity to "fix" the difference between the module name and the actual file name, but I guess I'll have to live with it (or hack a s/^lib/cyg/ replacement if G_WITH_CYGWIN too as others do, but this is clearly bad).

Created attachment 150350 [details] [review]
Drop the 'lib' prefix

Only for the record, above is an attempt at dropping the 'lib' prefix in generated modules. It should work pretty much everywhere as long as the module name doesn't start with 'lib'.

I've commited a variant of patch 150297. libpeas is now able to load plugins from source directory :-)