[libvirt] [PATCH v2 0/2] Makefile.am cleanups
Since v1: - push patch 1, 2 that had successful reviews - new patch 1 - rewrite old patch 3 to remove all use of gnulib from examples/, rather than being partitioned based on what uses config.h (in part by reverting Jan's recent patch that added config.h everywhere) Eric Blake (2): examples: Drop event-test.c dependency on gnulib <verify.h> examples: Avoid gnulib, have standalone examples examples/Makefile.am | 5 ++--- examples/admin/client_close.c | 2 -- examples/admin/client_info.c | 3 +-- examples/admin/client_limits.c | 2 -- examples/admin/list_clients.c | 2 -- examples/admin/list_servers.c | 2 -- examples/admin/logging.c | 2 -- examples/admin/threadpool_params.c | 2 -- examples/dommigrate/dommigrate.c | 2 -- examples/domsuspend/suspend.c | 2 -- examples/domtop/domtop.c | 2 -- examples/hellolibvirt/hellolibvirt.c | 2 -- examples/object-events/event-test.c | 12 ++++++++---- examples/openauth/openauth.c | 2 -- examples/rename/rename.c | 2 -- 15 files changed, 11 insertions(+), 33 deletions(-) -- 2.20.1
Pulling in gnulib just for the <verify.h> header is rather expensive, especially since that header does not require us to link against gnulib. It's better to make the event-test example be standalone by just open-coding a more limited form of a verify() macro that depends on modern gcc (we have enough CI coverage that even though the verify is now a no-op in older setups, we will still notice if we fail to add an event - as a quick test, I still provoked a compile failure on Fedora 29 by deleting a line from domainEvents). Signed-off-by: Eric Blake <eblake@redhat.com> --- examples/object-events/event-test.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/examples/object-events/event-test.c b/examples/object-events/event-test.c index 8499e0b38e..6694d3dffc 100644 --- a/examples/object-events/event-test.c +++ b/examples/object-events/event-test.c @@ -6,8 +6,6 @@ #include <signal.h> #include <inttypes.h> -#include <verify.h> - #define VIR_ENUM_SENTINELS #include <libvirt/libvirt.h> @@ -17,6 +15,14 @@ #define STREQ(a, b) (strcmp(a, b) == 0) #define NULLSTR(s) ((s) ? (s) : "<null>") +#if (4 < __GNUC__ + (6 <= __GNUC_MINOR__) \ + && (201112L <= __STDC_VERSION__ || !defined __STRICT_ANSI__) \ + && !defined __cplusplus) +# define verify(cond) _Static_assert(cond, "verify (" #cond ")") +#else +# define verify(cond) +#endif + #ifndef ATTRIBUTE_UNUSED # define ATTRIBUTE_UNUSED __attribute__((__unused__)) #endif -- 2.20.1
Commit 0c6ad476 updated gnulib, which rearranged some of the conditions in gnulib wrapper headers such that compilation started failing on BSD systems when the normal system <unistd.h> tried to include another system header but instead got a gnulib wrapper header in an incomplete state; this is because gnulib headers only work if <config.h> is included first. Commit b6f78259 papered over the symptoms of that by including <config.h> in all the examples. But this logic is backwards - if our examples are truly meant to be stand-alone, they should NOT depend on how libvirt was configured, and should NOT depend on the gnulib fixes for system quirks. In particular, if an example does not need to link against libgnulib.la, then it also does not need to use -Ignulib in its compile flags, and likewise does not need to include <config.h> since none of the gnulib wrapper headers should be interfering. So, revert (most of) b6f78259 (except for the bogus pre-patch use of "config.h" in admin/logging.c: if config.h is included, it should be via <> rather than "", and must be before any system headers); then additionally nuke all mention of <config.h>, -Ignulib, and -lgnulib, making all of the examples truly standalone. Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: rewrite to nuke rather than partition use of gnulib, revert Jan's patch that landed in meantime --- examples/Makefile.am | 5 ++--- examples/admin/client_close.c | 2 -- examples/admin/client_info.c | 3 +-- examples/admin/client_limits.c | 2 -- examples/admin/list_clients.c | 2 -- examples/admin/list_servers.c | 2 -- examples/admin/logging.c | 2 -- examples/admin/threadpool_params.c | 2 -- examples/dommigrate/dommigrate.c | 2 -- examples/domsuspend/suspend.c | 2 -- examples/domtop/domtop.c | 2 -- examples/hellolibvirt/hellolibvirt.c | 2 -- examples/object-events/event-test.c | 2 -- examples/openauth/openauth.c | 2 -- examples/rename/rename.c | 2 -- 15 files changed, 3 insertions(+), 31 deletions(-) diff --git a/examples/Makefile.am b/examples/Makefile.am index 61cf2af4a5..76907a1c8f 100644 --- a/examples/Makefile.am +++ b/examples/Makefile.am @@ -34,10 +34,9 @@ EXTRA_DIST = \ AM_CPPFLAGS = \ - -I$(top_builddir)/include -I$(top_srcdir)/include -I$(top_srcdir) \ - -I$(top_builddir)/gnulib/lib -I$(top_srcdir)/gnulib/lib + -I$(top_builddir)/include -I$(top_srcdir)/include -I$(top_srcdir) LDADD = $(STATIC_BINARIES) $(WARN_CFLAGS) $(COVERAGE_LDFLAGS) \ - $(top_builddir)/src/libvirt.la $(top_builddir)/gnulib/lib/libgnu.la \ + $(top_builddir)/src/libvirt.la \ $(top_builddir)/src/libvirt-admin.la noinst_PROGRAMS=dominfo/info1 dommigrate/dommigrate domsuspend/suspend \ diff --git a/examples/admin/client_close.c b/examples/admin/client_close.c index 9b7e1febb1..5c28ce7949 100644 --- a/examples/admin/client_close.c +++ b/examples/admin/client_close.c @@ -1,5 +1,3 @@ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <libvirt/libvirt.h> diff --git a/examples/admin/client_info.c b/examples/admin/client_info.c index 550a24824a..2c46ccf514 100644 --- a/examples/admin/client_info.c +++ b/examples/admin/client_info.c @@ -1,5 +1,4 @@ -#include <config.h> - +#define _GNU_SOURCE #include <stdio.h> #include <stdlib.h> #include <time.h> diff --git a/examples/admin/client_limits.c b/examples/admin/client_limits.c index a87cb52c82..69b931682c 100644 --- a/examples/admin/client_limits.c +++ b/examples/admin/client_limits.c @@ -1,5 +1,3 @@ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <libvirt/libvirt-admin.h> diff --git a/examples/admin/list_clients.c b/examples/admin/list_clients.c index 1d961a1ec9..15205a7bd1 100644 --- a/examples/admin/list_clients.c +++ b/examples/admin/list_clients.c @@ -1,5 +1,3 @@ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <time.h> diff --git a/examples/admin/list_servers.c b/examples/admin/list_servers.c index d48d87af6e..e53cd3b48f 100644 --- a/examples/admin/list_servers.c +++ b/examples/admin/list_servers.c @@ -1,5 +1,3 @@ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <libvirt/libvirt-admin.h> diff --git a/examples/admin/logging.c b/examples/admin/logging.c index 10952afa03..730ae40d9d 100644 --- a/examples/admin/logging.c +++ b/examples/admin/logging.c @@ -1,5 +1,3 @@ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <stdbool.h> diff --git a/examples/admin/threadpool_params.c b/examples/admin/threadpool_params.c index 9517dcb0f2..be833c2ea3 100644 --- a/examples/admin/threadpool_params.c +++ b/examples/admin/threadpool_params.c @@ -1,5 +1,3 @@ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <libvirt/libvirt-admin.h> diff --git a/examples/dommigrate/dommigrate.c b/examples/dommigrate/dommigrate.c index 71a0af4058..1b6072d138 100644 --- a/examples/dommigrate/dommigrate.c +++ b/examples/dommigrate/dommigrate.c @@ -20,8 +20,6 @@ * <http://www.gnu.org/licenses/>. */ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <libvirt/libvirt.h> diff --git a/examples/domsuspend/suspend.c b/examples/domsuspend/suspend.c index ac816044ef..af61129ecc 100644 --- a/examples/domsuspend/suspend.c +++ b/examples/domsuspend/suspend.c @@ -19,8 +19,6 @@ * <http://www.gnu.org/licenses/>. */ -#include <config.h> - #include <errno.h> #include <getopt.h> #include <libvirt/libvirt.h> diff --git a/examples/domtop/domtop.c b/examples/domtop/domtop.c index fb631781fd..4224e4c9ec 100644 --- a/examples/domtop/domtop.c +++ b/examples/domtop/domtop.c @@ -18,8 +18,6 @@ * <http://www.gnu.org/licenses/>. */ -#include <config.h> - #include <errno.h> #include <getopt.h> #include <libvirt/libvirt.h> diff --git a/examples/hellolibvirt/hellolibvirt.c b/examples/hellolibvirt/hellolibvirt.c index 02c4401987..bfb12846e6 100644 --- a/examples/hellolibvirt/hellolibvirt.c +++ b/examples/hellolibvirt/hellolibvirt.c @@ -2,8 +2,6 @@ * hypervisor and gather a few bits of information about domains. * Similar API's exist for storage pools, networks, and interfaces. */ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <libvirt/libvirt.h> diff --git a/examples/object-events/event-test.c b/examples/object-events/event-test.c index 6694d3dffc..53d95b10d0 100644 --- a/examples/object-events/event-test.c +++ b/examples/object-events/event-test.c @@ -1,5 +1,3 @@ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <string.h> diff --git a/examples/openauth/openauth.c b/examples/openauth/openauth.c index eef46d5f52..efd21c374f 100644 --- a/examples/openauth/openauth.c +++ b/examples/openauth/openauth.c @@ -1,8 +1,6 @@ /* This is a copy of the hellolibvirt example demonstaring how to use * virConnectOpenAuth with a custom auth callback */ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <string.h> diff --git a/examples/rename/rename.c b/examples/rename/rename.c index c64dd5baa3..85f18e9df3 100644 --- a/examples/rename/rename.c +++ b/examples/rename/rename.c @@ -16,8 +16,6 @@ * <http://www.gnu.org/licenses/>. */ -#include <config.h> - #include <stdio.h> #include <stdlib.h> #include <libvirt/libvirt.h> -- 2.20.1
On 1/7/19 11:58 PM, Eric Blake wrote:
Commit 0c6ad476 updated gnulib, which rearranged some of the conditions in gnulib wrapper headers such that compilation started failing on BSD systems when the normal system <unistd.h> tried to include another system header but instead got a gnulib wrapper header in an incomplete state; this is because gnulib headers only work if <config.h> is included first.
Commit b6f78259 papered over the symptoms of that by including <config.h> in all the examples. But this logic is backwards - if our examples are truly meant to be stand-alone, they should NOT depend on how libvirt was configured, and should NOT depend on the gnulib fixes for system quirks. In particular, if an example does not need to link against libgnulib.la, then it also does not need to use -Ignulib in its compile flags, and likewise does not need to include <config.h> since none of the gnulib wrapper headers should be interfering.
So, revert (most of) b6f78259 (except for the bogus pre-patch use of "config.h" in admin/logging.c: if config.h is included, it should be via <> rather than "", and must be before any system headers); then additionally nuke all mention of <config.h>, -Ignulib, and -lgnulib, making all of the examples truly standalone.
Apparently it's now too standalone - Travis complains that mingw now fails: domtop/domtop.c: In function 'print_cpu_usage': domtop/domtop.c:229:15: warning: unknown conversion type character 'l' in format [-Wformat=] DEBUG("now_params=%llu then_params=%llu now=%llu then=%llu", ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ... domtop/domtop.c: In function 'do_top': domtop/domtop.c:268:22: error: storage size of 'action_stop' isn't known struct sigaction action_stop; ^~~~~~~~~~~ so at least domtop.c was actually depending on gnulib fixing printf and providing sigaction. Rewriting the printf uses to favor PRIu64 might help, but I'm not sure how to work around missing sigaction. I'm trying to reproduce the mingw cross-build locally, rather than iteratively hammering through more Travis tests, and hope to post a fixup patch later today. It may be that I go back to my earlier plan where some of the examples link against gnulib (but in that case, it will be just the tests that need it, rather than all the tests). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 1/8/19 6:58 AM, Eric Blake wrote:
Since v1: - push patch 1, 2 that had successful reviews - new patch 1 - rewrite old patch 3 to remove all use of gnulib from examples/, rather than being partitioned based on what uses config.h (in part by reverting Jan's recent patch that added config.h everywhere)
Eric Blake (2): examples: Drop event-test.c dependency on gnulib <verify.h> examples: Avoid gnulib, have standalone examples
examples/Makefile.am | 5 ++--- examples/admin/client_close.c | 2 -- examples/admin/client_info.c | 3 +-- examples/admin/client_limits.c | 2 -- examples/admin/list_clients.c | 2 -- examples/admin/list_servers.c | 2 -- examples/admin/logging.c | 2 -- examples/admin/threadpool_params.c | 2 -- examples/dommigrate/dommigrate.c | 2 -- examples/domsuspend/suspend.c | 2 -- examples/domtop/domtop.c | 2 -- examples/hellolibvirt/hellolibvirt.c | 2 -- examples/object-events/event-test.c | 12 ++++++++---- examples/openauth/openauth.c | 2 -- examples/rename/rename.c | 2 -- 15 files changed, 11 insertions(+), 33 deletions(-)
ACK to both. Michal
On Mon, Jan 07, 2019 at 11:58:40PM -0600, Eric Blake wrote:
Since v1: - push patch 1, 2 that had successful reviews - new patch 1 - rewrite old patch 3 to remove all use of gnulib from examples/, rather than being partitioned based on what uses config.h (in part by reverting Jan's recent patch that added config.h everywhere)
Eric Blake (2): examples: Drop event-test.c dependency on gnulib <verify.h> examples: Avoid gnulib, have standalone examples
examples/Makefile.am | 5 ++--- examples/admin/client_close.c | 2 -- examples/admin/client_info.c | 3 +-- examples/admin/client_limits.c | 2 -- examples/admin/list_clients.c | 2 -- examples/admin/list_servers.c | 2 -- examples/admin/logging.c | 2 -- examples/admin/threadpool_params.c | 2 -- examples/dommigrate/dommigrate.c | 2 -- examples/domsuspend/suspend.c | 2 -- examples/domtop/domtop.c | 2 -- examples/hellolibvirt/hellolibvirt.c | 2 -- examples/object-events/event-test.c | 12 ++++++++---- examples/openauth/openauth.c | 2 -- examples/rename/rename.c | 2 -- 15 files changed, 11 insertions(+), 33 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Eric Blake -
Ján Tomko -
Michal Privoznik