Re: [PATCH] [mingw|cygwin] Modify cwrapper to invoke target directly.

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Charles Wilson on 5/5/2008 6:23 PM:
| 2008-05-05  Charles Wilson  <...>
|
|       * libltdl/config/ltmain.m4sh (func_to_native_path):

I've become accustomed to using a 1-line summary in my ChangeLog messages
prior to the first '* file:' line; that way, the summary can be reused as
the git commit summary.

|
| I'm sorta thinking I should rename the func_to_native* functions
s/native/host/ ?

Yes, that would be appropriate.  I'd like to see that, and other cleanups
below, before approving.  I haven't tested yet, but the concepts look sane
by inspection.

| +func_to_native_path ()
| +{
| +  func_to_native_path_result="$1"
| +  if test -n "$1" ; then
| +    case $host in
| +      *mingw* )
| +        lt_sed_naive_backslashify='s|\\\\*|\\|g;s|/|\\|g;s|\\|\\\\|g'
| +        case $build in
| +          *mingw* ) # actually, msys
| +            # awkward: cmd appends spaces to result
| +            lt_sed_strip_trailing_spaces="s/[ ]*\$//"
| +            func_to_native_path_tmp1=`( cmd //c echo "$1" | $SED -e
"$lt_sed_strip_trailing_spaces" ) 2>/dev/null || echo ""`
| +            func_to_native_path_result=`echo
"$func_to_native_path_tmp1" | $SED -e "$lt_sed_naive_backslashify"`

Some pretty long lines; is it worth trying to wrap at 80 columns?


| +            ;;
| +          *cygwin* )
| +            func_to_native_path_tmp1=`cygpath -w "$1"`
| +            func_to_native_path_result=`echo
"$func_to_native_path_tmp1" | $SED -e "$lt_sed_naive_backslashify"`
| +            ;;
| +        esac
| +        if test -z "$func_to_native_path_result" ; then
| +          func_error "Could not determine native path corresponding to"
| +          func_error "  '$1'"
| +          func_error "Perhaps it doesn't exist."
| +          func_error "Continuing, but running uninstalled executables
may not work."

Doesn't func_error die when called?  If so, how do the subsequent three
errors get printed?  If not, the name seems a bit misleading...

| +func_to_native_pathlist ()
| +{
| +  func_to_native_pathlist_result="$1"
| +  if test -n "$1" ; then
| +    case $host in
| +      *mingw* )
| +        lt_sed_naive_backslashify='s|\\\\*|\\|g;s|/|\\|g;s|\\|\\\\|g'
| +        case $build in
| +          *mingw* | *cygwin* )
| +            # remove leading and trailing ':' from $1.  msys behavior is
| +            # inconsistent here, and cygpath turns them into into '.;'
and ';.'
| +            func_to_native_pathlist_tmp1="$1"
| +            func_to_native_pathlist_tmp2=`echo
"$func_to_native_pathlist_tmp1" | $SED -e 's|^:*||'`
| +            func_to_native_pathlist_tmp1=`echo
"$func_to_native_pathlist_tmp2" | $SED -e 's|:*$||'`

Avoid some forks - can't you combine this into one sed invocation?
Similar comment about long lines.

| +
| +static const char *dumpscript_opt  = "--lt-dump-script";
| +static const char *env_set_opt     = "--lt-env-set";
| +  /* argument is putenv-style "foo=bar", value of foo is set to bar */
| +static const char *env_prepend_opt = "--lt-env-prepend";
| +  /* argument is putenv-style "foo=bar", new value of foo is bar${foo} */
| +static const char *env_append_opt  = "--lt-env-append";
| +  /* argument is putenv-style "foo=bar", new value of foo is ${foo}bar */

It's a bit of a shame that we can't rely on getopt_long and get option
abbreviations; oh well.

| -         cat <<EOF
| +         cat <<"EOF"

I find 'cat <<\EOF' easier to type (one less character); but the result is
the same; if any part of EOF is quoted in any fashion, the entire here-doc
is parsed without expansion.

| +  target_name = (char *) xstrdup (base_name (actual_cwrapper_path));

Why the cast?  Shouldn't xstrdup output char* already?  Also, gnulib's
base_name mallocs; if we ever make libtool's base_name match, then this
would leak memory (ie. xstrdup of gnulib's base_name is wasteful).  But
your patch didn't affect base_name.

| +          if (i+1 < argc)

Coding style: s/i+1/i + 1/

| +            {
| +              lt_opt_process_env_set (argv[i+1]);
| +              i++; /* don't copy */
| +            }
| +          else
| +            {
| +              lt_fatal ("%s missing required argument", env_set_opt);
| +            }

Coding style: a single expression inside a control block does not need braces.

| +          continue;
| +        }
| +      if (strcmp (argv[i], env_prepend_opt) == 0)

This requires --lt-env-prepend foo=bar, rather than allowing
- --lt-env-prepend=foo=bar; is it worth allowing both syntax forms for
consistency with GNU coding standards?  Actually, since the wrapper has
such a limited usage, I'm probably okay with the single form.

| +      /* otherwise ... */
| +      newargz[++newargc] = xstrdup (argv[i]);

Shouldn't you handle "--" as the end of wrapper options, in case the user
really wants to pass --lt-env-* as the first option to the wrapped executable?

| -             cat <<EOF
| -  execv ("$lt_newargv0", newargz);
| +             cat <<"EOF"
| +  execv (newargz[0], newargz);

I would rather see argv[0] in the wrapped executable as the original name
of the wrapper script, rather than newargz[0].  That way, messages printed
in the wrapped executable have the simpler name of the cwrapper (ie. it's
much nicer, when invoking 'm4', to see 'm4: error' than
'/path/to/lt-m4.exe: error', without having to know that 'm4' is just a
cwrapper).

| +lt_extend_str (const char *orig_value, const char *add, int to_end)
| +{
| +  char *new_value;
| +  if (orig_value && *orig_value)
| +    {
| +      int len = strlen (add) + strlen (orig_value) + 1;
| +      new_value = XMALLOC (char, len);
| +      if (to_end)
| +        {
| +          strcpy (new_value, orig_value);
| +          strcat (new_value, add);

strcat can be inefficient if orig_value is long.  Why not cache the
lengths, then use strcpy into the correct offset?

| +  strncpy (*name, arg, len-1);
| +  (*name)[len-1] = '\0';

coding style: s/len-1/len - 1/

| +      char *new_value = lt_extend_str (getenv (name), value, 0);
| +      /* some systems can't cope with a ':'-terminated path #' */

What's up with the #' in the comment?

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkggWzYACgkQ84KuGfSFAYCnxQCgtjmEmZHH1OtgxLT3ACCYwW+6
iBMAoMpRj2Hqpi+SZhjoTANv/4I49Gy/
=t9g7
-----END PGP SIGNATURE-----