Message-ID: <931df2faa37692da100aed01c811ee081c5a7d93.camel@debian.org>
Date:   Thu, 21 Mar 2019 10:29:35 +0000
From:   Luca Boccassi <bluca@...ian.org>
To:     Andrey Ignatov <rdna@...com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "sdf@...ichev.me" <sdf@...ichev.me>
Subject: Re: [PATCH bpf-next v3] tools/bpf: generate pkg-config file for
 libbpf

On Wed, 2019-03-20 at 23:58 +0000, Andrey Ignatov wrote:
> luca.boccassi@...il.com <luca.boccassi@...il.com> [Wed, 2019-03-20
> 06:28 -0700]:
> > From: Luca Boccassi <bluca@...ian.org>
> > 
> > Generate a libbpf.pc file at build time so that users can rely
> > on pkg-config to find the library, its CFLAGS and LDFLAGS.
> > 
> > Signed-off-by: Luca Boccassi <bluca@...ian.org>
> > ---
> > v2: use QUIET_GEN instead of QUIET_LINK to generate pc file,
> >     save kernel version in its own variable instead of calling
> >     make inline
> > v3: use LIBBPF_VERSION instead of kernel_version
> > 
> >  tools/lib/bpf/.gitignore         |  1 +
> >  tools/lib/bpf/Makefile           | 18 +++++++++++++++---
> >  tools/lib/bpf/libbpf.pc.template | 11 +++++++++++
> >  3 files changed, 27 insertions(+), 3 deletions(-)
> >  create mode 100644 tools/lib/bpf/libbpf.pc.template
> > 
> > diff --git a/tools/lib/bpf/.gitignore b/tools/lib/bpf/.gitignore
> > index 4db74758c674..7d9e182a1f51 100644
> > --- a/tools/lib/bpf/.gitignore
> > +++ b/tools/lib/bpf/.gitignore
> > @@ -1,3 +1,4 @@
> >  libbpf_version.h
> > +libbpf.pc
> >  FEATURE-DUMP.libbpf
> >  test_libbpf
> > diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> > index 61aaacf0cfa1..891fe3da1410 100644
> > --- a/tools/lib/bpf/Makefile
> > +++ b/tools/lib/bpf/Makefile
> 
> Commenting here rather than on v1/v2 discussion.
> 
> Makefile on the github mirror is completely different one:
> https://github.com/libbpf/libbpf/blob/master/src/Makefile
> 
> It was done like this to avoid the need to patch copied Makefile,
> mirror
> as few files as possible and avoid things like kernel & tools UAPI
> comparison, "feature" tests and other kernel-tree-only stuff that is
> not
> applicable to out-of-tree code.
> 
> I.e. if somebody uses libbpf mirror and wants to support pkg-config
> they
> would need to make similar changes there.

I'm happy to work on a patch for that tree as well, once this one is
accepted.

> Just wondering if you plan to create a package using kernel tree?

Debian already is distributing a libbpf package built from the kernel
source tree, it will be part of Debian 10 (note: I'm not a Debian
kernel maintainer):

https://packages.debian.org/buster/libbpf4.19

