Re: MSYS+MSVC for libtool branch-2-0, take 2

[Ralf Wildenhues]

Hi Peter,

First, let me thank you for your hard work on this!
Then, let me give a couple of quick comments, and
sorry that I still haven't had a chance to test any
of it myself (so take the comments with a grain of salt).

* Peter Ekberg wrote on Thu, Jun 09, 2005 at 02:01:35PM CEST:
> 
> I have now fixed at least some issues with the previous
> patch. I changed $libname_spec to plain $name for MSVC
> as that seems to be the naming convention used by MS.

Hmm.  OK.

> I also fixed locating potential dependent libs without
> regard to the filename case (i.e -lwsock32 on the
> link line will be matched by the file WSOCK32.lib).

The file_magic_glob is really cute, I like it.  :)

> I realize that this still is not in shape for a commit.
> There's not even a changelog :-), but I'd really
> appreciate any pointers on how to get it into shape.

Sure.

I'd really like it if there were no two different functions 
func_extract_a_lib and func_extract_an_archive.  Something like:
(watch out: func_show_eval is Libtool HEAD notation; untested)

func_extract_an_archive ()
{
    $opt_debug
    # save nonlocal variables
    f_ex_an_ar_dir_save=$output_objdir
    output_objdir=$1; shift
    f_ex_an_ar_oldlib_save=$oldlib
    oldlib=$1
    f_ex_an_ar_name_save=$name

    func_show_eval "$extract_old_archive_cmds"
    
    if ( eval "$list_old_archive_cmd" | sort | sort -uc >/dev/null 2>&1); then
     :
    else
     func_fatal_error "object name conflicts in archive: $output_objdir/$oldlib"
    fi

    # restore nonlocal variables
    oldlib=$f_ex_an_ar_oldlib_save
    name=$f_ex_an_ar_name_save
    output_objdir=$f_ex_an_ar_dir_save

}

and then in libtool.m4:

# MSVC
AR='link -LIB -NOLOGO'
# for other MSVC versions that might be just 'LIB', ...
list_old_archive_cmd='$AR -LIST "$oldlib"'
extract_old_archive_cmds='$AR -LIST "$oldlib" | while read name; do
                        (cd $output_objdir && link -LIB -NOLOGO -EXTRACT:$name 
$oldlib );
                      done'

# Default
list_old_archive_cmd='$AR t "$oldlib"'
extract_old_archive_cmds='$AR x "$oldlib"'

