[libvirt] [PATCH 0/3] Makefile.am cleanups
I noticed from IRC that Travis has been failing builds since our Jan 1 update of gnulib. I'm not sure what changed in the gnulib files to trip up clang, and I'm not really set up to test that this fixes the problem seen by Travis (it works locally for me using gcc - but gcc wasn't seeing the failure), but the ideas in the patch make sense. So, unless there is an easy way to make Travis run on candidate patches, I'll wait for a review rather than pushing this as a build-breaker. Eric Blake (3): maint: Drop unused GETTEXT_CPPFLAGS variable maint: Prefer AM_CPPFLAGS over INCLUDES maint: Avoid -Ignulib on standalone examples examples/Makefile.am | 8 +++++++- gnulib/lib/Makefile.am | 2 -- gnulib/tests/Makefile.am | 2 -- src/Makefile.am | 6 +++--- tests/Makefile.am | 6 +++--- tools/Makefile.am | 4 ++-- 6 files changed, 15 insertions(+), 13 deletions(-) -- 2.20.1
Commit c0a8ea45 removed the use of gettextize, and the setting of GETTEXT_CPPFLAGS, but did not scrub the now-unused variable from Makefile.am snippets. Signed-off-by: Eric Blake <eblake@redhat.com> --- gnulib/lib/Makefile.am | 2 -- gnulib/tests/Makefile.am | 2 -- src/Makefile.am | 4 ++-- tests/Makefile.am | 2 +- tools/Makefile.am | 2 +- 5 files changed, 4 insertions(+), 8 deletions(-) diff --git a/gnulib/lib/Makefile.am b/gnulib/lib/Makefile.am index f098e823be..601f74073c 100644 --- a/gnulib/lib/Makefile.am +++ b/gnulib/lib/Makefile.am @@ -26,5 +26,3 @@ SUFFIXES = noinst_LTLIBRARIES = include gnulib.mk - -INCLUDES = -I$(top_srcdir) $(GETTEXT_CPPFLAGS) diff --git a/gnulib/tests/Makefile.am b/gnulib/tests/Makefile.am index df4af59a82..7062cbaf87 100644 --- a/gnulib/tests/Makefile.am +++ b/gnulib/tests/Makefile.am @@ -18,8 +18,6 @@ include gnulib.mk -INCLUDES = $(GETTEXT_CPPFLAGS) - GNULIB_TESTS0 = GNULIB_TESTS1 = $(GNULIB_TESTS) if WITH_EXPENSIVE_TESTS diff --git a/src/Makefile.am b/src/Makefile.am index e2b89e27e8..3ac6dcad16 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -24,7 +24,7 @@ abs_topsrcdir = $(shell cd $(top_srcdir) && pwd) # No libraries with the exception of LIBXML should be listed # here. List them against the individual XXX_la_CFLAGS targets -# that actually use them. Also keep GETTEXT_CPPFLAGS at the end. +# that actually use them. INCLUDES = -I../gnulib/lib \ -I$(top_srcdir)/gnulib/lib \ -I$(top_srcdir) \ @@ -35,7 +35,7 @@ INCLUDES = -I../gnulib/lib \ -DIN_LIBVIRT \ -Dabs_topbuilddir="\"$(abs_topbuilddir)\"" \ -Dabs_topsrcdir="\"$(abs_topsrcdir)\"" \ - $(GETTEXT_CPPFLAGS) + $(NULL) WARN_CFLAGS += $(STRICT_FRAME_LIMIT_CFLAGS) diff --git a/tests/Makefile.am b/tests/Makefile.am index 3b413dca43..69dd45728d 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -31,7 +31,7 @@ INCLUDES = \ -I$(top_builddir)/src -I$(top_srcdir)/src \ -I$(top_srcdir)/src/util \ -I$(top_srcdir)/src/conf \ - $(GETTEXT_CPPFLAGS) + $(NULL) WARN_CFLAGS += $(RELAXED_FRAME_LIMIT_CFLAGS) diff --git a/tools/Makefile.am b/tools/Makefile.am index 1dc009c4fb..fadd09977e 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -21,7 +21,7 @@ INCLUDES = \ -I$(top_builddir)/src -I$(top_srcdir)/src \ -I$(top_srcdir)/src/util \ -I$(top_srcdir) \ - $(GETTEXT_CPPFLAGS) + $(NULL) WARN_CFLAGS += $(STRICT_FRAME_LIMIT_CFLAGS) -- 2.20.1
On Fri, Jan 04, 2019 at 02:12:18PM -0600, Eric Blake wrote:
Commit c0a8ea45 removed the use of gettextize, and the setting of GETTEXT_CPPFLAGS, but did not scrub the now-unused variable from Makefile.am snippets.
Signed-off-by: Eric Blake <eblake@redhat.com> --- gnulib/lib/Makefile.am | 2 -- gnulib/tests/Makefile.am | 2 -- src/Makefile.am | 4 ++-- tests/Makefile.am | 2 +- tools/Makefile.am | 2 +- 5 files changed, 4 insertions(+), 8 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Our use of INCLUDES in Makefile.am hearkens back to when we had to cater to automake 1.9.6 (thanks, RHEL 5) which lacked AM_CPPFLAGS. Modern Automake flags a warning that INCLUDES is deprecated, and now that we mandate RHEL 7 or better (see commit c1bc9c66), we no longer have to cater to the old spelling. Signed-off-by: Eric Blake <eblake@redhat.com> --- examples/Makefile.am | 3 ++- src/Makefile.am | 2 +- tests/Makefile.am | 4 ++-- tools/Makefile.am | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/examples/Makefile.am b/examples/Makefile.am index 5b1f6a0523..61cf2af4a5 100644 --- a/examples/Makefile.am +++ b/examples/Makefile.am @@ -33,7 +33,8 @@ EXTRA_DIST = \ $(wildcard $(srcdir)/xml/test/*.xml) -INCLUDES = -I$(top_builddir)/include -I$(top_srcdir)/include -I$(top_srcdir) \ +AM_CPPFLAGS = \ + -I$(top_builddir)/include -I$(top_srcdir)/include -I$(top_srcdir) \ -I$(top_builddir)/gnulib/lib -I$(top_srcdir)/gnulib/lib LDADD = $(STATIC_BINARIES) $(WARN_CFLAGS) $(COVERAGE_LDFLAGS) \ $(top_builddir)/src/libvirt.la $(top_builddir)/gnulib/lib/libgnu.la \ diff --git a/src/Makefile.am b/src/Makefile.am index 3ac6dcad16..d3e8a1b572 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -25,7 +25,7 @@ abs_topsrcdir = $(shell cd $(top_srcdir) && pwd) # No libraries with the exception of LIBXML should be listed # here. List them against the individual XXX_la_CFLAGS targets # that actually use them. -INCLUDES = -I../gnulib/lib \ +AM_CPPFLAGS = -I../gnulib/lib \ -I$(top_srcdir)/gnulib/lib \ -I$(top_srcdir) \ -I../include \ diff --git a/tests/Makefile.am b/tests/Makefile.am index 69dd45728d..f74d8463b6 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -24,7 +24,7 @@ abs_topsrcdir = $(shell cd $(top_srcdir) && pwd) SHELL = $(PREFERABLY_POSIX_SHELL) -INCLUDES = \ +AM_CPPFLAGS = \ -I$(top_builddir) -I$(top_srcdir) \ -I$(top_builddir)/gnulib/lib -I$(top_srcdir)/gnulib/lib \ -I$(top_builddir)/include -I$(top_srcdir)/include \ @@ -63,7 +63,7 @@ QEMULIB_LDFLAGS = \ -rpath /evil/libtool/hack/to/force/shared/lib/creation \ $(MINGW_EXTRA_LDFLAGS) -INCLUDES += \ +AM_CPPFLAGS += \ -DTEST_DRIVER_DIR=\"$(top_builddir)/src/.libs\" PROBES_O = diff --git a/tools/Makefile.am b/tools/Makefile.am index fadd09977e..613c9a77f0 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -15,7 +15,7 @@ ## License along with this library. If not, see ## <http://www.gnu.org/licenses/>. -INCLUDES = \ +AM_CPPFLAGS = \ -I$(top_builddir)/include -I$(top_srcdir)/include \ -I$(top_builddir)/gnulib/lib -I$(top_srcdir)/gnulib/lib \ -I$(top_builddir)/src -I$(top_srcdir)/src \ -- 2.20.1
On Fri, Jan 04, 2019 at 02:12:19PM -0600, Eric Blake wrote:
Our use of INCLUDES in Makefile.am hearkens back to when we had to cater to automake 1.9.6 (thanks, RHEL 5) which lacked AM_CPPFLAGS. Modern Automake flags a warning that INCLUDES is deprecated, and now that we mandate RHEL 7 or better (see commit c1bc9c66), we no longer have to cater to the old spelling.
Signed-off-by: Eric Blake <eblake@redhat.com> --- examples/Makefile.am | 3 ++- src/Makefile.am | 2 +- tests/Makefile.am | 4 ++-- tools/Makefile.am | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 1/7/19 5:19 AM, Ján Tomko wrote:
On Fri, Jan 04, 2019 at 02:12:19PM -0600, Eric Blake wrote:
Our use of INCLUDES in Makefile.am hearkens back to when we had to cater to automake 1.9.6 (thanks, RHEL 5) which lacked AM_CPPFLAGS. Modern Automake flags a warning that INCLUDES is deprecated, and now that we mandate RHEL 7 or better (see commit c1bc9c66), we no longer have to cater to the old spelling.
Signed-off-by: Eric Blake <eblake@redhat.com> --- examples/Makefile.am | 3 ++- src/Makefile.am | 2 +- tests/Makefile.am | 4 ++-- tools/Makefile.am | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com>
I've pushed the first two while revisiting the third for a v2. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Travis shows that clang (but not gcc) fails to build after our recent gnulib updates, due to: CC domtop/domtop.o In file included from dommigrate/dommigrate.c:26: In file included from ../gnulib/lib/stdlib.h:100: In file included from ../gnulib/lib/unistd.h:40: In file included from /usr/include/unistd.h:638: In file included from ../gnulib/lib/sys/select.h:110: In file included from ../gnulib/lib/signal.h:67: ../gnulib/lib/pthread.h:74:3: error: "Please include config.h first." #error "Please include config.h first." This probably stems from gcc and clang having a subtle difference in -isystem vs. -I include behaviors on #include_next, where that difference then results in clang trying to pick up same-named gnulib headers merely because their directory was in the search path, while gcc did not hit that problem. But for our examples that are intended to be standalone, we want to work with the bare-bones libc, without any interference from our <config.h> or from gnulib. The easiest way to do that is by using per-binary CPPFLAGS, and including gnulib headers only for example binaries that still use <config.h>. Signed-off-by: Eric Blake <eblake@redhat.com> --- I don't have a local clang setup, so I'm relying on CI testing to prove whether this fixes the build failures that Travis reported... --- examples/Makefile.am | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/examples/Makefile.am b/examples/Makefile.am index 61cf2af4a5..d85e1d5ace 100644 --- a/examples/Makefile.am +++ b/examples/Makefile.am @@ -34,7 +34,8 @@ EXTRA_DIST = \ AM_CPPFLAGS = \ - -I$(top_builddir)/include -I$(top_srcdir)/include -I$(top_srcdir) \ + -I$(top_builddir)/include -I$(top_srcdir)/include -I$(top_srcdir) +GNULIB_CPPFLAGS = \ -I$(top_builddir)/gnulib/lib -I$(top_srcdir)/gnulib/lib LDADD = $(STATIC_BINARIES) $(WARN_CFLAGS) $(COVERAGE_LDFLAGS) \ $(top_builddir)/src/libvirt.la $(top_builddir)/gnulib/lib/libgnu.la \ @@ -49,13 +50,16 @@ noinst_PROGRAMS=dominfo/info1 dommigrate/dommigrate domsuspend/suspend \ dominfo_info1_SOURCES = dominfo/info1.c dommigrate_dommigrate_SOURCES = dommigrate/dommigrate.c domsuspend_suspend_SOURCES = domsuspend/suspend.c +domsuspend_suspend_CPPFLAGS = $(AM_CPPFLAGS) $(GNULIB_CPPFLAGS) domtop_domtop_SOURCES = domtop/domtop.c +domtop_domtop_CPPFLAGS = $(AM_CPPFLAGS) $(GNULIB_CPPFLAGS) hellolibvirt_hellolibvirt_SOURCES = hellolibvirt/hellolibvirt.c object_events_event_test_CFLAGS = \ $(WARN_CFLAGS) \ $(NULL) object_events_event_test_SOURCES = object-events/event-test.c +object_events_event_test_CPPFLAGS = $(AM_CPPFLAGS) $(GNULIB_CPPFLAGS) openauth_openauth_SOURCES = openauth/openauth.c rename_rename_SOURCES = rename/rename.c @@ -67,6 +71,7 @@ admin_client_limits_SOURCES = admin/client_limits.c admin_client_info_SOURCES = admin/client_info.c admin_client_close_SOURCES = admin/client_close.c admin_logging_SOURCES = admin/logging.c +admin_logging_CPPFLAGS = $(AM_CPPFLAGS) $(GNULIB_CPPFLAGS) INSTALL_DATA_LOCAL = UNINSTALL_LOCAL = -- 2.20.1
On Fri, Jan 04, 2019 at 02:12:20PM -0600, Eric Blake wrote:
Travis shows that clang (but not gcc) fails to build after our recent gnulib updates, due to:
This is a BSD vs Linux thing, we use Travis to get Mac OS X coverage. I regulary build with clang and did not hit such error.
CC domtop/domtop.o In file included from dommigrate/dommigrate.c:26: In file included from ../gnulib/lib/stdlib.h:100: In file included from ../gnulib/lib/unistd.h:40: In file included from /usr/include/unistd.h:638: In file included from ../gnulib/lib/sys/select.h:110: In file included from ../gnulib/lib/signal.h:67: ../gnulib/lib/pthread.h:74:3: error: "Please include config.h first." #error "Please include config.h first."
The reason for this change is gnulib commit 6954995d https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=6954995d which started including unistd.h through stdlib.h for some platforms, thus requiring us to include config.h. I did not notice this patch before pushing my "include config.h in examples" solution: https://libvirt.org/git/?p=libvirt.git;a=object;h=6954995d
This probably stems from gcc and clang having a subtle difference in -isystem vs. -I include behaviors on #include_next, where that difference then results in clang trying to pick up same-named gnulib headers merely because their directory was in the search path, while gcc did not hit that problem.
IIUC for both compilers the expected behavior is to include gnulib headers, otherwise there's no point in having gnulib at all.
But for our examples that are intended to be standalone, we want to work with the bare-bones libc, without any interference from our <config.h> or from gnulib. The easiest way to do that is by using per-binary CPPFLAGS, and including gnulib headers only for example binaries that still use <config.h>.
Signed-off-by: Eric Blake <eblake@redhat.com>
--- I don't have a local clang setup, so I'm relying on CI testing to prove whether this fixes the build failures that Travis reported... --- examples/Makefile.am | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
Having standalone examples is a nicer soultion than mine. However, with my commit reverted, tuning examples/admin/logging.c is needed to make the build pass on FreeBSD for me: diff --git a/examples/admin/logging.c b/examples/admin/logging.c index dc1b23aab5..1e72cdd601 100644 --- a/examples/admin/logging.c +++ b/examples/admin/logging.c @@ -1,8 +1,8 @@ +#include "config.h" #include <stdio.h> #include <stdlib.h> #include <stdbool.h> -#include "config.h" #include <unistd.h> #include <libvirt/libvirt-admin.h> #include <libvirt/virterror.h> This makes me think that the remaining examples don't really need gnulib, they just include <config.h> to make compilation pass while including <unistd.h>. Jano
On 1/7/19 5:35 AM, Ján Tomko wrote:
On Fri, Jan 04, 2019 at 02:12:20PM -0600, Eric Blake wrote:
Travis shows that clang (but not gcc) fails to build after our recent gnulib updates, due to:
This is a BSD vs Linux thing, we use Travis to get Mac OS X coverage. I regulary build with clang and did not hit such error.
Aha, so it wasn't a compiler difference, but a libc difference. I can update the commit message accordingly.
CC domtop/domtop.o In file included from dommigrate/dommigrate.c:26: In file included from ../gnulib/lib/stdlib.h:100: In file included from ../gnulib/lib/unistd.h:40: In file included from /usr/include/unistd.h:638: In file included from ../gnulib/lib/sys/select.h:110: In file included from ../gnulib/lib/signal.h:67: ../gnulib/lib/pthread.h:74:3: error: "Please include config.h first." #error "Please include config.h first."
The reason for this change is gnulib commit 6954995d https://git.savannah.gnu.org/gitweb/?p=gnulib.git;a=commitdiff;h=6954995d which started including unistd.h through stdlib.h for some platforms, thus requiring us to include config.h.
I did not notice this patch before pushing my "include config.h in examples" solution: https://libvirt.org/git/?p=libvirt.git;a=object;h=6954995d
Indeed, now that I see your thread, I still like this solution better (the examples really are meant to be standalone, so let's get gnulib out of the way rather than polluting the files with an otherwise useless config.h include). I'm fine with reverting your reversion in order to with this approach, once we solve the remaining issues...
This probably stems from gcc and clang having a subtle difference in -isystem vs. -I include behaviors on #include_next, where that difference then results in clang trying to pick up same-named gnulib headers merely because their directory was in the search path, while gcc did not hit that problem.
IIUC for both compilers the expected behavior is to include gnulib headers, otherwise there's no point in having gnulib at all.
Rather, the point is that if you -Ignulib, you must also -lgnulib. If your examples are intended to be standalone (no -lgnulib), then you do NOT want to compile with gnulib headers OR with config.h. Yes, I need to fix the wording in that paragraph to point out that the real culprit is that a recent gnulib change caused BSD libc headers to cycle through a different inclusion order than previously (and not a compiler difference), while still emphasizing that the real solution is not to include config.h first, but to get rid of the gnulib headers.
But for our examples that are intended to be standalone, we want to work with the bare-bones libc, without any interference from our <config.h> or from gnulib. The easiest way to do that is by using per-binary CPPFLAGS, and including gnulib headers only for example binaries that still use <config.h>.
Signed-off-by: Eric Blake <eblake@redhat.com>
--- I don't have a local clang setup, so I'm relying on CI testing to prove whether this fixes the build failures that Travis reported... --- examples/Makefile.am | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
Having standalone examples is a nicer soultion than mine. However, with my commit reverted, tuning examples/admin/logging.c is needed to make the build pass on FreeBSD for me: diff --git a/examples/admin/logging.c b/examples/admin/logging.c index dc1b23aab5..1e72cdd601 100644 --- a/examples/admin/logging.c +++ b/examples/admin/logging.c @@ -1,8 +1,8 @@ +#include "config.h" #include <stdio.h> #include <stdlib.h> #include <stdbool.h>
-#include "config.h" #include <unistd.h>
_If_ config.h is needed, then it is ALWAYS needed first (before ANY system headers), whether or not we also link against gnulib. But either logging.c links against gnulib (in which case your suggestion of hoisting config.h first is right), or it doesn't (in which case we should get rid of the config.h include altogether, to prove that the example is standalone; as well as tweak my patch to remove the GNULIB_CPPFLAGS addition to admin_logging_CPPFLAGS). I'll post a v2 along those lines.
#include <libvirt/libvirt-admin.h> #include <libvirt/virterror.h>
This makes me think that the remaining examples don't really need gnulib, they just include <config.h> to make compilation pass while including <unistd.h>.
Yes, I'll check ALL of the examples in my v2. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Ján Tomko