Re: [PATCH] Add --lt-* options to shell wrapper
Hi Chuck, Den 2010-02-19 03:48 skrev Charles Wilson: 69: dlloader API FAILED (dlloader-api.at:416) *snip* 069: -- who knows, but it's somewhere in libltdl not libtool --- expout 2010-02-18 03:12:56.23160 -0500 +++ .../tests/testsuite.dir/at-groups/69/stdout 2010-02-18 03:13:09.26160 -0500 @@ -9,11 +9,7 @@ first_open denies a request result: module_symbol first_open denies a request -last_open (last): last_ctx -last_sym (last): last_ctx -result: last_symbol -first_open denies a request last_open denies a request +lt_dlopen failed: file not found first_exit: first_ctx -last_close (last): last_ctx last_exit: last_ctx This failure is curious. It doesn't happen when I run the testsuite on Cygwin, so I'm wondering what's causing the badness... How did you configure your libtool? I have tested with a plain old ../configure, and with --disable-shared which makes the test skip and with --disable-static with no effect on the test. Do you have some verbose output to share? Cheers (and sorry for the delay on this) Peter -- They are in the crowd with the answer before the question. Why do you dislike Jeopardy?
Re: [PATCH] Add --lt-* options to shell wrapper
Ralf Wildenhues wrote: 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. Squashed together and reposted from: http://lists.gnu.org/archive/html/libtool-patches/2009-12/msg00015.html and followon http://lists.gnu.org/archive/html/libtool-patches/2010-01/msg00048.html with more additions arising from Ralf's further comments this week: * libltdl/config/ltmain.m4sh (func_emit_wrapper_impl): Renamed to... (func_emit_wrapper): This. Old version removed. (func_parse_lt_options, func_lt_dump_args, func_exec_program_core): Use $LINENO, not @@LINENO@@. (func_opts_contain_lt): Removed. (func_exec_program): Rewrite to avoid forks and requoting. This (combination) was the second of the two patches whose /first/ 72hour rule invocation was here: http://lists.gnu.org/archive/html/libtool-patches/2010-02/msg3.html Starting a new clock. No regressions attributable to these changes, on cygwin or mingw. No test failures at all on linux. Consolidated changelog: 2010-02-18 Charles Wilson ... Add --lt-* options to shell wrapper * libltdl/config/ltmain.m4sh (func_emit_wrapper): Call func_parse_lt_options early. Use func_exec_program. (func_emit_wrapper:func_parse_lt_options): New function. (func_emit_wrapper:func_lt_dump_args): New function. (func_emit_wrapper:func_exec_program_core): New function. (func_emit_wrapper:func_exec_program): New function. Test results: It does not appear that there were any regressions caused by THESE changes. There are a few items that need investigation, but...later. cygwin == All old tests passed. New tests: 48: deplib in subdir FAILED (deplib-in-subdir.at:140) 69: dlloader API FAILED (dlloader-api.at:416) 92: C++ exception handling FAILED (exceptions.at:254) 102: Run tests with low max_cmd_len FAILED (cmdline_wrap.at:43) 048: -- libtool: install: cp .libs/m3.exe .../tests/testsuite.dir/048/inst/bin/m3.exe ../../libtool/tests/deplib-in-subdir.at:140: $LIBTOOL --mode=clean rm -f sub1/liba1.la sub1/liba2.la sub1/liba3.la sub2/subsub/libb1.la sub2/subsub/libb2.la sub2/subsub/libb3.la sub3/subsub/m1$EXEEXT sub3/subsub/m2$EXEEXT sub3/subsub/m3$EXEEXT stderr: .../libtool: line 1693: sub3/subsub/sub3/subsub/.libs/m1_ltshwrapper: No such file or directory in func_mode_uninstall, func_ltwrapper_scriptname_result is wrong because objdir has been reset to be fullpathto, rather than just .libs, before call to func_ltwrapper_scriptname. So, when trying to source the script, it dies. 069: -- who knows, but it's somewhere in libltdl not libtool --- expout 2010-02-18 03:12:56.23160 -0500 +++ .../tests/testsuite.dir/at-groups/69/stdout 2010-02-18 03:13:09.26160 -0500 @@ -9,11 +9,7 @@ first_open denies a request result: module_symbol first_open denies a request -last_open (last): last_ctx -last_sym (last): last_ctx -result: last_symbol -first_open denies a request last_open denies a request +lt_dlopen failed: file not found first_exit: first_ctx -last_close (last): last_ctx last_exit: last_ctx 092: -- segfault in executable. Pretty sure this isn't a problem in ltmain.m4sh test X$host != X$build test -x $lt_exe exit 77; exit $lt_status; fi stderr: exceptions_in_prog caught: exception in program exceptions_in_lib caught inside lib: exception in library caught: exception from library exceptions_in_module caught inside module: exception in module caught: exception from module .../tests/testsuite.dir/at-groups/92/test-source: line 377: 5920 Segmentation fault (core dumped) $LIBTOOL --mode=execute -dlopen m/module.la $lt_exe stdout: ../../libtool/tests/exceptions.at:254: exit code was 139, expected 0 102: -- low max_cmd_len: repeat of above errs mingw: == FAIL: tests/tagdemo-make.test FAIL: tests/tagdemo-make.test FAIL: tests/tagdemo-make.test FAIL: tests/tagdemo-make.test 27: C++ convenience archives FAILED (convenience.at:99) 48: deplib in subdir FAILED (deplib-in-subdir.at:140) 62: C++ subdir-objects FAILED (am-subdir.at:148) 89: (C++ template tests):simple template test FAILED (template.at:89) 90: (C++ template tests):template test with subdirs FAILED (template.at:241) 91: C++ static constructors FAILED (ctor.at:62) 92: C++ exception handling FAILED (exceptions.at:242) 102: Run tests with low max_cmd_len FAILED
Re: [PATCH] Add --lt-* options to shell wrapper
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
Re: [PATCH] Add --lt-* options to shell wrapper
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
Re: [PATCH] Add --lt-* options to shell wrapper
* Charles Wilson wrote on Sun, Feb 14, 2010 at 03:36:57PM CET: Charles Wilson wrote: Charles Wilson wrote: Charles Wilson wrote: This one, I think is OK for pre-2.2.8 -- what do you guys think? OK to push? In response to review comments over here: Re: [PATCH] Enable runtime cwrapper debugging; add tests http://lists.gnu.org/archive/html/libtool-patches/2009-12/msg00014.html I've created a followon patch to this one which allows the cwrapper tests to pass on platforms which don't use the cwrapper but instaed use the shell wrapper (e.g. linux). ping... ping again. 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. The reference to _AC_INIT_PREPARE is not needed. 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. 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. I know you deserve a better review, but I've been AFK and the 72 hours are almost over. Cheers, Ralf
Re: [PATCH] Add --lt-* options to shell wrapper
Charles Wilson wrote: Charles Wilson wrote: This one, I think is OK for pre-2.2.8 -- what do you guys think? OK to push? In response to review comments over here: Re: [PATCH] Enable runtime cwrapper debugging; add tests http://lists.gnu.org/archive/html/libtool-patches/2009-12/msg00014.html I've created a followon patch to this one which allows the cwrapper tests to pass on platforms which don't use the cwrapper but instaed use the shell wrapper (e.g. linux). ping...
Re: [PATCH] Add --lt-* options to shell wrapper
Charles Wilson wrote:
This one, I think is OK for pre-2.2.8 -- what do you guys think?
OK to push?
In response to review comments over here:
Re: [PATCH] Enable runtime cwrapper debugging; add tests
http://lists.gnu.org/archive/html/libtool-patches/2009-12/msg00014.html
I've created a followon patch to this one which allows the cwrapper
tests to pass on platforms which don't use the cwrapper but instaed use
the shell wrapper (e.g. linux).
If this follow-on, and the original patch for this thread, are approved,
then I'll squash these two patches into a single commit and update the
log before pushing.
Full test suite in progress on cygwin (but cwrapper test passes, as do a
number of spot-checked tests in the old test suite). Linux results:
Old: All 94 tests passed
New: Only two unexpected results (in particular, the cwrapper test passed)
21: passing CXX flags through libtool FAILED (flags.at:24)
100: Run tests with low max_cmd_len FAILED
(cmdline_wrap.at:43)
Err.. 21 should have been skipped, because I haven't installed g++ on
the linux box yet. And 100 is just a repeat of 21.
--
Chuck
Update sh wrapper per review comments
libltdl/config/ltmain.m4sh (func_emit_wrapper): Rename to
(func_emit_wrapper_impl): this. Change debug messages to
conform to GCS. In debug mode, print a banner with known
content before any other output.
(func_emit_wrapper): New function calls *_impl and adds
line number information.
diff --git a/libltdl/config/ltmain.m4sh b/libltdl/config/ltmain.m4sh
index 2ebf12e..bb89cef 100644
--- a/libltdl/config/ltmain.m4sh
+++ b/libltdl/config/ltmain.m4sh
@@ -2332,7 +2332,7 @@ func_extract_archives ()
}
-# func_emit_wrapper [arg=no]
+# func_emit_wrapper_impl [arg=no]
#
# Emit a libtool wrapper script on stdout.
# Don't directly open a file because we may want to
@@ -2346,9 +2346,9 @@ func_extract_archives ()
# will assume that the directory in which it is stored is
# the $objdir directory. This is a cygwin/mingw-specific
# behavior.
-func_emit_wrapper ()
+func_emit_wrapper_impl ()
{
- func_emit_wrapper_arg1=${1-no}
+ func_emit_wrapper_impl_arg1=${1-no}
$ECHO \
#! $SHELL
@@ -2412,7 +2412,7 @@ _LTECHO_EOF'
# Very basic option parsing. These options are (a) specific to
# the libtool wrapper, (b) are identical between the wrapper
# /script/ and the wrapper /executable/ which is used only on
-# windows platforms, and (c) all exist in the --lt- namespace
+# windows platforms, and (c) all begin with the string --lt-
# (application programs are unlikely to have options which match
# this pattern).
#
@@ -2443,6 +2443,11 @@ func_parse_lt_options ()
;;
esac
done
+
+ # Print the debug banner immediately:
+ if test -n \\$lt_option_debug\; then
+echo \${outputname}:${output}:@@LINENO@@: libtool wrapper (GNU $PACKAGE$TIMESTAMP) $VERSION\ 12
+ fi
}
# Sets opts_contain_lt_result to \yes\ if the
@@ -2469,7 +2474,7 @@ func_lt_dump_args ()
lt_dump_args_N=1;
for lt_arg
do
-\$ECHO \(main) newargz[\$lt_dump_args_N] : \$lt_arg\
+\$ECHO \${outputname}:${output}:@@LINENO@@: newargv[\$lt_dump_args_N]: \$lt_arg\
lt_dump_args_N=\`expr \$lt_dump_args_N + 1\`
done
}
@@ -2483,7 +2488,7 @@ func_exec_program_core ()
*-*-mingw | *-*-os2* | *-cegcc*)
$ECHO \
if test -n \\$lt_option_debug\; then
-\$ECHO \(main) lt_argv_zero : \$progdir\$program\ 12
+\$ECHO \${outputname}:${output}:@@LINENO@@: newargv[0]: \$progdir\$program\ 12
func_lt_dump_args \${1+\\...@\} 12
fi
exec \\$progdir\$program\ \${1+\\...@\}
@@ -2493,7 +2498,7 @@ func_exec_program_core ()
*)
$ECHO \
if test -n \\$lt_option_debug\; then
-\$ECHO \(main) lt_argv_zero : \$progdir/\$program\ 12
+\$ECHO \${outputname}:${output}:@@LINENO@@: newargv[0]: \$progdir/\$program\ 12
func_lt_dump_args \${1+\\...@\} 12
fi
exec \\$progdir/\$program\ \${1+\\...@\}
@@ -2559,7 +2564,7 @@ func_exec_program ()
# Usually 'no', except on cygwin/mingw when embedded into
# the cwrapper.
- WRAPPER_SCRIPT_BELONGS_IN_OBJDIR=$func_emit_wrapper_arg1
+ WRAPPER_SCRIPT_BELONGS_IN_OBJDIR=$func_emit_wrapper_impl_arg1
if test \\$WRAPPER_SCRIPT_BELONGS_IN_OBJDIR\ = \yes\; then
# special case for '.'
if test \\$thisdir\ = \.\; then
@@ -2659,7 +2664,32 @@ func_exec_program ()
fi\
}
-
+# func_emit_wrapper [arg1=no]
+#
+# Emit a libtool wrapper script on stdout. See
+# func_emit_wrapper_impl for a more throrough description.
+# This function merely serves to process that function's
+# result to insert line numbers, because we can't assume
+# that \$LINENO works.
+func_emit_wrapper ()
+{
+ # sed script adapted from autoconf's AS_LINENO_PREPARE
+ func_emit_wrapper_impl ${1-no} |\
+ sed -n '
+p
+/@@LINENO@@/=
+' | sed '
+ s/@@LINENO@@.*/-/
+ t lineno
+ b
+ :lineno
+ N
+ :loop
+
Re: [PATCH] Add --lt-* options to shell wrapper
Charles Wilson wrote: * libltdl/config/ltmain.m4sh (func_emit_wrapper) [func_parse_lt_options]: New function. [func_opts_contain_lt]: New function. [func_lt_dump_args]: New function. [func_exec_program_core]: New function, refactored from [main]. [func_exec_program]: New function. [main]: Call func_parse_lt_options early. Use func_exec_program. (func_emit_cwrapperexe_src) [main]: Reword error message to align with (upcoming) documentation. --- For symmetry, support --lt-debug and --lt-dump-script in the shell wrapper, as well as in the cwrapper. Ping? http://lists.gnu.org/archive/html/libtool-patches/2009-07/msg00014.html This is the second of a series of three patches, that encompass previously proposed changes to the wrapper system. This one just tries to ensure that both the w-script and the w-exe accept the same --lt- options, and reject un-implemented --lt- ones. It arose as a result of discussions surrounding http://lists.gnu.org/archive/html/libtool-patches/2009-06/msg00031.html It has been part of the cygwin libtool distribution for five months, and the mingw libtool distribution for three. However, since all platforms (not just cygwin and mingw) use the wrapper script, this patch will affect them, too. I've tested on linux, where it works fine, but not any other platforms. I'm going to be (re)raising all of my old, outstanding patches over the next week. Some, I think, are OK for immediate push, even 'relatively close to 2.2.8'. Others may be too big a change to consider at this point, and that's fine. Just let me know if you guys think a particular patch should be deferred until post-2.2.8 and I'll take it off the table. This one, I think is OK for pre-2.2.8 -- what do you guys think? OK to push? -- Chuck