> > @@ -80,6 +80,7 @@ libdir_SQ = $(subst ','\'',$(libdir))
> >  libdir_relative_SQ = $(subst ','\'',$(libdir_relative))
> >  
> >  LIB_FILE = libbpf.a libbpf.so
> > +PC_FILE = libbpf.pc
> >  
> >  VERSION		= $(BPF_VERSION)
> >  PATCHLEVEL	= $(BPF_PATCHLEVEL)
> > @@ -137,7 +138,7 @@ GLOBAL_SYM_COUNT = $(shell readelf -s --wide
> > $(BPF_IN) | \
> >  VERSIONED_SYM_COUNT = $(shell readelf -s --wide $(OUTPUT)libbpf.so
> > | \
> >  			      grep -Eo '[^ ]+@...BPF_' | cut -d@ -f1 |
> > sort -u | wc -l)
> >  
> > -CMD_TARGETS = $(LIB_FILE)
> > +CMD_TARGETS = $(LIB_FILE) $(PC_FILE)
> >  
> >  CXX_TEST_TARGET = $(OUTPUT)test_libbpf
> >  
> > @@ -180,6 +181,12 @@ $(OUTPUT)libbpf.a: $(BPF_IN)
> >  $(OUTPUT)test_libbpf: test_libbpf.cpp $(OUTPUT)libbpf.a
> >  	$(QUIET_LINK)$(CXX) $(INCLUDES) $^ -lelf -o $@
> >  
> > +$(OUTPUT)libbpf.pc:
> > +	$(QUIET_GEN)sed -e "s|@...FIX@|$(prefix)|" \
> > +		-e "s|@...DIR@|$(libdir_SQ)|" \
> > +		-e "s|@...SION@|$(LIBBPF_VERSION)|" \
> > +		< $@...mplate > $@
> > +
> >  check: check_abi
> >  
> >  check_abi: $(OUTPUT)libbpf.so
> > @@ -209,7 +216,12 @@ install_headers:
> >  		$(call do_install,libbpf.h,$(prefix)/include/bpf,644);
> >  		$(call do_install,btf.h,$(prefix)/include/bpf,644);
> >  
> > -install: install_lib
> > +install_pkgconfig: $(PC_FILE)
> > +	$(call QUIET_INSTALL, $(PC_FILE)) \
> > +		$(call
> > do_install,$(PC_FILE),$(libdir_SQ)/pkgconfig,644)
> > +
> > +
> > +install: install_lib install_pkgconfig
> 
> Should it be part of 'install'? Not everyone needs pc file.  Also pc
> template below relies on headers to be installed along with the
> library
> itself, but headers are not installed as part of 'install' target,
> they're in a separate 'install_headers' one, so 'install_pkgconfig'
> and
> 'install_headers' should probably be consistent in this regard.

I believe it should be part of the install rule - if somebody does not
need the pc file, they can simply ignore it - it's just a small text
file, it won't break anything. The vast majority of libraries that have
a pc file just ship it, and users that don't want to use them, just
don't.

The reason to always ship it is so that downstream projects can rely on
it existing if libbpf is installed, without having to do awkward dances
and fallbacks if packagers forget or fail to notice that there's an
additional special install rule. In other words, a Meson project can
rely on "dependency('libbpf')" and an autotools project can rely on
"PKG_CHECK_MODULES([libbpf], [libbpf]" being accurate and
authoritative. Which doesn't mean it's mandatory to use - just that
there are strong guarantees for users and distributors, if they choose
to use those macros and tools. It makes things simpler and nicer for
them, and "just work" in the most common use case.

And it's not dependent on install_headers if we change the include flag
- -I/usr/include actually gets automatically pruned by pkg-config --
cflags. It is dependent on it if we keep -I/usr/include/bpf, but I'll
change it as you suggested, more on that below.

> >  ### Cleaning rules
> >  
> > @@ -219,7 +231,7 @@ config-clean:
> >  
> >  clean:
> >  	$(call QUIET_CLEAN, libbpf) $(RM) $(TARGETS) $(CXX_TEST_TARGET)
> > \
> > -		*.o *~ *.a *.so .*.d .*.cmd LIBBPF-CFLAGS
> > +		*.o *~ *.a *.so .*.d .*.cmd *.pc LIBBPF-CFLAGS
> >  	$(call QUIET_CLEAN, core-gen) $(RM) $(OUTPUT)FEATURE-
> > DUMP.libbpf
> >  
> >  
> > diff --git a/tools/lib/bpf/libbpf.pc.template
> > b/tools/lib/bpf/libbpf.pc.template
> > new file mode 100644
> > index 000000000000..0cac2f5f54a6
> > --- /dev/null
> > +++ b/tools/lib/bpf/libbpf.pc.template
> > @@ -0,0 +1,11 @@
> > +prefix=@...FIX@
> > +libdir=@...DIR@
> > +includedir=${prefix}/include/bpf
> 
> I guess it should be includedir=${prefix}/include.
> 
> Paths in '#include' are usually provided relative to
> ${prefix}/include,
> e.g. all programs I have access to and that use libbpf do this:
> 
>   #include <bpf/bpf.h>
>   #include <bpf/libbpf.h>
> 
> 
> But `includedir=${prefix}/include/bpf` will force it to be:
>   #include <bpf.h>
>   #include <libbpf.h>
> 
> , what may create inconsistency with already written code.

Given /usr/include is always included by the compiler, one can actually
use both ways, even in the same program, and it seems to work fine
(just tested to be 100% sure) - so there's no breakage, but there is
certainly inconsistency, especially as you say with existing code, as
the pc file was not there from day 1. And I like consistency too :-)

> I also checked .pc files on my devbox and this is what I see:
> 
>   % grep -h includedir /usr/share/pkgconfig/*.pc | sort | uniq -c
>        28 Cflags: -I${includedir}
>         1 Cflags: -I${includedir}/X11/dri
>        29 includedir=/usr/include

pkg-config files from libraries are installed under
/usr/lib/*/pkgconfig, rather than /usr/share, if you run the command
there you'll see:

