On Thu, 2012-08-09 at 10:01 +0800, Deng Zhengrong wrote:
> ---
> configure.ac | 15 ++++++++++++++-
> src/Makefile.am | 12 ++++++++++++
> 2 files changed, 26 insertions(+), 1 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index ffb2a35..9cb5aa6 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -581,10 +581,21 @@ AC_CHECK_HEADERS_ONCE([valgrind/memcheck.h])
>
> #### check test framework ####
>
> -PKG_CHECK_MODULES(LIBCHECK, [ check ])
> +AC_ARG_ENABLE([check],
> + AS_HELP_STRING([--disable-check],[Disable unit tests]))
If the help text is "Disable unit tests" (which is good in my opinion),
I think the AC_ARG_ENABLE parameter should be "tests" instead of
"check", and "--disable-check" should be "--disable-tests".
> +
> +AS_IF([test "x$enable_check" != "xno"],
> + [PKG_CHECK_MODULES(LIBCHECK, [ check ], HAVE_LIBCHECK=1, HAVE_LIBCHECK=0)],
> + HAVE_LIBCHECK=0)
> +
> +AS_IF([test "x$enable_check" = "xyes" && test "x$HAVE_LIBCHECK" = "x0"],
> + [AC_MSG_ERROR([*** check library not found])])
> +
> AC_SUBST(LIBCHECK_CFLAGS)
> AC_SUBST(LIBCHECK_LIBS)
>
> +AM_CONDITIONAL([HAVE_LIBCHECK], [test "x$HAVE_LIBCHECK" = x1])
> +
> #### json parsing ####
>
> PKG_CHECK_MODULES(LIBJSON, [ json >= 0.9 ])
> @@ -1393,6 +1404,7 @@ AS_IF([test "x$HAVE_SIMPLEDB" = "x1"], ENABLE_SIMPLEDB=yes, ENABLE_SIMPLEDB=no)
> AS_IF([test "x$HAVE_ESOUND" = "x1"], ENABLE_ESOUND=yes, ENABLE_ESOUND=no)
> AS_IF([test "x$HAVE_ESOUND" = "x1" -a "x$USE_PER_USER_ESOUND_SOCKET" = "x1"], ENABLE_PER_USER_ESOUND_SOCKET=yes, ENABLE_PER_USER_ESOUND_SOCKET=no)
> AS_IF([test "x$HAVE_GCOV" = "x1"], ENABLE_GCOV=yes, ENABLE_GCOV=no)
> +AS_IF([test "x$HAVE_LIBCHECK" = "x1"], ENABLE_LIBCHECK=yes, ENABLE_LIBCHECK=no)
> AS_IF([test "x$enable_legacy_database_entry_format" != "xno"], ENABLE_LEGACY_DATABASE_ENTRY_FORMAT=yes, ENABLE_LEGACY_DATABASE_ENTRY_FORMAT=no)
>
> echo "
> @@ -1440,6 +1452,7 @@ echo "
> Enable speex (resampler, AEC): ${ENABLE_SPEEX}
> Enable WebRTC echo canceller: ${ENABLE_WEBRTC}
> Enable gcov coverage: ${ENABLE_GCOV}
> + Enable check unit tests: ${ENABLE_LIBCHECK}
Since you disable all tests if the check framework is not available,
regardless of whether the tests use the check framework or not, I think
the message should be just "Enable unit tests", without the "check"
specifier. And instead of printing the ENABLE_LIBCHECK value, you should
print the enable_tests value (or maybe there should be a new variable
ENABLE_TESTS to be more consistent).
> Database
> tdb: ${ENABLE_TDB}
> gdbm: ${ENABLE_GDBM}
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 2f20df2..dfa055e 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -297,6 +297,7 @@ TESTS_norun += \
> alsa-time-test
> endif
>
> +if HAVE_LIBCHECK
> TESTS_ENVIRONMENT=MAKE_CHECK=1
> TESTS = $(TESTS_default)
>
> @@ -309,6 +310,17 @@ endif
> check-daemon: $(TESTS_daemon)
> PATH=$(builddir):${PATH} $(top_srcdir)/src/tests/test-daemon.sh $(TESTS_daemon)
>
> +else
> +TESTS_ENVIRONMENT=
> +TESTS =
> +noinst_PROGRAMS =
> +check_PROGRAMS =
I think you should check ENABLE_TESTS instead of HAVE_LIBCHECK, if
you're going to disable all tests.
> +
> +check-daemon:
> + @echo "Please don't specify \"--disable-check\" and make sure check library is installed!"
I think "Tests are disabled" would be a more appropriate message,
because that's the problem. After stating the problem, giving hints for
how to resolve the problem would be fine, though.
Also, I think that make should be somehow informed that an error
happened. Maybe add a call to false (I mean /bin/false, in case it was
unclear) after the echo? It feels a bit clumsy to me, though, but I'm
not aware of other ways to do it (I'm not really a make expert).
--
Tanu
