On 09/20/2012 09:01 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange@xxxxxxxxxx>
>
> 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
>
> Doing all this correctly is rather difficult, typically
> done by copy+paste of a previous usage. Further small
> improvements people make are not applied to all previous
> usages.
On it's own, this patch merely introduces files that are unused, but it
doesn't make sense to push before the release unless we also take one of
the other commits in the series that uses it.
>
> Improve this by creating some helper macros to apply
> good practice. First, to perform the actual checks:
>
> LIBVIRT_CHECK_LIB([SELINUX],[selinux],[getfilecon][selinux/selinux.h])
Missing a comma. Also, autoconf ignores whitespace after comma, so I'd
write this as:
LIBVIRT_CHECK_LIB([SELINUX], [selinux],
[getfilecon], [selinux/selinux.h])
>
> This checks for 'getfilecon' in libselinux.so, and the
More precisely, in '-lselinux', since this construct works even for
cygwin where such a library [if it existed] would be named cygselinux.dll.
> existance of 'selinux/selinux.h' header file. If successful
s/existance/existence/
> it sets SELINUX_CFLAGS and SELINUX_LIBS. The WITH_SELINUX
> config.h macro and WITH_SELINUX make conditional are also
> defined.
>
> Finally to print a summary of CFLAGS & LIBs found (if any):
>
> LIBVIRT_RESULT_LIB([SELINUX],[selinux])
>
> Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
> ---
> m4/virt-lib.m4 | 217 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> m4/virt-result.m4 | 9 +++
> 2 files changed, 226 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..86a6aa0
> --- /dev/null
> +++ b/m4/virt-lib.m4
> @@ -0,0 +1,217 @@
> +dnl
Missing a copyright header.
> +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([WITH_VAR],[NAME_VAR],[LIBNAME],[FUNCNAME],[HDRNAME])
5 arguments listed here, but only 4 listed in the commit message.
> +dnl
> +dnl WITH_VAR: Suffix for the WITH_XXX variable in config.h & conditional in make,
> +dnl and prefix for the XXX_CFLAGS and XXX_LIBS make variables
> +dnl NAME_VAR: Suffix for the --with-XXX configure arg and $with_XXX configure variable
Will these two values always be the same case-insensitive string? If
so, you only need one of the two of them, and use m4 to do the
case-conversion as part of this macro, using functions such as m4_toupper.
> +dnl LIBNAME: Suffix for the library name libXXX.so (typically same as NAME_VAR)
I'd allow this to be a default, where the user can write:
[NAME_VAR], [],
instead of:
[NAME_VAR], [NAME_VAR],
in the common case (of course, you demonstrated sanlock_client as a
counter-example that warrants the separate arg). To do this, you
replace use of $3 with m4_default([$3], [$2]).
> +dnl FUNCNAME: Name of function to check for in libXXX.so
> +dnl HDRNAME: Name of header file to check for
> +dnl
> +dnl eg
s/eg/e.g./
> +dnl
> +dnl LIBVIRT_CHECK_LIB([SELINUX],[selinux],[selinux],[getfilecon],[selinux/selinux.h])
> +dnl LIBVIRT_CHECK_LIB([SANLOCK],[sanlock],[sanlock_client],[sanlock_init],[sanlock.h])
> +dnl LIBVIRT_CHECK_LIB([LIBATTR],[libattr],[attr],[getxattr],[attr/attr.h])
Long lines. Encourage wrapping at a sensible point.
> +dnl
> +AC_DEFUN([LIBVIRT_CHECK_LIB],[
> + AS_VAR_PUSHDEF([config_var],[WITH_$1])
> + AS_VAR_PUSHDEF([make_var],[WITH_$1])
> + AS_VAR_PUSHDEF([cflags_var],[$1_CFLAGS])
> + AS_VAR_PUSHDEF([libs_var],[$1_LIBS])
> + AS_VAR_PUSHDEF([name_var],[$2])
> + AS_VAR_PUSHDEF([arg_var],[with-$2])
> + AS_VAR_PUSHDEF([with_var],[with_$2])
> + AS_VAR_PUSHDEF([libname_var],[$3])
> + AS_VAR_PUSHDEF([funcname_var],[$4])
> + dnl Does not work - '.' and '/' get changed into '_'
> + dnl AS_VAR_PUSHDEF([hdrname_var],[$5])
> +
> + AC_ARG_WITH([name_var],
> + AC_HELP_STRING([--arg_var],
Missing quoting. You need [] around the second argument of AC_ARG_WITH.
> + [with lib$3 support @<:@default=check@:>@]),[],[with_var][=check])
AC_HELP_STRING is obsolete. Autoconf recommends using AS_HELP_STRING.
Line wrapping is hard to follow; I'd put a line break after the
HELP_STRING() and before the [] third argument of AC_ARG_WITH.
> +
> + old_LIBS="$LIBS"
> + old_CFLAGS="$CFLAGS"
> + AS_VAR_SET([cflags_var],[])
> + AS_VAR_SET([libs_var],[])
> +
> + fail=0
> + if test "$with_var" != "no" ; then
Technically, $with_var is user-supplied, and can therefore start with
text that can confuse (non-compliant) versions of 'test'; it's better in
this case to write test "x$with_var" != "xno" (here, and anywhere else a
shell variable with user-supplied contents is probed). Also, here, you
are directly probing with_var...
> + if test "$with_var" != "yes" && test "$with_var" != "check" ; then
> + AS_VAR_SET([cflags_var],[-I$with_var/include])
> + AS_VAR_SET([libs_var],[-L$with_var/lib])
> + fi
> + CFLAGS="$CFLAGS $cflags_var"
> + LIBS="$LIBS $libs_var"
> + AC_CHECK_LIB([$3], funcname_var, [],[
> + if test "$with_var" != "check"; then
> + fail=1
> + fi
> + AS_VAR_SET([with_var],[no])
> + ])
> + if test "$fail" = "0" && test "$with_var" != "no" ; then
> + AC_CHECK_HEADER([$5], [
> + AS_VAR_SET([with_var],[yes])
...but here you are using the AS_VAR_* wrappers. If you plan on using
the AS_VAR wrappers, then all uses of the variable, including probing
its value, have to go through AS_VAR (that is, $with_var might not be
valid shell code compared to AS_VAR_GET([with_var]), depending on what
the user gave as input to this macro). But it looks like you intend for
this macro to always be used with literal strings rather than shell
indirections, at which point AS_VAR_PUSHDEF/AS_VAR_POPDEF may be
overkill, and you could get away with simpler m4_pushdef/m4_popdef.
> + ],[
> + if test "$with_var" != "check"; then
> + fail=1
> + fi
> + AS_VAR_SET([with_var],[no])
> + ])
> + fi
> + fi
> +
> + LIBS="$old_LIBS"
> + CFLAGS="$old_CFLAGS"
Technically, the "" aren't necessary here, but they don't hurt either.
> +
> + if test $fail = 1; then
> + AC_MSG_ERROR([You must install the lib$3 library & headers to compile libvirt])
> + fi
> +
> + if test "$with_var" = "yes" ; then
> + if test -z "$libs_var" ; then
> + AS_VAR_SET([libs_var],["-l$3"])
> + else
> + AS_VAR_SET([libs_var],["$]libs_var[ -l$3"])
> + fi
> +
> + AC_DEFINE_UNQUOTED(config_var, 1, [whether $3 is available])
> + fi
> +
> + AM_CONDITIONAL(make_var, [test "$with_var" = "yes"])
> +
> + AC_SUBST(cflags_var)
> + AC_SUBST(libs_var)
> +])
> +
> +dnl
> +dnl Probe for existance of libXXXX, or alternatively libYYYY and set WITH_XXX
s/existance/existence/
> +dnl config header var, WITH_XXXX make conditional and with_XXX configure shell
> +dnl var.
> +dnl
> +dnl LIBVIRT_CHECK_LIB_FALLBACK([WITH_VAR],[WITH_VAR_2],[NAME_VAR],
> +dnl [LIBNAME],[LIBNAME2],[FUNCNAME],[FUNCNAME2],[HDRNAME])
A lot of autoconf macros have the style of providing two arguments, one
expanded if the check was successful, and the other if it failed. I'm
wondering if it would be better to have:
LIBVIRT_CHECK_LIB([WITH_VAR], [NAME_VAR], [LIBNAME],
[FUNCNAME], [HDRNAME], [if-found], [if-not-found])
where this macro would not be needed, but you would instead write it as:
LIBVIRT_CHECK_LIB([WITH_VAR], [NAME_VAR], [LIBNAME],
[FUNCNAME], [HDRNAME], [],
[LIBVIRT_CHECK_LIB([WITH_VAR2], [NAME_VAR], [LIBNAME2],
[FUNCNAME2], [HDRNAME])])
> +dnl
> +dnl WITH_VAR: Suffix for the WITH_XXX variable in config.h & conditional in make,
> +dnl and prefix for the XXX_CFLAGS and XXX_LIBS make variables
> +dnl WITH_VAR2: Suffix for the WITH_XXX variable in config.h & conditional in make
> +dnl if the fallback library was required
> +dnl NAME_VAR: Suffix for the --with-XXX configure arg and $with_XXX configure variable
> +dnl LIBNAME: Suffix for the library name libXXX.so (typically same as NAME_VAR)
> +dnl LIBNAME2: Suffix for the library name libYYY.so fallback choice
> +dnl FUNCNAME: Name of function to check for in libXXX.so
> +dnl FUNCNAME2: Name of function to check for in libYYY.so
> +dnl HDRNAME: Name of header file to check for
Why don't we need a HDRNAME2? And should we make it easy to default
contents where any *2 variable left blank has the same value as the
non-2 variable?
> + AS_VAR_PUSHDEF([name_var],[$3])
> + AS_VAR_PUSHDEF([arg_var],[with-$3])
> + AS_VAR_PUSHDEF([with_var],[with_$3])
> + AS_VAR_PUSHDEF([libname_var],[$4])
> + AS_VAR_PUSHDEF([libname2_var],[$5])
> + AS_VAR_PUSHDEF([funcname_var],[$6])
> + AS_VAR_PUSHDEF([funcname2_var],[$7])
> + dnl AS_VAR_PUSHDEF([hdrname_var],[$8])
> +
> + AC_ARG_WITH([name_var],
> + AC_HELP_STRING([--arg_var],
> + [with lib$3 support @<:@default=check@:>@]),[],[with_var][=check])
Same comments as before. Also, here, you could write:
[with lib]with_var[ support ...]
instead of having to look up what $3 meant.
> +
> + if test $fail = 1; then
> + AC_MSG_ERROR([You must install the lib$4 library & headers to compile libvirt])
> + fi
If you do use my suggestion of an if-found/if-not-found argument, then
this would be better as:
if test $fail = 1; then
m4_default([if-not-found-arg], [AC_MSG_ERROR([...])])
fi
so that failure to find the library is fatal only if the macro caller
didn't supply alternate if-not-found code.
> +dnl
> +dnl To be called after a LIBVIRT_CHECK_LIB or LIBVIRT_CHECK_LIB_FALLBACK
> +dnl invocation to print the result status
> +dnl
> +dnl LIBVIRT_RESULT_LIB([WITH_VAR],[NAME_VAR])
> +dnl
> +dnl WITH_VAR: Prefix for the XXX_CFLAGS and XXX_LIBS make variables
> +dnl NAME_VAR: Suffix for the --with-XXX configure arg and $with_XXX configure variable
> +dnl
> +dnl LIBVIRT_RESULT_LIB([SELINUX],[selinux])
> +dnl
> +AC_DEFUN([LIBVIRT_RESULT_LIB],[
> + AS_VAR_PUSHDEF([cflags_var],[$1_CFLAGS])
> + AS_VAR_PUSHDEF([libs_var],[$1_LIBS])
> + AS_VAR_PUSHDEF([name_var],[$2])
> + AS_VAR_PUSHDEF([with_var],[with_$2])
> +
> + LIBVIRT_RESULT(name_var, [$with_var], [CFLAGS=$cflags_var LIBS=$libs_var])
> +])
Missing matching AS_VAR_POPDEF, throughout the patch. Every
AS_VAR_PUSHDEF (or m4_pushdef) needs a paired cleanup within the same
macro definition.
> diff --git a/m4/virt-result.m4 b/m4/virt-result.m4
> new file mode 100644
> index 0000000..c2e1517
> --- /dev/null
> +++ b/m4/virt-result.m4
> @@ -0,0 +1,9 @@
> +AC_DEFUN([LIBVIRT_RESULT], [
Missing a copyright and documentation. Why does this need to be a
separate file?
> + if test "$2" = "no" || test -z "$3" ; then
> + printf -v STR "%8s: %-3s" "$1" "$2"
printf -v is not portable. This has to be written:
STR=`printf "%8s: %-3s" "$1" "$2"`
> + else
> + printf -v STR "%8s: %-3s (%s)" "$1" "$2" "$3"
> + fi
> +
> + AC_MSG_NOTICE([$STR])
> +])
>
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list