Re: [PATCH 08/10] Add more tests about AUTOMAKE_OPTIONS.

[Ralf Wildenhues]

* Stefano Lattarini wrote on Sun, Jan 02, 2011 at 06:45:07PM CET:
> On Sunday 02 January 2011, Ralf Wildenhues wrote:
> > * address@hidden wrote on Thu, Dec 23, 2010 at 12:27:44PM CET:

> > > +cat > Makefile.am <<'END'
> > > +AUTOMAKE_OPTIONS = $(foo) foreign
> > > +AUTOMAKE_OPTIONS += ${bar}
> > > +foo = $(foo1)
> > > +foo1 = ${foo2}
> > > +foo2 = -Wnone
> > > +foo2 += $(foo3)
> > > +foo3 = -Wno-error
> > > +bar = -Wportability
> > 
> > This seems a wee bit convoluted (i.e., hard to parse by human upon first
> > reading), but oh well, that's more of a testsuite QoI nit.
> >
> Do you have any clarifying comment to suggest?  I'd be happy to add it.

How about this?
 # The following should expand to `-Wnone -Wno-error foreign -Wportability'.

Hey, we could even grep for that in the verbose output somewhere ...

> > > +$ACLOCAL
> > > +AUTOMAKE_fails
> > 
> > What are the expected failures here?
> >
> Aren't the next greps explicit enough?  If not, would this comment help
> in clarifying the situation?
> 
>  # Automake options 'tar-v7', 'tar-ustar' and 'tar-pax' can only be used
>  # as argument to AM_INIT_AUTOMAKE, and not in AUTOMAKE_OPTIONS.

The comment definitely helps, and should be enough IMHO.  Thank you.

> In the meantime, I've added it just before the call to AUTOMAKE_fails;
> let me know if it's not clear enough.
> 
> > (Not sure if they should be mentioned in the test source, but
> > I sure am curious about what automake prints.)
> >
> Here it is:
> 
>   Makefile3.am:2: error: option `tar-v7' can only be used as argument to 
> AM_INIT_AUTOMAKE
>   Makefile3.am:2: but not in AUTOMAKE_OPTIONS makefile statements
>   Makefile2.am:6: error: option `tar-ustar' can only be used as argument to 
> AM_INIT_AUTOMAKE
>   Makefile2.am:6: but not in AUTOMAKE_OPTIONS makefile statements
>   Makefile1.am:1: error: option `tar-pax' can only be used as argument to 
> AM_INIT_AUTOMAKE
>   Makefile1.am:1: but not in AUTOMAKE_OPTIONS makefile statements
>   Makefile.am:3:   `Makefile0.am' included from here
>   Makefile0.am:4:   `Makefile1.am' included from here

Thanks.

> > Just to be sure, this patch does not rely upon any of the previous ones,
> > right?
> >
> Hmm... I didn't pay attention to this fact before, but yes, I believe
> the patch should not depend on earlier ones.
> 
> Anyway, I bacame aware the necessity of the new tests only when I got to
> modify the automake code dealing with AUTOMAKE_OPTIONS; and that's why
> I didn't add them in an earlier patch.

Well, if you like, you can just push this patch to master independently;
or leave it here if you prefer.

> > > +grep '^Makefile1\.am:1:.*tar-pax' stderr
> > > +grep '^Makefile2\.am:6:.*tar-ustar' stderr
> > > +grep '^Makefile3\.am:2:.*tar-v7' stderr
> > > +grep '^Makefile\.am:3:.*Makefile0\.am.*included from here' stderr
> > > +grep '^Makefile0\.am:4:.*Makefile1\.am.*included from here' stderr
> > 
> > > +cat stderr \
> > > +  | grep -v '^Makefile\.am:3:'  \
> > > +  | grep -v '^Makefile0\.am:4:' \
> > > +  | grep -v '^Makefile1\.am:1:' \
> > > +  | grep -v '^Makefile2\.am:6:' \
> > > +  | grep -v '^Makefile3\.am:2:' \
> > > +  | grep . && Exit 1
> > 
> > What is this for, to ensure there are not more warnings?
> >
> In truth, it's to ensure that there are no warnings referring to botched
> line numbers.  Technically, the grep above might be a little too strict
> in this respect, but since our Makefile.ams are so simple, that shouldn't
> be a problem.
> 
> JTBS, I've added a couple of new comments about this too.

Cool.

> > How about just testing for 5 lines of output?
> >
> They are in fact eight, which shows ...
> 
> > Even that is a bit fragile though, as we might have multi-line warnings.
> >
> ... that you're right here.  I'd personally leave the above grepping
> alone.  Objections?

All better, no objections!

Thanks!
Ralf