That would enable you to kill some more of the system-dependency of the
patch in ltmain.  It would also enable us to test for and use `ar' if
it's available and working.  Does the presumably quadratic loop for MSVC
hurt for large archives?

Also note that I intended to parametrize more of the $AR handling anyway
(for omitting $RANLIB).

More comments to be found inline (some are really intended as hints
rather than anything else).

Cheers,
Ralf

| Index: config/ltmain.m4sh
| ===================================================================
| RCS file: /cvsroot/libtool/libtool/config/ltmain.m4sh,v
| retrieving revision 1.1.2.54
| diff -u -u -5 -r1.1.2.54 ltmain.m4sh
| --- config/ltmain.m4sh        5 Jun 2005 17:35:11 -0000       1.1.2.54
| +++ config/ltmain.m4sh        9 Jun 2005 11:45:21 -0000
| @@ -923,10 +923,29 @@
|        compile_command=`$ECHO "X$compile_command" | $Xsed -e "s% 
@address@hidden"`
|        finalize_command=`$ECHO "X$finalize_command" | $Xsed -e "s% 
@address@hidden"`
|      fi
|  }
|  
| +# func_extract_a_lib dir oldlib
| +func_extract_a_lib ()
| +{
| +    $opt_debug
| +    f_ex_a_lib_dir="$1"; shift
| +    f_ex_a_lib_oldlib="$1"
| +    link -LIB -NOLOGO -LIST "$f_ex_a_lib_oldlib" | while read name
| +    do
| +      $show "(cd $f_ex_a_lib_dir && link -LIB -NOLOGO -EXTRACT:$name 
$f_ex_a_lib_oldlib)"
| +      $run eval "(cd \$f_ex_a_lib_dir && link -LIB -NOLOGO -EXTRACT:\$name 
\$f_ex_a_lib_oldlib)" || exit $?
| +    done
| +    if (link -LIB -NOLOGO -LIST "$f_ex_a_lib_oldlib" | sort | sort -uc 
>/dev/null 2>&1); then
| +     :
| +    else
| +     func_fatal_error "object name conflicts in archive: 
$f_ex_a_lib_dir/$f_ex_a_lib_oldlib"
| +    fi
| +}
| +
| +
|  # func_extract_an_archive dir oldlib
|  func_extract_an_archive ()
|  {
|      $opt_debug
|      f_ex_an_ar_dir="$1"; shift
| @@ -1002,12 +1021,20 @@
|           func_extract_an_archive "$my_xdir" "$my_xabs"
|         fi # $darwin_arches
|       fi # $run
|        ;;
|        *)
| -        func_extract_an_archive "$my_xdir" "$my_xabs"
| +     case $with_gcc/$host in
| +     no/*-*-mingw* | /*-*-mingw* | no/*-*-cygwin* | /*-*-cygwin*)
| +       # assume MSVC
| +       func_extract_a_lib "$my_xdir" "$my_xabs"
| +     ;;
| +     *)
| +       func_extract_an_archive "$my_xdir" "$my_xabs"
|       ;;
| +     esac
| +      ;;
|        esac
|        my_oldobjs="$my_oldobjs "`find $my_xdir -name \*.$objext -print -o 
-name \*.lo -print | $NL2SP`
|      done
|  
|      func_extract_archives_result="$my_oldobjs"

Above two hunks should be dealt with above.

| @@ -2596,10 +2623,11 @@
|           continue
|           ;;
|         *-*-mingw* | *-*-os2*)
|           # These systems don't actually have a C library (as such)
|           test "X$arg" = "X-lc" && continue
| +         test "X$with_gcc" != "Xyes" -a "X$arg" = "X-lm" && continue

Does MSVC not have libm?  Does anybody know how Borland fares here?

|           ;;
|         *-*-openbsd* | *-*-freebsd* | *-*-dragonfly*)
|           # Do not include libc due to us having libc/libc_r.
|           test "X$arg" = "X-lc" && continue
|           ;;
| @@ -3149,10 +3177,27 @@
|               fi
|               break 2
|             fi
|           done
|         done
| +       if test "$found" != "yes"; then
| +         case $linkmode/$with_gcc/$host in
| +         prog/no/*-*-mingw* | prog//*-*-mingw* | prog/no/*-*-cygwin* | 
prog//*-*-cygwin*)

I don't like this $host distinction here much.  Let's see if we can kill
it.

| +           # assume MSVC, which will look in $LIB for libraries
| +           save_ifs="$IFS"; IFS=';'

We have $PATH_SEPARATOR.

| +           for msvc_libdir in $LIB; do
| +             IFS="$save_ifs"
| +             # assume windows drives are accessible as /D, where D is a 
drive letter.

I don't think you can make this assumption.
I believe we should probably try to generalize fix_srcfile_path (as in:
either you have `cygpath' available or you can make some assumption
about drive letters which is true for MinGW.


| +             if test -f "/$($ECHO "X$msvc_libdir" | Xsed -e s.\\\\./.g -e 
s.\\:..)/${name}.lib"; then
| +               found=no
| +               break
| +             fi
| +           done
| +           IFS="$save_ifs"
| +           ;;
| +         esac
| +       fi
|         if test "$found" != yes; then
|           # deplib doesn't seem to be a libtool library
|           if test "$linkmode,$pass" = "prog,link"; then
|             compile_deplibs="$deplib $compile_deplibs"
|             finalize_deplibs="$deplib $finalize_deplibs"
| @@ -4075,10 +4120,21 @@
|             case " $tmp_libs " in
|             *" $deplib "*) ;;
|             *) tmp_libs="$tmp_libs $deplib" ;;
|             esac
|             ;;
| +         -l*)
| +           if test "$linkmode" = prog -a "X$with_gcc" != Xyes; then
| +             case $host in
| +             *-*-mingw* | *-*-cygwin*)
| +               # assume MSVC
| +               deplib=`$ECHO "X${deplib}.lib" | $Xsed -e 's/^-l//'`
| +               ;;
| +             esac
| +           fi
| +           tmp_libs="$tmp_libs $deplib"
| +           ;;

Don't like this system-dependency here either.
Wonder if we need another switch or $libname_spec would suffice here..

|           *) tmp_libs="$tmp_libs $deplib" ;;
|           esac
|         done
|         eval $var=\"$tmp_libs\"
|       done # for var
| @@ -4656,12 +4712,17 @@
|                 ;;
|               esac
|             fi
|             if test -n "$a_deplib" ; then
|               libname=`eval "\\$ECHO \"$libname_spec\""`
| +             if test -n "$file_magic_glob"; then
| +               libnameglob=`$ECHO "X$libname" | $Xsed -e $file_magic_glob`
| +             else
| +               libnameglob=$libname
| +             fi
|               for i in $lib_search_path $sys_lib_search_path 
$shlib_search_path; do
| -               potential_libs=`ls $i/$libname[[.-]]* 2>/dev/null`
| +               potential_libs=`ls $i/$libnameglob[[.-]]* 2>/dev/null`
|                 for potent_lib in $potential_libs; do
|                     # Follow soft links.
|                     if ls -lLd "$potent_lib" 2>/dev/null |
|                        $GREP " -> " >/dev/null; then
|                       continue
| @@ -4680,11 +4741,18 @@
|                       esac
|                     done
|                     if eval $file_magic_cmd \"\$potlib\" 2>/dev/null |
|                        $SED -e 10q |
|                        $EGREP "$file_magic_regex" > /dev/null; then
| -                     newdeplibs="$newdeplibs $a_deplib"
| +                     case $with_gcc/$host in
| +                     no/*-*-mingw* | /*-*-mingw* | no/*-*-cygwin* | 
/*-*-cygwin*)
| +                       newdeplibs="$newdeplibs ${name}.lib"
| +                       ;;
| +                     *)
| +                       newdeplibs="$newdeplibs $a_deplib"
| +                       ;;
| +                     esac
|                       a_deplib=""
|                       break 2
|                     fi
|                 done
|               done
| @@ -5680,11 +5748,13 @@
|  */
|  EOF
|           cat >> $cwrappersource<<"EOF"
|  #include <stdio.h>
|  #include <stdlib.h>
| +#ifndef _MSC_VER
|  #include <unistd.h>
| +#endif

