Ralf Wildenhues wrote: > * Charles Wilson wrote on Mon, Jun 22, 2009 at 03:50:42AM CEST: >> * libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src) >> [ltwrapper_debugprintf]: Renamed to... > > I think functions should still be put in (parens) in the ChangeLog > entry, not in [brackets], according to GCS.
What about C functions that are emitted in a HERE document as part of the execution of a SH function? That's the distinction I was trying to draw with the variant enclosures. How about: * libltdl/config/ltmain.m4sh (func_emit_cwrapperexe_src:ltwrapper_debugprintf): Renamed to... (func_emit_cwrapperexe_src:lt_debugprintf): this. Only >> [lt_debugprintf]: this. Only print messages if lt_debug != 0. >> [file scope]: Add constants and variables to support new --lt-debug >> option. Remove LTWRAPPER_DEBUGPRINTF macro. >> [main]: Consolidate option parsing. Ensure first use of lt_debugprintf >> occurs after option parsing. Add stanza to parse for --lt-debug and >> set lt_debug variable. >> [all]: Use lt_debugprintf () instead of LTWRAPPER_DEBUGPRINTF (()). >> * tests/cwrapper.at: Add new tests for --lt-debug and -DLT_DEBUGWRAPPER. > The testsuite additions fail on GNU/Linux (with the respective patch for > the shell wrapper applied), for several reasons, the first two of which > are not fixed by your patch update: > > - the program is actually .libs/lt-usea there, (but it might also be > .libs/usea, or just usea on other systems), See next item. > - the shell wrapper outputs 'lt_argv_zero' not 'argv[0]' in its debug > output, Actually, the problem is this: the binary wrapper does the following (end of long lines deleted; unimportant lines deleted): (main) argv[0] : ./bmp2tiff (main) program_name : bmp2tiff (find_executable) : ./bmp2tiff (check_executable) : [snipped] (main) found exe (before symlink chase) at : [snipped] checking path component for symlinks: ... ... (main) found exe (after symlink chase) at : [snipped] (main) libtool target name: bmp2tiff.exe (lt_setenv) setting 'BIN_SH' to 'xpg4' (lt_setenv) setting 'DUALCASE' to '1' (lt_update_lib_path) modifying 'PATH' by prepending [snipped] (lt_setenv) setting 'PATH' to [snipped] (lt_update_exe_path) modifying 'PATH' by prepending [snipped] (lt_setenv) setting 'PATH' to[snipped] (main) lt_argv_zero : /full/path/to/./.libs/bmp2tiff.exe (main) newargz[0] : /full/path/to/./.libs/bmp2tiff.exe (main) newargz[1] : --help While the shell wrapper only prints the last bit (and doesn't show both lt_argz_zero and argv[0], because we really don't have access to any data inside the shell or child process to determine if they WERE different). $ ./bmp2tiff --lt-debug --help (main) lt_argv_zero : /full/path/to/.libs/bmp2tiff (main) newargz[1] : --help However, these precise details are not what that test is supposed to be checking. We're really only trying to determine that the debug output HAPPENS, not what it IS. I think the right approach is to modify both the shell wrapper and C wrapper so that the FIRST thing they output, if --lt-debug, is some unique magic. Maybe: libtool wrapper (GNU libtool 1.3110 2009-07-01) 2.2.7a (or maybe) foo:lt-foo.c:25: libtool wrapper (GNU libtool 1.3110 2009-07-01) 2.2.7a and then modify the test to check for that, instead of worrying about platform idiosyncratic stuff like /-vs-\, [.exe], or [lt-]. I prefer the former, but I don't really care. See GCS discussion below. > - the cwrapper debugging activated at compile time, tested in the second > half of cwrapper.at, does not enable debugging for the shell wrapper. Hmm, ok. That's easy enough to fix. > On w32 systems, the patch changes the API of many (but not all) > uninstalled programs generated by libtool: those which get a cwrapper. > The semantics of when a program gets a cwrapper is currently not > documented, but you have posted another patch for this, so let's discuss > this with that patch. Right. > More nits below. > > Thanks. Apologies for the immense delay. > >> --- a/libltdl/config/ltmain.m4sh >> +++ b/libltdl/config/ltmain.m4sh > >> @@ -2881,6 +2873,7 @@ void lt_update_exe_path (const char *name, const char >> *value); >> void lt_update_lib_path (const char *name, const char *value); >> char **prepare_spawn (char **argv); >> void lt_dump_script (FILE *f); >> + > > No need for this hunk. Ack. >> EOF >> >> cat <<EOF >> @@ -2932,6 +2925,10 @@ static const size_t opt_prefix_len = >> LTWRAPPER_OPTION_PREFIX_LENGTH; >> static const char *ltwrapper_option_prefix = LTWRAPPER_OPTION_PREFIX; >> >> static const char *dumpscript_opt = LTWRAPPER_OPTION_PREFIX >> "dump-script"; >> +static const size_t dumpscript_opt_len = LTWRAPPER_OPTION_PREFIX_LENGTH + >> 11; > > Why are we using these _len variables and strncmp in this code again? > strcmp is fine and safe and portable and used already, and strncmp is > needed only for the test of an unknown option in our domain, no? OK. I was trying to prepare the way for a possible later improvement, where we allow users to modify LTWRAPPER_OPTION_PREFIX via a configure argument. This way, if the client application's cmdline arguments include items that begin with '--lt-', the client package maintainer can move the libtool-specific arguments into a different prefix. But, you're correct: right now there's no need for this. (And even then, there probably would be no need; as far as the C code is concerned, LT_WRAPPER_OPTION_PREFIX and all of these const strings are exactly that: pre-specified constant strings of a fixed length.) Will revise. >> + /* first use of lt_debugprintf AFTER parsing options */ >> + lt_debugprintf ("(main) argv[0] : %s\n", argv[0]); >> + lt_debugprintf ("(main) program_name : %s\n", program_name); > > 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. At least s/ *:/:/ would be good, but I guess > this can also be part of a separate patch. No, I might as well do it all at once while I revise the other stuff. I'll post (all) of the revisions as an incremental patch, for easier review. We can't really follow ALL of the GCS: the wrapper has no business providing --version and --help (especially as those would conflict with similar options provided by the wrapped program). AFAICT, the GCS only specifies what *error* messages are supposed to look like, for interactive and non-interactive programs. It also specifies what the --version and --help outputs ought to look like (neither of which apply here). It suggests that --verbose be used (but again, we shouldn't usurp ability of the wrapped program to respond to that option, so that doesn't apply here either). But I can't see where the GCS specifies what other user messages are supposed to look like, nor debugging ones. I can follow the 'error message' style, if you like. But even so, that style is just: program:source-file-name:lineno: message program: message for non-interactive programs (and 'message' should not begin with a capital letter, and should not end with a period). So, instead of (main) argv[0] : %s (main) program_name : %s We'd just have foo.exe:lt-foo.c:57: (main) argv[0] : %s foo.exe:lt-foo.c:58: (main) program_name : %s Now, given that the number of source code lines will range from 1 to 3 digits, there's no reason anymore to try and ensure that the debug output "lines up" as I was doing in the past. So... foo.exe:lt-foo.c:57: (main) argv[0]: %s foo.exe:lt-foo.c:58: (main) program_name: %s Now, I like having a reference to the function in the error message, but I suppose that's not really necessary if the line number is included. Your call, Ralf. For the shell script, it'll probably look like foo:foo:23: (main) argv[0]: %s foo:foo:24: (main) program_name: %s ^^^ hmm. program name == source code file name. Meh. What do you think? >> --- a/tests/cwrapper.at >> +++ b/tests/cwrapper.at >> @@ -79,5 +79,57 @@ for restrictive_flags in '-Wall -Werror' '-std=c89 -Wall >> -Werror' '-std=c99 -Wal >> LT_AT_EXEC_CHECK([./usea], [0], [ignore], [ignore], []) >> done >> >> + >> +# Make sure wrapper debugging works, when activated at runtime >> +# This is not part of the loop above, because we >> +# need to check, not ignore, the output. >> +CFLAGS="$orig_CFLAGS" > > This line needed for? Because the previous loop had (iterative) set CFLAGS to various other values. We need to reset to the 'baseline' original value. >> +cat "$orig_LIBTOOL" > ./libtool >> +LIBTOOL=./libtool > > LIBTOOL=$orig_LIBTOOL ? Sure, that makes sense. My only reason for doing it this way was that I knew there WAS a ./libtool file in the current directory, created by the preceeding test loop. In the early days, I was concerned that the following test might use 'the old ./libtool' by mistake, regardless of what $LIBTOOL was set to. As it happens, that isn't a problem, so we can do it your way. >> +AT_CHECK([$LIBTOOL --mode=compile $CC $CPPFLAGS $CFLAGS -c liba.c], >> + [], [ignore], [ignore]) >> +AT_CHECK([$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -version-info=0.0.0 >> -no-undefined -o liba.la -rpath /foo liba.lo], >> + [], [ignore], [ignore]) >> +AT_CHECK([test -f liba.la]) >> + >> +AT_CHECK([$CC $CPPFLAGS $CFLAGS -c usea.c], >> + [], [ignore], [ignore]) >> +AT_CHECK([$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -o usea$EXEEXT >> usea.$OBJEXT liba.la], >> + [], [ignore], [ignore]) >> +LT_AT_EXEC_CHECK([./usea], [0], [ignore], [stderr], [--lt-debug]) >> +LT_AT_UNIFY_NL([stderr]) >> +AT_CHECK([grep '^(main) argv\[[0\]][[ \t]]*: \./usea' stderr], [0], >> [ignore], [ignore]) > >> +# Make sure wrapper debugging works, when activated at compile time. > > Forgot to remove this part, or was this intentional? No, the two comments are different. The earlier line was: # Make sure wrapper debugging works, when activated at runtime ^^^^^^^ I'll rewrite both, to emphasize the difference: # RUN-TIME activation of wrapper debugging ... # COMPILE-TIME activation of wrapper debugging -- Chuck