Re: Testing automake on Cygwin 1.7

[Stefano Lattarini]

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