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

[Luiz Capitulino <lcapitulino@redhat.com>]

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.