On 01/10/2013 01:18 PM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Most checks for libraries take the same format
* --with-libFOO=yes|no|check|/some/path argument * check for a function NNN in libFOO.so * check for a header file DDD/HHH.h * Define a WITH_FOO config.h symbol * Define a WITH_FOO make conditional * Substitute FOO_CFLAGS and FOO_LIBS make variables * Print CFLAGS & LIBS summary at the end
General impression - nice! Putting on my autoconf maintainer hat: full of non-robust underquoted potential gotchas, for example if someone ever tries to link to a library named DNL. I'll go ahead and point out LOTS of instances of where you would use "m4_defn([var])" instead of plain "var" to avoid those issues, but before you hack up code to deal with all my nits (and realize that a lot more lines of code need the same "fixes" than just those I called out), read this entire message.
--- m4/virt-lib.m4 | 391 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ m4/virt-result.m4 | 29 ++++ 2 files changed, 420 insertions(+) create mode 100644 m4/virt-lib.m4 create mode 100644 m4/virt-result.m4
diff --git a/m4/virt-lib.m4 b/m4/virt-lib.m4 new file mode 100644 index 0000000..669ec66 --- /dev/null +++ b/m4/virt-lib.m4 @@ -0,0 +1,391 @@ +dnl +dnl virt-lib.m4: Helper macros for checking for libraries
+ +dnl Probe for existance of libXXXX and set WITH_XXX
s/existance/existence/
+dnl config header var, WITH_XXXX make conditional and +dnl with_XXX configure shell var. +dnl +dnl LIBVIRT_CHECK_LIB([CHECK_NAME], [LIBRARY_NAME], +dnl [FUNCTION_NAME], [HEADER_NAME]) +dnl
+dnl +AC_DEFUN([LIBVIRT_CHECK_LIB],[ + m4_pushdef([check_name], [$1]) + m4_pushdef([library_name], [$2]) + m4_pushdef([function_name], [$3]) + m4_pushdef([header_name], [$4]) + + m4_pushdef([check_name_lc], m4_translit(check_name, `A-Z', `a-z'))
Pedantic: Underquoted (using a library named 'libdnl' would wreak havoc) Real (but harmless) bug: you used the wrong quote characters ([] not `'); not to mention autoconf already provides a helper macro m4_tolower that does the same thing in less typing. Recommendation: replace this line with either: libvirt-quality: m4_pushdef([check_name_lc], m4_tolower(check_name)) pedantic-quality: m4_pushdef([check_name_lc], m4_tolower(m4_defn([check_name])))
+ + m4_pushdef([config_var], [WITH_]check_name)
Pedantic-quality: m4_pushdef([config_var], [WITH_]m4_defn([check_name]))
+ m4_pushdef([make_var], [WITH_]check_name) + m4_pushdef([cflags_var], check_name[_CFLAGS]) + m4_pushdef([libs_var], check_name[_LIBS]) + m4_pushdef([arg_var], [with-]check_name_lc) + m4_pushdef([with_var], [with_]check_name_lc)
etc. with s/var/m4_defn([var])/
+ + AC_ARG_WITH(check_name_lc, + [AS_HELP_STRING([--arg_var], + [with lib]]m4_dquote(library_name)[[ support @<:@default=check@:>@])], + [],[with_var][=check])
We hashed this one out on IRC :) It's fine for libvirt, but for pedantic quality, it would be better as: AC_ARG_WITH(m4_defn([check_name_lc]), [AS_HELP_STRING([--]m4_defn([arg_var]), [with lib]]m4_dquote( m4_defn([library_name]))[[ support @<:@default=check@:>@])], [], m4_defn([with_var])[=check])
+ + old_LIBS="$LIBS" + old_CFLAGS="$CFLAGS"
Pedantic: The "" are not necessary here, but don't hurt either.
+ m4_expand(cflags_var[=]) + m4_expand(libs_var[=])
Overkill; you could get by with: libvirt quality: cflags_var= libs_var= Pedantic quality: m4_defn([cflags_var])= m4_defn([libs_var])=
+ + fail=0 + if test "x$with_var" != "xno" ; then
Pedantic: if test "x$m4_defn([with_var])" != "xno"; then
+ if test "x$with_var" != "xyes" && test "x$with_var" != "xcheck" ; then + m4_expand(cflags_var=["-I$with_var/include"]) + m4_expand(libs_var=["-L$with_var/lib"])
Overkill. libvirt-quality: cflags_var="-I$with_var/include" Pedantic: m4_defn([cflags_var])="-I$m4_defn([with_var])/include"
+ fi + CFLAGS="$CFLAGS $cflags_var" + LIBS="$LIBS $libs_var" + AC_CHECK_LIB(library_name, function_name, [],[
AC_CHECK_LIB(m4_defn([library_name]), m4_defn([function_name]), [], [
+ if test "x$with_var" != "xcheck"; then + fail=1 + fi + m4_expand(with_var[=no])
m4_defn([with_var])=no
+ ]) + if test "$fail" = "0" && test "x$with_var" != "xno" ; then + AC_CHECK_HEADER([header_name], [
AC_CHECK_HEADER(m4_defn([header_name]), [
+ m4_expand(with_var[=yes])
m4_defn([with_var])=yes
+ ],[ + if test "x$with_var" != "xcheck"; then + fail=1 + fi + m4_expand(with_var[=no]) + ]) + fi + fi + + LIBS="$old_LIBS" + CFLAGS="$old_CFLAGS"
Again, "" aren't needed, but don't hurt.
+ + if test $fail = 1; then + AC_MSG_ERROR([You must install the lib]library_name[ library & headers to compile libvirt])
It would be really nice if you could run './configure' once and know _all_ of the libraries to be installed, rather than having to run once per library because each missing library aborts the script immediately. I can probably do that as a followup patch, where instead of directly issuing the error, we instead append the latest error string to a series of messages, then use a single m4_wrap to do AC_MSG_ERROR at the end of all collected messages. But that doesn't impact this patch.
+ else + if test "x$with_var" = "xyes" ; then + if test "x$libs_var" = 'x' ; then + m4_expand(libs_var=["-l]library_name["])
Overkill. libvirt-quality: libs_var="-l[]library_name" pedantic: m4_defn([libs_var])="-l[]m4_defn([library_name])"
+ else + m4_expand(libs_var=["$libs_var -l]library_name["]) + fi + AC_DEFINE_UNQUOTED(config_var, 1, [whether lib]library_name[ is available])
Pedantic: AC_DEFINE_UNQUOTED(m4_defn([config_var]), [1], [whether lib]m4_defn([library_name])[ is available])
+ fi + + AM_CONDITIONAL(make_var, [test "x$with_var" = "xyes"]) + + AC_SUBST(cflags_var)
AC_SUBST(m4_defn([cflags_var]))
+dnl +dnl CHECK_NAME_ALT: Suffix/prefix used to set additional +dnl variables if alternative check suceeds
s/suceeds/succeeds/
+dnl config.h: WITH_XXX macro +dnl Makefile: WITH_XXX conditional +dnl NB all vars for CHECK_NAME are also set +dnl LIBRARY_NAME_ALT: alternative library name to check for +dnl FUNCTION_NAME_ALT: alternative function name to check for +dnl HEADER_NAME_ALT: alterantive header file to check for
s/alterantive/alternative/
+ m4_pushdef([check_name_lc], m4_translit(check_name, `A-Z', `a-z'))
Again, wrong quote characters, and m4_tolower already does this for you.
+ +dnl +dnl Probe for existance of libXXXX and set WITH_XXX
s/existance/existence/
+AC_DEFUN([LIBVIRT_CHECK_PKG],[ + m4_pushdef([check_name], [$1]) + m4_pushdef([pc_name], [$2]) + m4_pushdef([pc_version], [$3]) + + m4_pushdef([check_name_lc], m4_translit(check_name, `A-Z', `a-z'))
And again.
+ AC_ARG_WITH(check_name_lc, + [AS_HELP_STRING([--arg_var], + [with ]]m4_dquote(pc_name)[[ (>= ]]m4_dquote(pc_version)[[) support @<:@default=check@:>@])],
This is a long line; you can break anywhere after a ( with no impact, to stay within 80 columns, such as ... m4-dquote( pc_version)...
+ +dnl +dnl To be used after a call to LIBVIRT_CHECK_LIB, +dnl LIBVIRT_CHECK_LIB_ALT or LIBVIRT_CHECK_PKG +dnl to print the result status +dnl +dnl LIBVIRT_RESULT_LIB([CHECK_NAME]) +dnl +dnl CHECK_NAME: Suffix/prefix used for variables / flags, in uppercase. +dnl +dnl LIBVIRT_RESULT_LIB([SELINUX]) +dnl +AC_DEFUN([LIBVIRT_RESULT_LIB],[ + m4_pushdef([check_name], [$1]) + + m4_pushdef([check_name_lc], m4_translit(check_name, `A-Z', `a-z'))
One more place for m4_tolower
+++ b/m4/virt-result.m4
+ +AC_DEFUN([LIBVIRT_RESULT], [
No doc comment beforehand? Basically, all of my suggestions for using more m4_defn are for safety against tricky library names, but don't really impact the common case where a library name cannot be confused with an m4 macro. And libvirt doesn't use any libraries with an unsafe name. My priority listing of which things to fix (or ignore): * spelling errors - must fix before committing * use m4_tolower instead of m4_translit - please fix before committing * doc comment for LIBVIRT_RESULT - please fix before committing * avoid m4_expand - up to you, but it looks simpler if you fix to at least libvirt quality * use m4_defn in more places - up to you, and I'll look the other way if you choose not to fix ACK once you cover at least the first bullet, and preferably at least the first 3 bullets; and of course if your testing of later patches in the series shows that things still work. I don't need to see a v2 unless you go for all 5 bullets. (Part of me wonders if I should take the best parts of these macros, make them more robust to quoting, and make it officially part of some future release of autoconf.) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org