We should probably put a check for unistd.h (and sys/stat.h?) in
libtool.m4 and use that.  What do you think?  Also check for stat()
function?  Is this idea overkill?

|  #include <malloc.h>
|  #include <stdarg.h>
|  #include <assert.h>
|  #include <ctype.h>
|  #include <string.h>
| @@ -5732,15 +5802,19 @@
|    if (stale) { free ((void *) stale); stale = 0; } \
|  } while (0)
|  
|  /* -DDEBUG is fairly common in CFLAGS.  */
|  #undef DEBUG
| +#ifdef _MSC_VER
| +# define DEBUG()
| +#else
|  #if defined DEBUGWRAPPER
|  # define DEBUG(format, ...) fprintf(stderr, format, __VA_ARGS__)
|  #else
|  # define DEBUG(format, ...)
|  #endif
| +#endif

Is your MSVC version not C99 compliant?
Does _MSC_VER reveal the version of the compiler?

|  
|  const char *program_name = NULL;
|  
|  void * xmalloc (size_t num);
|  char * xstrdup (const char *string);
| @@ -5844,10 +5918,13 @@
|  
|    DEBUG("(check_executable)  : %s\n", path ? (*path ? path : "EMPTY!") : 
"NULL!");
|    if ((!path) || (!*path))
|      return 0;
|  
| +#ifdef _MSC_VER
| +  return 1;
| +#else
|    if ((stat (path, &st) >= 0) &&
|        (
|       /* MinGW & native WIN32 do not support S_IXOTH or S_IXGRP */
|  #if defined (S_IXOTH)
|         ((st.st_mode & S_IXOTH) == S_IXOTH) ||
| @@ -5858,10 +5935,11 @@
|         ((st.st_mode & S_IXUSR) == S_IXUSR))
|        )
|      return 1;
|    else
|      return 0;
| +#endif
|  }
|  
|  /* Searches for the full path of the wrapper.  Returns
|     newly allocated full path name if found, NULL otherwise */
|  char *
| Index: m4/libtool.m4
| ===================================================================
| RCS file: /cvsroot/libtool/libtool/m4/libtool.m4,v
| retrieving revision 1.125.2.58
| diff -u -u -5 -r1.125.2.58 libtool.m4
| --- m4/libtool.m4     6 Jun 2005 16:13:23 -0000       1.125.2.58
| +++ m4/libtool.m4     9 Jun 2005 11:45:22 -0000
| @@ -1816,10 +1816,12 @@
|        ;;
|      esac
|      ;;
|  
|    *)
| +    # Assume MSVC
| +    libname_spec='$name'
|      library_names_spec='${libname}`echo ${release} | $SED -e 
's/[[.]]/-/g'`${versuffix}${shared_ext} $libname.lib'

Doesn't library_names_spec need to be case-ignoring as well (I don't
know actually).

|      ;;
|    esac
|    dynamic_linker='Win32 ld.exe'
|    # FIXME: first we should search . and the directory the executable is in
| @@ -2648,18 +2650,30 @@
|  tpf*)
|    lt_cv_deplibs_check_method=pass_all
|    ;;
|  esac
|  ])
| +
| +case $host_os in
| +cygwin* | mingw* | pw32*)
| +  file_magic_glob=`echo aAbBcCdDeEfFgGhHiIjJkKlLmMnNoOpPqQrRsStTuUvVwWxXyYzZ 
| sed -e "s/\(..\)/s\/[[\1]]\/[[\1]]\/g;/g"`
| +  ;;
| +*)
| +  file_magic_glob=
| +  ;;
| +esac
| +
|  file_magic_cmd=$lt_cv_file_magic_cmd
|  deplibs_check_method=$lt_cv_deplibs_check_method
|  test -z "$deplibs_check_method" && deplibs_check_method=unknown
|  
|  _LT_DECL([], [deplibs_check_method], [1],
|      [Method to check whether dependent libraries are shared objects])
|  _LT_DECL([], [file_magic_cmd], [1],
|      [Command to use when deplibs_check_method == "file_magic"])
| +_LT_DECL([], [file_magic_glob], [1],
| +    [How to find potential files when deplibs_check_method == "file_magic"])
|  ])# _LT_CHECK_MAGIC_METHOD
|  
|  
|  # LT_PATH_NM
|  # ----------
| @@ -3941,17 +3955,20 @@
|        # Tell ltmain to make .lib files, not .a files.
|        libext=lib
|        # Tell ltmain to make .dll files, not .so files.
|        shrext_cmds=".dll"
|        # FIXME: Setting linknames here is a bad hack.
| -      _LT_TAGVAR(archive_cmds, $1)='$CC -o $lib $libobjs $compiler_flags 
`$ECHO "X$deplibs" | $Xsed -e '\''s/ -lc$//'\''` -link -dll~linknames='
| +      _LT_TAGVAR(archive_cmds, $1)='$CC -o $lib $libobjs $compiler_flags 
`$ECHO "X$deplibs" | $Xsed -e '\''s/ -lc$//'\''` -link -dll~linknames=~mv 
.libs/${libname}`$ECHO "X${release}" | $Xsed -e s/[.]/-/g`${versuffix}.lib 
.libs/${libname}.lib'

