| Message ID | 20190320132828.30131-1-luca.boccassi@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next,v3] tools/bpf: generate pkg-config file for libbpf | expand |
luca.boccassi@gmail.com <luca.boccassi@gmail.com> [Wed, 2019-03-20 06:28 -0700]: > From: Luca Boccassi <bluca@debian.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@debian.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. Just wondering if you plan to create a package using kernel tree? > @@ -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 '[^ ]+@LIBBPF_' | 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|@PREFIX@|$(prefix)|" \ > + -e "s|@LIBDIR@|$(libdir_SQ)|" \ > + -e "s|@VERSION@|$(LIBBPF_VERSION)|" \ > + < $@.template > $@ > + > 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. > ### 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=@PREFIX@ > +libdir=@LIBDIR@ > +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. 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 > + > +Name: libbpf > +URL: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > +Description: Linux kernel BPF library > +Version: @VERSION@ > +Libs: -L${libdir} -lbpf > +Requires.private: libelf > +Cflags: -I${includedir} > -- > 2.20.1 >
On Wed, 2019-03-20 at 23:58 +0000, Andrey Ignatov wrote: > luca.boccassi@gmail.com <luca.boccassi@gmail.com> [Wed, 2019-03-20 > 06:28 -0700]: > > From: Luca Boccassi <bluca@debian.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@debian.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 '[^ ]+@LIBBPF_' | 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|@PREFIX@|$(prefix)|" \ > > + -e "s|@LIBDIR@|$(libdir_SQ)|" \ > > + -e "s|@VERSION@|$(LIBBPF_VERSION)|" \ > > + < $@.template > $@ > > + > > 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=@PREFIX@ > > +libdir=@LIBDIR@ > > +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!
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 @@ -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 '[^ ]+@LIBBPF_' | 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|@PREFIX@|$(prefix)|" \ + -e "s|@LIBDIR@|$(libdir_SQ)|" \ + -e "s|@VERSION@|$(LIBBPF_VERSION)|" \ + < $@.template > $@ + 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 ### 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=@PREFIX@ +libdir=@LIBDIR@ +includedir=${prefix}/include/bpf + +Name: libbpf +URL: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git +Description: Linux kernel BPF library +Version: @VERSION@ +Libs: -L${libdir} -lbpf +Requires.private: libelf +Cflags: -I${includedir}