Patchwork [25/29] Add check support

login
register
mail settings
Submitter Luiz Capitulino
Date Aug. 26, 2009, 5:05 p.m.
Message ID <1251306352-31316-26-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/32204/
State Superseded
Headers show

Comments

Luiz Capitulino - Aug. 26, 2009, 5:05 p.m.
Check is a unit testing framework for C.

All the QObjects have unit-tests and more will be written for the
future data types.

More info about check can be found at:

http://check.sourceforge.net/

Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 Makefile  |    4 ++++
 configure |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 0 deletions(-)
Juan Quintela - Aug. 26, 2009, 7:34 p.m.
Luiz Capitulino <lcapitulino@redhat.com> 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.
Luiz Capitulino - Aug. 26, 2009, 8:39 p.m.
On Wed, 26 Aug 2009 21:34:20 +0200
Juan Quintela <quintela@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> 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?

 CONFIG_CHECK_UTESTS or CONFIG_UTESTS will do, I think.

 Although it might be important to keep '--enable-check' as
a configure option, because the library itself is called
'check'.

 Also I haven't get feedback from the maintainers about this
stuff yet, I mean, I still don't know if they agree on having
unit-tests.

> 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.

 Ok, but you are talking about code in staging right?

 As I told you on IRC, I'm a bit reluctant to change a patchset based
on upstream master to rely on something that's on staging.

 The reason is not only because patches may be dropped from staging,
but also because: 1. there may be more patches on staging that
conflicts with this series and 2. reviewers not following staging
may get confused

 So, we should change the process to always base a patchset against
staging or wait for the code to arrive on master to make the
appropriate changes.

> >  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.

 Will do if we have a new rule for this, but --disable
is redundant as check support is disabled by default.

> >  ##########################################
> > +# 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.

 Will have a look.

> 
> 
> >  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 :)

 It just adds infrastructure that will be used by the following
patches.
Juan Quintela - Aug. 26, 2009, 9:53 p.m.
Luiz Capitulino <lcapitulino@redhat.com> wrote:
> On Wed, 26 Aug 2009 21:34:20 +0200
> Juan Quintela <quintela@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> 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?
>
>  CONFIG_CHECK_UTESTS or CONFIG_UTESTS will do, I think.

That works for me.

>  Although it might be important to keep '--enable-check' as
> a configure option, because the library itself is called
> 'check'.

For coherence I will call it --enable-check-utests.


>> > +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.
>
>  Ok, but you are talking about code in staging right?

No, we are moving everything to this style.  In staging are the changes
for the other stuff in qemu.

>  As I told you on IRC, I'm a bit reluctant to change a patchset based
> on upstream master to rely on something that's on staging.

This is a change that everything is suffering.  No dependencies on
staging at all.  The code that I showed you works today :)

>> I guess you want to add something here to tools, otherwise, you are
>> doing nothing here :)
>
>  It just adds infrastructure that will be used by the following
> patches.

ah, ok. didn't looked more, sorry.

Later, Juan.

Patch

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
+
 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"
 io_thread="no"
 nptl="yes"
 mixemu="no"
@@ -482,6 +483,8 @@  for opt do
   ;;
   --disable-curl) curl="no"
   ;;
+  --enable-check) check="yes"
+  ;;
   --disable-nptl) nptl="no"
   ;;
   --enable-mixemu) mixemu="yes"
@@ -600,6 +603,7 @@  echo "  --disable-vnc-tls        disable TLS encryption for VNC server"
 echo "  --disable-vnc-sasl       disable SASL encryption for VNC server"
 echo "  --disable-curses         disable curses output"
 echo "  --disable-curl           disable curl connectivity"
+echo "  --enable-check           enable check unit-tests"
 echo "  --disable-bluez          disable bluez stack connectivity"
 echo "  --disable-kvm            disable KVM acceleration support"
 echo "  --disable-nptl           disable usermode NPTL support"
@@ -1089,6 +1093,25 @@  EOF
 fi # test "$curl"
 
 ##########################################
+# check probe
+
+if test "$check" = "yes" ; then
+    `pkg-config --libs check > /dev/null 2> /dev/null` || check="no"
+fi
+
+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"
+
+##########################################
 # bluez support probe
 if test "$bluez" = "yes" ; then
   `pkg-config bluez 2> /dev/null` || bluez="no"
@@ -1486,6 +1509,7 @@  fi
 echo "SDL support       $sdl"
 echo "curses support    $curses"
 echo "curl support      $curl"
+echo "check support     $check"
 echo "mingw32 support   $mingw32"
 echo "Audio drivers     $audio_drv_list"
 echo "Extra audio cards $audio_card_list"
@@ -1672,6 +1696,11 @@  if test "$curl" = "yes" ; then
   echo "CONFIG_CURL=y" >> $config_host_mak
   echo "CURL_CFLAGS=$curl_cflags" >> $config_host_mak
 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
 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
   fi
 fi
 echo "TOOLS=$tools" >> $config_host_mak