Re: [patch] win32: eliminate wrapper script in main build dir

[Ralf Wildenhues]

Hello Charles,

Some additional comments:

* Charles Wilson wrote on Sun, Jun 10, 2007 at 08:23:58PM CEST:
>
> --- libltdl/config/ltmain.m4sh        9 Jun 2007 17:46:40 -0000       1.79
> +++ libltdl/config/ltmain.m4sh        10 Jun 2007 07:22:05 -0000

> +func_ltwrapper_executable_p ()
> +{
> +    func_ltwrapper_executable_p_result=no
> +    if ! func_ltwrapper_script_p "$1" ; then
> +      case "$1" in
> +        *.exe ) if grep "$magic" "$1" >/dev/null ; then
> +                    func_ltwrapper_executable_p_result=yes
> +                fi ;;
> +        * )     if grep "$magic" "${1}.exe" >/dev/null ; then
> +                    func_ltwrapper_executable_p_result=yes
> +                fi ;;
> +      esac
> +    fi
> +    test "$func_ltwrapper_executable_p_result" = "yes"
> +}

Let's keep as much code once as possible, and kill some superfluous
quotes:

func_ltwrapper_executable_p ()
{
  func_ltwrapper_exec_suffix=
  case $1 in
  *.exe) ;;
  *) func_ltwrapper_exec_suffix=.exe ;;
  esac
  grep "$magic" "$1$func_ltwrapper_exec_suffix" >/dev/null
}

Of course this is treading in dangerous waters, as not all systems' grep
commands work well with non text files (and POSIX does not demand it
either).  I suppose we should 2>&1 here to suppress error output (and
live with the occasional core file), just like we do in func_lalib_p.

> +# func_ltwrapper_temp_scriptname file
> +# Assumes file is an ltwrapper_executable
> +# uses $file to determine the appropriate filename for a
> +# temporary ltwrapper_script.
> +func_ltwrapper_temp_scriptname ()
> +{
> +    func_ltwrapper_temp_scriptname_result=""
> +    if func_ltwrapper_executable_p "$1"; then
> +        func_dirname "$1"
> +        func_basename "$1"

Side note: there should be a function that does both dirname and
basename at once, for speed.  (Not in this patch, but later.)
Another instance below.

[...]
>  # func_ltwrapper_p file
> -# True iff FILE is a libtool wrapper script.
> +# True iff FILE is a libtool wrapper script or wrapper executable
>  # This function is only a basic sanity check; it will hardly flush out
>  # determined imposters.
>  func_ltwrapper_p ()
>  {
> -    func_lalib_p "$1"
> +    func_ltwrapper_script_p "$1" || func_ltwrapper_execuable_p "$1"
>  }

Typo: s/execuable/executable/

> @@ -2085,14 +2139,19 @@
>       # Do a test to see if this is really a libtool program.
>       case $host in
>       *cygwin*|*mingw*)
> -         func_stripname '' '.exe' "$file"
> -         wrapper=$func_stripname_result
> +            if func_ltwrapper_executable_p "$file"; then
> +           func_ltwrapper_temp_scriptname "$file"
> +           wrapper="$func_ltwrapper_temp_scriptname_result"
> +            else
> +              func_stripname '' '.exe' "$file"
> +              wrapper="$func_stripname_result"

There is no need for double quotes on the right hand side of
assignments (2 instances).

> +            fi
>           ;;
>       *)
>           wrapper=$file
>           ;;
>       esac
> -     if func_ltwrapper_p "$wrapper"; then
> +     if func_ltwrapper_script_p "$wrapper"; then
>         notinst_deplibs=
>         relink_command=
>  
> @@ -2561,6 +2620,7 @@
>    char *tmp_pathspec;
>    char *actual_cwrapper_path;
>    char *shwrapper_name;
> +  intptr_t rval = 127;
>    FILE *shwrapper;
>  
>    const char *dumpscript_opt = "--lt-dump-script";
> @@ -2688,19 +2748,28 @@
>           case $host_os in
>             mingw*)
>               cat <<EOF
> -  execv ("$lt_newargv0", (const char * const *) newargz);
> +  /* execv doesn't actually work on mingw as expected on unix */
> +  rval = _spawnv (_P_WAIT, "$lt_newargv0", (const char * const *) newargz);
> +  if (rval == -1)
> +    {
> +      /* failed to start process */
> +      LTWRAPPER_DEBUGPRINTF (("(main) failed to launch target 
> \"$lt_newargv0\": errno = %d\n", errno)); 
> +      return 127;
> +    }
> +  return rval;
> +}
>  EOF
>               ;;
>             *)
>               cat <<EOF
>    execv ("$lt_newargv0", newargz);
> +  return rval; /* =127, but avoids unused variable warning */
> +}

Is this code ever to be run on a different system than MinGW?  What
about cross-compile setups?  (Maybe you've addressed this in one of
the comments of all your patches and I've overlooked it now?)

> @@ -6708,8 +6777,13 @@
>       esac
>       case $host in
>         *cygwin* | *mingw* )
> -         output_name=`basename $output`
> -         output_path=`dirname $output`
> +         func_basename "$output"
> +         output_name="$func_basename_result"
> +         func_dirname "$output"
> +         output_path="$func_dirname_result"

Superfluous quotes as above (2 instances).

I would prefer some shorter names in this.  Consider a followup patch to
  s/func_emit_libtool_wrapper_script/func_emit_wrapper/g
  s/func_emit_libtool_cwrapperexe_source/func_emit_cwrapperexe_src/g

as approved.

With this patch, and Debian etch's MinGW cross compiler package mingw32
I get some test failures in the new suite.  The old suite has failed
demo-hardcode.test and demo-relink.test for a long time (let's not worry
about them now).  The new suite fails tests 25 26 32 54 now, which it
didn't back on 2007-02-24, which is when I last tested it.  I haven't
localized it any further yet, so it may be completely independent of
this patch.  It's fine with me if you ignore that for now and we look at
it after this patch is in, but it should not be forgotten.

Please hold on for 48 hours, if there are no further objections until
then, then consider the patch approved with Noah's and above nits
addressed.

Cheers, thanks for all your work, and sorry for the huge delay,
Ralf