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

[Charles Wilson]

Noah Misch wrote:
> Overall, the patch looks suitable.  Some minor comments:

> > +    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.

That's a good idea; I'll do that.

> > +    fi     
>
> Trim trailing white space from this line.

done

> > +        else
> > +          if func_ltwrapper_executable_p "$file"; then
>
> Use `elif'.

done

> > +            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.

OK.

> > @@ -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?

Yes, going back at least to 2000.  There have been issues with the 
_INTPTR_T_DEFINED (__intptr_t_defined on cygwin) guards clashing (or, 
rather, the absence of these guards causing the typedef itself to clash) 
with the winapi headers.  But as the wrapper.c code does not include any 
winapi headers, that's not an issue here.

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

No, I didn't convert spaces to tabs (at least, not that I can tell. Each 
line still begins with a tab both before the patch and after). The 
difference is I corrected the actual indentation of a small section of 
the block I was already changing for other reasons:

> > +
> > +           # 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.

Done.

I'm still testing the revised patch; I'll post it with an updated 
changelog tomorrow.

--
Chuck