Re: [PATCH] {maint} Add some tests on 'cygnus' mode

[Stefano Lattarini]

On Wednesday 22 December 2010, Ralf Wildenhues wrote:
> * Stefano Lattarini wrote on Tue, Dec 21, 2010 at 11:53:50PM CET:
> > In view of a (partly-completed, soon-to-be-posted) patch series that
> > should fix Automake bug#7669[1][2], I'd like to improve the testsuite
> > coverage regarding the "cygnus" mode.  Attached is a patch (for maint)
> > which does that.
> > 
> > [1] ``option "foreign" after "-Wall" turns off and portability warnings.''
> > [2] a.k.a. PR/547 in the older gnats database
> > 
> > Note that some of the new testcases are not really relevant for the
> > refactoring, but I'd like to add them anyway while I'm at it.
> > 
> > OK to apply the attached patch to maint?
> 
> I think master is sufficient for this.
>
OK.

> > I'll wait the customary 72 hours before pushing.
> 
> FWIW I will be off-net completely starting sometime tomorrow and ending
> sometime on the 28th.
>
Noted, thanks.

> > Subject: [PATCH] Add some tests on 'cygnus' mode.
> > 
> > * tests/clean2.test: Renamed to ...
> 
> Erm.
>
Yes, I know, but the name `clean2' is *so* non-descriptive ...
I've reverted the renaming (not the edits) for the moment, but I'd
like a blessing allowing me to rename the test in a follow-up patch.

> > * tests/cygnus-distclean.test: ... this, and extended somewhat.
> > * tests/cygnus-check-without-all.test: New test.
> > * tests/cygnus-dependency-tracking.test: Likewise.
> > * tests/cygnus-distclean.test: Likewise.
> > * tests/cygnus-imply-foreign.test: Likewise.
> > * tests/cygnus-no-dist.test: Likewise.
> > * tests/cygnus-no-installinfo.test: Likewise.
> > * tests/cygnus-requires-maintainer-mode.test: Likewise.
> > * tests/Makefile.am (TESTS): Update.
> 
> > --- /dev/null
> > +++ b/tests/cygnus-dependency-tracking.test
> 
> > +# Check that cygnus mode disables automatically dependency tracking.
> 
> s/automatically/automatic/
>
Fixed.

> > +# And check that this *cannot* be overridden.
> 
> I think this is one of the major reason the classical Cygnus trees (GCC,
> src/binutils, gdb) have mostly moved away from using the cygnus option,
> with only a few uses remaining in sub trees.
> 
> IOW, this might not be such a great feature.
>
I agree it's a misfeature indeed.  But the main purpose of the new test
cases is to prevent obvious regressions in the soon-to-follow patch
series, and/or to pinpoint where and when a bad 'cygnus' behaviour is
fixed, so this test makes sense IMHO.

> (But OTOH it is not worth worrying about it much either.  The cygnus
> option exists to keep old code working.)
>
I agree.

> > +# Check that 'cygnus' mode imply 'foreign' mode.
> 
> > +# We want complete control automake flags.
> > +AUTOMAKE=`(set $AUTOMAKE && echo $1)` || Exit 99
> 
> This is ugly.  What if I use
>   AUTOMAKE='perl -I foo /path/to/automake-1.X'
>
Ouch.

I'd might just say "please use a wrapper script", but this might
make testing more complicated than it needs to be ...

> IOW, why not save the value of $AUTOMAKE without arguments in
> defs-static.in before changing it, and using that here?
>
... especially since this workaroud is much better.

Done with commit v1.11-430-gd40b95d and pushed to tests-init; see:
 <http://lists.gnu.org/archive/html/automake-patches/2010-12/msg00131.html>
I will merge tests-init to master before rebasing this patch on master.

> > --- /dev/null
> > +++ b/tests/cygnus-no-dist.test
> 
> > +# Check that cygnus mode forbids creation of distribution tarball.
> 
> > +for target in dist distdir distcheck dist-all dist-gzip; do
> > +  $MAKE -n $target >out 2>&1 && { cat out; Exit 1; }
> > +  cat out
> > +  grep $target out
> 
> What is this grep for?
>
To "ensure" that `$MAKE -n' failed for the expected reason ("I don't
know how to make the target $target"), and not for some other bogus
reason.

> Grepping make -n output is not such a good idea, the output is
> quite variable between makes (but then again, haven't checked either).
>
I know, but I assumed that all make implementations out there report
at least the name of the target they're not able to build ...

I'd rather relax the test only if and when we encounter a make program
that fails to report such a basically decent error message.  OK?

> > +done
> > +
> > +# Now check that cygnus mode in a subdirectory disables
> > +# distribution-building in that subdirectory.
> > +
> > +cat > Makefile.am <<'END'
> > +SUBDIRS = sub1 sub2
> > +END
> > +
> > +mkdir sub1 sub2
> > +: > sub1/Makefile.am
> > +cat > sub2/Makefile.am <<'END'
> > +# The `-Wall' after `cygnus' should ensure no warning gets
> > +# unintentionally disabled.  We are particularily interested
> > +# in overridewarnings, for when (below) we add the `distdir'
> 
> override warnings
>
Oops (unfortunately the vim spellchecker in "shell-mode" doesn't
look at things inside here documents ...).  Fixed.

> > +# target.
> > +AUTOMAKE_OPTIONS = cygnus -Wall
> > +END
> 
> > --- a/tests/clean2.test
> > +++ b/tests/cygnus-no-installinfo.test
> 
> > +# FIXME: -Wno-override work around a buglet in definition of $(MAKEINFO)
> 
> works in the definition
>
Oops (unfortunately the vim spellchecker is not grammar-aware, either).
Fixed.

I will push by tomorrow if there are no further objections.

Regards,
   Stefano