In general, you can't use .libs, it fails on FAT volumes.  We have
$objdir for this.

Also I wonder why you deemed this necessary.

Moreover, independently of your patch, I believe we see some redundancy
here as we filter out ` -lc' here and in ltmain.  Wonder whether we can
remove one of them?

| +      _LT_TAGVAR(module_cmds, $1)='$CC -o $lib $libobjs $compiler_flags 
`$ECHO "X$deplibs" | $Xsed -e '\''s/ -lc$//'\''` -link -dll~linknames='
|        # The linker will automatically build a .lib file if we build a DLL.
|        _LT_TAGVAR(old_archive_from_new_cmds, $1)='true'
| -      # FIXME: Should let the user specify the lib program.
| -      _LT_TAGVAR(old_archive_cmds, $1)='lib /OUT:$oldlib$oldobjs$old_deplibs'
| -      _LT_TAGVAR(fix_srcfile_path, $1)='`cygpath -w "$srcfile"`'
| +      # FIXME: Should let the user specify the link program.
| +      _LT_TAGVAR(old_archive_cmds, $1)='link -LIB -NOLOGO 
-OUT:$oldlib$oldobjs$old_deplibs'

This should use $AR.

| +      #_LT_TAGVAR(fix_srcfile_path, $1)='`cygpath -w "$srcfile"`'

Don't comment this out unconditionally.  It's ok to have a test here or
parameterize according to cygwin vs mingw.

|        _LT_TAGVAR(enable_shared_with_static_runtimes, $1)=yes
| +      # Don't use ranlib
| +      _LT_TAGVAR(old_postinstall_cmds, $1)='chmod 644 $oldlib'

Why is this necessary?  If ranlib is not found ($RANLIB empty), it
should not be used anyway.

|        ;;
|  
|      darwin* | rhapsody*)
|        case $host_os in
|        rhapsody* | darwin1.[[012]])
| 
|