[libvirt] [PATCHv2] build: consistently use CFLAGS
According to the automake manual, CPPFLAGS (aka INCLUDES, as spelled in automake 1.9.6) should only include -I, -D, and -U directives; more generic directives like -Wall belong in CFLAGS since they affect more phases of the build process. Therefore, we should be sticking CFLAGS additions into a CFLAGS container, not a CPPFLAGS container. * src/Makefile.am (INCLUDES): Move CFLAGS items... (AM_CFLAGS): ...to their proper location. * python/Makefile.am (INCLUDES, AM_CFLAGS): Likewise. * tests/Makefile.am (INCLUDES, AM_CFLAGS): Likewise. --- v1 had no review: https://www.redhat.com/archives/libvir-list/2011-May/msg01934.html changes from v1: rebase to latest I tested it on top of these two patches, hopefully they'll be ack'd first: https://www.redhat.com/archives/libvir-list/2011-June/msg01537.html https://www.redhat.com/archives/libvir-list/2011-June/msg01486.html python/Makefile.am | 5 +++-- src/Makefile.am | 15 +++++++-------- tests/Makefile.am | 8 +++++--- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/python/Makefile.am b/python/Makefile.am index 432ad70..0edb3e4 100644 --- a/python/Makefile.am +++ b/python/Makefile.am @@ -3,12 +3,13 @@ SUBDIRS= . tests INCLUDES = \ - $(WARN_CFLAGS) \ - $(PYTHON_INCLUDES) \ + $(PYTHON_INCLUDES) \ -I$(top_srcdir)/include \ -I$(top_builddir)/include \ -I$(top_builddir)/$(subdir) +AM_CFLAGS = $(WARN_CFLAGS) + DOCS_DIR = $(datadir)/doc/libvirt-python-$(LIBVIRT_VERSION) DOCS = ${srcdir}/TODO diff --git a/src/Makefile.am b/src/Makefile.am index 4ba3ea7..10ee20a 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -3,20 +3,19 @@ # 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$(top_srcdir)/gnulib/lib \ +INCLUDES = -I$(top_srcdir)/gnulib/lib \ -I../gnulib/lib \ -I../include \ -I@top_srcdir@/src/util \ -I@top_srcdir@/include \ - $(DRIVER_MODULE_CFLAGS) \ + -DIN_LIBVIRT + +AM_CFLAGS = $(DRIVER_MODULE_CFLAGS) \ $(LIBXML_CFLAGS) \ $(WARN_CFLAGS) \ - $(LOCK_CHECKING_CFLAGS) \ - -DIN_LIBVIRT \ - $(WIN32_EXTRA_CFLAGS) - -AM_CFLAGS = $(COVERAGE_CFLAGS) + $(LOCK_CHECKING_CFLAGS) \ + $(WIN32_EXTRA_CFLAGS) \ + $(COVERAGE_CFLAGS) AM_LDFLAGS = $(COVERAGE_LDFLAGS) EXTRA_DIST = $(conf_DATA) diff --git a/tests/Makefile.am b/tests/Makefile.am index 38a353f..984d8fd 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -8,15 +8,17 @@ INCLUDES = \ -I$(top_srcdir)/include \ -I$(top_srcdir)/src \ -I$(top_srcdir)/src/util \ - -I$(top_srcdir)/src/conf \ + -I$(top_srcdir)/src/conf + +AM_CFLAGS = \ $(LIBXML_CFLAGS) \ $(GNUTLS_CFLAGS) \ $(SASL_CFLAGS) \ $(SELINUX_CFLAGS) \ $(APPARMOR_CFLAGS) \ $(YAJL_CFLAGS) \ - $(COVERAGE_CFLAGS) \ - $(WARN_CFLAGS) + $(COVERAGE_CFLAGS) \ + $(WARN_CFLAGS) if WITH_DRIVER_MODULES INCLUDES += \ -- 1.7.4.4
On 06/30/2011 02:09 PM, Eric Blake wrote:
According to the automake manual, CPPFLAGS (aka INCLUDES, as spelled in automake 1.9.6) should only include -I, -D, and -U directives; more generic directives like -Wall belong in CFLAGS since they affect more phases of the build process. Therefore, we should be sticking CFLAGS additions into a CFLAGS container, not a CPPFLAGS container.
* src/Makefile.am (INCLUDES): Move CFLAGS items... (AM_CFLAGS): ...to their proper location. * python/Makefile.am (INCLUDES, AM_CFLAGS): Likewise. * tests/Makefile.am (INCLUDES, AM_CFLAGS): Likewise. ---
ACK.
On 06/30/2011 05:12 PM, Laine Stump wrote:
On 06/30/2011 02:09 PM, Eric Blake wrote:
According to the automake manual, CPPFLAGS (aka INCLUDES, as spelled in automake 1.9.6) should only include -I, -D, and -U directives; more generic directives like -Wall belong in CFLAGS since they affect more phases of the build process. Therefore, we should be sticking CFLAGS additions into a CFLAGS container, not a CPPFLAGS container.
* src/Makefile.am (INCLUDES): Move CFLAGS items... (AM_CFLAGS): ...to their proper location. * python/Makefile.am (INCLUDES, AM_CFLAGS): Likewise. * tests/Makefile.am (INCLUDES, AM_CFLAGS): Likewise. ---
ACK.
Hmm, my testing is showing that I've probably botched something over a month of rebasing, and I'll need a v3 to fix a compilation error from an unfound header. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 06/30/2011 07:12 PM, Laine Stump wrote:
On 06/30/2011 02:09 PM, Eric Blake wrote:
According to the automake manual, CPPFLAGS (aka INCLUDES, as spelled in automake 1.9.6) should only include -I, -D, and -U directives; more generic directives like -Wall belong in CFLAGS since they affect more phases of the build process. Therefore, we should be sticking CFLAGS additions into a CFLAGS container, not a CPPFLAGS container.
* src/Makefile.am (INCLUDES): Move CFLAGS items... (AM_CFLAGS): ...to their proper location. * python/Makefile.am (INCLUDES, AM_CFLAGS): Likewise. * tests/Makefile.am (INCLUDES, AM_CFLAGS): Likewise. ---
ACK.
I spoke too soon - this patch causes /usr/include/libxml2 to be lost from the include path, which breaks the build.
On 06/30/2011 07:29 PM, Laine Stump wrote:
On 06/30/2011 07:12 PM, Laine Stump wrote:
On 06/30/2011 02:09 PM, Eric Blake wrote:
According to the automake manual, CPPFLAGS (aka INCLUDES, as spelled in automake 1.9.6) should only include -I, -D, and -U directives; more generic directives like -Wall belong in CFLAGS since they affect more phases of the build process. Therefore, we should be sticking CFLAGS additions into a CFLAGS container, not a CPPFLAGS container.
* src/Makefile.am (INCLUDES): Move CFLAGS items... (AM_CFLAGS): ...to their proper location. * python/Makefile.am (INCLUDES, AM_CFLAGS): Likewise. * tests/Makefile.am (INCLUDES, AM_CFLAGS): Likewise. ---
ACK.
I spoke too soon - this patch causes /usr/include/libxml2 to be lost from the include path, which breaks the build.
Okay, I found the problem. ACK with the following patch squashed in. (I checked all the other ..._CFLAGS in this particular Makefile and this was the only one missing AM_CFLAGS. diff --git a/src/Makefile.am b/src/Makefile.am index 2b6d0fb..4250f60 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -639,7 +639,7 @@ noinst_LTLIBRARIES += libvirt_driver_vmware.la libvirt_la_BUILT_LIBADD += libvirt_driver_vmware.la endif libvirt_driver_vmware_la_CFLAGS = \ - -I@top_srcdir@/src/conf -I@top_srcdir@/src/vmx + -I@top_srcdir@/src/conf -I@top_srcdir@/src/vmx $(AM_CFLAGS) if WITH_DRIVER_MODULES libvirt_driver_vmware_la_LIBADD = ../gnulib/lib/libgnu.la libvirt_driver_vmware_la_LDFLAGS = -module -avoid-version
On 06/30/2011 05:51 PM, Laine Stump wrote:
ACK.
I spoke too soon - this patch causes /usr/include/libxml2 to be lost from the include path, which breaks the build.
Okay, I found the problem. ACK with the following patch squashed in. (I checked all the other ..._CFLAGS in this particular Makefile and this was the only one missing AM_CFLAGS.
Actually, several footest_CFLAGS in tests/Makefile.am were also missing AM_CFLAGS.
diff --git a/src/Makefile.am b/src/Makefile.am index 2b6d0fb..4250f60 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -639,7 +639,7 @@ noinst_LTLIBRARIES += libvirt_driver_vmware.la libvirt_la_BUILT_LIBADD += libvirt_driver_vmware.la endif libvirt_driver_vmware_la_CFLAGS = \ - -I@top_srcdir@/src/conf -I@top_srcdir@/src/vmx + -I@top_srcdir@/src/conf -I@top_srcdir@/src/vmx $(AM_CFLAGS)
Thanks; squashed in and pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Laine Stump