Re: [PATCH dwarves] libbpf: allow to use packaged version

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Mon, Jan 04, 2021 at 12:23:25PM -0800, Andrii Nakryiko escreveu:
> On Sun, Jan 3, 2021 at 1:30 PM Luca Boccassi <bluca@debian.org> wrote:
> >
> > 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.

Hey, having tools/perf/ and tools/lib/perf, tools/lib/bpf, etc all in
one place is really nice :-)

> > You know very well that it's not about caring or not caring :-) There
> > are simply conflicting interests, and both sides are valid.
 
> I didn't mean it in a negative way. Different priorities and interests
> is a better way to put it, sure.

I think the default should be submodules as we're still very much in
flux, adding new features, etc.

Disto maintainers can make sure they know what they're doing when making
tools use libbpf as a separate package.

I'm coming back from vacation, so trying to zip thru a lot of emails, so
just thought about sharing my first reaction to this in a quick way.

Regards,

- Arnaldo
 
> > 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.
> 
> It's quite risky, as there are plenty of (sometimes private)
> integrations that assume such a layout of libbpf, so we'll be breaking
> them badly. Makefile-based projects do `make install_headers` to put
> *only exported* headers into the desired location. I'm not sure what's
> the best way to achieve the same with Cmake, though.
> 
> One quick and easy hack would be to put a symlink lib/include/bpf ->
> lib/bpf/src into pahole repo. And add -Ilib/include to let compiler
> handle <bpf/*.h> properly.
> 
> >
> > --
> > Kind regards,
> > Luca Boccassi

-- 

- Arnaldo