Re: [cygwin] cwrapper emits wrapper script

[Ralf Wildenhues]

Hello Charles,

Thanks for the patch again.  First review:

* Charles Wilson wrote on Sat, Apr 21, 2007 at 03:03:02AM CEST:
> This patch depends on this one: 
> http://lists.gnu.org/archive/html/libtool-patches/2007-04/msg00048.html

(unfortunately, due to idiocy on my part, that patch will have to go
through another iteration.  Review to come up in 1-2 days, hopefully.)

> With this patch, after a successful build the following files are created:
>    foo.exe       -- the wrapper executable. It does NOT launch the
>                     wrapper script 'foo' in thisdir (.)
>    foo           -- a wrapper script. It could be used to launch the
>                     target executable, but isn't normally used for that.
>                     This wrapper is sourced by func_source() when
>                     necessary.
>    .libs/foo.exe -- the target executable
> 
> When the wrapper foo.exe is launched, it generates a new wrapper script
>    .libs/foo_ltshwrapper

Hmm, I'm wondering whether we should keep prefixing within .libs.  Maybe
  .libs/ltsh-foo
?  WDYT?

> At present, the .libs/foo_ltwrappersh is recreated every single time the 
> wrapper executable is run; later, timestamps could be used to avoid 
> this, but that's an optimization.

> Furthermore, the wrapper executable can be invoked with the 
> '-ltdumpscript' option, which will emit the script on *stdout*.

The "interesting" option name is to guard against valid program flags,
right?  What do you think about --lt-dump-script?  It would not fit in
with libtool's other flags, though.  Or maybe an environment variable
rather than a flag?  (I'm simply unsure myself, gathering opinions, not
telling you to change your code here.)

> This patch ALSO fixes the wrapper executable on mingw, by 
> DOS-izing "/bin/sh" when emitting those lines of the executable 
> wrapper's source code.

Thanks, that should fix
<http://lists.gnu.org/archive/html/bug-libtool/2007-03/msg00008.html>.
Please note though that translating a path with MSYS can be done like
this (when $build = $host):
    cmd //c echo "$srcfile"

but in the cross-compile case, we need to do more work, see this report
<http://lists.gnu.org/archive/html/bug-libtool/2007-02/msg00000.html>
(note I'm not asking you to do this work here; actually, I'd like to ask
you not to fix even more different things with one patch.  Merely noting
it in case you're interested.)

>  37: compiling softlinked libltdl  FAILED (nonrecursive.at:90)
>  38: compiling copied libltdl      FAILED (nonrecursive.at:114)
>  39: installable libltdl           FAILED (nonrecursive.at:140)
>  40: compiling softlinked libltdl  FAILED (recursive.at:67)
>  41: compiling copied libltdl      FAILED (recursive.at:87)
>  42: installable libltdl           FAILED (recursive.at:109)

Ah, ok.  Without your patch, I don't get these, but I have 2.61 and 1.10
installed in my MSYS/MinGW, which would explain this.  I also don't
think they have to do with your patch, but will check.

> Failed test was: tests/mdemo-dryrun.test -- false alarm:
> 
> $ diff before after
> 66c66
> < drwxr-xr-x    2 cwilson  Administ        0 Apr 20  2007 bin
> ---
> > drwxr-xr-x    2 cwilson  Administ        0 Apr 20 20:29 bin
> 
> Probably need another 'sleep 1' somewhere, but that's outside the scope 
> of this patch.

Hmm, maybe one after the `rm -f "$prefix/bin/..."' and one after the
`$MAKE uninstall' line?

> --- libltdl/config/ltmain.m4sh        2007-04-19 19:54:53.500000000 -0400
> +++ libltdl/config/ltmain.m4sh        2007-04-20 03:20:46.281250000 -0400
> @@ -2301,6 +2301,20 @@
>      file=\`ls -ld \"\$thisdir/\$file\" | ${SED} -n 's/.*-> //p'\`
>    done
>  
> +  # cygwin/mingw cwrapper will rewrite this line:
> +  WRAPPER_SCRIPT_BELONGS_IN_OBJDIR=no
> +  if test \"\$WRAPPER_SCRIPT_BELONGS_IN_OBJDIR\" = \"yes\"; then

(Not sure about this WRAPPER_SCRIPT_BELONGS_IN_OBJDIR thing yet.)

> +    # special case for '.'
[...]

> @@ -2482,12 +2497,11 @@
>    if (stale) { free ((void *) stale); stale = 0; } \
>  } while (0)
>  
> -/* -DDEBUG is fairly common in CFLAGS.  */
> -#undef DEBUG
> +#undef LTWRAPPER_DEBUGPRINTF
>  #if defined DEBUGWRAPPER
> -# define DEBUG(format, ...) fprintf(stderr, format, __VA_ARGS__)
> +# define LTWRAPPER_DEBUGPRINTF(format, ...) fprintf(stderr, format, 
> __VA_ARGS__)
>  #else
> -# define DEBUG(format, ...)
> +# define LTWRAPPER_DEBUGPRINTF(format, ...)

What's the reason for this change?

> @@ -2496,41 +2510,156 @@
>  char * xstrdup (const char *string);
>  const char * base_name (const char *name);
>  char * find_executable(const char *wrapper);
> +char * chase_symlinks(const char *pathspec);
> +int    make_executable(const char *path);
>  int    check_executable(const char *path);
>  char * strendzap(char *str, const char *pat);
>  void lt_fatal (const char *message, ...);
>  
> +static const char* script_text = 
> +EOF
> +
> +         func_emit_libtool_wrapper_script |\
> +             $SED -e 's/\([\\"]\)/\\\1/g' |\
> +             $SED -e 's/\(WRAPPER_SCRIPT_BELONGS_IN_OBJDIR\)=.*/\1=yes/' |\
> +             $SED -e 's/^/"/' -e 's/$/\\n"/' 

You can merge the 3 sed scripts into one, either by
  $SED 's/.../.../
        s/.../.../
        s/.../.../'

or by multiple -e.  Also, after a pipe (|), there's no need to escape
the newline.

> +         $ECHO ";"

Please use plain `echo' here.  $ECHO is for stuff that can contain \t,
begin with -n, and the like.

> +
> +         cat <<EOF
>  int
>  main (int argc, char *argv[])
>  {
>    char **newargz;
> +  char *tmp_pathspec;
> +  char *actual_cwrapper_path;
> +  char *shwrapper_name;
> +  FILE *shwrapperFILE;

  FILE *shwrapper;

looks much less hungarian.  ;-)

> +
> +  const char* dumpscript_opt = "-ltdumpscript";
> +  size_t dumpscript_len = strlen(dumpscript_opt);
>    int i;
>  
>    program_name = (char *) xstrdup (base_name (argv[0]));
> -  DEBUG("(main) argv[0]      : %s\n",argv[0]);
> -  DEBUG("(main) program_name : %s\n",program_name);
> +  LTWRAPPER_DEBUGPRINTF("(main) argv[0]      : %s\n",argv[0]);
> +  LTWRAPPER_DEBUGPRINTF("(main) program_name : %s\n",program_name);
> +
> +  /* very simple arg parsing; don't want to rely on getopt */
> +  for (i=1; i<argc; i++)
> +  {
> +    if (strncmp(argv[i], dumpscript_opt, dumpscript_len) == 0)

Please use strcmp, and drop dumpscript_len.

> +    {
> +      printf("%s", script_text);
> +      return 0;
> +    }
> +  }
> +
>    newargz = XMALLOC(char *, argc+2);
>  EOF
>  
> -         cat <<EOF
> +         case $host_os in
> +           mingw*)
> +             # such a shame msys has no cygpath-like program...

