Bug in C02cont.ztst's -N test

Bug in C02cont.ztst's -N test

[Jason L Tibbitts III]

C02cont.ztst has a test for -N which tries to disable itself if the
filesystem is mounted with noatime:

  if [[ $OSTYPE == "cygwin" ]]; then
    ZTST_skip="[[ -N file ]] not supported on Cygwin"
  elif (( isnfs )); then
    ZTST_skip="[[ -N file ]] not supported with NFS"
  elif test -f /etc/mtab && { grep $(df . 2>/dev/null| tail -n1 | awk '{print $1}') /etc/mtab | grep -q noatime; }; then
    ZTST_skip="[[ -N file ]] not supported with noatime file system"
  else
    [[ -N $newnewnew && ! -N $unmodified ]]
  fi

When building zsh in a chroot created by the "mock" tool (used in Fedora
and derivatives to build packages in a clean chroot) it's possible for
"df ." to give the following:

Filesystem     1K-blocks      Used Available Use% Mounted on
-              492127564 331926080 135179772  72% /

And, of course, "grep - /etc/mtab" errors out.

I guess the shortest solution is to add "--" in there, but that isn't
going to help the test do what it's supposed to do.

 - J<

Re: Bug in C02cont.ztst's -N test

[Bart Schaefer]

On Apr 28,  7:20pm, Jason L Tibbitts III wrote:
} Subject: Bug in C02cont.ztst's -N test
}
} When building zsh in a chroot created by the "mock" tool (used in Fedora
} and derivatives to build packages in a clean chroot)

This test has been changed as of 2015/12/31 so the newer test should be
in zsh-5.2.  What version are you attempting to build?

The new test is

  elif { df -k -- ${$({mount || /sbin/mount} | awk '/noatime/ {print $1,$3}'):-""} | tr -s ' ' | fgrep "$(df -k . | tail -1 | tr -s ' ')" } >&/dev/null; then
    ZTST_skip="[[ -N file ]] not supported with noatime file system"

