[PATCH] do not augment environment

[PATCH] do not augment environment

[Bruce Korb]

[-- Attachment #1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2: 0001-do-not-augment-environment.patch --]
[-- Type: text/x-patch, Size: 3599 bytes --]

From b7a04164b9de62fe1e80f061f926c9255c84e710 Mon Sep 17 00:00:00 2001
From: Bruce Korb <bkorb@gnu.org>
Date: Fri, 21 Sep 2012 16:33:44 -0700
Subject: [PATCH] do not augment environment

* libguile/dynl.c (augment_env): no longer used.  Removed.
  (sysdep_dynl_init): remove code that fiddled LD_LIBRARY_PATH
  -- aka SHARED_LIBRARY_PATH_VARIABLE
  (augment_env): removed.  Not used any more.
---
 libguile/dynl.c |   67 +++++++++++++++++++++----------------------------------
 1 file changed, 25 insertions(+), 42 deletions(-)

diff --git a/libguile/dynl.c b/libguile/dynl.c
index 72305a4..999627a 100644
--- a/libguile/dynl.c
+++ b/libguile/dynl.c
@@ -81,7 +81,25 @@ sysdep_dynl_link (const char *fname, const char *subr)
   lt_dlhandle handle;
 
   if (fname != NULL)
-    handle = lt_dlopenext (fname);
+    {
+      char * buf;
+
+      handle = lt_dlopenext (fname);
+      if (handle == NULL)
+        do
+          {
+            static char const ext_dir[] = SCM_EXTENSIONS_DIR;
+            static char const lib_dir[] = SCM_LIB_DIR;
+            buf = alloca (max(sizeof (ext_dir), sizeof (lib_dir))
+                          + strlen (fname) + 1); // fault on failure
+            sprintf (buf, "%s/%s", lib_dir, fname);
+            handle = lt_dlopenext (buf);
+            if (handle != NULL)
+              break;
+            sprintf (buf, "%s/%s", ext_dir, fname);
+            handle = lt_dlopenext (buf);
+          } while (0);
+    }
   else
     /* Return a handle for the program as a whole.  */
     handle = lt_dlopen (NULL);
@@ -120,30 +138,6 @@ sysdep_dynl_value (const char *symb, void *handle, const char *subr)
   return fptr;
 }
 
-/* Augment environment variable VARIABLE with VALUE, assuming VARIABLE
-   is a path kind of variable.  */
-static void
-augment_env (const char *variable, const char *value)
-{
-  const char *env;
-
-  env = getenv (variable);
-  if (env != NULL)
-    {
-      char *new_value;
-      static const char path_sep[] = { LT_PATHSEP_CHAR, 0 };
-
-      new_value = alloca (strlen (env) + strlen (value) + 2);
-      strcpy (new_value, env);
-      strcat (new_value, path_sep);
-      strcat (new_value, value);
-
-      setenv (variable, new_value, 1);
-    }
-  else
-    setenv (variable, value, 1);
-}
-
 static void
 sysdep_dynl_init ()
 {
@@ -152,26 +146,15 @@ sysdep_dynl_init ()
   lt_dlinit ();
 
   env = getenv ("GUILE_SYSTEM_EXTENSIONS_PATH");
-  if (env && strcmp (env, "") == 0)
-    /* special-case interpret system-ltdl-path=="" as meaning no system path,
-       which is the case during the build */
-    ;
-  else if (env)
+  if (env == NULL)
+    return;
+
+  /* special-case interpret system-ltdl-path=="" as meaning no system path,
+     which is the case during the build */
+  if (*env != '\0')
     /* FIXME: should this be a colon-separated path? Or is the only point to
        allow the build system to turn off the installed extensions path? */
     lt_dladdsearchdir (env);
-  else
-    {
-      /* Add SCM_LIB_DIR and SCM_EXTENSIONS_DIR to the loader's search
-	 path.  `lt_dladdsearchdir' and $LTDL_LIBRARY_PATH can't be used
-	 for that because they are searched before the system-dependent
-	 search path, which is the one `libtool --mode=execute -dlopen'
-	 fiddles with (info "(libtool) Libltdl Interface").  See
-	 <http://lists.gnu.org/archive/html/guile-devel/2010-11/msg00095.html>
-	 for details.  */
-      augment_env (SHARED_LIBRARY_PATH_VARIABLE, SCM_LIB_DIR);
-      augment_env (SHARED_LIBRARY_PATH_VARIABLE, SCM_EXTENSIONS_DIR);
-    }
 }
 
 scm_t_bits scm_tc16_dynamic_obj;
