On Mon, 2021-01-04 at 12:23 -0800, Andrii Nakryiko wrote:
> 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.
> > 
> > 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 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.

Sure, that works for me if it's acceptable as a solution. Sent v3 that
implements it. Thanks for the suggestion.

-- 
Kind regards,
Luca Boccassi