Re: [PATCH] [cygwin|mingw] fix dlpreopen with --disable-static take 2

[Ralf Wildenhues]

Hello Charles,

I haven't looked at your patches in detail yet, but a couple of things
caught my eye:

* Charles Wilson wrote on Sat, Jan 03, 2009 at 02:39:15AM CET:
> diff --git a/libltdl/config/general.m4sh b/libltdl/config/general.m4sh
> index 4bc304c..c4de91a 100644
> --- a/libltdl/config/general.m4sh
> +++ b/libltdl/config/general.m4sh
[...]
> +# func_tr_sh
> +# turn $1 into a string suitable for a shell variable name
> +# result is stored in $func_tr_sh_result
> +func_tr_sh ()
> +{
> +  func_tr_sh_result=`echo "$1" | $SED -e 's/[^A-Za-z0-9_]/_/g'`
> +  # ensure result begins with non-digit
> +  case "$func_tr_sh_result" in
> +    [A-Za-z_][A-Za-z0-9_] ) ;;
> +    * ) func_tr_sh_result=_$func_tr_sh_result ;;
> +  esac
> +}
>  ]])

Let's not waste processes when we don't have to, with something like
this untested bit:

func_tr_sh ()
{
  case $1 in
    [!a-zA-Z_]* | *[!a-zA-Z_0-9]*)
      func_tr_sh_result=`$ECHO "$1" | $SED 's/^[^a-zA-Z]/_/; 
s/[^a-zA-Z0-9]/_/g'`
      ;;
    *)
      func_tr_sh_result=$1
      ;;
  esac
}

> --- a/libltdl/config/ltmain.m4sh
> +++ b/libltdl/config/ltmain.m4sh

> @@ -1988,7 +1988,7 @@ extern \"C\" {
>             eval '$GREP -f "$output_objdir/$outputname.exp" < "$nlist" > 
> "$nlist"T'
>             eval '$MV "$nlist"T "$nlist"'
>             case $host in
> -             *cygwin | *mingw* | *cegcc* )
> +             *cygwin* | *mingw* | *cegcc* )
>                 eval "echo EXPORTS "'> "$output_objdir/$outputname.def"'
>                 eval 'cat "$nlist" >> "$output_objdir/$outputname.def"'
>                 ;;

Is this fixing a bug?  If yes, then it should be a separate patch,
documented in the ChangeLog entry, done likely in all other such
instances of missing '*' (I haven't found any), and would be obviously
correct and ok to push.  Please, please don't mix heavy patches with
such cleanups.  It only leads to cleanups being delayed.


In your "take 3" of this patch series, you have this hunk:

| @@ -2217,7 +2217,7 @@ func_win32_libid ()
|      ;;
|    *ar\ archive*) # could be an import, or static
|      if eval $OBJDUMP -f $1 | $SED -e '10q' 2>/dev/null |
| -       $EGREP 'file format (pe-i386(.*architecture: 
i386)?|pe-arm-wince|pe-x86-64)' >/dev/null; then
| +       $EGREP 'file format (pei?-i386(.*architecture: 
i386)?|pe-arm-wince|pe-x86-64)' >/dev/null; then
|        win32_nmres=`eval $NM -f posix -A $1 |
|         $SED -n -e '
|             1,100{

Now, my memory is really bad about win32 semantics, but wasn't it
exactly pei-i386 libraries that we wanted to not match here?

More generally, I have a feeling that this function is badly
conditioned: it needs adjustment fairly often, it is unclear to me which
cases exactly it tries to exclude (for starters: why is the file format
test needed at all?), and when things fail here, they do so very
unobviously for the libtool user.  These issues could IMVHO be
ameliorated by having some test cases that show the fine line between
import libraries that are acceptable, and static ones that are not.
So that when we port this to the next w32 system, we just have to run
the test suite and fix it until it passes.  What do you think?

Thanks,
Ralf