-- 
1.7.10.4

Re: [PATCH] do not augment environment

[Bruce Korb]

Thank you for looking into the patch.
Could you give me an idea as to when it will be in a release?
Thank you so much!  Regards, Bruce

Re: [PATCH] do not augment environment

[Mark H Weaver]

Hi Bruce,

Thanks for the patch, but I think it needs more work before it can be
committed.  See below for my comments.

> diff --git a/libguile/dynl.c b/libguile/dynl.c
> index 72305a4..999627a 100644
> --- a/libguile/dynl.c
> +++ b/libguile/dynl.c
> @@ -81,7 +81,25 @@ sysdep_dynl_link (const char *fname, const char *subr)
>    lt_dlhandle handle;
>
>    if (fname != NULL)
> -    handle = lt_dlopenext (fname);
> +    {
> +      char * buf;
> +
> +      handle = lt_dlopenext (fname);
> +      if (handle == NULL)
> +        do
> +          {
> +            static char const ext_dir[] = SCM_EXTENSIONS_DIR;
> +            static char const lib_dir[] = SCM_LIB_DIR;
> +            buf = alloca (max(sizeof (ext_dir), sizeof (lib_dir))
> +                          + strlen (fname) + 1); // fault on failure
> +            sprintf (buf, "%s/%s", lib_dir, fname);
> +            handle = lt_dlopenext (buf);
> +            if (handle != NULL)
> +              break;
> +            sprintf (buf, "%s/%s", ext_dir, fname);
> +            handle = lt_dlopenext (buf);
> +          } while (0);
> +    }
>    else
>      /* Return a handle for the program as a whole.  */
>      handle = lt_dlopen (NULL);

I see a few problems above:

* You assume that the directory separator is '/'.

* You should not do the extra search if 'fname' is an absolute
   pathname, and I'm not sure whether it should be done for relative
   pathnames containing directory separators.  Does anyone else have
   thoughts on that?

* As a stylistic issue, I don't like your trick of breaking out of the
   do-while.  I'd prefer something closer to this (but with the above
   problems addressed):

+      handle = lt_dlopenext (fname);
+      if (handle == NULL)
+        {
+          static char const ext_dir[] = SCM_EXTENSIONS_DIR;
+          static char const lib_dir[] = SCM_LIB_DIR;
+          buf = alloca (max(sizeof (ext_dir), sizeof (lib_dir))
+                        + strlen (fname) + 1); // fault on failure
+          sprintf (buf, "%s/%s", lib_dir, fname);
+          handle = lt_dlopenext (buf);
+          if (handle == NULL)
+            {
+              sprintf (buf, "%s/%s", ext_dir, fname);
+              handle = lt_dlopenext (buf);
+            }
+        }

> @@ -152,26 +146,15 @@ sysdep_dynl_init ()
>    lt_dlinit ();
>
>    env = getenv ("GUILE_SYSTEM_EXTENSIONS_PATH");
> -  if (env && strcmp (env, "") == 0)
> -    /* special-case interpret system-ltdl-path=="" as meaning no system path,
> -       which is the case during the build */
> -    ;
> -  else if (env)
> +  if (env == NULL)
> +    return;
> +
> +  /* special-case interpret system-ltdl-path=="" as meaning no system path,
> +     which is the case during the build */
> +  if (*env != '\0')
>      /* FIXME: should this be a colon-separated path? Or is the only point to
>         allow the build system to turn off the installed extensions path? */
>      lt_dladdsearchdir (env);
> -  else
> -    {
> -      /* Add SCM_LIB_DIR and SCM_EXTENSIONS_DIR to the loader's search
> -	 path.  `lt_dladdsearchdir' and $LTDL_LIBRARY_PATH can't be used
> -	 for that because they are searched before the system-dependent
> -	 search path, which is the one `libtool --mode=execute -dlopen'
> -	 fiddles with (info "(libtool) Libltdl Interface").  See
> -	 <http://lists.gnu.org/archive/html/guile-devel/2010-11/msg00095.html>
> -	 for details.  */
> -      augment_env (SHARED_LIBRARY_PATH_VARIABLE, SCM_LIB_DIR);
> -      augment_env (SHARED_LIBRARY_PATH_VARIABLE, SCM_EXTENSIONS_DIR);
> -    }
>  }

