[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 25/29] Add check support
From: |
Luiz Capitulino |
Subject: |
[Qemu-devel] Re: [PATCH 25/29] Add check support |
Date: |
Wed, 26 Aug 2009 17:39:41 -0300 |
On Wed, 26 Aug 2009 21:34:20 +0200
Juan Quintela <address@hidden> wrote:
> 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?
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.
- [Qemu-devel] [PATCH 17/29] monitor: Drop handler_8 and handler_9, (continued)
- [Qemu-devel] [PATCH 17/29] monitor: Drop handler_8 and handler_9, Luiz Capitulino, 2009/08/26
- [Qemu-devel] [PATCH 18/29] monitor: Port handler_10 to use QDict, Luiz Capitulino, 2009/08/26
- [Qemu-devel] [PATCH 19/29] monitor: Split monitor_handle_command(), Luiz Capitulino, 2009/08/26
- [Qemu-devel] [PATCH 20/29] monitor: Drop unused macros, Luiz Capitulino, 2009/08/26
- [Qemu-devel] [PATCH 21/29] monitor: Drop str_allocated[], Luiz Capitulino, 2009/08/26
- [Qemu-devel] [PATCH 22/29] monitor: Drop args[] handling code, Luiz Capitulino, 2009/08/26
- [Qemu-devel] [PATCH 23/29] monitor: fail when 'i' type is greater than 32-bit, Luiz Capitulino, 2009/08/26
- [Qemu-devel] [PATCH 24/29] monitor: Update supported types documentation, Luiz Capitulino, 2009/08/26
- [Qemu-devel] [PATCH 25/29] Add check support, Luiz Capitulino, 2009/08/26
- [Qemu-devel] [PATCH 26/29] Introduce QInt unit-tests, Luiz Capitulino, 2009/08/26
- [Qemu-devel] [PATCH 27/29] Introduce QString unit-tests, Luiz Capitulino, 2009/08/26
- [Qemu-devel] [PATCH 28/29] Introduce QDict test data file, Luiz Capitulino, 2009/08/26
- [Qemu-devel] [PATCH 29/29] Introduce QDict unit-tests, Luiz Capitulino, 2009/08/26
- Re: [Qemu-devel] [PATCH v2 00/29] QMonitor, Vincent Hanquez, 2009/08/28