| Message ID | 1251306352-31316-26-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
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.
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.
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.
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
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(-)