$ grep -h includedir /usr/lib/*/pkgconfig/*.pc | sort | uniq -c
      2 archincludedir=${includedir}/${arch}
     85 Cflags: -I${includedir}
      9 Cflags: -I${includedir} 
      1 Cflags: -I${includedir}  
      1 Cflags: -I${includedir}/atk-1.0
      1 Cflags: -I${includedir}/at-spi-2.0
      1 Cflags: -I${includedir}/at-spi2-atk/2.0
      1 Cflags: -I${includedir}/blkid
      1 Cflags: -I${includedir}/cairo
     13 Cflags: -I${includedir}/cairo 
      1 Cflags: -I${includedir}/dbus-1.0
      1 Cflags: -I${includedir}/dbus-1.0 -I${libdir}/dbus-1.0/include 
      3 Cflags: -I${includedir} -D_REENTRANT
      1 Cflags: -I${includedir}/et -I${includedir}
      1 Cflags: -I${includedir}/freetype2
      1 Cflags: -I${includedir}/fribidi
      1 Cflags: -I${includedir}/gck-1
      2 Cflags: -I${includedir}/gcr-3
      2 Cflags: -I${includedir}/gdk-pixbuf-2.0
      1 Cflags: -I${includedir}/gio-unix-2.0
      1 Cflags: -I${includedir}/glib-2.0 -I${libdir}/glib-2.0/include
      2 Cflags: -I${includedir}/gtk-2.0 
      2 Cflags: -I${includedir}/gtk-2.0 -I${libdir}/gtk-2.0/include 
      8 Cflags: -I${includedir}/gtk-3.0 
      1 Cflags: -I${includedir}/gtk-3.0/unix-print
      1 Cflags: -I${includedir}/gtk-unix-print-2.0
      3 Cflags: -I${includedir}/harfbuzz
      1 Cflags: -I${includedir} -I${includedir}/alsa
      4 Cflags: -I${includedir} -I${includedir}/libdrm
      1 Cflags: -I${includedir} -I${includedir}/libdrm -I${includedir}/libdrm/nouveau
      1 Cflags: -I${includedir}/libmount
      2 Cflags: -I${includedir}/libnl3
      1 Cflags: -I${includedir}/libnm
      1 Cflags: -I${includedir}/libsecret-1
      1 Cflags: -I${includedir}/libxml2 
      1 Cflags: -I${includedir}/p11-kit-1
      4 Cflags: -I${includedir}/pango-1.0
      1 Cflags: -I${includedir}/pgm-5.2
      1 Cflags: -I${includedir}/pixman-1
      3 Cflags: -I${includedir} -pthread
      3 Cflags: -I${includedir}/python2.7  -I${includedir}/x86_64-linux-gnu/python2.7
      3 Cflags: -I${includedir}/python3.7m -I${includedir}/x86_64-linux-gnu/python3.7m
      1 Cflags: -I${includedir}/SDL -D_GNU_SOURCE=1 -D_REENTRANT
      1 Cflags: -I${includedir}/uuid
      6 Cflags: -isystem ${includedir}
      1 Cflags: -isystem ${includedir}/bsd -DLIBBSD_OVERLAY
      1 # -I${includedir}/alsa below is just for backward compatibility
      3 includedir = ${prefix}/include
    171 includedir=${prefix}/include
      1 includedir=${prefix}/include/ayatana-messaging-menu
      2 includedir=${prefix}/include/libpng16
      6 includedir=${prefix}/include/mit-krb5
      1 includedir=${prefix}/include/PCSC
      4 includedir=${prefix}/include/x86_64-linux-gnu
     19 includedir=/usr/include
      2 rubyarchhdrdir=${archincludedir}/${RUBY_VERSION_NAME}
      2 rubyhdrdir=${includedir}/${RUBY_VERSION_NAME}
      2 sitearchhdrdir=${sitearchincludedir}/${RUBY_VERSION_NAME}/site_ruby
      2 sitearchincludedir=${includedir}/${sitearch}
      2 vendorarchhdrdir=${sitearchincludedir}/${RUBY_VERSION_NAME}/vendor_ruby

$ grep -h includedir /usr/lib/pkgconfig/*.pc | sort | uniq -c
      1 Cflags: -I${includedir}/libpurple
      1 Cflags: -I${includedir}/pidgin
      2 includedir=${prefix}/include


The vast majority does just include the canonical /usr/include path,
but it's not uncommon to include the subdirectory as well. That's done
so that users can include just the headers, which will work on any
platform, and also when there are multiple source versions of the same
library available on the system at the same time, and then a user picks
the right pkg-config to parse at build time - the code doesn't have to
change the include path. But it is by no mean a hard rule, and it's up
to the individual developers.

Having said that, none of those apply at the moment and I don't see
that they will in the future - certainly there's no multi-platform
requirement, and hopefully there won't be backward-incompatible API
breakages, hint hint :-)

And there is a good reason to just include /usr/include - so that pkg-
config omits it by default, and there's no dependency on the headers
actually being installed (if -I/usr/include/bpf is passed, then
compiling with werror fails if the directory doesn't exist).

So I've changed the path in v4 to just -I${prefix}/include.

Thanks for the review!

-- 
Kind regards,
Luca Boccassi

Powered by blists - more mailing lists