(that whole "elif ...; then" is one long line containing "" in case it
gets wrapped or misjoined by email clients.  However I guess a line of
output from df that begins with a hyphen would still break that.


diff --git a/Test/C02cond.ztst b/Test/C02cond.ztst
index 88cad0d..f9fc185 100644
--- a/Test/C02cond.ztst
+++ b/Test/C02cond.ztst
@@ -154,7 +154,7 @@
     ZTST_skip="[[ -N file ]] not supported on Cygwin"
   elif (( isnfs )); then
     ZTST_skip="[[ -N file ]] not supported with NFS"
-  elif { df -k -- ${$({mount || /sbin/mount} | awk '/noatime/ {print $1,$3}'):-""} | tr -s ' ' | fgrep "$(df -k . | tail -1 | tr -s ' ')" } >&/dev/null; then
+  elif { df -k -- ${$({mount || /sbin/mount} | awk '/noatime/ {print $1,$3}'):-""} | tr -s ' ' | fgrep -- "$(df -k . | tail -1 | tr -s ' ')" } >&/dev/null; then
     ZTST_skip="[[ -N file ]] not supported with noatime file system"
   else
     [[ -N $newnewnew && ! -N $unmodified ]]

Re: Bug in C02cont.ztst's -N test

[Jason L Tibbitts III]

>>>>> "BS" == Bart Schaefer <schaefer@brasslantern.com> writes:

BS> This test has been changed as of 2015/12/31 so the newer test should
BS> be in zsh-5.2.  What version are you attempting to build?

5.2.  When I fetch and unpack a clean copy of
http://www.zsh.org/pub/zsh-5.2.tar.xz, zsh-5.2/Test/C02Cond.ztst
contains:

[...]
  print -u $ZTST_fd 'This test takes two seconds...'
  sleep 2
  cat $unmodified
  touch $newnewnew
  if [[ $OSTYPE == "cygwin" ]]; then
    ZTST_skip="[[ -N file ]] not supported on Cygwin"
  elif (( isnfs )); then
    ZTST_skip="[[ -N file ]] not supported with NFS"
  elif test -f /etc/mtab && { grep $(df . 2>/dev/null| tail -n1 | awk '{print $1}') /etc/mtab | grep -q noatime; }; then
    ZTST_skip="[[ -N file ]] not supported with noatime file system"
  else
    [[ -N $newnewnew && ! -N $unmodified ]]
  fi
0:-N cond
[...]

I'll pull the latest git and see if the test fares any better there.

 - J<

Re: Bug in C02cont.ztst's -N test

[Jason L Tibbitts III]

>>>>> "BS" == Bart Schaefer <schaefer@brasslantern.com> writes:

BS> This test has been changed as of 2015/12/31 so the newer test should
BS> be in zsh-5.2.

Also, wasn't 5.2 released 2015-12-02?

 - J<

Re: Bug in C02cont.ztst's -N test

[Jason L Tibbitts III]

Sorry to spam.

I patched in the test from current git plus your patch, and while the
test still fails, at least it fails due the actual check not working
instead of an error elsewhere.  In this case I was building on a
filesystem mounted with noatime (because I forgot how I configured
something), but the test simply has no way to tell as it it doesn't
appear possible to get the mount options from within the chroot.  Which
is not really the test's fault.  I can see, maybe, an environment
variable or option to make check which disables the test, but otherwise
I don't think it's a big deal at all.

 - J<

Re: Bug in C02cont.ztst's -N test

[Bart Schaefer]

On Fri, Apr 29, 2016 at 1:31 PM, Jason L Tibbitts III <tibbs@math.uh.edu> wrote:
>>>>>> "BS" == Bart Schaefer <schaefer@brasslantern.com> writes:
>
> BS> This test has been changed as of 2015/12/31 so the newer test should
> BS> be in zsh-5.2.
>
> Also, wasn't 5.2 released 2015-12-02?

Yes, I misread a git log and convinced myself that we went straight
from 5.2 to 5.2-dev-1 in January, when in fact there were six weeks of
5.2-dev-0 in between.

Re: Bug in C02cont.ztst's -N test

[Daniel Shahaf]

Jason L Tibbitts III wrote on Fri, Apr 29, 2016 at 17:38:41 -0500:
> In this case I was building on a filesystem mounted with noatime
>(because I forgot how I configured something), but the test simply has
>no way to tell as it it doesn't appear possible to get the mount
>options from within the chroot.

I suppose the test could just do away with the df/mtab parsing and
simply do:

    touch foo
    sleep 2
    cat foo
    if (( $(zstat +atime foo) > $(zstat +mtime foo) )); then
      [[ ! -N foo ]]
    else
      ZTST_skip="cwd appears to be mounted noatime"
    fi

Bonus points if there's a way to write the condition portably without
relying on _other_ zsh functionality...

Re: Bug in C02cont.ztst's -N test

[Matthew Martin]

On Sat, Apr 30, 2016 at 12:46:39AM +0000, Daniel Shahaf wrote:
> Bonus points if there's a way to write the condition portably without
> relying on _other_ zsh functionality...

That was discussed on IRC last night.

diff --git a/Test/C02cond.ztst b/Test/C02cond.ztst
index 88cad0d..8343fe2 100644
--- a/Test/C02cond.ztst
+++ b/Test/C02cond.ztst
@@ -23,6 +23,7 @@
   zlnfs=${filetmpprefix}zlnfs
 
   touch $unmodified
+  unmodified_ls="$(ls -lu $unmodified)"
 
   touch zerolength
   chgrp $EGID zerolength
@@ -154,7 +155,7 @@
     ZTST_skip="[[ -N file ]] not supported on Cygwin"
   elif (( isnfs )); then
     ZTST_skip="[[ -N file ]] not supported with NFS"
-  elif { df -k -- ${$({mount || /sbin/mount} | awk '/noatime/ {print $1,$3}'):-""} | tr -s ' ' | fgrep "$(df -k . | tail -1 | tr -s ' ')" } >&/dev/null; then
+  elif sleep 58; [[ $unmodified_ls == "$(ls -lu $unmodified)" ]]; then
     ZTST_skip="[[ -N file ]] not supported with noatime file system"
   else
     [[ -N $newnewnew && ! -N $unmodified ]]

but who wants to sleep for 60 seconds? (There's a sleep 2 earlier.)

- Matthew Martin

Re: Bug in C02cont.ztst's -N test

[Daniel Shahaf]

Matthew Martin wrote on Fri, Apr 29, 2016 at 20:01:31 -0500:
> @@ -154,7 +155,7 @@
>      ZTST_skip="[[ -N file ]] not supported on Cygwin"
>    elif (( isnfs )); then
>      ZTST_skip="[[ -N file ]] not supported with NFS"
> -  elif { df -k -- ${$({mount || /sbin/mount} | awk '/noatime/ {print $1,$3}'):-""} | tr -s ' ' | fgrep "$(df -k . | tail -1 | tr -s ' ')" } >&/dev/null; then
> +  elif sleep 58; [[ $unmodified_ls == "$(ls -lu $unmodified)" ]]; then
>      ZTST_skip="[[ -N file ]] not supported with noatime file system"
>    else
>      [[ -N $newnewnew && ! -N $unmodified ]]
> 
> but who wants to sleep for 60 seconds? (There's a sleep 2 earlier.)

Couldn't we sleep less, e.g., with the following? —

    elif ! () {
             repeat 58 {
               sleep 1
               if [[ $unmodified_ls != "$(ls -lu $unmodified)" ]]; then return 0; fi
             }
             return 1
           }; then
      ZTST_skip="…"

I assume we don't need to bother with fancy exponential (doubling)
backoffs in this context...

Cheers,

Daniel

Re: Bug in C02cont.ztst's -N test

[Bart Schaefer]

We're gong to have to add "useless use of anonymous function" to go
with "useless use of cat".

On Fri, Apr 29, 2016 at 6:28 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Matthew Martin wrote on Fri, Apr 29, 2016 at 20:01:31 -0500:
>> +  elif sleep 58; [[ $unmodified_ls == "$(ls -lu $unmodified)" ]]; then
>>
>> but who wants to sleep for 60 seconds? (There's a sleep 2 earlier.)

Is there some magic reason it has to total 60 seconds?

>     elif ! () {
>              repeat 58 {
>                sleep 1
>                if [[ $unmodified_ls != "$(ls -lu $unmodified)" ]]; then return 0; fi
>              }
>              return 1
>            }; then

elif ! repeat 60; do sleep 1; ZTST_hashmark; [[ $unmodified_ls !=
"$(ls -lu $unmodified)" ]] && break; done; then

Re: Bug in C02cont.ztst's -N test

[Bart Schaefer]

On Apr 29,  8:51pm, Bart Schaefer wrote:
} Subject: Re: Bug in C02cont.ztst's -N test
}
} Is there some magic reason it has to total 60 seconds?

Oh, it's because not all versions of "ls" will show file times down to
the second, so we have to wait a full minute.  But that would mean
waiting one minute between creating and accessing the file, not just
between the runs of "ls".  

So what we need is to first determine whether {mount || /sbin/mount}
is going to return useful mount attribute information, and then do the
60-second sleep only if we can't trust it.

Wild guess without actually attempting to set up mock follows.  This
also handles the case where mount is neither in $PATH nor in /sbin.
Still breaks if "ls" is not in $PATH, but that seems unlikely.


diff --git a/Test/C02cond.ztst b/Test/C02cond.ztst
index f9fc185..189c233 100644
--- a/Test/C02cond.ztst
+++ b/Test/C02cond.ztst
@@ -146,7 +146,12 @@
 
 # can't be bothered with -S
 
-  print -u $ZTST_fd 'This test takes two seconds...'
+  if [[ ${mtab::="$({mount || /sbin/mount})"} = *[(]?*[)] ]]; then
+    print -u $ZTST_fd 'This test takes two seconds...'
+  else
+    unmodified_ls="$(ls -lu $unmodified)"
+    print -u $ZTST_fd 'This test takes up to 60 seconds...'
+  fi
   sleep 2
   cat $unmodified
   touch $newnewnew
@@ -154,7 +159,14 @@
     ZTST_skip="[[ -N file ]] not supported on Cygwin"
   elif (( isnfs )); then
     ZTST_skip="[[ -N file ]] not supported with NFS"
-  elif { df -k -- ${$({mount || /sbin/mount} | awk '/noatime/ {print $1,$3}'):-""} | tr -s ' ' | fgrep -- "$(df -k . | tail -1 | tr -s ' ')" } >&/dev/null; then
+  elif { (( ! $+unmodified_ls )) &&
+         { df -k -- ${$(print -r -- "$mtab" |
+                        awk '/noatime/ {print $1,$3}'):-""} | tr -s ' ' |
+           fgrep -- "$(df -k . | tail -1 | tr -s ' ')" } >&/dev/null } ||
+       { (( $+unmodified_ls )) &&
+         ! repeat 30; do ZTST_hashmark; sleep 2
+             [[ $unmodified_ls != "$(ls -lu $unmodified)" ]] && break
+	   done }; then
     ZTST_skip="[[ -N file ]] not supported with noatime file system"
   else
     [[ -N $newnewnew && ! -N $unmodified ]]

Re: Bug in C02cont.ztst's -N test

[Bart Schaefer]

On Apr 30, 10:55am, Bart Schaefer wrote:
}
} waiting one minute between creating and accessing the file, not just
} between the runs of "ls".  

