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 675977 - Don't hard-code library search paths
Don't hard-code library search paths
Status: RESOLVED FIXED
Product: glade
Classification: Applications
Component: general
3.12.x
Other Linux
: Normal normal
: ---
Assigned To: Glade 3 Maintainers
Glade 3 Maintainers
Depends on:
Blocks:
 
 
Reported: 2012-05-13 16:48 UTC by Michael Biebl
Modified: 2012-05-18 03:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] Don't hard-code library search paths (2.17 KB, patch)
2012-05-13 16:49 UTC, Michael Biebl
none Details | Review
Proposed patch (4.82 KB, patch)
2012-05-17 23:07 UTC, Juan Pablo Ugarte
none Details | Review

Description Michael Biebl 2012-05-13 16:48:23 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
Comment 1 Michael Biebl 2012-05-13 16:49:44 UTC
Created attachment 213961 [details] [review]
[PATCH] Don't hard-code library search paths
Comment 2 Juan Pablo Ugarte 2012-05-17 23:01:43 UTC
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
Comment 3 Juan Pablo Ugarte 2012-05-17 23:07:03 UTC
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
Comment 4 Tristan Van Berkom 2012-05-18 01:21:45 UTC
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.
Comment 5 Michael Biebl 2012-05-18 02:45:33 UTC
(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
Comment 6 Michael Biebl 2012-05-18 02:54:31 UTC
(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.
Comment 7 Juan Pablo Ugarte 2012-05-18 03:18:08 UTC
(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
Comment 8 Juan Pablo Ugarte 2012-05-18 03:19:14 UTC
[...]
> 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
Comment 9 Juan Pablo Ugarte 2012-05-18 03:30:41 UTC
(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