Re: parallel autotest [2/3]: Implement 'testsuite --jobs'.

[Eric Blake]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

According to Ralf Wildenhues on 5/25/2008 11:50 PM:
> There may be systems that support mknod but not mkfifo.  Not sure
> whether it's useful to support them.

Hi Ralf, and (finally) reviewing this...

>  
>  * Major changes in Autoconf 2.62a (2008-??-??)
>  
> +** Autotest testsuites accept an option --jobs[=N] for parallel testing.

Make sure you adjust this to account for 2.63 (hmm.  that means I should
try to resync from coreutils' maint.mk, which has since added logic to
commit the md5sum of the previous release's NEWS snippet in cfg.mk, to
make it easier to identify inadvertent changes to the wrong NEWS row when
doing 'make maintainer-distcheck').

>  
> address@hidden address@hidden@address@hidden
> address@hidden address@hidden
> address@hidden -j

This second line is redundant, since you used @ovar on the line above.

> +
> +Parallel mode requires the @command{mkfifo} command to work, and will be
> +silently disabled otherwise.

Good - the option always exists, but is a no-op where we can't get it to
work, because we have a sane fallback of linear execution.  Thus, if my
testing on cygwin shows problems (I still haven't had time to apply your
patch), it merely becomes a matter of adding cygwin to the list of reasons
to disable -j, alongside the existing reason of missing mkfifo.  Thus, I
think your patch is safe - even if it causes failures on some platforms
during our testing phase, we have some time before the next release to fix
them.  Please go ahead and apply, once you've addressed the remaining nits
in this mail.

> +     case $at_jobs in *[[!0-9]]*)
> +       AS_ERROR([non-numeric argument to -j/--jobs: $at_jobs]) ;;

Is there any way to feed back the user's spelling to this message, rather
than listing both spellings?  If you don't spot an easy way, then I guess
this is okay.

>  Execution tuning:
>    -C, --directory=DIR
>  [                 change to directory DIR before starting]
> +  -j[[N]], --jobs[[=N]]
> +[                 Allow N jobs at once; infinite jobs with no arg]

Don't we have the convention that in --help output, optional arguments for
long options are also optional for short args?  In other words, is it
better to try to write this, for compactness (after m4 processing)?

  -j, --jobs[=N]  Allow N jobs at once; ...

If there's room, should we mention that this defaults to 1?

> +if test $at_jobs -ne 1 &&
> +     rm -f "$at_job_fifo" &&
> +     ( mkfifo "$at_job_fifo" ) 2>/dev/null &&
> +     exec AT_JOB_FIFO_FD<> "$at_job_fifo"

Is exec n<> portable?  Yes, POSIX specifies it, but I haven't seen it used
anywhere else in autoconf.  I guess we commit it now, and if it is not
portable, we wrap it inside an eval alongside the mkfifo check as a reason
to skip the parallel testing.

> +then
> +  # FIFO job dispatcher.
> +  echo
> +  if test $at_jobs -eq 0; then
> +    set X $at_groups; shift; address@hidden:@]
>    fi
> -  at_func_group_postprocess
> -  test -f "$at_stop_file" && break
> -  at_first=false
> -done
> +  # Turn jobs into a list of numbers, starting from 1.
> +  at_joblist=`AS_ECHO([" $at_groups_all "]) | \
> +    sed -e 's/\( '$at_jobs'\) .*/\1/'`

In general, we've been omitting the -e.

> +
> +  set X $at_joblist
> +  shift
> +  for at_group in $at_groups; do
> +    (
> +      # Start one test group.
> +      at_func_group_prepare
> +      if cd "$at_group_dir" &&
> +      at_func_test $at_group &&
> +      . "$at_test_source" # AT_JOB_FIFO_FD<&-
> +      then :; else
> +     AS_WARN([unable to parse test group: $at_group])
> +     at_failed=:
> +      fi
> +      at_func_group_postprocess
> +      echo token >&AT_JOB_FIFO_FD
> +    ) &
> +    shift # Consume one token.
> +    if test address@hidden:@] -gt 0; then :; else
> +      read at_token <&AT_JOB_FIFO_FD || break
> +      set x $[*]
> +    fi
> +    test -f "$at_stop_file" && break
> +    at_first=false
> +  done
> +  # Read back the remaining ($at_jobs - 1) tokens.
> +  set X $at_joblist
> +  shift
> +  if test address@hidden:@] -gt 0; then
> +    shift
> +    for at_job
> +    do
> +      read at_token
> +    done <&AT_JOB_FIFO_FD
> +  fi
> +  exec AT_JOB_FIFO_FD<&-
> +  wait

Looks sane, on a first reading.

> +
> +# Ensure we really are faster than sequential execution:
> +# the testsuite should have completed before we kill it.
> +# Unfortunately, the return value of wait is unreliable,
> +# so we check that kill fails.
> +#
> +# Since the testsuite startup time can be high, we compare
> +# it against a sequential testsuite run in a subdirectory,
> +# where only a subset of tests is run.
> +
> +# The parallel scheduler requires mkfifo to work.
> +AT_CHECK([mkfifo fifo || exit 77])

Good - we skip the test on losing platforms only after validating that
sequential runs still work.

> +mkdir serial
> +AT_CHECK([$CONFIG_SHELL ./micro-suite --jobs=[]AT_PARALLEL_TESTS_RUN & ]dnl
> +         [sleep 3 && ]dnl
> +         [cd serial && $CONFIG_SHELL ../micro-suite -3 >/dev/null && ]dnl

Should that -3 instead be written as -m4_decr(AT_PARALLEL_TESTS_RUN), in
case someone plays with tuning the parameters?

> +         [{ kill $! && exit 1; :; }], [], [stdout], [ignore])
> +AT_CHECK([grep -c '^.\{53\}ok' stdout], [], [AT_PARALLEL_TESTS_TOTAL
> +])
> +AT_CHECK([grep 'AT_PARALLEL_TESTS_TOTAL tests' stdout], [], [ignore])
> +
> +AT_CLEANUP
> +

- --
Don't work too hard, make some time for fun as well!

Eric Blake             address@hidden
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Public key at home.comcast.net/~ericblake/eblake.gpg
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkjc3hwACgkQ84KuGfSFAYBtaACgzwrnUhgbBcUlm0wKZrnsjyqE
mjMAnil0yiRw5AFcJa4bIuiz058b/F6q
=NnNj
-----END PGP SIGNATURE-----