Let's simplify all this to something like this (untested!):

  lt_mingwSHELL=`( cmd //c echo $SHELL ) 2>/dev/null || echo $SHELL`
  case $lt_mingwSHELL in
  *.exe | *.EXE) ;;
  *) lt_mingwSHELL=$lt_mingwSHELL.exe ;;
  esac

Probably for the cross-compile + simulator case we'd need to allow for
some override.  Not sure if we want to factor out the path translation
(also not sure if we couldn't just use $fix_srcfile_path for this, and
not distinguish between cygwin and mingw at all, but that would be a
more far-reaching change).

[...]
> +         case $host_os in
> +           mingw*)
> +         cat <<"EOF"
> +  {
> +     char* p = newargz[1];
> +     while(*p!='\0') {
> +       if (*p == '\\') {

strchr?

> +         *p = '/';
> +       }
> +       p++;
> +     }
> +  }
> +EOF
> +         ;;
> +         esac
> +
> +         cat <<"EOF"
> +  XFREE( shwrapper_name );
> +  XFREE( actual_cwrapper_path );
> +
> +  if ( (shwrapperFILE = fopen (newargz[1], "wb")) == 0 )

Why binary?

> +  {
> +    lt_fatal("Could not open %s for writing", newargz[1]);
> +  }
> +  fprintf (shwrapperFILE, "%s", script_text);
> +  fclose (shwrapperFILE);
> +
> +  make_executable( newargz[1] );
> +  
>    for (i = 1; i < argc; i++)
>      newargz[i+1] = xstrdup(argv[i]);
>    newargz[argc+1] = NULL;
>  
>    for (i=0; i<argc+1; i++)
>    {
> -    DEBUG("(main) newargz[%d]   : %s\n",i,newargz[i]);
> +    LTWRAPPER_DEBUGPRINTF("(main) newargz[%d]   : %s\n",i,newargz[i]);
>      ;

What's that extra ; for BTW?

>    }
>  

> @@ -2715,6 +2873,62 @@
>  }
>  
>  char *
> +chase_symlinks(const char *pathspec)
> +{
> +#ifndef S_ISLNK
> +    return xstrdup ( pathspec );

Please use GCS formatting, as far as possible (several instances),
esp.: spaces before opening paren, no spaces after nor before closing.

> +#else
> +    char buf[LT_PATHMAX];
> +    struct stat s;
> +    int rv = 0;
> +    char* tmp_pathspec = xstrdup (pathspec);
> +    char* p;
> +    int has_symlinks = 0;
> +    while (strlen(tmp_pathspec) && !has_symlinks)
> +    {
> +        LTWRAPPER_DEBUGPRINTF("checking path component for symlinks: %s\n", 
> tmp_pathspec);
> +        if (lstat (tmp_pathspec, &s) == 0)
> +        {
> +            if (S_ISLNK(s.st_mode) != 0)
> +            {
> +                has_symlinks = 1;
> +                break;
> +            }
> +
> +            /* search backwards for last DIR_SEPARATOR */
> +            p = tmp_pathspec + strlen(tmp_pathspec) - 1;
> +            while ( (p > tmp_pathspec) && (! IS_DIR_SEPARATOR(*p)) )
> +                p--;

strrchr?

> +            if ( (p == tmp_pathspec) && (! IS_DIR_SEPARATOR(*p)) )
[...]

Apologies for being so picky.  Also please note that I haven't had a
chance to test this patch yet, so if you would like me to do it before
you work on it again, please say so.

Cheers, and thanks,
Ralf