[Qemu-devel] Re: [PATCH 25/29] Add check support

[Juan Quintela]

Luiz Capitulino <address@hidden> wrote:
> Check is a unit testing framework for C.

Some notes.  Configuration has changed to be a bit more consistent.
Below I change the configuration bits to new style.

Can I suggest that you name the configuration something different?

CONFIG_CHECK

means whatever, some for "check" variable in configure.

check_suite
test_suite

or anything else?
I think that "check" alone is not enough descriptive, but this is just a
nit pit.


Comments about new style config

>
> diff --git a/Makefile b/Makefile
> index bdb6b39..efeb6ba 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -180,6 +180,10 @@ qemu-io$(EXESUF):  qemu-io.o qemu-tool.o cmd.o 
> $(block-obj-y)
>  qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
>       $(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@,"  GEN   $@")
>  
> +ifdef CONFIG_CHECK
> +LIBS += $(CHECK_LIBS)
> +endif

We changed how to add libraries, you can remove this three lines.
See below where to add it.

>  clean:
>  # avoid old build problems by removing potentially incorrect old files
>       rm -f config.mak config.h op-i386.h opc-i386.h gen-op-i386.h op-arm.h 
> opc-arm.h gen-op-arm.h
> diff --git a/configure b/configure
> index 5c1065f..18cb586 100755
> --- a/configure
> +++ b/configure
> @@ -196,6 +196,7 @@ build_docs="yes"
>  uname_release=""
>  curses="yes"
>  curl="yes"
> +check="no"

new style:
check=""

>  io_thread="no"
>  nptl="yes"
>  mixemu="no"
> @@ -482,6 +483,8 @@ for opt do
>    ;;
>    --disable-curl) curl="no"
>    ;;
> +  --enable-check) check="yes"
> +  ;;
     --disable-check) check="no"
     ;;

Add both.

>  ##########################################
> +# check probe
> +
> +if test "$check" = "yes" ; then
> +    `pkg-config --libs check > /dev/null 2> /dev/null` || check="no"
> +fi

Remove this bit.

> +if test "$check" = "yes" ; then
> +    check="no"
> +    cat > $TMPC << EOF
> +#include <check.h>
> +int main(void) { suite_create("yeah"); return 0; }
> +EOF
> +    check_libs=`pkg-config --libs check`
> +    if $cc ${ARCH_CFLAGS} $check_libs -o $TMPE $TMPC > /dev/null 2> 
> /dev/null ; then
> +        check="yes"
> +    fi
> +fi # test "$check"

Rewrote this as:

if test "$check" != "no" ; then
  cat > $TMPC << EOF
#include <check.h>
int main(void) { suite_create("yeah"); return 0; }
EOF
  check_libs=`pkg-config --libs check`
  if compile_prog "" $check_libs ; then
    check=yes
    libs_tools="$check_libs $libs_tools"
  else
    if test "$check" = "yes" ; then
      echo "`check` not found and requested.  Please install"
      exit 1
    fi
    check=no
  fi
fi # test "$check"

Things that changed:
- two spaces indentation
- use compile_prog funciton
- Add check_libs here, not in Makefile

On my patches on staging, there is a function called feature_not_found

echo + exit in function will become:

feature_not_found "check"

just if your series got merged after my changes in staging.  nothing
else needs to be changed.


>  fi
> +if test "$check" = "yes" ; then
> +  echo "CONFIG_CHECK=y" >> $config_host_mak
> +  echo "CHECK_LIBS=$check_libs" >> $config_host_mak
> +  echo "#define CONFIG_CHECK 1" >> $config_host_h
> +fi

two last lines not needed, it becomes:

> +if test "$check" = "yes" ; then
> +  echo "CONFIG_CHECK=y" >> $config_host_mak
> +fi

>  if test "$brlapi" = "yes" ; then
>    echo "CONFIG_BRLAPI=y" >> $config_host_mak
>  fi
> @@ -1723,6 +1752,9 @@ if test `expr "$target_list" : ".*softmmu.*"` != 0 ; 
> then
>    tools="qemu-img\$(EXESUF) $tools"
>    if [ "$linux" = "yes" ] ; then
>        tools="qemu-nbd\$(EXESUF) qemu-io\$(EXESUF) $tools"
> +    if [ "$check" = "yes" ]; then
> +      tools="$tools"
> +    fi

I guess you want to add something here to tools, otherwise, you are
doing nothing here :)

That is it.

Later, Juan.