Re: [1.5.26] new test using 'make install DESTDIR=...'

[Ralf Wildenhues]

Hello Michael,

* Michael Haubenwallner wrote on Mon, Mar 17, 2008 at 09:49:01AM CET:
> On Sat, 2008-03-15 at 18:36 +0100, Ralf Wildenhues wrote:
> > * Michael Haubenwallner wrote on Fri, Mar 14, 2008 at 05:53:48PM CET:
> > 
> > Thank you for the patch.  Is there anything keeping you from moving to
> > Libtool-2.2?
> 
> I'm working on gentoo-alt/prefix (using portage on different platforms
> into some prefix as non-root), so I'm not really a package maintainer.
> Sometimes we just apply libtool patches in-place, sometimes we need a
> full autoreconf, but I haven't tried yet if it works smoothly for a
> package originally using libtool-1.5 to be libtoolized with libtool-2.2.
> Is it intended to have this working smoothly ?

Yes.  If it doesn't, you should report that as bug.  Note a number of
bugs have been found in 2.2 since already, so you can now help to ensure
that 2.2.2, which is due in a few weeks, works as well as possible.

> I don't really care for another 1.5 release, basically just want to have
> some agreement on the patch to know I'm on the right way - and in case
> there is another 1.5 release I can drop this patch.

ATM I don't think anybody is asking for another 1.5 release.

I'd rather avoid having to review your 1.5 DESTDIR fixes; 2.2 has fixed
issues in a different way and I'd prefer users to go with that.  OTOH,
testsuite additions for HEAD are almost always good, so let's take a
look, see the review below.

> > > It simply works when using something like the second attached patch to
> > > fall back to "guess we'll fake it" (like all my other platforms),
> > > although hardcode_direct IMHO is generally a bad idea when some RUNPATH
> > > can be encoded, even without using DESTDIR.
> > 
> > We've put some fixes in HEAD for this, notably for AIX and OpenBSD.
> 
> Is there a release containing them already ?

2.2 contains all DESTDIR-related fixes that I know of.

Cheers,
Ralf

| diff -rNu libtool-1.5.26.vanilla/tests/defs 
libtool-1.5.26.instd-test/tests/defs
| --- libtool-1.5.26.vanilla/tests/defs 2005-07-08 17:50:35.000000000 +0200
| +++ libtool-1.5.26.instd-test/tests/defs      2008-03-14 10:49:53.254752000 
+0100

Please note that in HEAD, tests/defs is created from tests/defs.m4sh.

| @@ -42,6 +42,16 @@
|    prefix=NONE
|  fi
|  
| +image="./_image"
| +if test "$need_image" = yes; then
| +  # An absolute path to an image directory.
| +  test -d $image || mkdir $image
| +  image=`cd $image && pwd`

Please double-quote $image here.

| +else
| +  test -d $image && rm -rf $image

Likewise, twice.  I get shudders with an 'rm -rf' and a potentially
unsafe name.

| +  image=
| +fi
| +
|  # Extract CC from the libtool configuration
|  eval `$libtool --config | grep '^CC='`
|  
| diff -rNu libtool-1.5.26.vanilla/tests/depdemo-instd.test 
libtool-1.5.26.instd-test/tests/depdemo-instd.test
| --- libtool-1.5.26.vanilla/tests/depdemo-instd.test   1970-01-01 
01:00:00.000000000 +0100
| +++ libtool-1.5.26.instd-test/tests/depdemo-instd.test        2008-03-14 
10:49:53.264743000 +0100
| @@ -0,0 +1,68 @@
| +#! /bin/sh
| +# depdemo-instd.test - try installing from the ../depdemo subdirectory via 
DESTDIR

FWIW, I'd prefer the name depdemo-destdir.test.

| +
| +# Test script header.
| +need_prefix=yes
| +need_image=yes
| +if test -z "$srcdir"; then
| +  srcdir=`echo "$0" | sed 's%/[^/]*$%%'`
| +  test "$srcdir" = "$0" && srcdir=.
| +  test "${VERBOSE+set}" != "set" && VERBOSE=yes
| +fi
| +. $srcdir/defs || exit 1
| +
| +# Check that things are built.
| +if test -f ../depdemo/depdemo$EXEEXT; then :
| +else
| +  echo "You must run depdemo-make.test before $0" 1>&2
| +  exit 77
| +fi

HEAD has func_require for this, and the initial blurb may be simplified
for it, too.

| +# Change to our build directory.
| +cd ../depdemo || exit 1
| +
| +echo "= Running $make install 'DESTDIR=$image' in ../depdemo"
| +$make install DESTDIR="${image}" || exit 1
| +
| +echo "= Moving out of DESTDIR"
| +
| +rm -f fail
| +{ ( cd "${image}/${prefix}"; tar cf - . ) || touch fail ; } |
| +{ ( cd "${prefix}" ; tar xfv - ) || touch fail ; }

Putting the tar option v after f is not portable, AFAIK.

I prefer the idiom
  cd "$whereever" && $command

so that the command won't go berzerk in some random directory.
This is mere defensive habit, and I think it's a good thing to
do even if before the presence of a directory has been checked
(has it for ${prefix}?).

This test is missing suitable
  libtool --mode=finish

commands for $prefix/lib, which is required for some systems.

| +if [ -f fail ]; then

Please use test instead of [ ... ], just for consistency.

| +     echo "$0: failed to merge from ${image}/${prefix} to ${prefix}" 1>&2
| +     rm -f fail
| +     exit 1
| +fi
| +rm -rf "${image}/${prefix}"
| +leftovers=`find "${image}" ! -type d ! -name '.*' -print`
| +if test -n "$leftovers"; then
| +  echo "= Leftover after merging from ${image}/${prefix}:"
| +  ls -l $leftovers
| +  ls -l $leftovers > ~/devel/savannah/arse

Erm, that file name surely is not portable.  ;-)

Also, I'm not sure what error this leftover checking is guarding
against, but maybe I just haven't seen this kind of error.

| +  exit 1
| +fi
| +rm -rf "${image}"
| +
| +echo "= Executing installed programs"
| +
| +status=0
| +if $prefix/bin/depdemo_static; then :

Double-quote $prefix.

| +else
| +  echo "$0: cannot execute $prefix/bin/depdemo_static" 1>&2
| +  status=1
| +fi
| +
| +if $prefix/bin/depdemo; then :

Likewise.

| +else
| +  echo "$0: cannot execute $prefix/bin/depdemo" 1>&2
| +
| +  # Simple check to see if they are superuser.
| +  if test -w /; then :
| +  else
| +    echo "You may need to run $0 as the superuser."
| +  fi
| +  status=1
| +fi
| +
| +exit $status