In the current code, SCM_LIB_DIR and SCM_EXTENSIONS_DIR are only
added to the library search path when GUILE_SYSTEM_EXTENSIONS_PATH is 
not set.  Your patch mishandles this, because it _always_ acts as if 
SCM_LIB_DIR and SCM_EXTENSIONS_DIR have been added to the search path.

Would you be willing to produce an updated patch that fixes these problems?

    Thanks,
      Mark

Re: [PATCH] do not augment environment

[Bruce Korb]

Hi Mark,

On 09/30/12 19:38, Mark H Weaver wrote:
> Thanks for the patch, but I think it needs more work before it can be
> committed.  See below for my comments.

You're welcome!  Just FYI, I've got no ego wrapped up in being the
one to type in characters, so fixing things up for preferred style
or oversights would be no concern to me at all.

> * You assume that the directory separator is '/'.

Either that, or a wrapper environment fixes it up.
If there is a conventional way of dealing with Microsoft-did-it-their-way
issues, I don't know what it is.  I'm not overly familiar with Guile code.

> * You should not do the extra search if 'fname' is an absolute
>   pathname, and I'm not sure whether it should be done for relative
>   pathnames containing directory separators.  Does anyone else have
>   thoughts on that?

It is already an "unusual case" path.  The extra check would save a few cpu
cycles when the unusual case was going to fail.  This saves one or two cycles
in the marginally more common case of the unusual case succeeding.

> * As a stylistic issue, I don't like your trick of breaking out of the
>   do-while.  I'd prefer something closer to this (but with the above
>   problems addressed):

I consider it a developed style. :) The deeper the logic nesting, the
more complex the code, to my eyes anyway.  In this particular case,
we're talking about "break" in the third level instead of two statements.
I definitely think the two statements make for microscopically more
complex code.  That is likely much outweighed by familiarity with
the technique.

All that notwithstanding, it's your code and what I provided I considered
mostly a guideline for what needs to happen to eliminate the LD_LIBRARY_PATH
fiddling.  Just let me know how to plug in the Microsoft directory
separator when needed and I'll resubmit the patch.  NOTE:  I'll be out
of town the 10th through 20th.

Thanks - Bruce

Re: [PATCH] do not augment environment

[Bruce Korb]

Hi,

P.S.

On 09/30/12 19:38, Mark H Weaver wrote:
> In the current code, SCM_LIB_DIR and SCM_EXTENSIONS_DIR are only
> added to the library search path when GUILE_SYSTEM_EXTENSIONS_PATH
> is not set.  Your patch mishandles this, because it _always_ acts
> as if SCM_LIB_DIR and SCM_EXTENSIONS_DIR have been added to the search path.

Always inserting "GUILE_SYSTEM_EXTENSIONS_PATH=" into the environment
is a better fix than erasing LD_LIBRARY_PATH, but I hadn't read and
digested the source.  Thanks for the pointer.

Re: [PATCH] do not augment environment

[Mark H Weaver]

On 10/01/2012 10:39 AM, Bruce Korb wrote:
> On 09/30/12 19:38, Mark H Weaver wrote:
>> In the current code, SCM_LIB_DIR and SCM_EXTENSIONS_DIR are only
>> added to the library search path when GUILE_SYSTEM_EXTENSIONS_PATH
>> is not set.  Your patch mishandles this, because it _always_ acts
>> as if SCM_LIB_DIR and SCM_EXTENSIONS_DIR have been added to the search path.
>
> Always inserting "GUILE_SYSTEM_EXTENSIONS_PATH=" into the environment
> is a better fix than erasing LD_LIBRARY_PATH, but I hadn't read and
> digested the source.  Thanks for the pointer.

