On Sun, Jan 3, 2021 at 1:30 PM Luca Boccassi <bluca@xxxxxxxxxx> 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@xxxxxxxxxx> > > 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@xxxxxxxxxx> > > > --- > > > 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