I wrote that and I then I forgot to actually move the "cat".

Replacement patch:

diff --git a/Test/C02cond.ztst b/Test/C02cond.ztst
index f9fc185..d7a40ce 100644
--- a/Test/C02cond.ztst
+++ b/Test/C02cond.ztst
@@ -146,17 +146,28 @@
 
 # can't be bothered with -S
 
-  print -u $ZTST_fd 'This test takes two seconds...'
-  sleep 2
-  cat $unmodified
-  touch $newnewnew
+  if [[ ${mtab::="$({mount || /sbin/mount})"} = *[(]?*[)] ]]; then
+    print -u $ZTST_fd 'This test takes two seconds...'
+  else
+    unmodified_ls="$(ls -lu $unmodified)"
+    print -u $ZTST_fd 'This test takes up to 60 seconds...'
+  fi
   if [[ $OSTYPE == "cygwin" ]]; then
     ZTST_skip="[[ -N file ]] not supported on Cygwin"
   elif (( isnfs )); then
     ZTST_skip="[[ -N file ]] not supported with NFS"
-  elif { df -k -- ${$({mount || /sbin/mount} | awk '/noatime/ {print $1,$3}'):-""} | tr -s ' ' | fgrep -- "$(df -k . | tail -1 | tr -s ' ')" } >&/dev/null; then
+  elif { (( ! $+unmodified_ls )) &&
+         { sleep 2; cat $unmodified } &&
+         { df -k -- ${$(print -r -- "$mtab" |
+                        awk '/noatime/ {print $1,$3}'):-""} | tr -s ' ' |
+           fgrep -- "$(df -k . | tail -1 | tr -s ' ')" } >&/dev/null } ||
+       { (( $+unmodified_ls )) &&
+         ! repeat 30; do ZTST_hashmark; sleep 2; cat $unmodified
+             [[ $unmodified_ls != "$(ls -lu $unmodified)" ]] && break
+	   done }; then
     ZTST_skip="[[ -N file ]] not supported with noatime file system"
   else
