Re: [PATCH] Add --lt-* options to shell wrapper

[Ralf Wildenhues]

Hi Charles,

* Charles Wilson wrote on Wed, Feb 17, 2010 at 03:30:47PM CET:
> Ralf Wildenhues wrote:
> > The option parsing in the original patch is overkill -- no need to
> > re-quote things if all you're going to do is remove a couple of entries
> > from "$@", that can be done with
> >   set x "$@"
> >   shift
> >   shift
> > 
> > type handling.
> 
> No, actually it can't.

Wrong.  The if-true branch of this:

+  if test -n \"\$opts_contain_lt_result\"; then
+    # the following is adapted from _AC_INIT_PREPARE, except
+    # (1) we don't care about duplicates, and
+    # (2) we strip out --lt-*, not --no-create/--no-recursion/--silent
+    lt_wrapper_args=
+    for lt_wr_arg
+    do
+      case \$lt_wr_arg in
+      --lt-*) continue ;;
+      *\\'*)
+        lt_wr_arg=\`\$ECHO \"X\$lt_wr_arg\" |
+          $SED -e \"s/^X//\" -e \"s/'/'\\\\\\\\\\\\\\\\''/g\"\`
+        ;;
+      esac
+      lt_wrapper_args=\"\$lt_wrapper_args '\$lt_wr_arg'\"
+    done
+    eval func_exec_program_core \$lt_wrapper_args
+  else
+    func_exec_program_core \${1+\"address@hidden"}
+  fi

can be written, modulo the top layer of quoting, without forks:

     for lt_wr_arg
     do
       case $lt_wr_arg in
       --lt-*) ;;
       *) set x "$@" "$lt_wr_arg"; shift;;
       esac
       shift
     done
     func_exec_program_core ${1+"$@"}

> > The reference to _AC_INIT_PREPARE is not needed.
> 
> I'll remove it. (But that should have been a hint, if autoconf needed
> complicated requoting, for why 'set x "$@"' isn't sufficient when
> removing arbitrary, not initial, values from "$@" -- otherwise, why does
> autoconf do it?)

configure needs requoting because it needs to use eval, and that either
because it may add arguments that need quoting, and/or because it cannot
work off of "$@" throughout the script.  The above doesn't have these
limitations.

Forgetting double-quotes in the eval line

+    eval func_exec_program_core \$lt_wrapper_args

which, without top-level quoting would have needed to be

     eval func_exec_program_core "$lt_wrapper_args"

and with top-level needs at least

     eval func_exec_program_core \"\$lt_wrapper_args\"

in order to preserve e.g., two consecutive spaces.

We should ensure that such issues do not happen (by exposing them in the
testsuite, if we don't do that already).

> > Did you consider that the program we're wrapping might have argument
> > structure like
> >   --some-option some-arbitrary-argument-to-this-option
> > 
> > and that the arbitrary argument might reasonably start with --lt-?
> > Just sayin.
> 
> Yes. Discussed two years ago:

Ah, ok.  Drop this remark of mine then.  Thanks.

> > The followon patch adds even more bloat for $LINENO which I don't
> > understand what you're guarding against, and who this is trying to
> > help.
> 
> You said:
> http://lists.gnu.org/archive/html/libtool-patches/2010-01/msg00029.html
> > Aside, all these messages from the wrapper do not conform to the GNU
> > Coding Standards, which has pretty detailed requirements about how
> > they should look like.
> 
> So, in order to make the debug messages from the shwrapper follow the
> GCS, I added @@LINENO@@.  I couldn't use $LINENO, because it's not
> supported by all shells -- and allowing the existing ltmain.sh LINENO
> emulation to do the job would result in the emitted scripts reporting
> the line number within the *libtool* script, not within the shwrapper
> script itself.

Suitably escaped, $LINENO support should work well enough for the
shell that we pick, and for the simple use case within the wrapper
script; see autoconf.info(Special Shell Variables).  I simply don't
see the need for special code, that's the only part which I should
have been complaining about here.

> I'm a little confused here, Ralf.  You make a comment and require
> revisions to a patch...and then, you call those revisions (more) bloat.
> We now have two examples:
> 
>  (a) Adding --lt- handling to the shell wrapper at all was /your/
> suggestion, so that the cwrapper and shwrapper had the same external
> API. In doing this, we removed several pre-existing --lt- options from
> the cwrapper, mostly to pare down what had to be implemented in the
> shwrapper, as well as to minimize what had to be documented (and thus
> carved into stone). But even to implement that subset, requires code
> (and unfortunately, a substantial amount of it).  Which you now call bloat.
> 
>  (b) Requiring strict compliance with the GCS means that messages must
> report their lineno.  This requires code -- but you call that "more" bloat.
> 
> Either you want these things, or you don't -- and if you do, then its
> unfair to call the code that implements them "bloat" without proposing
> an alternate, more efficient, means of achieving them.

See above for the two cases.  With them fixed, I think the patch looks
to be in fairly good shape, except that shell quoting bugs are really
hard to detect when reading the doubly-quoted code.  So please fix
above, resend, and apply if you don't hear back after 72 hours.

Thank you for your patience with me,
Ralf