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

[Charles Wilson]

Ralf Wildenhues wrote:
> 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
> }

OK.

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

Yes.  I mentioned this as a possible portability problem here:
http://lists.gnu.org/archive/html/libtool-patches/2007-04/msg00052.html
But as you note above, and as I noted then, we're already relying on 
grepping a binary file in func_ltwrapper_p/func_lalib_p.

I'll add the 2>&1 redirection.

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

Ack.

> [...]
> >  # 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/

Hmm...thanks.  I see why this typo didn't me a problem, but it should be 
fixed.

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

Ack.

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

In one of the previous patches, we added this bit of support

+           if test -n "$TARGETSHELL" ; then
+             # no path translation at all
+             lt_newargv0=$TARGETSHELL
+           else
+             case "$host" in
+               *mingw* )
< lots of ugly code to sniff out the 'cmd shell' >
+                 ;;
+               * ) lt_newargv0=$SHELL ;;
+             esac
+           fi

for cross-compiling in general (but mainly linux->mingw/wine cross, IIRC).

Now, in this current patch, we only use _spawnv() if $host == mingw. 
Maybe we could be more clever than that, and only use _spawnv if mingw 
and native.  However, we'd then need to do something about this:

            # 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

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

Ack.

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

OK.

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

This pass native (FYI).

> 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 got those failures (native), until I fixed tests/destdir.at to include 
$EXEEXT on $bindir/m:

-if test "$OBJDUMP" != false && ($OBJDUMP -p $bindir/m) >/dev/null 2>&1; 
then
-  AT_CHECK([$OBJDUMP -p $bindir/m | $EGREP -i "R(UN)?PATH.*$DESTDIR"], [1])
+if test "$OBJDUMP" != false && ($OBJDUMP -p $bindir/m$EXEEXT) 
>/dev/null 2>&1; then
+  AT_CHECK([$OBJDUMP -p $bindir/m$EXEEXT | $EGREP -i 
"R(UN)?PATH.*$DESTDIR"], [1])

Perhaps that "fix" breaks cross?  Objdump complains very loudly if its 
target does not exist.  On native, $bindir/m doesn't exist, but 
$bindir/m.exe does.  I imagine on cross, if m is built using the host 
compiler, the situation may be reversed, but that's just a guess.

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

OK.  With regards to 32 and 54, those should be expected.  32 
(lt_dlopenadvise) is known-bad for win32 targets, and 54 (low 
max_cmd_len) has always been broken for me on both cygwin and mingw.

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

OK.  I could use some advice on the following: trying to add a new magic 
string to the executable, I did the obvious (shown after variable 
replacement, etc:

static const char *MAGIC_EXE = "%%%MAGIC EXE variable%%%";

But the compiler stripped the symbol and the initialization string out 
of the executable.  The following works, provided touch_mem() is called 
during main():

static const char * MAGIC_EXE = "%%%MAGIC EXE variable%%%";
static const char * MAGIC_EXE_REF;
static void touch_mem ()
{
    /* try to ensure MAGIC_EXE doesn't get optimized out */
    volatile const char *data = (volatile const char *)MAGIC_EXE;
    MAGIC_EXE_REF = (const char *)data;
}

But geez is that ugly.  I was considering exporting the symbol from the exe:

const char * __attribute__ ((dllexport)) MAGIC_EXE = "...";

but that'll break cross-compiles, if the wrapper is compiled using the 
host compiler.  I think just declaring the variable non-static will work 
(that's my next test), but will it _always_ work (e.g. retain the 
initilization string in the executable), even with compile/link flags 
"$LTCFLAGS -s" as is used currently, on all hosts?

const char * MAGIC_EXE = "...";

Advice?

--
Chuck