+    touch $newnewnew
     [[ -N $newnewnew && ! -N $unmodified ]]
   fi
 0:-N cond

Re: Bug in C02cont.ztst's -N test

[Daniel Shahaf]

Bart Schaefer wrote on Sat, Apr 30, 2016 at 11:11:31 -0700:
> +         ! repeat 30; do ZTST_hashmark; sleep 2; cat $unmodified

«repeat 30 sleep 2» could sleep more than 30⋅2 seconds, so perhaps
change the «repeat 30» to «until (( $(date +%s) > $starttime + 60 ))».

Re: Bug in C02cont.ztst's -N test

[Bart Schaefer]

On May 1,  1:48am, Daniel Shahaf wrote:
} Subject: Re: Bug in C02cont.ztst's -N test
}
} change the "repeat 30" to "until (( $(date +%s) > $starttime + 60 ))".

At this point we're doing a test in some kind of automated build system
so quite frankly as long as the loop doesn't go LESS than 60 seconds I
really don't care; but using "date" is unnecessary.

    SECONDS=0
    until (( SECONDS >= 60 )) ...


-- 
Barton E. Schaefer

Re: Bug in C02cont.ztst's -N test

[Jun T.]

On 2016/05/01, at 3:11, Bart Schaefer <schaefer@brasslantern.com> wrote:
> diff --git a/Test/C02cond.ztst b/Test/C02cond.ztst
(snip)
> 
> -  sleep 2
> -  cat $unmodified
> -  touch $newnewnew
(snip)
> +  elif { (( ! $+unmodified_ls )) &&
> +         { sleep 2; cat $unmodified } &&
(snip)
> 
>   else
> +    touch $newnewnew

Moving 'touch $newnewnew' into the 'else' block causes a problem
on a system where the test for -N is skipped, for example Cygwin.
The file newnewnew is not created at all, and the next test
  [[ $newnewnew -nt $zlnfs && ! ($unmodified -nt $zlnfs) ]] 
fails.

diff --git a/Test/C02cond.ztst b/Test/C02cond.ztst
index b0e84dd..63c89dd 100644
--- a/Test/C02cond.ztst
+++ b/Test/C02cond.ztst
@@ -152,12 +152,14 @@
     unmodified_ls="$(ls -lu $unmodified)"
     print -u $ZTST_fd 'This test takes up to 60 seconds...'
   fi
+  sleep 2
+  touch $newnewnew
   if [[ $OSTYPE == "cygwin" ]]; then
     ZTST_skip="[[ -N file ]] not supported on Cygwin"
   elif (( isnfs )); then
     ZTST_skip="[[ -N file ]] not supported with NFS"
   elif { (( ! $+unmodified_ls )) &&
-         { sleep 2; cat $unmodified } &&
+         cat $unmodified &&
          { df -k -- ${$(print -r -- "$mtab" |
                         awk '/noatime/ {print $1,$3}'):-""} | tr -s ' ' |
            fgrep -- "$(df -k . | tail -1 | tr -s ' ')" } >&/dev/null } ||
@@ -168,7 +170,6 @@
 	   done }; then
     ZTST_skip="[[ -N file ]] not supported with noatime file system"
   else
