On Sun, 2021-01-03 at 11:10 -0800, Andrii Nakryiko wrote:
> On Sat, Jan 2, 2021 at 10:25 AM Luca Boccassi <bluca@debian.org>
> wrote:
> > 
> > Add a new CMake option, LIBBPF_EMBEDDED, to switch between the
> > embedded version and the system version (searched via pkg-config)
> > of libbpf. Set the system version as the default.
> > 
> 
> There's been a lot of arguments about libbpf as a submodule, so I
> don't think we need to go about pros and cons again, but I just
> wanted
> to cast my vote against doing this at all. Having pahole built with
> libbpf statically (only) was a great thing for me so far with
> iterating quickly and adopting new APIs without overcomplicating the
> source code with all sorts of feature detection code. Without it,
> adopting libbpf's faster string deduplication, BTF writing APIs,
> module/split BTFs, etc, etc, would be much bigger PITA and/or would
> prolong such changes. To the point that I personally wouldn't bother
> with some of them at all. Distro maintainers obviously don't care
> about such inconveniences for developers, but it's a real factor in
> determining what kind of functionality is implemented in pahole, so I
> hope Arnaldo won't dismiss this without thinking about this
> carefully.

You know very well that it's not about caring or not caring :-) There
are simply conflicting interests, and both sides are valid.
I believe we can have best of both worlds with this. The user who does
the build is empowered to choose what they prefer. The additional cost
would be to set the correct minimum version required in the CMake file,
as it's done with other dependencies, including CMake itself.

> > Signed-off-by: Luca Boccassi <bluca@debian.org>
> > ---
> > Note: I can switch the default around if you prefer.
> 
> With BCC you seem to go with the default preserving existing behavior
> (using libbpf from a submodule), so if we do this, I think the
> default
> should be flipped.

Sure, sent v2.

> > 
> >  CMakeLists.txt   | 49 +++++++++++++++++++++++++++++++++++---------
> > ----
> >  btf_encoder.c    |  5 +++++
> >  btf_loader.c     |  4 ++++
> >  libbtf.c         |  6 ++++++
> >  libbtf.h         |  4 ++++
> >  pahole.c         |  4 ++++
> >  pahole_strings.h |  4 ++++
> >  strings.c        |  4 ++++
> >  8 files changed, 67 insertions(+), 13 deletions(-)
> > 
> 
> [...]
> 
> >  #include "dutil.h"
> > +#ifdef LIBBPF_FOUND
> > +#include <bpf/libbpf.h>
> > +#else
> >  #include "lib/bpf/src/libbpf.h"
> > +#endif
> 
> this is horrible, are you sure there is no way to make <bpf/libbpf.h>
> work for both cases?

It really is, but unfortunately I don't see other ways that are not
just as horrible. Suggestions welcome. The only thing I can think of,
is if libbpf used the same directory hierarchy in-tree as it does in
the installed tree. Ie: move those headers from libbpf/src/foo.h to
libbpf/include/bpf/foo.h. Then we would just have the -Ilib/bpf/include
in CPPFLAGS for the embedded build defined in the CMake files, and the
sources would only use the "system" includes everywhere, without
ifdeffery.

Given you maintain libbpf, would that be something you'd accept? It's
quite a common pattern for libraries to store their public headers in a
separate directory in the git tree, after all.

-- 
Kind regards,
Luca Boccassi