On Friday 25 November 2011, Peter Rosin wrote:
>
> >> Second is txinfo21, which stumbles because the directory timestamp
> >> is not properly updated when a file changes inside the dir.
> >>
> >> From the end of the log:
> >> ...
> >> make[1]: Leaving directory `/home/peda/automake/tests/txinfo21.dir'
> >> + test -d main.html
> >> + test -d sub/main2.html
> >> + test -d rec/main3.html
> >> + is_newest main.html main.texi
> >> ++ find main.html main.texi -newer main.html
> >> + is_newest_files=main.html/index.html
> >> + test -z main.html/index.html
> >> + exit_status=1
> >> + set +e
> >> + cd /home/peda/automake/tests
> >> + case $exit_status,$keep_testdirs in
> >> + test 0 '!=' 0
> >> + echo 'txinfo21: exit 1'
> >> txinfo21: exit 1
> >> + exit 1
> >>
> >>
> >> Can be fixed like this, but I don't know if that's appropriate...
> >>
> >> diff --git a/tests/txinfo21.test b/tests/txinfo21.test
> >> index ae1d985..3d608e5 100755
> >> --- a/tests/txinfo21.test
> >> +++ b/tests/txinfo21.test
> >> @@ -94,12 +94,12 @@ test -d main.html
> >> test -d sub/main2.html
> >> test -d rec/main3.html
> >>
> >> -# Rebuilding main.html should cause its timestamp to be updated.
> >> -is_newest main.html main.texi
> >> +# Rebuilding main.html/index.html should cause its timestamp to be
> >> updated.
> >> +is_newest main.html/index.html main.texi
> >> $sleep
> >> touch main.texi
> >> $MAKE html
> >> -is_newest main.html main.texi
> >> +is_newest main.html/index.html main.texi
> >>
> >> $MAKE clean
> >> test ! -d main.html
> >>
> > Hmm... we could make the testing of the directory timestamp conditional
> > to the test being run on MinGW/Cygwin, in order not to reduce coverage
> > on other systems. Something like this maybe?
> >
> > # At least on Cygwin, the timestamp of a directory might not be
> > # properly updated when a file inside it is changed, and we need
> > # to account for that.
> > mkdir tmp.d
> > : > tmp.f
> > : > tmp.d/tmp.f
> > $sleep
> > touch tmp.d/tmp.f
> > if is_newest tmp.d tmp.f; then
> > have_correct_dir_timestamp=yes
> > else
> > have_correct_dir_timestamp=no
> > fi
> > rm -rf tmp.*
> >
> > # Rebuilding main.html should cause the timestamp of the regular
> > # file `main.html/index.htm' to be updated.
> > is_newest main.html/index.html main.texi
> > # Rebuilding main.html should cause its timestamp to be updated,
> > # But main.html is a directory, so be prepared to account for
> > # the Cygwin limitation described above.
> > if test $have_correct_dir_timestamp = yes; then
> > is_newest main.html main.texi || Exit 1
> > fi
> > $sleep
> > touch main.texi
> > $MAKE html
> > if test $have_correct_dir_timestamp = yes; then
> > is_newest main.html main.texi || Exit 1
> > fi
> > is_newest main.html/index.html main.texi
>
> That works, but isn't it an awful lot of code for a corner case?
>
Possibly, but I prefer to be conservative in tests, and not relax
them unless necessary. Also, since the new code is extensivley
commented, a future developer will know why it's there, and thus
also whether dropping it would be acceptable.
> You choice though.
>
I'd say we keep this. I'll prepare a patch tomorrow.
> >>
> >> diff --git a/tests/distcheck-override-infodir.test
> >> b/tests/distcheck-override-in
> >> index 19ad3d1..3cf38c5 100755
> >> --- a/tests/distcheck-override-infodir.test
> >> +++ b/tests/distcheck-override-infodir.test
> >> @@ -32,11 +32,11 @@ info_TEXINFOS = main.texi
> >> ## Sanity check.
> >> installcheck-local:
> >> if test x$${infodir+set} != xset; then \
> >> - ls -l "$(DESTDIR)/$(prefix)/blah/blah/foobar/" || exit 1; \
> >> - test -f "$(DESTDIR)/$(prefix)/blah/blah/foobar/dir" || exit 1; \
> >> + ls -l "$(DESTDIR)$(prefix)/blah/blah/foobar/" || exit 1; \
> >> + test -f "$(DESTDIR)$(prefix)/blah/blah/foobar/dir" || exit 1; \
> >> else \
> >> - ls -l "$(DESTDIR)/$$infodir/" || exit 1; \
> >> - test -f "$(DESTDIR)/$$infodir/dir" || exit 1; \
> >> + ls -l "$(DESTDIR)$$infodir/" || exit 1; \
> >> + test -f "$(DESTDIR)$$infodir/dir" || exit 1; \
> >> fi
> >> END
> >>
> > This looks good, thanks. Maybe we could also add a comment stating
> > that the lack of a slash after $(DESTDIR) is deliberate?
>
> I don't think we should, because it is never correct to add the
> extra slash after $(DESTDIR)
>
Right.
> and adding a comment about it just
> adds to the confusion by somehow stating that it would be
> desirable to add that slash, but that we sadly can't. On the
> contrary, it's just plain wrong with the slash, and you'd have
> to add that comment all over the place in order to be consistent.
> But again, your choice.
>
Let's just drop any hypotetical extra comment. I'll prepare a patch
in your name if you don't beat me at it.
> >> So, with those two fixes, one fail left, i.e. transform2.test.
> >>
> > Could you let me know if my proposed change above make it correctly
> > skipped? If yes, I'll prepare three proper patches tomorrow -- unless
> > you want to beat me at it ;-), in which case, please put a reference
> > to this thread in the git commit message and in the ChangeLog entries.
>
> It would be better with an xfail in my opinion, if that's possible to
> accomplish conditionally?
>
Sadly, not easily (we could extend XFAIL_TESTS at configure time if we
detect the above Cygwin limitation, but that's quote involved, and IMO
would put the logic in the wrong place, i.e., configure.ac). Maybe in
a next automake version we could add a new special exit status for test
scripts to signal "expected failure", like e.g. `77' is used to signal
"skipped test"? Still, that's for automake 1.11.3 at most, so I see
only two ways out for now:
1. Add the test skip I proposed to branch-1.11 only, but *remove it*
after the 1.11.2 release.
2. Just let the test fail, and be prepared to deal with some spurious
reports.
I'm 60-40 in favor of 2, since it doesn't add yet more noise to our
repository. WDYT?
Thanks,
Stefano
