On Mon, 2021-01-04 at 12:23 -0800, Andrii Nakryiko wrote: > 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. 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
Attachment:
signature.asc
Description: This is a digitally signed message part