Are you deploying this fix in widely-used software, or just within your 
private environment?  If the latter, that's fine, and you can disregard 
the rest of this message.

If the former, then we need to be careful.  By setting 
GUILE_SYSTEM_EXTENSIONS_PATH="", you will break Guile programs that need 
to load extensions, not just within your own program, but within any 
subprocesses that might be unrelated to your project.

Also, our internal hack of setting GUILE_SYSTEM_EXTENSIONS="" during the 
Guile build is not documented as far as I know, and might change in the 
future.  It's actually a bad hack, for the same reasons given above. 
For example, if 'gcc' started using Guile, then setting 
GUILE_SYSTEM_EXTENSIONS="" within the Guile build could break gcc.

I admit that we created this mess by deploying a bad hack, but let's try 
to avoid getting ourselves into an even bigger mess by deploying more 
bad hacks that lock us into preserving our current hacks.

Before I spend any more time thinking about this, can you please answer 
my question above?

      Thanks,
        Mark

Re: [PATCH] do not augment environment

[Bruce Korb]

On 10/01/12 09:59, Mark H Weaver wrote:
> On 10/01/2012 10:39 AM, Bruce Korb wrote:
>> On 09/30/12 19:38, Mark H Weaver wrote:
>>> In the current code, SCM_LIB_DIR and SCM_EXTENSIONS_DIR are only
>>> added to the library search path when GUILE_SYSTEM_EXTENSIONS_PATH
>>> is not set.  Your patch mishandles this, because it _always_ acts
>>> as if SCM_LIB_DIR and SCM_EXTENSIONS_DIR have been added to the search path.
>>
>> Always inserting "GUILE_SYSTEM_EXTENSIONS_PATH=" into the environment
>> is a better fix than erasing LD_LIBRARY_PATH, but I hadn't read and
>> digested the source.  Thanks for the pointer.
> 
> Are you deploying this fix in widely-used software, or just within your private environment?  If the latter, that's fine, and you can disregard the rest of this message.
> 
> If the former, then we need to be careful.  By setting GUILE_SYSTEM_EXTENSIONS_PATH="", you will break Guile programs that need to load extensions, not just within your own program, but within any subprocesses that might be unrelated to your project.
> 
> Also, our internal hack of setting GUILE_SYSTEM_EXTENSIONS="" during the Guile build is not documented as far as I know, and might change in the future.  It's actually a bad hack, for the same reasons given above. For example, if 'gcc' started using Guile, then setting GUILE_SYSTEM_EXTENSIONS="" within the Guile build could break gcc.
> 
> I admit that we created this mess by deploying a bad hack, but let's try to avoid getting ourselves into an even bigger mess by deploying more bad hacks that lock us into preserving our current hacks.
> 
> Before I spend any more time thinking about this, can you please answer my question above?

autogen is used within GCC for constructing the top level Makefile.am's and
for cobbling together all the hackery required to fix up broken include files.
There are another dozen or so lesser known projects that use it for
building out Java Beans infrastructure and similar repetitive tasks.
The most common use is likely option processing, but that use would surely
never bump into problems with GUILE_SYSTEM_EXTENSIONS_PATH.
(NTP and GNU TLS are two well known users of the option stuff.)
I control all the associated templates and they do not use any "subprograms"
implemented in Scheme/Guile.

In general, I've found my clients to be reluctant users of scheme
and it is actually more a barrier to use rather than a facilitator of use.
In other words, I suspect, but do not know, that there would be little issue.

WRT my problem, I have several choices that I would activate only when
the problematical versions of Guile are detected (2.0.1 thru 2.0.?? - what ??):

1. putenv("LD_LIBRARY_PATH=");
2. putenv("GUILE_SYSTEM_EXTENSIONS_PATH=");
3. putenv("GUILE_SYSTEM_EXTENSIONS_PATH=/dev/null");
4. any other suggestions?

