Re: [PATCH] Add test case for 69e77671 (cwrapper PATH manipulation order)

[Ralf Wildenhues]

Hi Charles,

* address@hidden wrote on Sun, Oct 03, 2010 at 11:15:17PM CEST:
> * tests/cwrapper.at: Add new test 'cwrapper and installed shared
> libraries.'

OK with nits addressed.  You may want to use a ChangeLog and/or --author
entry that suitably documents the main author of the patch.

Thanks,
Ralf

> --- a/tests/cwrapper.at
> +++ b/tests/cwrapper.at
> @@ -134,3 +134,73 @@ done
>  
>  AT_CLEANUP
>  
> +
> +AT_SETUP([cwrapper and installed shared libraries])
> +AT_KEYWORDS([libtool])
> +
> +# make sure existing libtool is configured for shared libraries
> +AT_CHECK([$LIBTOOL --features | grep 'disable shared libraries' && exit 77],
> +      [1], [ignore])

Let's be positive:

AT_CHECK([$LIBTOOL --features | grep 'enable shared libraries' || exit 77],
         [], [ignore])

> +# FIXME with shared_fails for this test on AIX
> +# copy from link-order2.at:
> +eval `$LIBTOOL --config | $EGREP '^(shlibpath_var|allow_undefined_flag)='`
> +
> +undefined_setting=-no-undefined
> +shared_fails=no
> +case $host_os,$LDFLAGS,$allow_undefined_flag in
> +aix*,*-brtl*,*) ;;
> +aix*) shared_fails=yes ;;
> +darwin*,*,*-flat_namespace*) undefined_setting= ;;
> +darwin*,*,*) shared_fails=yes ;;
> +esac
> +# end of copy from link-order2.at
> +
> +LDFLAGS="$LDFLAGS $undefined_setting"

Let's replace these 15 lines with
  LDFLAGS="$LDFLAGS -no-undefined"

because I don't see how this test should need to depend on them at all.
Since the test is explicitly about the cwrapper, I'd probably prefer to
skip it on systems that have a problem with the test but don't use the
cwrapper anyway.  If you agree then I can just test this later and we
keep the simple code for now.

> +inst=`pwd`/inst
> +libdir=$inst/lib
> +bindir=$inst/bin
> +mkdir $inst $libdir $bindir
> +
> +# we must build foo library in a separate directory to avoid on some
> +# platforms shared library to be loaded from "current" directory

I have trouble parsing this sentence, and it is lacking punctuation and
capitalization.  How about this?

  # Build the library in a separate directory to avoid the special case
  # of loading from the current directory.

> +mkdir foo
> +cd foo
> +# build and install "old" library version
> +AT_DATA([a.c], [[
> +int liba_ver (void) { return(1); }

Please no parens for argument to return, that is not a function.
Three instances.

> +]])
> +$LIBTOOL --mode=compile --tag=CC $CC $CPPFLAGS $CFLAGS -c a.c
> +$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -version-info=0.0.0 -o liba.la 
> -rpath $libdir a.lo
> +$LIBTOOL --mode=install $lt_INSTALL liba.la $libdir

Is there any, however unlikely, chance that these $LIBTOOL commands fail
on some system or setup?  Then they should be wrapped in
  AT_CHECK([...], [], [ignore], [ignore])

> +# build a new library version
> +AT_DATA([a.c], [[
> +int liba_ver (void) { return(2); }
> +]])
> +$LIBTOOL --mode=compile --tag=CC $CC $CPPFLAGS $CFLAGS -c a.c
> +$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -version-info=0.0.0 -o liba.la 
> -rpath $libdir a.lo

Ditto.

> +cd ..
> +
> +# build and run test application
> +AT_DATA([m.c], [[
> +extern int liba_ver (void);
> +int main (void)
> +{
> +  int r = (liba_ver () == 2) ? 0 : 1;
> +  return(r);
> +}
> +]])
> +
> +$LIBTOOL --mode=compile --tag=CC $CC $CPPFLAGS $CFLAGS -c m.c
> +
> +$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -o m1$EXEEXT m.$OBJEXT foo/liba.la
> +LT_AT_EXEC_CHECK([./m1], [0], [ignore], [ignore], [])
> +
> +$LIBTOOL --mode=link $CC $CFLAGS $LDFLAGS -o m2$EXEEXT m.$OBJEXT foo/liba.la 
> -L$inst/lib
> +LT_AT_EXEC_CHECK([./m2], [0], [ignore], [ignore], [])
> +
> +AT_CLEANUP