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

[Charles Wilson]

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