Re: [PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)
Hi,
Mark H Weaver m...@netris.org skribis:
I wrote:
I've read the message referenced above several times, but I've failed to
understand why we cannot use 'lt_dladdsearchdir' to augment the path, as
shown in the first code excerpt of that message:
env = getenv (GUILE_SYSTEM_EXTENSIONS_PATH);
[...]
if (env)
lt_dladdsearchdir (env);
else
{
lt_dladdsearchdir (SCM_LIB_DIR);
lt_dladdsearchdir (SCM_EXTENSIONS_DIR);
}
Actually, I can already see a problem with this. SCM_LIB_DIR is usually
(always?) already present in the library search paths before Guile is
initialized. It is set to $(libdir) by libguile/Makefile. Isn't
$(libdir) where libguile is installed?
In 1.8 and earlier, extensions were installed under $libdir, and it’s
just an obvious convenience to ensure that $libdir is in the search
path.
Note that lt_dladdsearchdir /appends/ its arguments to the search path.
[...]
With this in mind, I now propose the following:
env = getenv (GUILE_SYSTEM_EXTENSIONS_PATH);
if (env)
lt_dladdsearchdir (env);
else
lt_dladdsearchdir (SCM_EXTENSIONS_DIR);
and then to run the uninstalled guile, we'd need to set (or augment) one
of the *_LIBRARY_PATH environment variables to point within the build
directory, and also set GUILE_SYSTEM_EXTENSIONS_PATH.
What do you think?
First, I think we don’t want to change the behavior at all in 2.0,
because that’s a sure way to surely get reports of angry users.
Other than that, in 2.1 we could consider dropping $libdir like this,
and making sure all extensions get installed in the right place. That
would probably make sense.
On a related topic, why is libguilereadline-v-18.* installed in
SCM_LIB_DIR and not SCM_EXTENSIONS_DIR?
Historical reasons. We should probably change it in 2.1, and also
remove the “lib” prefix.
WDYT?
Ludo’.
Re: [PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)
I wrote:
I've read the message referenced above several times, but I've failed to
understand why we cannot use 'lt_dladdsearchdir' to augment the path, as
shown in the first code excerpt of that message:
env = getenv (GUILE_SYSTEM_EXTENSIONS_PATH);
[...]
if (env)
lt_dladdsearchdir (env);
else
{
lt_dladdsearchdir (SCM_LIB_DIR);
lt_dladdsearchdir (SCM_EXTENSIONS_DIR);
}
Actually, I can already see a problem with this. SCM_LIB_DIR is usually
(always?) already present in the library search paths before Guile is
initialized. It is set to $(libdir) by libguile/Makefile. Isn't
$(libdir) where libguile is installed?
A common case where this causes problems (the same one Bruce ran into)
is when SCM_LIB_DIR is /usr/lib, but the user has configured
/usr/local/lib to precede /usr/lib in the library search path.
Therefore, we must not add SCM_LIB_DIR to LD_LIBRARY_PATH, nor to
LTDL_LIBRARY_PATH, nor via 'lt_dladdsearchdir', or else we will reverse
the ordering of /usr/local/lib and /usr/lib that many people depend
upon. The only possible option is to manually search it after all else
has failed, but it seems to me that we shouldn't be adding SCM_LIB_DIR
at all. It should already be there.
SCM_EXTENSIONS_DIR is another matter. It is unlikely to already be
present in the library search path, and in any case should not contain
anything other than Guile extensions. Therefore, it is appropriate to
do: lt_dladdsearchdir (SCM_EXTENSIONS_DIR) unless
GUILE_SYSTEM_EXTENSIONS_PATH is set.
With this in mind, I now propose the following:
--8---cut here---start-8---
env = getenv (GUILE_SYSTEM_EXTENSIONS_PATH);
if (env)
lt_dladdsearchdir (env);
else
lt_dladdsearchdir (SCM_EXTENSIONS_DIR);
--8---cut here---end---8---
and then to run the uninstalled guile, we'd need to set (or augment) one
of the *_LIBRARY_PATH environment variables to point within the build
directory, and also set GUILE_SYSTEM_EXTENSIONS_PATH.
What do you think?
On a related topic, why is libguilereadline-v-18.* installed in
SCM_LIB_DIR and not SCM_EXTENSIONS_DIR?
Thanks,
Mark
Re: [PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)
Hi, Mark H Weaver m...@netris.org skribis: l...@gnu.org (Ludovic Courtès) writes: Mark H Weaver m...@netris.org skribis: Following Bruce's suggestion, it causes 'sysdep_dynl_link' to manually search additional directories if 'lt_dlopenext' fails to find the library in the default paths. Thus, that doesn’t solve the problem described at http://lists.gnu.org/archive/html/guile-devel/2010-11/msg00095.html, right? To solve it, we’d have to do our own lookup unconditionally. [...] As I understand it, the reason given for why we cannot use that approach is that 'libtool --mode=execute -dlopen' would not work properly, Exactly. but why do we have to do it that way? It is a fact that some projects (at least some of mine) have been using that idiom, because that’s the Libtool way to say “hey, load this particular file, not one that may be found in the search path.” See, for example, http://git.savannah.gnu.org/cgit/gnutls.git/tree/guile/pre-inst-guile.in. So the goal is to keep that working. Ideally, I would accept any solution that (1) gets rid of the LD_LIBRARY_PATH export, and (2) can be shown with strace to preserve the extension search order. How does that sound? (BTW, the above message was followed-up at http://lists.gnu.org/archive/html/guile-devel/2011-02/msg00075.html.) Thanks, Ludo’.
Re: [PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)
l...@gnu.org (Ludovic Courtès) writes: Mark H Weaver m...@netris.org skribis: As I understand it, the reason given for why we cannot use that approach is that 'libtool --mode=execute -dlopen' would not work properly, Exactly. but why do we have to do it that way? It is a fact that some projects (at least some of mine) have been using that idiom, Do these projects also set GUILE_SYSTEM_EXTENSIONS_PATH? because that’s the Libtool way to say “hey, load this particular file, not one that may be found in the search path.” I would refine this to load this particular file, not one that may be found in LD_LIBRARY_PATH or the default system library search paths, *unless* the library is found in either LTDL_LIBRARY_PATH or in some path added via 'lt_dladdsearchdir' et al. In other words, libtool's own preferred mechanism for programs to augment their internal search paths gives those internal paths priority over the files specified by 'libtool --mode=execute -dlopen'. So it seems to me that either this is a bug in libtool that should be fixed, or it is not appropriate to expect this idiom to override a program's extensions. Having said that, I acknowledge that the existence of programs that depend upon this idiom to override Guile's SCM_EXTENSIONS_DIR force us to accommodate them somehow, at least in 2.0. Ideally, I would accept any solution that (1) gets rid of the LD_LIBRARY_PATH export, and (2) can be shown with strace to preserve the extension search order. As I pointed out in my recent followup, the current extension search order is fundamentally broken, because in the common case of SCM_LIB_DIR=/usr/lib, it typically puts /usr/lib before /usr/local/lib in the search path. This will cause problems for users that depend upon /usr/local/lib preceding /usr/lib when loading libraries from Guile. Therefore, I don't think we should try to exactly preserve the current broken search order. At the very least, I think we need to avoid adding SCM_LIB_DIR anywhere other than as a last resort (which means a manual search), and perhaps not even then. For SCM_EXTENSIONS_DIR, if we need to allow programs to override it using 'libtool --mode=execute -dlopen' (and without setting GUILE_SYSTEM_EXTENSIONS_PATH) then I guess the only good option is to manually search for SCM_EXTENSIONS_DIR after all else has failed. Does that make sense? The one remaining question is where to put a non-empty GUILE_SYSTEM_EXTENSIONS_PATH in the library search paths. It's hard for me to answer that question without knowing how this variable is supposed to be used. It's not documented, but its name suggests that it should override the value of SCM_EXTENSIONS_DIR, and therefore be put where SCM_EXTENSIONS_DIR would have been put. To my mind, a sensible use for this variable is to allow the user to place Guile extensions in directories other than $(libdir)/guile/2.0/extensions/, for example somewhere within their home directory. So a user might reasonably set this variable in their shell startup files. However, if they set the variable to anything at all, then they would also need to include $(libdir)/guile/2.0/extensions/ somewhere within it. If we want your idiom to override that directory, then GUILE_SYSTEM_EXTENSIONS_PATH also needs to be placed where SCM_EXTENSIONS_DIR would have been put. I realize that backward compatibility is important, but it's also important to fix broken behavior and improve things over time, when it can be done without significantly burdening users in practice. What do you think? Mark
Re: [PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)
Hi Mark, Just a quick note. Now with GUILE_SYSTEM_EXTENSIONS_PATH, this could be very practical when developing a module. However, if a module is already present AND is inside SCM_LIB_DIR or SCM_EXTENSIONS_DIR, it would never override that situation. I believe there should be a way to be able to override the normal behavior. Regards, Sjoerd On 10/03/2012 12:31 PM, Mark H Weaver wrote: Hello all, Here's a preliminary patch to avoid modifying LD_LIBRARY_PATH. Following Bruce's suggestion, it causes 'sysdep_dynl_link' to manually search additional directories if 'lt_dlopenext' fails to find the library in the default paths. However, I took a somewhat different approach, and tried to be more careful with regard to portability and correctness. I read the code of libltdl, and mimicked their handling of path separators and directory separators, to ensure that this patch will not reduce our portability. I also followed their lead in deciding when to perform a search. If any directory separators are present (even if it's a relative pathname), then libltdl does not perform a search. I used precisely the same criterion to decide whether to search additional directories. So what additional directories does it search? If GUILE_SYSTEM_EXTENSIONS_PATH is set (even if it's empty), then it specifies the additional directories to search. If it's unset, then the default is to search SCM_LIB_DIR and SCM_EXTENSIONS_DIR. *** Note that this changes the search order in the case where GUILE_SYSTEM_EXTENSIONS_PATH is set to a non-empty string. Currently, a non-empty GUILE_SYSTEM_EXTENSIONS_PATH is passed to lt_dladdsearchdir, so it is searched before LTDL_LIBRARY_PATH and LD_LIBRARY_PATH, but this patch causes GUILE_SYSTEM_EXTENSIONS_PATH to be searched last, to be consistent with the handling of the default directories SCM_LIB_DIR and SCM_EXTENSIONS_DIR. This seems sensible to me. Does anyone see a problem with this change? This patch also adds robust handling of the case where GUILE_SYSTEM_EXTENSIONS_PATH contains more than one path component. See below for my preliminary patch. I have not yet tested it carefully. It's a context diff, because 'diff' made a mess of the unified diff. Comments and suggestions solicited. Mark
Re: [PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)
Sjoerd van Leent Privé svanle...@gmail.com writes: Just a quick note. Now with GUILE_SYSTEM_EXTENSIONS_PATH, this could be very practical when developing a module. However, if a module is already present AND is inside SCM_LIB_DIR or SCM_EXTENSIONS_DIR, it would never override that situation. I believe there should be a way to be able to override the normal behavior. If GUILE_SYSTEM_EXTENSIONS_PATH is set, then neither SCM_LIB_DIR nor SCM_EXTENSIONS_DIR are added to the search path. This is currently the case, and my patch would not change that. There is one complication though: SCM_LIB_DIR is normally $prefix/lib, which is usually in the default system library search path. However, it is still possible to override the normal behavior by manually modifying LD_LIBRARY_PATH. Is there any reason why that's a suboptimal solution for use when developing a module? Having said this, I'm not convinced that my patch does the right thing. I'm still trying to understand the needed requirements. I'll talk more about this is my response to Ludovic. Thanks for the feedback! Regards, Mark
Re: [PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)
Hi Ludovic!
Thanks for the review. I agree with your stylistic suggestions and will
update my patch accordingly. What I'd like to discuss here is the logic
of the library search order.
l...@gnu.org (Ludovic Courtès) writes:
Mark H Weaver m...@netris.org skribis:
Following Bruce's suggestion, it causes 'sysdep_dynl_link' to manually
search additional directories if 'lt_dlopenext' fails to find the
library in the default paths.
Thus, that doesn’t solve the problem described at
http://lists.gnu.org/archive/html/guile-devel/2010-11/msg00095.html,
right?
To solve it, we’d have to do our own lookup unconditionally.
I've read the message referenced above several times, but I've failed to
understand why we cannot use 'lt_dladdsearchdir' to augment the path, as
shown in the first code excerpt of that message:
--8---cut here---start-8---
env = getenv (GUILE_SYSTEM_EXTENSIONS_PATH);
[...]
if (env)
lt_dladdsearchdir (env);
else
{
lt_dladdsearchdir (SCM_LIB_DIR);
lt_dladdsearchdir (SCM_EXTENSIONS_DIR);
}
--8---cut here---end---8---
(although I would enhance that code to properly handle multiple path
components in GUILE_SYSTEM_EXTENSIONS_PATH).
As I understand it, the reason given for why we cannot use that approach
is that 'libtool --mode=execute -dlopen' would not work properly, but
why do we have to do it that way?
Within 'meta/uninstalled-env', why not set GUILE_SYSTEM_EXTENSIONS_PATH
to point to the libraries and extensions in the build directory? In
fact, you seem to suggest that fix near the end the Nov 2010 message
referenced above. Why was that solution not adopted?
Thanks,
Mark
Re: [PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)
Hi Mark!
Thanks for being quicker and more active than me! ;-)
Overall, the approach of mimicking what the lookup procedure of
‘lt_dlopenext’ sounds good to me.
Mark H Weaver m...@netris.org skribis:
Following Bruce's suggestion, it causes 'sysdep_dynl_link' to manually
search additional directories if 'lt_dlopenext' fails to find the
library in the default paths.
Thus, that doesn’t solve the problem described at
http://lists.gnu.org/archive/html/guile-devel/2010-11/msg00095.html,
right?
To solve it, we’d have to do our own lookup unconditionally.
So what additional directories does it search?
If GUILE_SYSTEM_EXTENSIONS_PATH is set (even if it's empty), then it
specifies the additional directories to search. If it's unset, then the
default is to search SCM_LIB_DIR and SCM_EXTENSIONS_DIR.
*** Note that this changes the search order in the case where
GUILE_SYSTEM_EXTENSIONS_PATH is set to a non-empty string.
Currently, a non-empty GUILE_SYSTEM_EXTENSIONS_PATH is passed to
lt_dladdsearchdir, so it is searched before LTDL_LIBRARY_PATH and
LD_LIBRARY_PATH, but this patch causes GUILE_SYSTEM_EXTENSIONS_PATH to
be searched last, to be consistent with the handling of the default
directories SCM_LIB_DIR and SCM_EXTENSIONS_DIR. This seems sensible to
me. Does anyone see a problem with this change?
The point of $GUILE_SYSTEM_EXTENSIONS_PATH is to avoid using installed
Guile extensions while building Guile itself. Wouldn’t the change
defeat that when $LTDL_LIBRARY_PATH or $LD_LIBRARY_PATH point to
previously installed extensions? I’d rather not change anything.
Also, could you test compare the actual searches for both the patch and
unpatched dynl.c with strace? For instance, with:
$ LTDL_LIBRARY_PATH= LD_LIBRARY_PATH= strace -f -o x1
/before-patch/meta/guile -c '(use-modules (ice-9 readline))'
$ LTDL_LIBRARY_PATH= LD_LIBRARY_PATH= strace -f -o x2 /after-patch/meta/guile
-c '(use-modules (ice-9 readline))'
$ LTDL_LIBRARY_PATH=/path/to/common/extensiondir strace -f -o y1
/before-patch/meta/guile -c '(use-modules (ice-9 readline))'
$ LTDL_LIBRARY_PATH=/path/to/common/extensiondir strace -f -o y2
/after-patch/meta/guile -c '(use-modules (ice-9 readline))'
It’s an area where it’s very easy to introduce hard-to-find bugs, so I’m
a bit wary.
Note that the final patch will also need to revert the configury added
in e66ff09a.
Minor stylistic comments:
+/* 'system_extensions_path' is used by 'sysdep_dynl_link' to search for
+ dynamic libraries as a last resort, when they cannot be found in the
+ usual library search paths. */
+static char *system_extensions_path;
No need to repeat the variable name, nor to say where it’s used IMO.
+ if (fname == NULL)
+{
+ /* Return a handle for the program as a whole. */
+ handle = lt_dlopen (NULL);
+}
No extra brace.
+ /* 'fname' contains no directory separators and was not in the
+ usual library search paths, so now we search for it in the
+ directories specified in 'system_extensions_path'. */
Should be FNAME and SYSTEM_EXTENSIONS_PATH (capitals) when referring to
the value of these variables (info (standards) Comments).
+ char *fname_attempt = malloc (strlen (system_extensions_path)
++ strlen (fname)
++ 1 /* for directory separator */
++ 1); /* for null terminator */
Use scm_gc_malloc_pointerless, and remove the corresponding
dynwind_free.
“+ 2” with no comment would be fine.
+ /* Iterate over the components of 'system_extensions_path' */
Capitalize too (other occurrences omitted).
+ system_extensions_path = (char *) malloc (strlen (SCM_LIB_DIR)
++ strlen (SCM_EXTENSIONS_DIR)
++ 1 /* for path separator
*/
++ 1); /* for null terminator
*/
+ assert (system_extensions_path != NULL);
Use scm_gc_malloc_pointerless, no cast, and remove the assert.
Thanks!
Ludo’.
[PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)
Hello all,
Here's a preliminary patch to avoid modifying LD_LIBRARY_PATH.
Following Bruce's suggestion, it causes 'sysdep_dynl_link' to manually
search additional directories if 'lt_dlopenext' fails to find the
library in the default paths. However, I took a somewhat different
approach, and tried to be more careful with regard to portability and
correctness.
I read the code of libltdl, and mimicked their handling of path
separators and directory separators, to ensure that this patch will not
reduce our portability.
I also followed their lead in deciding when to perform a search. If any
directory separators are present (even if it's a relative pathname),
then libltdl does not perform a search. I used precisely the same
criterion to decide whether to search additional directories.
So what additional directories does it search?
If GUILE_SYSTEM_EXTENSIONS_PATH is set (even if it's empty), then it
specifies the additional directories to search. If it's unset, then the
default is to search SCM_LIB_DIR and SCM_EXTENSIONS_DIR.
*** Note that this changes the search order in the case where
GUILE_SYSTEM_EXTENSIONS_PATH is set to a non-empty string.
Currently, a non-empty GUILE_SYSTEM_EXTENSIONS_PATH is passed to
lt_dladdsearchdir, so it is searched before LTDL_LIBRARY_PATH and
LD_LIBRARY_PATH, but this patch causes GUILE_SYSTEM_EXTENSIONS_PATH to
be searched last, to be consistent with the handling of the default
directories SCM_LIB_DIR and SCM_EXTENSIONS_DIR. This seems sensible to
me. Does anyone see a problem with this change?
This patch also adds robust handling of the case where
GUILE_SYSTEM_EXTENSIONS_PATH contains more than one path component.
See below for my preliminary patch. I have not yet tested it carefully.
It's a context diff, because 'diff' made a mess of the unified diff.
Comments and suggestions solicited.
Mark
diff --git a/libguile/dynl.c b/libguile/dynl.c
index a2ae6e2..149ed26 100644
*** a/libguile/dynl.c
--- b/libguile/dynl.c
***
*** 26,31
--- 26,33
#endif
#include alloca.h
+ #include assert.h
+ #include string.h
/* dynl.c dynamically linkload object files.
Author: Aubrey Jaffer
***
*** 37,43
solution would probably be a shared libgcc. */
#undef NDEBUG
- #include assert.h
static void
maybe_drag_in_eprintf ()
--- 39,44
***
*** 75,92
*/
/* njrev: not threadsafe, protection needed as described above */
static void *
sysdep_dynl_link (const char *fname, const char *subr)
{
lt_dlhandle handle;
! if (fname != NULL)
! handle = lt_dlopenext (fname);
else
! /* Return a handle for the program as a whole. */
! handle = lt_dlopen (NULL);
! if (NULL == handle)
{
SCM fn;
SCM msg;
--- 76,165
*/
/* njrev: not threadsafe, protection needed as described above */
+
+ /* 'system_extensions_path' is used by 'sysdep_dynl_link' to search for
+dynamic libraries as a last resort, when they cannot be found in the
+usual library search paths. */
+ static char *system_extensions_path;
+
static void *
sysdep_dynl_link (const char *fname, const char *subr)
{
lt_dlhandle handle;
! if (fname == NULL)
! {
! /* Return a handle for the program as a whole. */
! handle = lt_dlopen (NULL);
! }
else
! {
! handle = lt_dlopenext (fname);
!
! if (handle == NULL
! #ifdef LT_DIRSEP_CHAR
!strchr (fname, LT_DIRSEP_CHAR) == NULL
! #endif
!strchr (fname, '/') == NULL)
! {
! /* 'fname' contains no directory separators and was not in the
! usual library search paths, so now we search for it in the
! directories specified in 'system_extensions_path'. */
! char *fname_attempt = malloc (strlen (system_extensions_path)
! + strlen (fname)
! + 1 /* for directory separator */
! + 1); /* for null terminator */
! char *path; /* remaining path to search */
! char *end; /* end of current path component */
! char *s;
!
! if (fname_attempt != NULL)
! {
! scm_dynwind_begin (0);
! scm_dynwind_free (fname_attempt);
!
! /* Iterate over the components of 'system_extensions_path' */
! for (path = system_extensions_path;
!*path != '\0';
!path = (*end == '\0') ? end : (end + 1))
! {
! /* Find end of pathname component */
! end = strchr (path, LT_PATHSEP_CHAR);
! if (end == NULL)
! end = strchr (path, '\0');
!
! /* Skip empty path components */
! if (path == end)
! continue;
!
! /* Construct