I think #3 would likely be safest for my environment, but I would certainly like your opinion.

Thank you!

Regards, Bruce

P.S. To be clear:
I would not bind you into keeping the null extensions hack.  I would stop
using it upon detecting a Guile version that does not hack LD_LIBRARY_PATH.

[PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)

[Mark H Weaver]

[-- Attachment #1: Type: text/plain, Size: 1876 bytes --]

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



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Do not augment LD_LIBRARY_PATH v1 --]
[-- Type: text/x-diff, Size: 7309 bytes --]

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 link&load 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 'fname_attempt', starting with path component */
!                   s = fname_attempt;
!                   memcpy (s, path, end - path);
!                   s += end - path;
! 
!                   /* Append directory separator, but avoid duplicates */
!                   if (s[-1] != '/'
! #ifdef LT_DIRSEP_CHAR
!                       && s[-1] != LT_DIRSEP_CHAR
! #endif
!                       )
!                     *s++ = '/';
! 
!                   /* Finally, append 'fname' with null terminator */
!                   strcpy (s, fname);
! 
!                   /* Try to load it, and terminate the search if successful */
!                   handle = lt_dlopenext (fname_attempt);
!                   if (handle != NULL)
!                     break;
!                 }
! 
!               scm_dynwind_end ();
!             }
!         }
!     }
  
!   if (handle == NULL)
      {
        SCM fn;
        SCM msg;
***************
*** 120,149 ****
    return fptr;
  }
  
- /* Augment environment variable VARIABLE with VALUE, assuming VARIABLE
-    is a path kind of variable.  */
- static void
- augment_env (const char *variable, const char *value)
- {
-   const char *env;
- 
-   env = getenv (variable);
-   if (env != NULL)
-     {
-       char *new_value;
-       static const char path_sep[] = { LT_PATHSEP_CHAR, 0 };
- 
-       new_value = alloca (strlen (env) + strlen (value) + 2);
-       strcpy (new_value, env);
-       strcat (new_value, path_sep);
-       strcat (new_value, value);
- 
-       setenv (variable, new_value, 1);
-     }
-   else
-     setenv (variable, value, 1);
- }
- 
  static void
  sysdep_dynl_init ()
  {
--- 193,198 ----
***************
*** 151,176 ****
  
    lt_dlinit ();
  
    env = getenv ("GUILE_SYSTEM_EXTENSIONS_PATH");
!   if (env && strcmp (env, "") == 0)
!     /* special-case interpret system-ltdl-path=="" as meaning no system path,
!        which is the case during the build */
!     ;
!   else if (env)
!     /* FIXME: should this be a colon-separated path? Or is the only point to
!        allow the build system to turn off the installed extensions path? */
!     lt_dladdsearchdir (env);
    else
      {
!       /* Add SCM_LIB_DIR and SCM_EXTENSIONS_DIR to the loader's search
! 	 path.  `lt_dladdsearchdir' and $LTDL_LIBRARY_PATH can't be used
! 	 for that because they are searched before the system-dependent
! 	 search path, which is the one `libtool --mode=execute -dlopen'
! 	 fiddles with (info "(libtool) Libltdl Interface").  See
! 	 <http://lists.gnu.org/archive/html/guile-devel/2010-11/msg00095.html>
! 	 for details.  */
!       augment_env (SHARED_LIBRARY_PATH_VARIABLE, SCM_LIB_DIR);
!       augment_env (SHARED_LIBRARY_PATH_VARIABLE, SCM_EXTENSIONS_DIR);
      }
  }
  
--- 200,232 ----
  
    lt_dlinit ();
  
