automake-patches
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Modernize, improve and/or extend test scripts `conf*.test'.


From: Stefano Lattarini
Subject: Re: [PATCH] Modernize, improve and/or extend test scripts `conf*.test'.
Date: Sun, 8 Aug 2010 15:11:13 +0200
User-agent: KMail/1.13.3 (Linux/2.6.30-2-686; KDE/4.4.4; i686; ; )

At Sunday 08 August 2010, Ralf Wildenhues wrote:
> Hi Stefano,
> 
> * Stefano Lattarini wrote on Sun, Jun 27, 2010 at 06:14:41PM CEST:
> > More testsuite tweakings.  But this patch also contains some
> > "real" improvements and useful extensions IMO.
> 
> OK with nits below addressed.  If I'm appearing too grumpy,
No, you're not.  Being picky in reviews is not grumpiness, just
carefulness.
> then I'm sorry about that, so let me tell you that I'm grateful that
> you've done all this testsuite work,
Thanks.
> and I'm sorry there is such a high review delay.
Well, I assume this is not done on purpose (right? ;-), so don't worry.

> > * tests/config.test: Renamed to ...
> > * tests/confh3.test: ... this.  Fix m4 quoting in `configure.in',
> 
> I'm not overly fond of the two renamings in this patch, again for
> old log file reasons: confh2 and confh3 used to be tests, but they
> were removed (and, BTW, for a very good reason: they did not check
> effects, but internal details of makefiles ;-) see
>   git log -p -- tests/confh2.test tests/confh3.test
> 
> Consequently, I prefer not reusing those names.
A valid concern.
> As to renaming them to, say, confh[67].test, well, seems slightly
> more sensible
I'd like to do this.  In the meantime, I've amended my patch to do so.
> but still has the problem that old log files will be less helpful
> then.
Still, I think that a more rational naming/organization of the many
automake tests would be valuable.

In the end, would you strongly prefer to drop the renaming altogether?
If not, I'll apply the amended renaming.

> > --- a/tests/confh.test
> > +++ b/tests/confh.test
> > 
> > @@ -33,24 +33,26 @@ mkdir include
> > 
> >  : > include/Makefile.am
> >  : > include/config.h.in
> > 
> > -$ACLOCAL || Exit 1
> > -$AUTOMAKE || Exit 1
> > -
> > -(sed -n -e '/^DIST_COMMON =.*\\$/ {
> > -   :loop
> > -   p
> > -   n
> > -   t clear
> > -   :clear
> > -   s/\\$/\\/
> > -   t loop
> > -   p
> > -   n
> > -   }' -e '/^DIST_COMMON =/ p' Makefile.in | grep acconfig.h) || Exit 1
> > +$ACLOCAL
> > +$AUTOMAKE
> > +
> > +perl -ne '
> > +if (s/^DIST_COMMON *=//)
> > +{
> > +  $_ .= <> while (s/\\$//);
> > +  $_ = " $_ ";
> > +  s/\s+/ /g;
> > +  print "$_\n";
> > +  exit 0;
> > +}' Makefile.in > dc.txt
> 
> This could instead use extract_variable which we've discussed
> before somewhere, right?  ;-)
I will surely refactor this when I'll submit my (already big, and growing)
patch series about testsuite refactoring.  But before venturing to do that,
I'd like to have most of the pending patches committed.  This one included.

> > diff --git a/tests/conf2.test b/tests/confh2.test
> > similarity index 84%
> > rename from tests/conf2.test
> > rename to tests/confh2.test
> > index 8e53733..fce254e 100755
> > --- a/tests/conf2.test
> > +++ b/tests/confh2.test
> > @@ -21,12 +21,8 @@
> > 
> >  set -e
> > 
> > -cat > configure.in << 'END'
> > -AC_INIT
> > -AM_INIT_AUTOMAKE(nonesuch, nonesuch)
> > -AM_CONFIG_HEADER(config.h two.h)
> > -AC_PROG_CC
> > -AC_OUTPUT(Makefile)
> > +cat >> configure.in << 'END'
> > +AM_CONFIG_HEADER([config.h two.h])
> > 
> >  END
> >  
> >  : > Makefile.am
> > 
> > @@ -36,3 +32,15 @@ END
> > 
> >  $ACLOCAL
> >  $AUTOMAKE
> > 
> > +
> > +# Try again with more macros.
> > +
> > +cat >> configure.in << 'END'
> > +AC_PROG_CC
> > +AC_OUTPUT
> > +END
> > +
> > +$ACLOCAL
> 
> This needs --force to be reliably effective.
Fixed.  I really need to learn more about Autotools caching...

> > +$AUTOMAKE
> > 
> > 
> > 
> > --- a/tests/confh4.test
> > +++ b/tests/confh4.test
> > 
> > @@ -50,4 +49,6 @@ mkdir include
> > 
> >  $ACLOCAL
> >  $AUTOMAKE
> > 
> > -$EGREP '^DEFAULT_INCLUDES =.* -I(\.|\$\(top_builddir\))/include' 
> > Makefile.in
> > +grep '^DEFAULT_INCLUDES =.* -I\$(top_builddir)/include' Makefile.in
> 
> This seems wrong to me, the previous code did the right thing by
> not exploiting undocumented information.
Where is DEFAULT_INCLUDES documented?  If it appears from the docs that I'm
exposing too much internal details, I'll revert the change ASAP.

> > --- a/tests/configure.test
> > +++ b/tests/configure.test
> > [CUT]
> Wow.  Fixing a merge error,
Well, techincally it wasn't an error, just a redundacy ;-)
> I guess I merged your previous patch
> for this test twice.  Sorry about that.

Thanks,
   Stefano



reply via email to

[Prev in Thread] Current Thread [Next in Thread]