[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Add --lt-* options to shell wrapper
From: |
Ralf Wildenhues |
Subject: |
Re: [PATCH] Add --lt-* options to shell wrapper |
Date: |
Wed, 17 Feb 2010 20:49:35 +0100 |
User-agent: |
Mutt/1.5.20 (2009-10-28) |
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