Re: [PATCH bpf-next v3 00/11] tools: Use consistent libbpf include paths everywhere

[Toke HÃiland-JÃrgensen]

Jesper Dangaard Brouer <brouer@xxxxxxxxxx> writes:

> On Thu, 16 Jan 2020 20:14:32 -0800
> Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote:
>
>> On Thu, Jan 16, 2020 at 02:22:11PM +0100, Toke HÃiland-JÃrgensen wrote:
>> > The recent commit 6910d7d3867a ("selftests/bpf: Ensure bpf_helper_defs.h are
>> > taken from selftests dir") broke compilation against libbpf if it is installed
>> > on the system, and $INCLUDEDIR/bpf is not in the include path.
>> > 
>> > Since having the bpf/ subdir of $INCLUDEDIR in the include path has never been a
>> > requirement for building against libbpf before, this needs to be fixed. One
>> > option is to just revert the offending commit and figure out a different way to
>> > achieve what it aims for.   
>> 
>> The offending commit has been in the tree for a week. So I applied Andrii's
>> revert of that change. It reintroduced the build dependency issue, but we lived
>> with it for long time, so we can take time to fix it cleanly.
>> I suggest to focus on that build dependency first.
>> 
>> > However, this series takes a different approach:
>> > Changing all in-tree users of libbpf to consistently use a bpf/ prefix in
>> > #include directives for header files from libbpf.  
>> 
>> I'm not sure it's a good idea. It feels nice, but think of a message we're
>> sending to everyone. We will get spamed with question: does bpf community
>> require all libbpf users to use bpf/ prefix ? What should be our answer?
>
> The answer should be: Yes. When libbpf install the header files the are
> installed under bpf/ prefix.  It is very confusing that samples and
> selftests can include libbpf.h without this prefix. Even worse
> including "bpf.h" pickup the libbpf version bpf/bpf.h, which have
> caused confusion.  The only reason for the direct "libbpf.h" include is
> historical, as there used-to-be a local file for that.

Agreed. Also, we are already telling people what the right include path
is in at least two ways - and currently they are incompatible:

- The pkg-config file included with libbpf has a notion of include path;
  which does *not* include the bpf/ subdirectory.

- The skeleton generator puts an '#include <libbpf.h>' line into the
  generated files.

With this series we'll at least be consistent.

>> Require or recommend? If require.. what for? It works as-is. If recommend then
>> why suddenly we're changing all files in selftests and samples?
>> There is no good answer here. I think we should leave the things as-is.
>
> I strongly believe we should correct this.  It doesn't make sense that
> someone copying out a sample or selftests, into a git-submodule libbpf
> (or distro installed libbpf-devel) have to understand that they have to
> update the include path for all the libbpf header files.

Yeah, I think being clear and explicit about what is the recommended way
to include libbpf is strictly an improvement. And making it possible to
move example programs seamlessly in and out of the kernel tree will only
make things easier for people.

I'll rebase and respin this series on top of the revert (and fix
Andrii's comments).

-Toke