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