From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MgPHn-0004jd-BC for qemu-devel@nongnu.org; Wed, 26 Aug 2009 16:40:03 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MgPHi-0004gz-CV for qemu-devel@nongnu.org; Wed, 26 Aug 2009 16:40:02 -0400 Received: from [199.232.76.173] (port=43141 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MgPHi-0004gt-5q for qemu-devel@nongnu.org; Wed, 26 Aug 2009 16:39:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30933) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MgPHh-0000aU-BX for qemu-devel@nongnu.org; Wed, 26 Aug 2009 16:39:57 -0400 Date: Wed, 26 Aug 2009 17:39:41 -0300 From: Luiz Capitulino Message-ID: <20090826173941.0f2c156b@doriath> In-Reply-To: References: <1251306352-31316-1-git-send-email-lcapitulino@redhat.com> <1251306352-31316-26-git-send-email-lcapitulino@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH 25/29] Add check support List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Juan Quintela Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, avi@redhat.com On Wed, 26 Aug 2009 21:34:20 +0200 Juan Quintela wrote: > Luiz Capitulino 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 > > +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 > 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.