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

[Ralf Wildenhues]

Hi Charles,

* 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.

> [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),
- the shell wrapper outputs 'lt_argv_zero' not 'argv[0]' in its debug
  output,
- the cwrapper debugging activated at compile time, tested in the second
  half of cwrapper.at, does not enable debugging for the shell wrapper.

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.

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.

>  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?

> +  /* 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.

> --- 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?

> +cat "$orig_LIBTOOL" > ./libtool

LIBTOOL=$orig_LIBTOOL   ?

> +LIBTOOL=./libtool

> +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?

> +# We structure this test as a loop, so that we can 'break' out of it
> +# if necessary -- even though the loop by design executes only once.
> +for debugwrapper_flags in '-DLT_DEBUGWRAPPER'; do
> +  CFLAGS="$orig_CFLAGS $debugwrapper_flags"
> +  sed "s/LTCFLAGS=.*/&' $debugwrapper_flags'/" < "$orig_LIBTOOL" > ./libtool
> +  LIBTOOL=./libtool
> +
> +  # make sure $debugwrapper_flags do not cause a failure
> +  # themselves (e.g. because a non-gcc compiler doesn't
> +  # understand them)
> +  $LIBTOOL --mode=compile $CC $CPPFLAGS $CFLAGS -c trivial.c || continue
> +
> +  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_AT_UNIFY_NL([stderr])
> +  AT_CHECK([grep '^(main) argv\[[0\]][[ \t]]*: \./usea' stderr], [0], 
> [ignore], [ignore])
> +done
> +
> +
>  AT_CLEANUP

Cheers,
Ralf