GNOME Bugzilla – Bug 675977
Don't hard-code library search paths
Last modified: 2012-05-18 03:30:41 UTC
Distributions like Debian or Ubuntu use multiarch paths for libraries [1], Fedora and others use /usr/lib64 for 64bit installations. Hard-coding the library search paths [2] when trying to load the glade catalog files breaks in such environments. The attached patch avoids this problem by calling g_module_open () without a directory component, so it searches the default system paths, which is basically what we want here. The order is like this: l/ glade modules dir 2/ install prefix / lib of glade 3/ /usr/local/lib 4/ system library paths. [1] http://wiki.debian.org/Multiarch/TheCaseForMultiarch [2] http://git.gnome.org/browse/glade/tree/gladeui/glade-utils.c#n883
Created attachment 213961 [details] [review] [PATCH] Don't hard-code library search paths
Review of attachment 213961 [details] [review]: Hard coded stuff is usually bad, so I really want to improve things in this regard. I wont apply the patch as is, because of the comments, but I will fix the problem. ::: gladeui/glade-utils.c @@ +858,3 @@ + path = g_module_build_path (library_path, library_name); + else + path = g_module_build_path (NULL, library_name); This if statement is the same as path = g_module_build_path (library_path, library_name); So this change is not needed @@ -884,3 @@ NULL, /* <-- dynamically allocated */ - "/lib", - "/usr/lib", if you remove these, then all the corresponding multi arch paths will be used by g_module_open() right? I am currently using Debian sid (amd64) and it only looks in /lib/ and /usr/lib
Created attachment 214285 [details] [review] Proposed patch Cleans up glade_util_load_library() a bit and makes sure it calls try_load_library() with NULL as library_path so that g_module_open() has a chance to load the library from the system defaults paths
Review of attachment 214285 [details] [review]: This seems a bit guessy: lib_dir = g_build_filename (prefix, "lib", NULL); But it's fine to apply this as is.. the default search path should anyway be covered by the NULL parameter to the g_module_ api (at least it "should" for the various platforms) and the GLADE_LIBDIR is important on normal unix/linux builds because it will definitely evaluate to 'lib64' wherever appropriate. Other than that I like the patch overall, one stylistic detail is that I think it would be nicer if you use G_N_ELEMENTS() rather than doing the devision manually.
(In reply to comment #2) > Review of attachment 213961 [details] [review]: > > Hard coded stuff is usually bad, so I really want to improve things in this > regard. > I wont apply the patch as is, because of the comments, but I will fix the > problem. > > ::: gladeui/glade-utils.c > @@ +858,3 @@ > + path = g_module_build_path (library_path, library_name); > + else > + path = g_module_build_path (NULL, library_name); > > This if statement is the same as > path = g_module_build_path (library_path, library_name); > So this change is not needed Yeah, makes sense. > > @@ -884,3 @@ > NULL, /* <-- dynamically allocated */ > - "/lib", > - "/usr/lib", > > if you remove these, then all the corresponding multi arch paths will be used > by g_module_open() right? > I am currently using Debian sid (amd64) and it only looks in /lib/ and /usr/lib If you are using the Debian glib packages, it should search in /lib, /usr/lib, /lib/<triplet>, /usr/lib/<triplet> I've tested and use this patch in the Debian glade package to load libraries from both multiarch and non-multiarch library paths. See http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=666241
(In reply to comment #3) > Created an attachment (id=214285) [details] [review] > Proposed patch > > Cleans up glade_util_load_library() a bit and makes sure it calls > try_load_library() with NULL as library_path so that g_module_open() has a > chance to load the library from the system defaults paths Was the following dropped on purpose: - if (!module) - g_critical ("Unable to load module '%s' from any search paths", - library_name); Imho it's a good idea to print an error message if the library could not be found.
(In reply to comment #4) > Review of attachment 214285 [details] [review]: > > This seems a bit guessy: > lib_dir = g_build_filename (prefix, "lib", NULL); Yes it is, but note its only for windows and mac, btw we are currently using g_build_filename (modules_dir, "..", "..", NULL) which is the same. [...] > Other than that I like the patch overall, one stylistic detail > is that I think it would be nicer if you use G_N_ELEMENTS() > rather than doing the devision manually. yeah I forgot about it BTW its fixed in master and glade-3-12
[...] > If you are using the Debian glib packages, it should search in /lib, /usr/lib, > /lib/<triplet>, /usr/lib/<triplet> I see, I was using a jhbuild version of glib :) > I've tested and use this patch in the Debian glade package to load libraries > from both multiarch and non-multiarch library paths. > See http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=666241
(In reply to comment #6) > (In reply to comment #3) > > Created an attachment (id=214285) [details] [review] [details] [review] > > Proposed patch > > > > Cleans up glade_util_load_library() a bit and makes sure it calls > > try_load_library() with NULL as library_path so that g_module_open() has a > > chance to load the library from the system defaults paths > > Was the following dropped on purpose: > > - if (!module) > - g_critical ("Unable to load module '%s' from any search paths", > - library_name); > > Imho it's a good idea to print an error message if the library could not be > found. yes, because it was being printed twice, btw its only removed in master