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

[Noah Misch]

Hi Charles,

Overall, the patch looks suitable.  Some minor comments:

On Sun, Jun 10, 2007 at 02:23:58PM -0400, Charles Wilson wrote:
> 2007-04-22  Charles Wilson  <address@hidden>
> 
>       * libltdl/config/ltmain.m4sh (func_ltwrapper_script_p):
>       new function detects if $1 is a libtool sh wrapper

See `info standards doc change' for change log style information.  The sentence
after the colon begins with a capital letter and ends with a period.  It is
enough to write "New function." when that is the case.

> --- libltdl/config/ltmain.m4sh        9 Jun 2007 17:46:40 -0000       1.79
> +++ libltdl/config/ltmain.m4sh        10 Jun 2007 07:22:05 -0000
> @@ -661,13 +661,61 @@
>      test "$lalib_p" = yes
>  }
>  
> +# func_ltwrapper_script_p file
> +# True iff FILE is a libtool wrapper script
> +# This function is only a basic sanity check; it will hardly flush out
> +# determined imposters.
> +func_ltwrapper_script_p ()
> +{
> +    func_lalib_p "$1"
> +}
> +
> +# func_ltwrapper_executable_p file
> +# True iff FILE is a libtool wrapper executable
> +# This function is only a basic sanity check; it will hardly flush out
> +# determined imposters.
> +func_ltwrapper_executable_p ()
> +{
> +    func_ltwrapper_executable_p_result=no
> +    if ! func_ltwrapper_script_p "$1" ; then

The `!' operator is not portable; use `if X; then :; else'.  You could instead
add a different magic string to executables, avoiding two forks for this test.

> +      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"
> +}
> +
> +# 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"
> +        func_stripname '' '.exe' "$func_basename_result"
> +     if test -z "$func_dirname_result"; then
> +          
> func_ltwrapper_temp_scriptname_result="./$objdir/${func_stripname_result}_ltshwrapperTMP"
> +        else
> +          
> func_ltwrapper_temp_scriptname_result="$func_dirname_result/$objdir/${func_stripname_result}_ltshwrapperTMP"
> +        fi
> +    fi     

Trim trailing white space from this line.

> +}
> +
>  # 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"
>  }
>  
>  
> @@ -1649,11 +1697,17 @@
>        -*) ;;
>        *)
>       # Do a test to see if this is really a libtool program.
> -     if func_ltwrapper_p "$file"; then
> -       func_source "$file"
> -
> +     if func_ltwrapper_script_p "$file"; then
> +          func_source "$file"
>         # Transform arg to wrapped name.
>         file="$progdir/$program"
> +        else
> +          if func_ltwrapper_executable_p "$file"; then

Use `elif'.

> +            func_ltwrapper_temp_scriptname "$file"
> +         func_source "$func_ltwrapper_temp_scriptname_result"
> +         # Transform arg to wrapped name.
> +         file="$progdir/$program"
> +          fi
>       fi

Place `file="$progdir/$program"' here, rather than keeping a copy in each branch
of the control flow.

> @@ -2561,6 +2620,7 @@
>    char *tmp_pathspec;
>    char *actual_cwrapper_path;
>    char *shwrapper_name;
> +  intptr_t rval = 127;

Do all interesting versions of Cygwin and MinGW have intptr_t?

> @@ -6717,19 +6791,30 @@
>  
>           func_emit_libtool_cwrapperexe_source > $cwrappersource
>  
> -       # we should really use a build-platform specific compiler
> -       # here, but OTOH, the wrappers (shell script and this C one)
> -       # are only useful if you want to execute the "real" binary.
> -       # Since the "real" binary is built for $host, then this
> -       # wrapper might as well be built for $host, too.
> -       $opt_dry_run || $LTCC $LTCFLAGS -s -o $cwrapper $cwrappersource
> +         # we should really use a build-platform specific compiler
> +         # here, but OTOH, the wrappers (shell script and this C one)
> +         # are only useful if you want to execute the "real" binary.
> +         # Since the "real" binary is built for $host, then this
> +         # wrapper might as well be built for $host, too.
> +         $opt_dry_run || $LTCC $LTCFLAGS -s -o $cwrapper $cwrappersource

As far as I can see, this chunk just converts spaces to tabs.  That may be a
worthy change, but it does not belong in this patch.

> +
> +         # Now, create the wrapper script for func_source use:
> +         func_ltwrapper_temp_scriptname $cwrapper
> +         $RM $func_ltwrapper_temp_scriptname_result
> +         trap "$RM $func_ltwrapper_temp_scriptname_result; exit 
> $EXIT_FAILURE" 1 2 15
> +         $opt_dry_run || {
> +           $cwrapper --lt-dump-script > 
> $func_ltwrapper_temp_scriptname_result
> +         }
> +         chmod +x $func_ltwrapper_temp_scriptname_result

This script need not be executable, since we only source it.