Re: [PATCH] {maint} python: extend and improve tests, fix minor glitches

[Ralf Wildenhues]

Hi Stefano,

* Stefano Lattarini wrote on Sun, Feb 06, 2011 at 06:32:56PM CET:
> Hello Ralf, and sorry for the delay.

No worries.  I'm waay more behind.

> On Thursday 03 February 2011, Ralf Wildenhues wrote:
> > * Stefano Lattarini wrote on Wed, Feb 02, 2011 at 12:10:15AM CET:
> > > A testsuite-enhancement patch stemmed from my brief foray into
> > > Automake's python support.  This patch is in small part cosmetic,
> > > but IMHO offers real improvements and valuable additions, and
> > > also fixes a couple of glitches in python.m4.
> > 
> > OK for master (branched off of maint, if you prefer) with nits
> > addressed.
> > 
> > Before pushing, please test on a system without a python interpreter
> > installed (you can rename you pythons temporarily).
> >
> Nah, IMHO is better to do something like:
> 
>   $ cd ~/src/automake/tests
>   $ mkdir xbin && cd xbin
>   $ for f in /bin/* /usr/bin/* /usr/local/bin/*; do
>   >   case $f in *python*);; *) ln -s $f .;; esac
>   > done
>   $ cd ..
>   $ PATH=`pwd`/xbin make check TESTS='...'

Neat; but also expensive.

> BTW, following your advice, I have uncovered a bug in python-dist.test.
> Fixed in the attached squash-in.

Cool.

> > > Subject: [PATCH] python: extend and improve tests, fix minor glitches
> > > 
> > > * tests/python-virtualenv.test: New test, checking that python
> > > support offered by automake works well with virtualenvs.
> > 
> > What is virtualenvs?
> > 
> "virtual python environments" created by the 'virtualenv' program.  I
> fixed the ChangeLog entry to be more precise (see attached squash-in).

Thanks.

> > And a no-contents version in instdir.test, analogously to the rest.
> > 
> Hmm... I see that this test lacks entries for the PROGRAMS, LIBRARIES
> and LTLIBRARIES primaries.  Is this deliberate?

I think so; to avoid requirements.

> Should that be fixed in a follow-up patch?

Well, the overlap of instdir.test with the instdir-*.test is pretty
large anyway, so it's not too important to have this full.

> > > --- a/tests/python2.test
> > > +++ b/tests/python2.test
> > 
> > > @@ -20,6 +20,8 @@
> > >  
> > >  set -e
> > >  
> > > +$ACLOCAL
> > > +
> > >  
> > >  echo 1. pythondir not defined
> > >  
> > > @@ -28,8 +30,8 @@ PYTHON = x
> > >  python_PYTHON = foo.py
> > >  END
> > >  
> > > -$ACLOCAL
> > >  AUTOMAKE_fails -a
> > > +grep 'pythondir.*undefined' stderr
> > 
> > These changes will make the test less resilient against changes to
> > python.m4.  I like the increased coverage but the increased brittleness
> > of the test is somewhat of a downer.  Hmm.
> >
> Would you maybe like a more lax regexp, e.g.:
>  $EGREP 'pythondir.*(un|not )defined' stderr
> or even:
>  $EGREP 'pythondir.*(un|not )defined|define.*pythondir' stderr
> 
> But IMHO this would be overkill, as changing the test in case the error
> messages are modifed should be a no-brainer.  For the moment, I've not
> touched this hunk (nor the similar ones below).

OK.  It's a close call anyway, and not a big problem either way.

> > > --- a/tests/python5.test
> > > +++ b/tests/python5.test
> > 
> > > @@ -24,16 +24,32 @@ set -e
> > >  
> > >  cat >>configure.in <<EOF
> > >  # Hopefully the Python team will never release such a version.
> > > -AM_PATH_PYTHON(9999.9)
> > > +AM_PATH_PYTHON([9999.9])
> > 
> > Nice that you do it here, but up in python.m4 you should then, too.
> >
> Definitely.  But in a follow-up patch IMHO (and since we are at it, we
> should fix underquoting in all the other *.m4 automake files).  I will
> submit this patch in the coming week.

OK.

> > > --- a/tests/python6.test
> > > +++ b/tests/python6.test

> > > +# Simulate no Python.
> > > +./configure PYTHON=:
> > > +test x"`cat py`" = x":"
> > 
> > A colon doesn't need quoting.
> >
> True, but it doesn't hurt either, and IMVHO improves consistency.
> Bikeshedding anyway,

True, but makes reading slightly harder IMVHO.

> so I'll simply remove it.

Thanks.

> > The cat py output doesn't either,
> > so this can just be
> >   test x`cat py` = x:
> >
> Hmmm... but why make the test less reliable in face of possible "weird"
> failures (in case, let's say, PYTHON gets erroneously redfined to ': ',
> note the trailing space)?
> 
> For the moment, I've kept the quotes around `cat py`.

OK with me.

> Attached is what I've squashed into the previous version of the patch.
> 
> I will push in 72 hours if there are no further objections.

OK but I have a question below.

> --- a/tests/instdir-ltlib.test
> +++ b/tests/instdir-ltlib.test

> @@ -14,7 +14,8 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> -# If $(libdir) is the empty string, then nothing should be installed there.
> +# If $(libdir) or $(pyexecdir) is the empty string, then nothing should
> +# be installed there.
>  # This test exercises the libtool code paths.
>  
>  required=libtoolize
> @@ -26,6 +27,7 @@ cat >>configure.in <<'END'
>  AC_PROG_CC
>  AM_PROG_CC_C_O
>  AC_PROG_LIBTOOL
> +AM_PATH_PYTHON
>  AC_OUTPUT
>  END
>  


> --- a/tests/instdir-prog.test
> +++ b/tests/instdir-prog.test

> @@ -14,7 +14,8 @@
>  # You should have received a copy of the GNU General Public License
>  # along with this program.  If not, see <http://www.gnu.org/licenses/>.
>  
> -# If $(bindir) is the empty string, then nothing should be installed there.
> +# If $(bindir), $(libdir) or $(pyexecdir) is the empty string, then
> +# nothing should be installed there.
>  # This test exercises the prog and libs code paths.
>  
>  . ./defs || Exit 1
> @@ -25,6 +26,7 @@ cat >>configure.in <<'END'
>  AC_PROG_CC
>  AM_PROG_CC_C_O
>  AC_PROG_RANLIB
> +AM_PATH_PYTHON
>  AC_OUTPUT
>  END
>  
> @@ -36,6 +38,8 @@ bin_PROGRAMS = p
>  nobase_bin_PROGRAMS = np sub/np
>  lib_LIBRARIES = libfoo.a
>  nobase_lib_LIBRARIES = libnfoo.a sub/libnfoo.a
> +pyexec_PROGRAMS = py
> +nobase_pyexec_PROGRAMS = npy sub/npy
>  END
>  
>  cat >p.c <<'END'

Do these two tests require python now?  In that case maybe the merging
wasn't such a good idea after all; several of the machines I test on do
not have python in the default PATH.

Thanks,
Ralf