+   /* Initialize 'system_extensions_path' from
+      $GUILE_SYSTEM_EXTENSIONS_PATH, or if that's not set:
+      <SCM_LIB_DIR> <LT_PATHSEP_CHAR> <SCM_EXTENSIONS_DIR>.
+ 
+      'lt_dladdsearchdir' can't be used because it is searched before the
+      system-dependent search path, which is the one 'libtool
+      --mode=execute -dlopen' fiddles with (info "(libtool) Libltdl
+      Interface").  See
+      <http://lists.gnu.org/archive/html/guile-devel/2010-11/msg00095.html>.
+ 
+      The environment variables $LTDL_LIBRARY_PATH and $LD_LIBRARY_PATH
+      can't be used because they would be propagated to subprocesses
+      which may cause problems for other programs.  See
+      <http://lists.gnu.org/archive/html/guile-devel/2012-09/msg00037.html> */
+ 
    env = getenv ("GUILE_SYSTEM_EXTENSIONS_PATH");
!   if (env)
!     system_extensions_path = env;
    else
      {
!       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);
!       sprintf (system_extensions_path, "%s%c%s",
!                SCM_LIB_DIR, LT_PATHSEP_CHAR, SCM_EXTENSIONS_DIR);
      }
  }
  

Re: [PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)

[Ludovic Courtès]

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 <mhw@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’.

Re: [PATCH] Don't augment LD_LIBRARY_PATH (was Re: [PATCH] do not augment environment)

[Sjoerd van Leent Privé]

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)

[Mark H Weaver]

Sjoerd van Leent Privé <svanleent@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)

[Mark H Weaver]

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.

ludo@gnu.org (Ludovic Courtès) writes:
> Mark H Weaver <mhw@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)

[Mark H Weaver]

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)

[Ludovic Courtès]

Hi,

Mark H Weaver <mhw@netris.org> skribis:

> ludo@gnu.org (Ludovic Courtès) writes:
>> Mark H Weaver <mhw@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)

[Mark H Weaver]

ludo@gnu.org (Ludovic Courtès) writes:
> Mark H Weaver <mhw@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)

[Ludovic Courtès]

Hi,

Mark H Weaver <mhw@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’.

[PATCH] Fix library search order and don't change LD_LIBRARY_PATH

[Mark H Weaver]

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]

Hello all,

Apologies for the delay in resolving this issue with LD_LIBRARY_PATH,
but we *do* plan to get this fix in 2.0.7.  After discussion with
Ludovic on IRC, I believe we've reached agreement.

I've attached a revised patch that incorporates Ludovic's suggestions.
Comments and suggestions solicited.

      Mark



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Fix library search order and don't change LD_LIBRARY_PATH --]
[-- Type: text/x-diff, Size: 6906 bytes --]

From 83167214083e24a8eeac6b6e04e8035497fff73a Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@netris.org>
Date: Tue, 27 Nov 2012 17:44:38 -0500
Subject: [PATCH] Fix library search order and don't change LD_LIBRARY_PATH

