GNOME Bugzilla – Bug 727663
W32: libgedit.dll loading and dependence are not quite right
Last modified: 2014-04-10 13:57:20 UTC
This was discussed on IRC already, so here's a short recap: There's libgedit.[so/dll] now, which holds the bulk of the gedit code. Gedit app is just a small program that calls a function from libgedit. Thus libgedit is not a normal library - it is not versioned, and other modules should not link to it, except for gedit plugins, which do have to link to it. On W32 this means one of the following: A) libgedit.dll must in bin subdirectory, where gedit.exe is B) libgedit.dll must be somewhere in PATH C) libgedit.dll can be anywhere, gedit.exe should load it manually when it starts. I don't like (A) for aesthetic reasons, i don't like (B) because playing with PATH is evil. (C) is kind of OK, but somewhat imperfect, and will force gedit to have separate W32-only gedit.c, and this does not scale well. So i opted to make libgedit a delay-loaded DLL, which is kind of like (C), except that it requires extra configure/makefile/libtool magic to pull off, but needs little W32-only code in gedit.c (and the amount of that code does not depend on how many functions we need to load from libgedit).
Created attachment 273620 [details] [review] W32: Remove custom libgedit.a import library Since we have a real, shared libgedit.dll, we can have a real libgedit.dll.a import library.
Created attachment 273621 [details] [review] Plugins need code from shared libgedit, so link them to it
Created attachment 273622 [details] [review] W32: Override libtool internal variables to produce a delay-load import library We remove --out-implib, add --output-def (where needed), and add extra call to dlltool to generate delay-load import library (name's the same as normal import library, so tools/applications won't notice the change). When application is linked to libgedit via such import library, the DLL is only loaded when first function from it is called. This gives us time to influence W32 runtime linker.
Created attachment 273623 [details] [review] W32: load libgedit.dll at runtime Since libgedit is linked as delay-loaded DLL, it's still not loaded when main() is called. Before we call functions from it, we must locate libgedit.dll and load it (otherwise, if it's not in PATH (which is how it should be!), calling its functions will crash the program). As comments indicate, the use of g_module_open() is not really necessary, but, given the circumstances, it is convenient and clean.
Created attachment 273624 [details] [review] Make gedit a module library This tells libtool to warn us about linking to it (the warnings are faux, but i don't know how to avoid them) AND to set shouldnotlink=yes in libgedit.la, which will prevent special W32 libtool logic from kicking in, which will prevent libtool from installing libgedit.dll into bindir. The alternative is to not to add -module, but just move libgedit.dll and libgedit.dll.a after they are installed.
Created attachment 273625 [details] [review] W32: Fix .pc file LDFLAGS (assuming libgedit is in <prefix>/lib/gedit)
The patch that adds $(top_builddir)/gedit/libgedit.la everywhere may be further refined to make it W32-only (i.e. $(top_builddir)/gedit/libgedit.la will be a result of expansion of a variable that expands to nothing on non-W32 platforms), if you think that linking to libgedit like that is no acceptable on *nix.
Review of attachment 273620 [details] [review]: See the minor comment, apart from that looks good. ::: gedit/Makefile.am @@ +304,3 @@ install-exec-hook: if PLATFORM_WIN32 $(mkinstalldirs) "$(DESTDIR)$(libdir)" can this be removed now too?
Review of attachment 273621 [details] [review]: ok
Review of attachment 273622 [details] [review]: See comment. ::: configure.ac @@ +66,3 @@ + echo EXPORTS > $output_objdir/$soname.def; + cat $export_symbols >> $output_objdir/$soname.def; + fi~ is this ~ a typo?
Review of attachment 273623 [details] [review]: See the comments. ::: gedit/gedit.c @@ +36,3 @@ +#ifdef G_OS_WIN32 +#include <gmodule.h> +GModule *libgedit_dll = NULL; this should be static. @@ +41,3 @@ + * point is to find and load libgedit.dll. + */ +void static void @@ +45,3 @@ +{ + gchar *dllpath; + gchar *prefix = g_win32_get_package_installation_directory_of_module (NULL); empty line after declaring vars @@ +46,3 @@ + gchar *dllpath; + gchar *prefix = g_win32_get_package_installation_directory_of_module (NULL); + if (NULL == prefix) use prefix == NULL and add {} In gedit we have all the conditions even if single with {} @@ +48,3 @@ + if (NULL == prefix) + return; + /* Instead of g_module_open () it may be possible to do any of the empty line before this part @@ +62,3 @@ +} + +void static void @@ +65,3 @@ +gedit_w32_unload_private_dll () +{ + if (libgedit_dll) use {}
Review of attachment 273624 [details] [review]: See comment ::: gedit/Makefile.am @@ +33,3 @@ gedit_libgedit_la_CPPFLAGS = $(gedit_common_cppflags) gedit_libgedit_la_CFLAGS = $(gedit_common_cflags) +gedit_libgedit_la_LDFLAGS = -module -avoid-version -export-dynamic -no-undefined -export-symbols-regex "^[^_].*" should this be win32 specific?
Review of attachment 273625 [details] [review]: OK
(In reply to comment #8) > Review of attachment 273620 [details] [review]: > > See the minor comment, apart from that looks good. > > ::: gedit/Makefile.am > @@ +304,3 @@ > install-exec-hook: > if PLATFORM_WIN32 > $(mkinstalldirs) "$(DESTDIR)$(libdir)" > > can this be removed now too? Yes, maybe. It may have to be re-added, if you decide to not to accept -module patch and move libgedit around after it's installed by way of makefile rules (not sure how it would look, i'm not a makefile guru)
(In reply to comment #10) > Review of attachment 273622 [details] [review]: > > See comment. > > ::: configure.ac > @@ +66,3 @@ > + echo EXPORTS > $output_objdir/$soname.def; > + cat $export_symbols >> $output_objdir/$soname.def; > + fi~ > > is this ~ a typo? I copied this verbatim from libtool. This '~' also got me wondering about its purpose. But things seem to work OK, so i left it as it was.
(In reply to comment #11) > Review of attachment 273623 [details] [review]: > > See the comments. > > > @@ +46,3 @@ > + gchar *dllpath; > + gchar *prefix = g_win32_get_package_installation_directory_of_module (NULL); > + if (NULL == prefix) > > use prefix == NULL and add {} In gedit we have all the conditions even if > single with {} Is "prefix == NULL" a matter of style? The reversal is to guard against "prefix = NULL" kind of typos, so i'd like to keep it, if i can.
(In reply to comment #12) > Review of attachment 273624 [details] [review]: > > See comment > > ::: gedit/Makefile.am > @@ +33,3 @@ > gedit_libgedit_la_CPPFLAGS = $(gedit_common_cppflags) > gedit_libgedit_la_CFLAGS = $(gedit_common_cflags) > +gedit_libgedit_la_LDFLAGS = -module -avoid-version -export-dynamic > -no-undefined -export-symbols-regex "^[^_].*" > > should this be win32 specific? Yes. The question is whether it should be added at all (as it causes libtool to spew warnings).
> On W32 this means one of the following: > A) libgedit.dll must in bin subdirectory, where gedit.exe is > B) libgedit.dll must be somewhere in PATH > C) libgedit.dll can be anywhere, gedit.exe should load it manually when > it starts. > (not reviewing single patches since I am writing from the phone) I am ok with the approach you are taking (nacho, it is similar to what we do at work for that gtk ui right?) but I would also not particularly mind just dropping the dll in bin... win32 and aesthethics are not a good match anyway :-) Another thing that occurred to me is that we can set some stuff from the installer if this makes things easier than finding things relative to the exe position question since I am a bit rusty about gtk win32 stuff: how do we find gtk.dll etc? if they are in bin too then the exercise is a bit pointless, if instead they are in lib why do we need to handle libgedit.dll manually?
(In reply to comment #18) > > On W32 this means one of the following: > > A) libgedit.dll must in bin subdirectory, where gedit.exe is > > B) libgedit.dll must be somewhere in PATH > > C) libgedit.dll can be anywhere, gedit.exe should load it manually when > > it starts. > > > > Another thing that occurred to me is that we can set some stuff from the > installer if this makes things easier than finding things relative to the exe > position > > question since I am a bit rusty about gtk win32 stuff: how do we find gtk.dll > etc? if they are in bin too then the exercise is a bit pointless, if instead > they are in lib why do we need to handle libgedit.dll manually? gtk (and all other DLLs) is in bin. They are in bin exactly because W32 linker normally can't be taught to look elsewhere. Otherwise DLLs would have been in lib. I don't want libgedit.dll in bin for the same reason why plugins are not in bin - these are private libraries that noone is supposed to link to. They have no version number. libgedit is also a private library (the only application that loads it is gedit; plugins get it only because gedit already has it when plugins are loaded) without a version number.
Considering that the gedit installer ends up installing stuff in C:\program files\gedit\ I am not very concerned about mixing public and private .dll in bin: *all* dll installed by gedit (including its copy of gtk etc) are private. That said you already made the patch to set the path so I do not mind much one way or the other
Created attachment 273985 [details] [review] W32: load libgedit.dll at runtime (v2) Since libgedit is linked as delay-loaded DLL, it's still not loaded when main() is called. Before we call functions from it, we must locate libgedit.dll and load it (otherwise, if it's not in PATH (which is how it should be!), calling its functions will crash the program). As comments indicate, the use of g_module_open() is not really necessary, but, given the circumstances, it is convenient and clean. v2: Fixed code style. Hooked type assignment onto DLL-loading routine to make sure that it's not done before DLL is loaded.
Created attachment 273986 [details] [review] W32: Move installed libgedit.dll into appropriate dir If libgedit is not made a -module library, it's installed into a weird place, and we have to fix the installation (this is where that install-exec-hook came in handy again).
Created attachment 273988 [details] [review] W32: load libgedit.dll at runtime (v3) Since libgedit is linked as delay-loaded DLL, it's still not loaded when main() is called. Before we call functions from it, we must locate libgedit.dll and load it (otherwise, if it's not in PATH (which is how it should be!), calling its functions will crash the program). As comments indicate, the use of g_module_open() is not really necessary, but, given the circumstances, it is convenient and clean. v2: Fixed code style. Hooked type assignment onto DLL-loading routine to make sure that it's not done before DLL is loaded. v3: Fixed code style again.
Review of attachment 273988 [details] [review]: Looks good.
Review of attachment 273986 [details] [review]: ok
Review of attachment 273622 [details] [review]: ok
Attachment 273620 [details] pushed as 0ec205b - W32: Remove custom libgedit.a import library Attachment 273621 [details] pushed as f9931fd - Plugins need code from shared libgedit, so link them to it Attachment 273622 [details] pushed as a321118 - W32: Override libtool internal variables to produce a delay-load import library Attachment 273988 [details] pushed as b2aa455 - W32: load libgedit.dll at runtime Attachment 273625 [details] pushed as e45b10a - W32: Fix .pc file LDFLAGS (assuming libgedit is in <prefix>/lib/gedit) Attachment 273986 [details] pushed as 76d1b63 - W32: Move installed libgedit.dll into appropriate dir