[libvirt] [PATCH 00/10] build: drop some old libs and relax GNU strictness
While testing these changes I found that the build is broken with old rbd on (unsupported) Debian 8. Fix the build by dropping support for old rbd. Ján Tomko (10): rbd: depend on diff_iterate2 rbd: drop support for ceph 0.94 build: drop check for SASL1 build: require yajl >= 2.0.3 json: assume WITH_YAJL2 build: remove WITH_YAJL2 virJSONValueToString: bail out early on error configure: split AM_INIT_AUTOMAKE into multiple lines configure.ac: drop -Wno-obsolete from AM_INIT_AUTOMAKE configure.ac: add foreign to AM_INIT_AUTOMAKE config-post.h | 1 - configure.ac | 12 ++++-- m4/virt-driver-qemu.m4 | 2 +- m4/virt-sasl.m4 | 5 +-- m4/virt-storage-rbd.m4 | 2 +- m4/virt-yajl.m4 | 5 +-- src/storage/storage_backend_rbd.c | 39 ------------------- src/util/virjson.c | 63 +++---------------------------- 8 files changed, 19 insertions(+), 110 deletions(-) -- 2.19.2
This function was added by commit 6d5b969d4206208e91ca807b09aac75f2f428393 librbd: add diff_iterate2 to API git describe: v0.94-1009-g6d5b969d42 contains: v9.0.1~55^2~27 Effectively bump the required version to 9.0.1 now that we no longer try to support Debian 8. Ubuntu 16.04 has 10.2.11: https://repology.org/project/ceph/versions Signed-off-by: Ján Tomko <jtomko@redhat.com> --- m4/virt-storage-rbd.m4 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/m4/virt-storage-rbd.m4 b/m4/virt-storage-rbd.m4 index f3d9d04908..a54cdb2cd1 100644 --- a/m4/virt-storage-rbd.m4 +++ b/m4/virt-storage-rbd.m4 @@ -32,7 +32,7 @@ AC_DEFUN([LIBVIRT_STORAGE_CHECK_RBD], [ old_LIBS="$LIBS" LIBS="$LIBS $LIBRBD_LIBS" - AC_CHECK_FUNCS([rbd_get_features],[],[LIBRBD_FOUND=no]) + AC_CHECK_FUNCS([rbd_diff_iterate2],[],[LIBRBD_FOUND=no]) AC_CHECK_FUNCS([rbd_list2]) LIBS="$old_LIBS" fi -- 2.19.2
On Wed, Apr 03, 2019 at 07:40:16AM +0200, Ján Tomko wrote:
This function was added by commit 6d5b969d4206208e91ca807b09aac75f2f428393 librbd: add diff_iterate2 to API git describe: v0.94-1009-g6d5b969d42 contains: v9.0.1~55^2~27
Effectively bump the required version to 9.0.1 now that we no longer try to support Debian 8.
Ubuntu 16.04 has 10.2.11: https://repology.org/project/ceph/versions
This version only arrived in RHEL-7.6. We've not said whether our support policy applies to only the version latest update or RHEL or not. To me though it feels too soon to mandate RHEL-7.6 only as people are generally pretty slow to update to latest releases.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- m4/virt-storage-rbd.m4 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/m4/virt-storage-rbd.m4 b/m4/virt-storage-rbd.m4 index f3d9d04908..a54cdb2cd1 100644 --- a/m4/virt-storage-rbd.m4 +++ b/m4/virt-storage-rbd.m4 @@ -32,7 +32,7 @@ AC_DEFUN([LIBVIRT_STORAGE_CHECK_RBD], [
old_LIBS="$LIBS" LIBS="$LIBS $LIBRBD_LIBS" - AC_CHECK_FUNCS([rbd_get_features],[],[LIBRBD_FOUND=no]) + AC_CHECK_FUNCS([rbd_diff_iterate2],[],[LIBRBD_FOUND=no]) AC_CHECK_FUNCS([rbd_list2]) LIBS="$old_LIBS" fi -- 2.19.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
On Wed, Apr 03, 2019 at 09:40:14 +0100, Daniel Berrange wrote:
On Wed, Apr 03, 2019 at 07:40:16AM +0200, Ján Tomko wrote:
This function was added by commit 6d5b969d4206208e91ca807b09aac75f2f428393 librbd: add diff_iterate2 to API git describe: v0.94-1009-g6d5b969d42 contains: v9.0.1~55^2~27
Effectively bump the required version to 9.0.1 now that we no longer try to support Debian 8.
Ubuntu 16.04 has 10.2.11: https://repology.org/project/ceph/versions
This version only arrived in RHEL-7.6. We've not said whether our support policy applies to only the version latest update or RHEL or not. To me though it feels too soon to mandate RHEL-7.6 only as people are generally pretty slow to update to latest releases.
Are people who are slow to update which is mostly to avoid breaking production systems really going to run upstream libvirtd?
On Wed, Apr 03, 2019 at 10:48:25AM +0200, Peter Krempa wrote:
On Wed, Apr 03, 2019 at 09:40:14 +0100, Daniel Berrange wrote:
On Wed, Apr 03, 2019 at 07:40:16AM +0200, Ján Tomko wrote:
This function was added by commit 6d5b969d4206208e91ca807b09aac75f2f428393 librbd: add diff_iterate2 to API git describe: v0.94-1009-g6d5b969d42 contains: v9.0.1~55^2~27
Effectively bump the required version to 9.0.1 now that we no longer try to support Debian 8.
Ubuntu 16.04 has 10.2.11: https://repology.org/project/ceph/versions
This version only arrived in RHEL-7.6. We've not said whether our support policy applies to only the version latest update or RHEL or not. To me though it feels too soon to mandate RHEL-7.6 only as people are generally pretty slow to update to latest releases.
Are people who are slow to update which is mostly to avoid breaking production systems really going to run upstream libvirtd?
Our platform support policy isn't merely about what hosts people can run libvirt on, it is also about what developers contributing to libvirt can use. Corporate developers cannot be assumed to have freedom to choose their development platform - RHEL-7 may well be the only platform they can use. I don't see a serious downside to keeping this ceph support around for longer, especially given that the new code was only just added a week ago & is easy to fix the mistake mentioned in the next patch 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 :|
On Wed, Apr 03, 2019 at 10:00:18 +0100, Daniel Berrange wrote:
On Wed, Apr 03, 2019 at 10:48:25AM +0200, Peter Krempa wrote:
On Wed, Apr 03, 2019 at 09:40:14 +0100, Daniel Berrange wrote:
On Wed, Apr 03, 2019 at 07:40:16AM +0200, Ján Tomko wrote:
This function was added by commit 6d5b969d4206208e91ca807b09aac75f2f428393 librbd: add diff_iterate2 to API git describe: v0.94-1009-g6d5b969d42 contains: v9.0.1~55^2~27
Effectively bump the required version to 9.0.1 now that we no longer try to support Debian 8.
Ubuntu 16.04 has 10.2.11: https://repology.org/project/ceph/versions
This version only arrived in RHEL-7.6. We've not said whether our support policy applies to only the version latest update or RHEL or not. To me though it feels too soon to mandate RHEL-7.6 only as people are generally pretty slow to update to latest releases.
Are people who are slow to update which is mostly to avoid breaking production systems really going to run upstream libvirtd?
Our platform support policy isn't merely about what hosts people can run libvirt on, it is also about what developers contributing to libvirt can use. Corporate developers cannot be assumed to have freedom to choose their development platform - RHEL-7 may well be the only platform they can use.
I think we should add a configure time check that advices them to look for a better job. Oh wait I'm 2 days late for such a thing probably.
Commit 21deeaf02fdf216b08210fc899579736973ca81d added a stub for volStorageBackendRBDGetFlags without marking the arguments as unused, thus breaking the build. Now that we require diff_iterate2 to be present, remove all the conditionally compiled code, including the stub function. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/storage/storage_backend_rbd.c | 39 ------------------------------- 1 file changed, 39 deletions(-) diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 61ebb9b541..6fea8fe8c7 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -436,7 +436,6 @@ volStorageBackendRBDGetFeatures(rbd_image_t image, return ret; } -#if LIBRBD_VERSION_CODE > 265 static int volStorageBackendRBDGetFlags(rbd_image_t image, const char *volname, @@ -500,31 +499,6 @@ virStorageBackendRBDSetAllocation(virStorageVolDefPtr vol, return ret; } -#else -static int -volStorageBackendRBDGetFlags(rbd_image_t image, - const char *volname, - uint64_t *flags) -{ - *flags = 0; - return 0; -} - -static int -volStorageBackendRBDUseFastDiff(uint64_t features ATTRIBUTE_UNUSED, - uint64_t feature_flags ATTRIBUTE_UNUSED) -{ - return false; -} - -static int -virStorageBackendRBDSetAllocation(virStorageVolDefPtr vol ATTRIBUTE_UNUSED, - rbd_image_t *image ATTRIBUTE_UNUSED, - rbd_image_info_t *info ATTRIBUTE_UNUSED) -{ - return false; -} -#endif static int volStorageBackendRBDRefreshVolInfo(virStorageVolDefPtr vol, @@ -1087,21 +1061,8 @@ virStorageBackendRBDSnapshotFindNoDiff(rbd_image_t image, /* The callback will set diff to non-zero if there is a diff */ diff = 0; -/* - * rbd_diff_iterate2() is available in versions above Ceph 0.94 (Hammer) - * It uses a object map inside Ceph which is faster than rbd_diff_iterate() - * which iterates all objects. - * LIBRBD_VERSION_CODE for Ceph 0.94 is 265. In 266 and upwards diff_iterate2 - * is available - */ -#if LIBRBD_VERSION_CODE > 265 r = rbd_diff_iterate2(image, snaps[i].name, 0, info.size, 0, 1, virStorageBackendRBDIterateCb, (void *)&diff); -#else - r = rbd_diff_iterate(image, snaps[i].name, 0, info.size, - virStorageBackendRBDIterateCb, (void *)&diff); -#endif - if (r < 0) { virReportSystemError(-r, _("failed to iterate RBD snapshot %s@%s"), imgname, snaps[i].name); -- 2.19.2
On Wed, Apr 03, 2019 at 07:40:17AM +0200, Ján Tomko wrote:
Commit 21deeaf02fdf216b08210fc899579736973ca81d added a stub for volStorageBackendRBDGetFlags without marking the arguments as unused, thus breaking the build.
IMHO we should just add the attribute annotation for fix the warning 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 :|
Even Debian 8 which we no longer support has 2.1.26. https://repology.org/project/cyrus-sasl/versions Signed-off-by: Ján Tomko <jtomko@redhat.com> --- m4/virt-sasl.m4 | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/m4/virt-sasl.m4 b/m4/virt-sasl.m4 index 6ef24fd4b9..7ab9b2c368 100644 --- a/m4/virt-sasl.m4 +++ b/m4/virt-sasl.m4 @@ -22,10 +22,7 @@ AC_DEFUN([LIBVIRT_ARG_SASL],[ ]) AC_DEFUN([LIBVIRT_CHECK_SASL],[ - LIBVIRT_CHECK_LIB_ALT([SASL], [sasl2], - [sasl_client_init], [sasl/sasl.h], - [SASL1], [sasl], - [sasl_client_init], [sasl/sasl.h]) + LIBVIRT_CHECK_LIB([SASL], [sasl2], [sasl_client_init], [sasl/sasl.h]) ]) AC_DEFUN([LIBVIRT_RESULT_SASL],[ -- 2.19.2
On Wed, Apr 03, 2019 at 07:40:18AM +0200, Ján Tomko wrote:
Even Debian 8 which we no longer support has 2.1.26.
https://repology.org/project/cyrus-sasl/versions
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- m4/virt-sasl.m4 | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/m4/virt-sasl.m4 b/m4/virt-sasl.m4 index 6ef24fd4b9..7ab9b2c368 100644 --- a/m4/virt-sasl.m4 +++ b/m4/virt-sasl.m4 @@ -22,10 +22,7 @@ AC_DEFUN([LIBVIRT_ARG_SASL],[ ])
AC_DEFUN([LIBVIRT_CHECK_SASL],[ - LIBVIRT_CHECK_LIB_ALT([SASL], [sasl2], - [sasl_client_init], [sasl/sasl.h], - [SASL1], [sasl], - [sasl_client_init], [sasl/sasl.h])
IIRC, this was some wierdness with Solaris which called the SASL2 library libsasl instead of libsasl2. Anyway we don't support Solaris so it doesn't matter anymore.
+ LIBVIRT_CHECK_LIB([SASL], [sasl2], [sasl_client_init], [sasl/sasl.h]) ])
AC_DEFUN([LIBVIRT_RESULT_SASL],[ -- 2.19.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 :|
On Wed, Apr 03, 2019 at 09:32:10AM +0100, Daniel P. Berrangé wrote:
On Wed, Apr 03, 2019 at 07:40:18AM +0200, Ján Tomko wrote:
Even Debian 8 which we no longer support has 2.1.26.
https://repology.org/project/cyrus-sasl/versions
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- m4/virt-sasl.m4 | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/m4/virt-sasl.m4 b/m4/virt-sasl.m4 index 6ef24fd4b9..7ab9b2c368 100644 --- a/m4/virt-sasl.m4 +++ b/m4/virt-sasl.m4 @@ -22,10 +22,7 @@ AC_DEFUN([LIBVIRT_ARG_SASL],[ ])
AC_DEFUN([LIBVIRT_CHECK_SASL],[ - LIBVIRT_CHECK_LIB_ALT([SASL], [sasl2], - [sasl_client_init], [sasl/sasl.h], - [SASL1], [sasl], - [sasl_client_init], [sasl/sasl.h])
IIRC, this was some wierdness with Solaris which called the SASL2 library libsasl instead of libsasl2. Anyway we don't support Solaris so it doesn't matter anymore.
Opps, meant to say Reviewed-by: Daniel P. Berrangé <berrange@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 pkg-config file was introduced by commit b729ded which was released in yajl 2.0.3. Since all our supported platforms include at least yajl 2.0.4, use pkg-config to detect the library and set the minimum to 2.0.3. https://repology.org/project/yajl/versions Signed-off-by: Ján Tomko <jtomko@redhat.com> --- m4/virt-driver-qemu.m4 | 2 +- m4/virt-yajl.m4 | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4 index 2934098cfd..0daf62b88e 100644 --- a/m4/virt-driver-qemu.m4 +++ b/m4/virt-driver-qemu.m4 @@ -29,7 +29,7 @@ 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]) + AC_MSG_ERROR([YAJL is required to build QEMU driver]) fi if test "$with_qemu" = "check"; then with_qemu=$with_yajl diff --git a/m4/virt-yajl.m4 b/m4/virt-yajl.m4 index 7e905c2f77..dfc920d1f2 100644 --- a/m4/virt-yajl.m4 +++ b/m4/virt-yajl.m4 @@ -24,10 +24,8 @@ AC_DEFUN([LIBVIRT_ARG_YAJL],[ AC_DEFUN([LIBVIRT_CHECK_YAJL],[ dnl YAJL JSON library http://lloyd.github.com/yajl/ - LIBVIRT_CHECK_LIB_ALT([YAJL], [yajl], - [yajl_parse_complete], [yajl/yajl_common.h], - [YAJL2], [yajl], - [yajl_tree_parse], [yajl/yajl_common.h]) + LIBVIRT_CHECK_PKG([YAJL], [yajl], [2.0.3]) + AC_DEFINE_UNQUOTED([WITH_YAJL2], 1, [whether YAJL2 is enabled]) ]) AC_DEFUN([LIBVIRT_RESULT_YAJL],[ -- 2.19.2
On Wed, Apr 03, 2019 at 07:40:19AM +0200, Ján Tomko wrote:
The pkg-config file was introduced by commit b729ded which was released in yajl 2.0.3.
Since all our supported platforms include at least yajl 2.0.4, use pkg-config to detect the library and set the minimum to 2.0.3.
https://repology.org/project/yajl/versions
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- m4/virt-driver-qemu.m4 | 2 +- m4/virt-yajl.m4 | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/m4/virt-driver-qemu.m4 b/m4/virt-driver-qemu.m4 index 2934098cfd..0daf62b88e 100644 --- a/m4/virt-driver-qemu.m4 +++ b/m4/virt-driver-qemu.m4 @@ -29,7 +29,7 @@ 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]) + AC_MSG_ERROR([YAJL is required to build QEMU driver])
s/YAYL/YAJL 2/
fi if test "$with_qemu" = "check"; then with_qemu=$with_yajl diff --git a/m4/virt-yajl.m4 b/m4/virt-yajl.m4 index 7e905c2f77..dfc920d1f2 100644 --- a/m4/virt-yajl.m4 +++ b/m4/virt-yajl.m4 @@ -24,10 +24,8 @@ AC_DEFUN([LIBVIRT_ARG_YAJL],[ AC_DEFUN([LIBVIRT_CHECK_YAJL],[ dnl YAJL JSON library http://lloyd.github.com/yajl/
- LIBVIRT_CHECK_LIB_ALT([YAJL], [yajl], - [yajl_parse_complete], [yajl/yajl_common.h], - [YAJL2], [yajl], - [yajl_tree_parse], [yajl/yajl_common.h]) + LIBVIRT_CHECK_PKG([YAJL], [yajl], [2.0.3]) + AC_DEFINE_UNQUOTED([WITH_YAJL2], 1, [whether YAJL2 is enabled]) ])
AC_DEFUN([LIBVIRT_RESULT_YAJL],[
Reviewed-by: Daniel P. Berrangé <berrange@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 :|
On Wed, Apr 03, Ján Tomko wrote:
Since all our supported platforms include at least yajl 2.0.4, use pkg-config to detect the library and set the minimum to 2.0.3.
In case SLE_12 is on the list of supported platforms, this kicked it off of the list. SLE_12 (and Leap 42.3) comes with version 2.0.1. SLE_15 has version 2.1.0. Olaf
On Wed, May 08, 2019 at 06:21:21PM +0200, Olaf Hering wrote:
On Wed, Apr 03, Ján Tomko wrote:
Since all our supported platforms include at least yajl 2.0.4, use pkg-config to detect the library and set the minimum to 2.0.3.
In case SLE_12 is on the list of supported platforms, this kicked it off of the list. SLE_12 (and Leap 42.3) comes with version 2.0.1. SLE_15 has version 2.1.0.
Yes, this is a mistake. https://libvirt.org/platforms.html "For distributions with long-lifetime releases, the project will aim to support the most recent major version at all times. Support for the previous major version will be dropped 2 years after the new major version is released. For the purposes of identifying supported software versions, the project will look at RHEL, Debian, Ubuntu LTS, and SLES distros. " SLE 15 was released July 2018, so we should be continuing to support SLE 12 until July 2020. So we need to fix this to allow fallback to the non-pkg-config detection for yajl. 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 :|
On Wed, 2019-05-08 at 17:29 +0100, Daniel P. Berrangé wrote:
On Wed, May 08, 2019 at 06:21:21PM +0200, Olaf Hering wrote:
On Wed, Apr 03, Ján Tomko wrote:
Since all our supported platforms include at least yajl 2.0.4, use pkg-config to detect the library and set the minimum to 2.0.3.
In case SLE_12 is on the list of supported platforms, this kicked it off of the list. SLE_12 (and Leap 42.3) comes with version 2.0.1. SLE_15 has version 2.1.0.
Yes, this is a mistake.
https://libvirt.org/platforms.html
"For distributions with long-lifetime releases, the project will aim to support the most recent major version at all times. Support for the previous major version will be dropped 2 years after the new major version is released. For the purposes of identifying supported software versions, the project will look at RHEL, Debian, Ubuntu LTS, and SLES distros. "
SLE 15 was released July 2018, so we should be continuing to support SLE 12 until July 2020.
So we need to fix this to allow fallback to the non-pkg-config detection for yajl.
Well that's a bummer, but at least we don't have to re-introduce support for YAJL 1 - just fix YAJL 2 detection. We should really add support for SLES, or whatever its CentOS equivalent is, to libvirt-jenkins-ci, though. -- Andrea Bolognani / Red Hat / Virtualization
On Thu, May 09, 2019 at 10:30:25AM +0200, Andrea Bolognani wrote:
On Wed, 2019-05-08 at 17:29 +0100, Daniel P. Berrangé wrote:
On Wed, May 08, 2019 at 06:21:21PM +0200, Olaf Hering wrote:
On Wed, Apr 03, Ján Tomko wrote:
Since all our supported platforms include at least yajl 2.0.4, use pkg-config to detect the library and set the minimum to 2.0.3.
In case SLE_12 is on the list of supported platforms, this kicked it off of the list. SLE_12 (and Leap 42.3) comes with version 2.0.1. SLE_15 has version 2.1.0.
Oops, so 'Leap 42.3' is the one I should be looking at to see what's in SLE_12? https://repology.org/project/yajl/versions I will look into it after lunch. Jano
Yes, this is a mistake.
https://libvirt.org/platforms.html
"For distributions with long-lifetime releases, the project will aim to support the most recent major version at all times. Support for the previous major version will be dropped 2 years after the new major version is released. For the purposes of identifying supported software versions, the project will look at RHEL, Debian, Ubuntu LTS, and SLES distros. "
SLE 15 was released July 2018, so we should be continuing to support SLE 12 until July 2020.
So we need to fix this to allow fallback to the non-pkg-config detection for yajl.
Well that's a bummer, but at least we don't have to re-introduce support for YAJL 1 - just fix YAJL 2 detection.
We should really add support for SLES, or whatever its CentOS equivalent is, to libvirt-jenkins-ci, though.
-- Andrea Bolognani / Red Hat / Virtualization
On Thu, May 09, 2019 at 10:40:51AM +0200, Ján Tomko wrote:
On Thu, May 09, 2019 at 10:30:25AM +0200, Andrea Bolognani wrote:
On Wed, 2019-05-08 at 17:29 +0100, Daniel P. Berrangé wrote:
On Wed, May 08, 2019 at 06:21:21PM +0200, Olaf Hering wrote:
On Wed, Apr 03, Ján Tomko wrote:
Since all our supported platforms include at least yajl 2.0.4, use pkg-config to detect the library and set the minimum to 2.0.3.
In case SLE_12 is on the list of supported platforms, this kicked it off of the list. SLE_12 (and Leap 42.3) comes with version 2.0.1. SLE_15 has version 2.1.0.
Oops, so 'Leap 42.3' is the one I should be looking at to see what's in SLE_12? https://repology.org/project/yajl/versions
Yes, there's an explanation of the versioning scheme here: https://lists.opensuse.org/opensuse-project/2017-04/msg00014.html openSUSE Leap 42.3 is eqiuv to SLE 12 sp3 So we can use Leap versions in repoology to track requirements for SLE, which I didn't realize was possible in the past. 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 :|
On Thu, May 09, 2019 at 10:30:25AM +0200, Andrea Bolognani wrote:
On Wed, 2019-05-08 at 17:29 +0100, Daniel P. Berrangé wrote:
On Wed, May 08, 2019 at 06:21:21PM +0200, Olaf Hering wrote:
On Wed, Apr 03, Ján Tomko wrote:
Since all our supported platforms include at least yajl 2.0.4, use pkg-config to detect the library and set the minimum to 2.0.3.
In case SLE_12 is on the list of supported platforms, this kicked it off of the list. SLE_12 (and Leap 42.3) comes with version 2.0.1. SLE_15 has version 2.1.0.
Yes, this is a mistake.
https://libvirt.org/platforms.html
"For distributions with long-lifetime releases, the project will aim to support the most recent major version at all times. Support for the previous major version will be dropped 2 years after the new major version is released. For the purposes of identifying supported software versions, the project will look at RHEL, Debian, Ubuntu LTS, and SLES distros. "
SLE 15 was released July 2018, so we should be continuing to support SLE 12 until July 2020.
So we need to fix this to allow fallback to the non-pkg-config detection for yajl.
Well that's a bummer, but at least we don't have to re-introduce support for YAJL 1 - just fix YAJL 2 detection.
We should really add support for SLES, or whatever its CentOS equivalent is, to libvirt-jenkins-ci, though.
Yeah, openSUSE Leap is the one we'd want to add support for. 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 :|
Now that we require YAJL2, drop the code dealing with YAJL 1. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virjson.c | 55 +++------------------------------------------- 1 file changed, 3 insertions(+), 52 deletions(-) diff --git a/src/util/virjson.c b/src/util/virjson.c index d5d66f879f..b163d5c1cb 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -33,16 +33,8 @@ # include <yajl/yajl_gen.h> # include <yajl/yajl_parse.h> -# ifdef WITH_YAJL2 -# define yajl_size_t size_t -# define VIR_YAJL_STATUS_OK(status) ((status) == yajl_status_ok) -# else -# define yajl_size_t unsigned int -# define yajl_complete_parse yajl_parse_complete -# define VIR_YAJL_STATUS_OK(status) \ - ((status) == yajl_status_ok || (status) == yajl_status_insufficient_data) -# endif - +# define yajl_size_t size_t +# define VIR_YAJL_STATUS_OK(status) ((status) == yajl_status_ok) #endif /* XXX fixme */ @@ -1819,41 +1811,18 @@ virJSONValueFromString(const char *jsonstring) virJSONValuePtr ret = NULL; int rc; size_t len = strlen(jsonstring); -# ifndef WITH_YAJL2 - yajl_parser_config cfg = { 0, 1 }; /* Match yajl 2 default behavior */ - VIR_AUTOPTR(virJSONValue) tmp = NULL; -# endif VIR_DEBUG("string=%s", jsonstring); -# ifdef WITH_YAJL2 hand = yajl_alloc(&parserCallbacks, NULL, &parser); -# else - hand = yajl_alloc(&parserCallbacks, &cfg, NULL, &parser); -# endif if (!hand) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to create JSON parser")); goto cleanup; } - /* Yajl 2 is nice enough to default to rejecting trailing garbage. - * Yajl 1.0.12 has yajl_get_bytes_consumed to make that detection - * simpler. But we're stuck with yajl 1.0.7 on RHEL 6, which - * happily quits parsing at the end of a valid JSON construct, - * with no visibility into how much more input remains. Wrapping - * things in an array forces yajl to confess the truth. */ -# ifdef WITH_YAJL2 + /* Yajl 2 is nice enough to default to rejecting trailing garbage. */ rc = yajl_parse(hand, (const unsigned char *)jsonstring, len); -# else - rc = yajl_parse(hand, (const unsigned char *)"[", 1); - parser.wrap = 1; - if (VIR_YAJL_STATUS_OK(rc)) - rc = yajl_parse(hand, (const unsigned char *)jsonstring, len); - parser.wrap = 0; - if (VIR_YAJL_STATUS_OK(rc)) - rc = yajl_parse(hand, (const unsigned char *)"]", 1); -# endif if (!VIR_YAJL_STATUS_OK(rc) || yajl_complete_parse(hand) != yajl_status_ok) { unsigned char *errstr = yajl_get_error(hand, 1, @@ -1875,17 +1844,6 @@ virJSONValueFromString(const char *jsonstring) virJSONValueFree(parser.head); } else { ret = parser.head; -# ifndef WITH_YAJL2 - /* Undo the array wrapping above */ - tmp = ret; - ret = NULL; - if (virJSONValueArraySize(tmp) > 1) - virReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse json %s: too many items present"), - jsonstring); - else - ret = virJSONValueArraySteal(tmp, 0); -# endif } cleanup: @@ -1977,22 +1935,15 @@ virJSONValueToString(virJSONValuePtr object, const unsigned char *str; char *ret = NULL; yajl_size_t len; -# ifndef WITH_YAJL2 - yajl_gen_config conf = { pretty ? 1 : 0, pretty ? " " : " "}; -# endif VIR_DEBUG("object=%p", object); -# ifdef WITH_YAJL2 g = yajl_gen_alloc(NULL); if (g) { yajl_gen_config(g, yajl_gen_beautify, pretty ? 1 : 0); yajl_gen_config(g, yajl_gen_indent_string, pretty ? " " : " "); yajl_gen_config(g, yajl_gen_validate_utf8, 1); } -# else - g = yajl_gen_alloc(&conf, NULL); -# endif if (!g) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to create JSON formatter")); -- 2.19.2
On Wed, Apr 03, 2019 at 07:40:20AM +0200, Ján Tomko wrote:
Now that we require YAJL2, drop the code dealing with YAJL 1.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virjson.c | 55 +++------------------------------------------- 1 file changed, 3 insertions(+), 52 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c index d5d66f879f..b163d5c1cb 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -33,16 +33,8 @@ # include <yajl/yajl_gen.h> # include <yajl/yajl_parse.h>
-# ifdef WITH_YAJL2 -# define yajl_size_t size_t -# define VIR_YAJL_STATUS_OK(status) ((status) == yajl_status_ok) -# else -# define yajl_size_t unsigned int -# define yajl_complete_parse yajl_parse_complete -# define VIR_YAJL_STATUS_OK(status) \ - ((status) == yajl_status_ok || (status) == yajl_status_insufficient_data) -# endif - +# define yajl_size_t size_t +# define VIR_YAJL_STATUS_OK(status) ((status) == yajl_status_ok)
IMHO we should be getting rid of these two macros too as they only exist to deal with portability differences, though I'm fine if that's done in a separate commit. Reviewed-by: Daniel P. Berrangé <berrange@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 :|
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- config-post.h | 1 - m4/virt-yajl.m4 | 1 - 2 files changed, 2 deletions(-) diff --git a/config-post.h b/config-post.h index 54731feff7..093f84a7ce 100644 --- a/config-post.h +++ b/config-post.h @@ -44,7 +44,6 @@ # undef WITH_SYSTEMD_DAEMON # undef WITH_VIRTUALPORT # undef WITH_YAJL -# undef WITH_YAJL2 #endif /* diff --git a/m4/virt-yajl.m4 b/m4/virt-yajl.m4 index dfc920d1f2..494e722963 100644 --- a/m4/virt-yajl.m4 +++ b/m4/virt-yajl.m4 @@ -25,7 +25,6 @@ AC_DEFUN([LIBVIRT_CHECK_YAJL],[ dnl YAJL JSON library http://lloyd.github.com/yajl/ LIBVIRT_CHECK_PKG([YAJL], [yajl], [2.0.3]) - AC_DEFINE_UNQUOTED([WITH_YAJL2], 1, [whether YAJL2 is enabled]) ]) AC_DEFUN([LIBVIRT_RESULT_YAJL],[ -- 2.19.2
On Wed, Apr 03, 2019 at 07:40:21AM +0200, Ján Tomko wrote:
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- config-post.h | 1 - m4/virt-yajl.m4 | 1 - 2 files changed, 2 deletions(-)
diff --git a/config-post.h b/config-post.h index 54731feff7..093f84a7ce 100644 --- a/config-post.h +++ b/config-post.h @@ -44,7 +44,6 @@ # undef WITH_SYSTEMD_DAEMON # undef WITH_VIRTUALPORT # undef WITH_YAJL -# undef WITH_YAJL2 #endif
/* diff --git a/m4/virt-yajl.m4 b/m4/virt-yajl.m4 index dfc920d1f2..494e722963 100644 --- a/m4/virt-yajl.m4 +++ b/m4/virt-yajl.m4 @@ -25,7 +25,6 @@ AC_DEFUN([LIBVIRT_CHECK_YAJL],[ dnl YAJL JSON library http://lloyd.github.com/yajl/
LIBVIRT_CHECK_PKG([YAJL], [yajl], [2.0.3]) - AC_DEFINE_UNQUOTED([WITH_YAJL2], 1, [whether YAJL2 is enabled]) ])
AC_DEFUN([LIBVIRT_RESULT_YAJL],[
Reviewed-by: Daniel P. Berrangé <berrange@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 :|
Now that we do not need to cater to YAJL 1, move the check for the return value of yajl_gen_alloc earlier, so that we can assume it was successful in later code. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virjson.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/util/virjson.c b/src/util/virjson.c index b163d5c1cb..91ce97a689 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -1939,16 +1939,14 @@ virJSONValueToString(virJSONValuePtr object, VIR_DEBUG("object=%p", object); g = yajl_gen_alloc(NULL); - if (g) { - yajl_gen_config(g, yajl_gen_beautify, pretty ? 1 : 0); - yajl_gen_config(g, yajl_gen_indent_string, pretty ? " " : " "); - yajl_gen_config(g, yajl_gen_validate_utf8, 1); - } if (!g) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to create JSON formatter")); goto cleanup; } + yajl_gen_config(g, yajl_gen_beautify, pretty ? 1 : 0); + yajl_gen_config(g, yajl_gen_indent_string, pretty ? " " : " "); + yajl_gen_config(g, yajl_gen_validate_utf8, 1); if (virJSONValueToStringOne(object, g) < 0) { virReportOOMError(); -- 2.19.2
On Wed, Apr 03, 2019 at 07:40:22AM +0200, Ján Tomko wrote:
Now that we do not need to cater to YAJL 1, move the check for the return value of yajl_gen_alloc earlier, so that we can assume it was successful in later code.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/util/virjson.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@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 :|
Make it work better with git history. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- configure.ac | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 880a3a7e40..104990c2c0 100644 --- a/configure.ac +++ b/configure.ac @@ -24,7 +24,14 @@ AH_BOTTOM([#include <config-post.h>]) AC_CONFIG_MACRO_DIR([m4]) dnl Make automake keep quiet about wildcards & other GNUmake-isms; also keep dnl quiet about the fact that we intentionally cater to automake 1.9 -AM_INIT_AUTOMAKE([-Wno-portability -Wno-obsolete tar-pax no-dist-gzip dist-xz subdir-objects]) +AM_INIT_AUTOMAKE([ + -Wno-portability + -Wno-obsolete + tar-pax + no-dist-gzip + dist-xz + subdir-objects + ]) dnl older automake's default of ARFLAGS=cru is noisy on newer binutils; dnl we don't really need the 'u' even in older toolchains. Then there is dnl older libtool, which spelled it AR_FLAGS -- 2.19.2
On Wed, Apr 03, 2019 at 07:40:23AM +0200, Ján Tomko wrote:
Make it work better with git history.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- configure.ac | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac index 880a3a7e40..104990c2c0 100644 --- a/configure.ac +++ b/configure.ac @@ -24,7 +24,14 @@ AH_BOTTOM([#include <config-post.h>]) AC_CONFIG_MACRO_DIR([m4]) dnl Make automake keep quiet about wildcards & other GNUmake-isms; also keep dnl quiet about the fact that we intentionally cater to automake 1.9 -AM_INIT_AUTOMAKE([-Wno-portability -Wno-obsolete tar-pax no-dist-gzip dist-xz subdir-objects]) +AM_INIT_AUTOMAKE([ + -Wno-portability + -Wno-obsolete + tar-pax + no-dist-gzip + dist-xz + subdir-objects + ]) dnl older automake's default of ARFLAGS=cru is noisy on newer binutils; dnl we don't really need the 'u' even in older toolchains. Then there is dnl older libtool, which spelled it AR_FLAGS
Reviewed-by: Daniel P. Berrangé <berrange@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 :|
Even Ubuntu 16.04 has automake 1.11. Now that we no longer cater to automake 1.9, drop the comment as well as the -Wno-obsolete option, since it does not seem to generate any warnings anymore. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- configure.ac | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index 104990c2c0..78e5ee9a28 100644 --- a/configure.ac +++ b/configure.ac @@ -22,11 +22,9 @@ AC_CONFIG_AUX_DIR([build-aux]) AC_CONFIG_HEADERS([config.h]) AH_BOTTOM([#include <config-post.h>]) AC_CONFIG_MACRO_DIR([m4]) -dnl Make automake keep quiet about wildcards & other GNUmake-isms; also keep -dnl quiet about the fact that we intentionally cater to automake 1.9 +dnl Make automake keep quiet about wildcards & other GNUmake-isms AM_INIT_AUTOMAKE([ -Wno-portability - -Wno-obsolete tar-pax no-dist-gzip dist-xz -- 2.19.2
On Wed, Apr 03, 2019 at 07:40:24AM +0200, Ján Tomko wrote:
Even Ubuntu 16.04 has automake 1.11. Now that we no longer cater to automake 1.9, drop the comment as well as the -Wno-obsolete option, since it does not seem to generate any warnings anymore.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- configure.ac | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@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 :|
We do not care about the portability warnings implied by the implicit 'gnu' option. Switch to 'foreign' to opt out of checking the files present in the top directory to let us drop ChangeLog completely. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- configure.ac | 1 + 1 file changed, 1 insertion(+) diff --git a/configure.ac b/configure.ac index 78e5ee9a28..a494a18eea 100644 --- a/configure.ac +++ b/configure.ac @@ -24,6 +24,7 @@ AH_BOTTOM([#include <config-post.h>]) AC_CONFIG_MACRO_DIR([m4]) dnl Make automake keep quiet about wildcards & other GNUmake-isms AM_INIT_AUTOMAKE([ + foreign -Wno-portability tar-pax no-dist-gzip -- 2.19.2
On Wed, Apr 03, 2019 at 07:40:25AM +0200, Ján Tomko wrote:
We do not care about the portability warnings implied by the implicit 'gnu' option. Switch to 'foreign' to opt out of checking the files present in the top directory to let us drop ChangeLog completely.
Signed-off-by: Ján Tomko <jtomko@redhat.com> --- configure.ac | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Daniel P. Berrangé <berrange@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 :|
participants (5)
-
Andrea Bolognani -
Daniel P. Berrangé -
Ján Tomko -
Olaf Hering -
Peter Krempa