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+\"\...@\"} + 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