-    touch $newnewnew
     [[ -N $newnewnew && ! -N $unmodified ]]
   fi
 0:-N cond

Re: Bug in C02cont.ztst's -N test

[Bart Schaefer]

On May 9, 10:42pm, Jun T. wrote:
}
} Moving 'touch $newnewnew' into the 'else' block causes a problem
} on a system where the test for -N is skipped, for example Cygwin.
} The file newnewnew is not created at all, and the next test
}   [[ $newnewnew -nt $zlnfs && ! ($unmodified -nt $zlnfs) ]] 
} fails.

Hrm.  My concern was that if we spend a full minute spinning to wait
for the noatime check, then $newnewnew won't be new new new *enough*
if it were created before that delay.  I hadn't noticed that there
were inter-dependent tests following later.

Do we need to "sleep 2" before touching $newnewnew, though?  Or could
that part stay in the "elif"?

Re: Bug in C02cont.ztst's -N test

[Jun T.]

2016/05/10 01:21, Bart Schaefer <schaefer@brasslantern.com> wrote:

> Do we need to "sleep 2" before touching $newnewnew, though?  Or could
> that part stay in the "elif"?

On a system where the nanosecond timestamps are not supported
(for example Mac OS X with the default file system HFS+),
in order for the test '$newnewnew -nt $zlnfs' to succeed,
the mtime's of the two files should have at least 1 second
difference. If the sleep is left in the 'elif' block,
then the test (for -nt) fails on OS X.

I tried with 'sleep 1' before 'touch $newnewnew' and it seems to
work. There are some tests between 'touch $zlnfs' (near the
top of the script) and the 'touch $newnewnew', so 'sleep 1' would
be enough even if there is some inaccuracy in sleep period.

Or it may be better to sleep 2 second just in case ...
Why does the original code sleeps 2 sec (instead of 1 sec).

Re: Bug in C02cont.ztst's -N test

[Bart Schaefer]

On May 10,  2:32am, Jun T. wrote:
}
} On a system where the nanosecond timestamps are not supported
} in order for the test '$newnewnew -nt $zlnfs' to succeed,
} the mtime's of the two files should have at least 1 second
} difference.

Aha.  Now that you mention it, I recall that ...

} Why does the original code sleeps 2 sec (instead of 1 sec).

... in some cases "sleep 1" will only sleep to the next whole-number
of seconds on the system clock, rather than sleeping a full second.
So "sleep 2" is needed to always make it at least one full second.

I've committed your patch.