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. You're assuming that any --lt- option will precede any options/args meant for the real executable, but that isn't necessarily the case, and is not currently required by the cwrapper. This patch is intended to duplicate, within the shwrapper, the current behavior of the cwrapper. If you want to impose a restriction that all --lt- options must appear first, in order to simplify the option handling in the shwrapper, then to be consistent you must also be proposing that the cwrapper code be changed. Yet, we discussed this two years ago (see below). > 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?) > 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: http://lists.gnu.org/archive/html/libtool-patches/2008-05/msg00053.html http://lists.gnu.org/archive/html/libtool-patches/2008-05/msg00063.html http://lists.gnu.org/archive/html/libtool-patches/2008-05/msg00064.html http://lists.gnu.org/archive/html/libtool-patches/2008-05/msg00066.html http://lists.gnu.org/archive/html/libtool-patches/2008-05/msg00067.html IMO, (1) a patch to change this behavior should either go first in cwrapper -- if that's what you are requiring -- and then this patch can be revised to mimic the new cwrapper behavior. or (2) This patch should go in as-is (modulo other changes below, and the existing followon patch), and then *second* followon patch to change the behavior of both the cwrapper and shwrapper option handling, and to simplify the code involved in doing that, should be considered separately. If that's what you're requiring. (3) This patch should go in as-is (modulo...) You pick. > 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. OTOH, I really didn't need to copy into the shwrapper all the extra gunk employed by ltmain.sh to emulate LINENO -- since I can simply sed out a magic symbol while emitting the script contents... 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. > I know you deserve a better review, but I've been AFK and the 72 hours > are almost over. Clock stopped. I'll repost a new revision soon. It will consist of: (a) this patch (b) the followon patch (c) remove references to _AC_INIT_PREPARE all squashed into a single patch. However, I'll wait until Ralf picks from options (1), (2), or (3) above, before doing any of this. -- Chuck