* libguile/dynl.c (system_extensions_path): New static variable.

  (sysdep_dynl_link): If 'lt_dlopenext' fails, manually search
  in 'system_extensions_path'.

  (augment_env): Remove.

  (sysdep_dynl_init): Don't change LD_LIBRARY_PATH, and don't use
  lt_dladdsearchdir if GUILE_SYSTEM_EXTENSIONS_PATH is set.  Instead,
  initialize 'system_extensions_path' from GUILE_SYSTEM_EXTENSIONS_PATH
  (or if it's unset: <SCM_LIB_DIR>:<SCM_EXTENSIONS_DIR>), and rely on
  sysdep_dynl_link to search those directories manually.
---
 libguile/dynl.c |  133 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 88 insertions(+), 45 deletions(-)

diff --git a/libguile/dynl.c b/libguile/dynl.c
index a2ae6e2..2948f78 100644
--- a/libguile/dynl.c
+++ b/libguile/dynl.c
@@ -26,6 +26,7 @@
 #endif
 
 #include <alloca.h>
+#include <string.h>
 
 /* "dynl.c" dynamically link&load object files.
    Author: Aubrey Jaffer
@@ -61,6 +62,7 @@ maybe_drag_in_eprintf ()
 #include "libguile/validate.h"
 #include "libguile/dynwind.h"
 #include "libguile/foreign.h"
+#include "libguile/gc.h"
 
 #include <ltdl.h>
 
@@ -75,18 +77,77 @@ maybe_drag_in_eprintf ()
 */
 /* njrev: not threadsafe, protection needed as described above */
 
+
+static char *system_extensions_path; /* library search path of last resort */
+
 static void *
 sysdep_dynl_link (const char *fname, const char *subr)
 {
   lt_dlhandle handle;
 
-  if (fname != NULL)
-    handle = lt_dlopenext (fname);
-  else
+  if (fname == NULL)
     /* Return a handle for the program as a whole.  */
     handle = lt_dlopen (NULL);
+  else
+    {
+      handle = lt_dlopenext (fname);
 
-  if (NULL == handle)
+      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
+             SYSTEM_EXTENSIONS_PATH. */
+          char *fname_attempt
+            = scm_gc_malloc_pointerless (strlen (system_extensions_path)
+                                         + strlen (fname) + 2,
+                                         "dynl fname_attempt");
+          char *path;  /* remaining path to search */
+          char *end;   /* end of current path component */
+          char *s;
+
+          /* Iterate over the components of SYSTEM_EXTENSIONS_PATH */
+          for (path = system_extensions_path;
+               *path != '\0';
+               path = (*end == '\0') ? end : (end + 1))
+            {
+              /* Find end of path component */
+              end = strchr (path, LT_PATHSEP_CHAR);
+              if (end == NULL)
+                end = strchr (path, '\0');
+
+              /* Skip empty path components */
+              if (path == end)
+                continue;
+
+              /* Construct FNAME_ATTEMPT, starting with path component */
+              s = fname_attempt;
+              memcpy (s, path, end - path);
+              s += end - path;
+
+              /* Append directory separator, but avoid duplicates */
+              if (s[-1] != '/'
+#ifdef LT_DIRSEP_CHAR
+                  && s[-1] != LT_DIRSEP_CHAR
+#endif
+                  )
+                *s++ = '/';
+
+              /* Finally, append FNAME (including null terminator) */
+              strcpy (s, fname);
+
+              /* Try to load it, and terminate the search if successful */
+              handle = lt_dlopenext (fname_attempt);
+              if (handle != NULL)
+                break;
+            }
+        }
+    }
+
+  if (handle == NULL)
     {
       SCM fn;
       SCM msg;
@@ -120,30 +181,6 @@ sysdep_dynl_value (const char *symb, void *handle, const char *subr)
   return fptr;
 }
 
-/* Augment environment variable VARIABLE with VALUE, assuming VARIABLE
-   is a path kind of variable.  */
-static void
-augment_env (const char *variable, const char *value)
-{
-  const char *env;
-
-  env = getenv (variable);
-  if (env != NULL)
-    {
-      char *new_value;
-      static const char path_sep[] = { LT_PATHSEP_CHAR, 0 };
-
-      new_value = alloca (strlen (env) + strlen (value) + 2);
-      strcpy (new_value, env);
-      strcat (new_value, path_sep);
-      strcat (new_value, value);
-
-      setenv (variable, new_value, 1);
-    }
-  else
-    setenv (variable, value, 1);
-}
-
 static void
 sysdep_dynl_init ()
 {
@@ -151,26 +188,32 @@ sysdep_dynl_init ()
 
   lt_dlinit ();
 
+  /* Initialize 'system_extensions_path' from
+     $GUILE_SYSTEM_EXTENSIONS_PATH, or if that's not set:
+     <SCM_LIB_DIR> <LT_PATHSEP_CHAR> <SCM_EXTENSIONS_DIR>.
+
+     'lt_dladdsearchdir' can't be used because it is searched before
+     the system-dependent search path, which is the one 'libtool
+     --mode=execute -dlopen' fiddles with (info "(libtool) Libltdl
+     Interface").  See
+     <http://lists.gnu.org/archive/html/guile-devel/2010-11/msg00095.html>.
+
+     The environment variables $LTDL_LIBRARY_PATH and $LD_LIBRARY_PATH
+     can't be used because they would be propagated to subprocesses
+     which may cause problems for other programs.  See
+     <http://lists.gnu.org/archive/html/guile-devel/2012-09/msg00037.html> */
+
   env = getenv ("GUILE_SYSTEM_EXTENSIONS_PATH");
-  if (env && strcmp (env, "") == 0)
-    /* special-case interpret system-ltdl-path=="" as meaning no system path,
-       which is the case during the build */
-    ;
-  else if (env)
-    /* FIXME: should this be a colon-separated path? Or is the only point to
-       allow the build system to turn off the installed extensions path? */
-    lt_dladdsearchdir (env);
+  if (env)
+    system_extensions_path = env;
   else
     {
-      /* Add SCM_LIB_DIR and SCM_EXTENSIONS_DIR to the loader's search
-	 path.  `lt_dladdsearchdir' and $LTDL_LIBRARY_PATH can't be used
-	 for that because they are searched before the system-dependent
-	 search path, which is the one `libtool --mode=execute -dlopen'
-	 fiddles with (info "(libtool) Libltdl Interface").  See
-	 <http://lists.gnu.org/archive/html/guile-devel/2010-11/msg00095.html>
-	 for details.  */
-      augment_env (SHARED_LIBRARY_PATH_VARIABLE, SCM_LIB_DIR);
-      augment_env (SHARED_LIBRARY_PATH_VARIABLE, SCM_EXTENSIONS_DIR);
+      system_extensions_path
+        = scm_gc_malloc_pointerless (strlen (SCM_LIB_DIR)
+                                     + strlen (SCM_EXTENSIONS_DIR) + 2,
+                                     "system_extensions_path");
+      sprintf (system_extensions_path, "%s%c%s",
+               SCM_LIB_DIR, LT_PATHSEP_CHAR, SCM_EXTENSIONS_DIR);
     }
 }
 
-- 
1.7.10.4

Re: [PATCH] Fix library search order and don't change LD_LIBRARY_PATH

[Ludovic Courtès]

Hi!

Mark H Weaver <mhw@netris.org> skribis:

> From 83167214083e24a8eeac6b6e04e8035497fff73a Mon Sep 17 00:00:00 2001
> From: Mark H Weaver <mhw@netris.org>
> Date: Tue, 27 Nov 2012 17:44:38 -0500
> Subject: [PATCH] Fix library search order and don't change LD_LIBRARY_PATH
>
> * libguile/dynl.c (system_extensions_path): New static variable.
>
>   (sysdep_dynl_link): If 'lt_dlopenext' fails, manually search
>   in 'system_extensions_path'.
>
>   (augment_env): Remove.
>
>   (sysdep_dynl_init): Don't change LD_LIBRARY_PATH, and don't use
>   lt_dladdsearchdir if GUILE_SYSTEM_EXTENSIONS_PATH is set.  Instead,
>   initialize 'system_extensions_path' from GUILE_SYSTEM_EXTENSIONS_PATH
>   (or if it's unset: <SCM_LIB_DIR>:<SCM_EXTENSIONS_DIR>), and rely on
>   sysdep_dynl_link to search those directories manually.

No need for blank lines above.

> +static char *system_extensions_path; /* library search path of last resort */

Please put the comment above, and rather something like:

  /* User-defined, LT_PATH_SEP-separated extension search path.  */

Hopefully this time we’re really done.  Thanks again for your work!

Ludo’.

Re: [PATCH] Fix library search order and don't change LD_LIBRARY_PATH

[Bruce Korb]

Thank you so much Mark and Ludovic!
Regards, Bruce

On Tue, Nov 27, 2012 at 2:54 PM, Mark H Weaver <mhw@netris.org> wrote:
> Hello all,
>
> Apologies for the delay in resolving this issue with LD_LIBRARY_PATH,
> but we *do* plan to get this fix in 2.0.7.  After discussion with
> Ludovic on IRC, I believe we've reached agreement.

Re: [PATCH] Fix library search order and don't change LD_LIBRARY_PATH

[Mark H Weaver]

I incorporated Ludovic's suggestions and pushed it to stable-2.0.

Thanks for bringing this problem to our attention, Bruce!

     Mark