On 10/28/18 8:01 PM, PiBa-NL wrote:
Hi Frederic,

Hello Pieter,

Sorry again for this late reply.

Op 19-10-2018 om 11:51 schreef Frederic Lecaille:
The idea of the script sounds good to me. About the script itself it is a nice work which could be a good start.

Just a few details below.

Note that "cat $file | grep something" may be shortened by "grep something $file". It would also be interesting to avoid creating temporary files as most as possible (at least testflist.lst is not necessary I think).

TARGET=$(haproxy -vv | grep "TARGET  = " | sed 's/.*= //')
OPTIONS=$(haproxy -vv | grep "OPTIONS = " | sed 's/.*= //')

may be shortened by these lines:

    { read TARGET; read OPTIONS; } << EOF
    $(./haproxy -vv |grep 'TARGET\|OPTIONS' | sed 's/.* = //')
Thanks, I've changed this.
which is portable, or something similar.

    sed 's/.*=//'| sed 's/,.*//'

may be shortened by

    sed -e 's/.*=//' -e 's/,.*//'
Thanks, I've changed this as well.

Also note, there are some cases where options are enabled without appearing in OPTIONS variable value.

For instance if you compile haproxy like that:

   $ make TARGET=linux2628

the support for the thread is enabled without appearing in OPTIONS variable value. I am not sure this is an issue at this time.
That could become an issue. but should be easy to solve adding a 'fake' option perhaps to check against.. Or adding a separate check perhaps I'm not sure yet.

With one test like that after having parsed OPTIONS, it would suffice at this time :

    if [ $TARGET = linux2628 ] ; then
      OPTIONS="$OPTIONS USE_xxx USE_xxx"

with USE_xxx the listed compilation options found in the Makefile file:

ifeq ($(TARGET),linux2628)
# This is for standard Linux >= 2.6.28 with netfilter, epoll, tproxy and splice
  USE_NETFILTER   = implicit
  USE_POLL        = implicit
  USE_EPOLL       = implicit
  USE_TPROXY      = implicit
  USE_CRYPT_H     = implicit
  USE_LIBCRYPT    = implicit
  USE_LINUX_SPLICE= implicit
  USE_LINUX_TPROXY= implicit
  USE_ACCEPT4     = implicit
  USE_FUTEX       = implicit
  USE_CPU_AFFINITY= implicit
  USE_DL          = implicit
  USE_RT          = implicit
  USE_THREAD      = implicit

Perhaps we could could only one line for the required options and excluded targets like that:

#EXCLUDED_TARGETS=freebsd,dos,windows     ;)
Added this option, but then i like the option of excluding single targets and having some comment behind it explaining the reason.. But i guess if one would want to know some comments above the setting could also 'explain' why that target is currently not 'valid' to run the test on. Should i remove the settings for the 'single' option/target.?

No. It is ok at this time.

New 'version' of the script attached.
It now supports a set of parameters to modify its behavior a little. And also checking for a 'version requirement'. So a H2 test doesn't have to fail on 1.7.

Very good work.

Should i 'ask' to delete old test result.? Or would there be a other better way to keep previous results separated from the current run?


Rename the old vtc files like that before running the tests:

bname=$(basename $i)
mv /tmp/$bname /tmp/old.$bname

with $i in $(find /tmp/ -type d -name "vtc.*")

or something like that.

please *add -type d* to the find command to list the temporary varnishtest directories.

And we should only keep temporaries directory of tests which have failed with -l varnishtest option currently enabled option. They are all the /tmp/vtc.* directories found after having run the tests.

So the script should list the remaining temporary directories after having run the tests. There is no need to parse the log file of this directories. We cannot rely on the fact there is no bug in varnishtest logging code ;)

If you could give it another look i would appreciate that. Are there any things that need to be added/changed about it before considering to add it to haproxy sources branch?

Yes a few details.

It seems there are remaining useless piped sed processes (sed | sed) or perhaps I have missed something.

There is no need to use awk to remove sections of lines. Have a look to cut command which is more lightweight.

tr command accepts escaped characters as '\n' for new line character which is more readable than '
' ;)

For me with these remaining modifications/cleanup this script could be added to haproxy sources.

Thanks again for this good work Pieter.



Reply via email to