[libvirt] [PATCH 3/3] maint: Avoid -Ignulib on standalone examples

[Eric Blake]

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 at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190107/99711478/attachment-0001.sig>