[libvirt] [PATCH 0/2] Require YAJL for QEMU build
This also fixes a problem that Cole reported on IRC where qemufirmware test is failing on FreeBSD because QEMU driver is enabled but there's no YAJL. Michal Prívozník (2): virt-yajl.m4: Drop useless check for qemu virt-driver-qemu.m4: Require YAJL m4/virt-driver-qemu.m4 | 9 +++++++++ m4/virt-yajl.m4 | 20 -------------------- 2 files changed, 9 insertions(+), 20 deletions(-) -- 2.19.2
Firstly, this code is lacking AC_REQUIRE() therefore it may happen that this code is executed before LIBVIRT_DRIVER_CHECK_QEMU is evaluated. Secondly, the code tries to detect installed version of qemu to learn if it uses HMP or QMP and enable YAJL based on that. Well, we support only QMP and also minimal required version of qemu is 1.5.0 so the check would have enabled yajl anyway. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-yajl.m4 | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/m4/virt-yajl.m4 b/m4/virt-yajl.m4 index c4ea0102a3..7e905c2f77 100644 --- a/m4/virt-yajl.m4 +++ b/m4/virt-yajl.m4 @@ -23,26 +23,6 @@ AC_DEFUN([LIBVIRT_ARG_YAJL],[ AC_DEFUN([LIBVIRT_CHECK_YAJL],[ dnl YAJL JSON library http://lloyd.github.com/yajl/ - if test "$with_qemu:$with_yajl" = yes:check; then - dnl Some versions of qemu require the use of yajl; try to detect them - dnl here, although we do not require qemu to exist in order to compile. - dnl This check mirrors src/qemu/qemu_capabilities.c - AC_PATH_PROGS([QEMU], [qemu-kvm qemu kvm qemu-system-x86_64], - [], [$PATH:/usr/bin:/usr/libexec]) - if test -x "$QEMU"; then - if $QEMU -help 2>/dev/null | grep -q libvirt; then - with_yajl=yes - else - [qemu_version_sed='s/.*ersion \([0-9.,]*\).*/\1/'] - qemu_version=`$QEMU -version | sed "$qemu_version_sed"` - case $qemu_version in - [[1-9]].* | 0.15.* ) with_yajl=yes ;; - 0.* | '' ) ;; - *) AC_MSG_ERROR([Unexpected qemu version string]) ;; - esac - fi - fi - fi LIBVIRT_CHECK_LIB_ALT([YAJL], [yajl], [yajl_parse_complete], [yajl/yajl_common.h], -- 2.19.2
On Wed, Mar 13, 2019 at 04:50:37PM +0100, Michal Privoznik wrote:
Firstly, this code is lacking AC_REQUIRE() therefore it may happen that this code is executed before LIBVIRT_DRIVER_CHECK_QEMU is evaluated.
Irrelevant. LIBVIRT_DRIVER_CHECK_QEMU does not actually change the with_qemu variable. It only depends on LIBVIRT_DRIVER_ARG_QEMU to set the default 'yes' or get the value from the command line.
Secondly, the code tries to detect installed version of qemu to learn if it uses HMP or QMP and enable YAJL based on that. Well, we support only QMP and also minimal required version of qemu is 1.5.0 so the check would have enabled yajl anyway.
IOW after this patch, not having YAJL is no longer fatal with the default ./configure command line on systems with a QEMU binary.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-yajl.m4 | 20 -------------------- 1 file changed, 20 deletions(-)
With the irrelevant part of the commit message dropped: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
There is not way that qemu driver can work without being able to format/parse JSON. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-driver-qemu.m4 | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4 index 80e1d3ad46..af8edaae0d 100644 --- a/m4/virt-driver-qemu.m4 +++ b/m4/virt-driver-qemu.m4 @@ -26,6 +26,15 @@ AC_DEFUN([LIBVIRT_DRIVER_ARG_QEMU], [ ]) AC_DEFUN([LIBVIRT_DRIVER_CHECK_QEMU], [ + dnl There is no way qemu driver will work without JSON support + AC_REQUIRE([LIBVIRT_CHECK_YAJL]) + if test "$with_qemu:$with_yajl" = "yes:no"; then + AC_MSG_ERROR([YAJL or YAJL2 is required to build QEMU driver]) + fi + if test "$with_qemu" = "check"; then + with_qemu=$with_yajl + fi + if test "$with_qemu" = "yes" ; then AC_DEFINE_UNQUOTED([WITH_QEMU], 1, [whether QEMU driver is enabled]) fi -- 2.19.2
On Wed, Mar 13, 2019 at 04:50:38PM +0100, Michal Privoznik wrote:
There is not way that qemu driver can work without being able to
s/not/no/
format/parse JSON.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-driver-qemu.m4 | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4 index 80e1d3ad46..af8edaae0d 100644 --- a/m4/virt-driver-qemu.m4 +++ b/m4/virt-driver-qemu.m4 @@ -26,6 +26,15 @@ AC_DEFUN([LIBVIRT_DRIVER_ARG_QEMU], [ ])
AC_DEFUN([LIBVIRT_DRIVER_CHECK_QEMU], [ + dnl There is no way qemu driver will work without JSON support + AC_REQUIRE([LIBVIRT_CHECK_YAJL]) + if test "$with_qemu:$with_yajl" = "yes:no"; then + AC_MSG_ERROR([YAJL or YAJL2 is required to build QEMU driver]) + fi + if test "$with_qemu" = "check"; then + with_qemu=$with_yajl + fi +
Note that the default for with_qemu is still 'yes', so developers would have to opt-in by passing --with-qemu=check Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Thu, Mar 14, 2019 at 11:41:31AM +0100, Ján Tomko wrote:
On Wed, Mar 13, 2019 at 04:50:38PM +0100, Michal Privoznik wrote:
There is not way that qemu driver can work without being able to
s/not/no/
format/parse JSON.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-driver-qemu.m4 | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4 index 80e1d3ad46..af8edaae0d 100644 --- a/m4/virt-driver-qemu.m4 +++ b/m4/virt-driver-qemu.m4 @@ -26,6 +26,15 @@ AC_DEFUN([LIBVIRT_DRIVER_ARG_QEMU], [ ])
AC_DEFUN([LIBVIRT_DRIVER_CHECK_QEMU], [ + dnl There is no way qemu driver will work without JSON support + AC_REQUIRE([LIBVIRT_CHECK_YAJL]) + if test "$with_qemu:$with_yajl" = "yes:no"; then + AC_MSG_ERROR([YAJL or YAJL2 is required to build QEMU driver]) + fi + if test "$with_qemu" = "check"; then + with_qemu=$with_yajl + fi +
Note that the default for with_qemu is still 'yes', so developers would have to opt-in by passing --with-qemu=check
IMHO we must fix that before we push this. We aim to have our default configure setup automatically probe things - raising a hard error by default when yajl is missing is not desirable unless the user has explicitly given --with-qemu=yes.
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The basic idea of our configure script is to probe for things rather than have them enabled by default. This is even more visible in the next commit where configure fails if qemu driver is enabled but no yajl is found. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-driver-qemu.m4 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4 index 80e1d3ad46..2912b3c462 100644 --- a/m4/virt-driver-qemu.m4 +++ b/m4/virt-driver-qemu.m4 @@ -18,7 +18,7 @@ dnl <http://www.gnu.org/licenses/>. dnl AC_DEFUN([LIBVIRT_DRIVER_ARG_QEMU], [ - LIBVIRT_ARG_WITH_FEATURE([QEMU], [QEMU/KVM], [yes]) + LIBVIRT_ARG_WITH_FEATURE([QEMU], [QEMU/KVM], [check]) LIBVIRT_ARG_WITH([QEMU_USER], [username to run QEMU system instance as], ['platform dependent']) LIBVIRT_ARG_WITH([QEMU_GROUP], [groupname to run QEMU system instance as], -- 2.19.2
On Thu, Mar 14, 2019 at 13:36:26 +0100, Michal Privoznik wrote:
The basic idea of our configure script is to probe for things rather than have them enabled by default. This is even more visible in the next commit where configure fails if qemu driver is enabled but no yajl is found.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- m4/virt-driver-qemu.m4 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4 index 80e1d3ad46..2912b3c462 100644 --- a/m4/virt-driver-qemu.m4 +++ b/m4/virt-driver-qemu.m4 @@ -18,7 +18,7 @@ dnl <http://www.gnu.org/licenses/>. dnl
AC_DEFUN([LIBVIRT_DRIVER_ARG_QEMU], [ - LIBVIRT_ARG_WITH_FEATURE([QEMU], [QEMU/KVM], [yes]) + LIBVIRT_ARG_WITH_FEATURE([QEMU], [QEMU/KVM], [check]) LIBVIRT_ARG_WITH([QEMU_USER], [username to run QEMU system instance as], ['platform dependent']) LIBVIRT_ARG_WITH([QEMU_GROUP], [groupname to run QEMU system instance as],
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
participants (4)
-
Daniel P. Berrangé -
Jiri Denemark -
Ján Tomko -
Michal Privoznik