Re: [PATCH] Enable runtime cwrapper debugging; add tests

[Charles Wilson]

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