From: Quentin Monnet <firstname.lastname@example.org> To: Andrii Nakryiko <email@example.com> Cc: Alexei Starovoitov <firstname.lastname@example.org>, Daniel Borkmann <email@example.com>, Andrii Nakryiko <firstname.lastname@example.org>, Networking <email@example.com>, bpf <firstname.lastname@example.org> Subject: Re: [PATCH bpf-next v2 6/9] bpf: iterators: install libbpf headers when building Date: Tue, 5 Oct 2021 21:03:23 +0100 [thread overview] Message-ID: <CACdoK4KC2Y69Sj2KmEFHtZctWeZfvUnckRY4Q1hpomHHr0CfDA@mail.gmail.com> (raw) In-Reply-To: <CACdoK4JpgKyeFAwwY=8V-WQO405-xkW+yS2qnfVv2tgoF-F3JA@mail.gmail.com> On Mon, 4 Oct 2021 at 22:30, Quentin Monnet <email@example.com> wrote: > > On Mon, 4 Oct 2021 at 20:11, Andrii Nakryiko <firstname.lastname@example.org> wrote: > > > > On Sat, Oct 2, 2021 at 3:12 PM Quentin Monnet <email@example.com> wrote: > > > > > > On Sat, 2 Oct 2021 at 21:27, Quentin Monnet <firstname.lastname@example.org> wrote: > > > > > > > > On Sat, 2 Oct 2021 at 00:20, Andrii Nakryiko <email@example.com> wrote: > > > > > > > > > > On Fri, Oct 1, 2021 at 4:09 AM Quentin Monnet <firstname.lastname@example.org> wrote: > > > > > > > > > > > > API headers from libbpf should not be accessed directly from the > > > > > > library's source directory. Instead, they should be exported with "make > > > > > > install_headers". Let's make sure that bpf/preload/iterators/Makefile > > > > > > installs the headers properly when building. > > > > > > > > > > > > > > > > -$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(OUTPUT) > > > > > > +$(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) \ > > > > > > + | $(LIBBPF_OUTPUT) $(LIBBPF_INCLUDE) > > > > > > > > > > Would it make sense for libbpf's Makefile to create include and output > > > > > directories on its own? We wouldn't need to have these order-only > > > > > dependencies everywhere, right? > > > > > > > > Good point, I'll have a look at it. > > > > Quentin > > > > > > So libbpf already creates the include (and parent $(DESTDIR)) > > > directory, so I can get rid of the related dependencies. But I don't > > > see an easy solution for the output directory for the object files. > > > The issue is that libbpf's Makefile includes > > > tools/scripts/Makefile.include, which checks $(OUTPUT) and errors out > > > > Did you check what benefits the use of tools/scripts/Makefile.include > > brings? Last time I had to deal with some non-trivial Makefile > > problem, this extra dance with tools/scripts/Makefile.include and some > > related complexities didn't seem very justified. So unless there are > > some very big benefits to having tool's Makefile.include included, I'd > > rather simplify libbpf's in-kernel Makefile and make it more > > straightforward. We have a completely independent separate Makefile > > for libbpf in Github, and I think it's more straightforward. Doesn't > > have to be done in this change, of course, but I was curious to hear > > your thoughts given you seem to have spent tons of time on this > > already. > > No, I haven't checked in details so far. I remember that several > elements defined in the Makefile.include are used in libbpf's > Makefile, and I stopped at that, because I thought that a refactoring > of the latter would be beyond the current set. But yes, I can have a > look at it and see if it's worth changing in a follow-up. Looking more at tools/scripts/Makefile.include: It's 160-line long and does not include any other Makefile, so there's nothing in it that we couldn't re-implement in libbpf's Makefile if necessary. This being said, it has a number of items that, I think, are good to keep there and share with the other tools. For example: - The $(EXTRA_WARNINGS) definitions - QUIET_GEN, QUIET_LINK, QUIET_CLEAN, which are not mandatory to have but integrate nicely with the way other tools (or kernel components) are built - Some overwrites for the toolchain, if $(LLVM) or $(CROSS_COMPILE) are set Thinking more about this, if we want to create the $(OUTPUT) directory in libbpf itself, we could maybe just enclose the check on its pre-existence in tools/scripts/Makefile.include with a dedicated variable ("ifneq ($(_skip_output_check),) ...") and set the latter in Makefile.include. This way we wouldn't have to change the current Makefile infra too much, and can keep the include. Quentin
next prev parent reply other threads:[~2021-10-05 20:03 UTC|newest] Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-10-01 11:08 [PATCH bpf-next v2 0/9] install libbpf headers when using the library Quentin Monnet 2021-10-01 11:08 ` [PATCH bpf-next v2 1/9] tools: bpftool: remove unused includes to <bpf/bpf_gen_internal.h> Quentin Monnet 2021-10-01 11:08 ` [PATCH bpf-next v2 2/9] tools: bpftool: install libbpf headers instead of including the dir Quentin Monnet 2021-10-01 22:55 ` Andrii Nakryiko 2021-10-01 11:08 ` [PATCH bpf-next v2 3/9] tools: resolve_btfids: install libbpf headers when building Quentin Monnet 2021-10-01 23:01 ` Andrii Nakryiko 2021-10-01 11:08 ` [PATCH bpf-next v2 4/9] tools: runqslower: " Quentin Monnet 2021-10-01 11:08 ` [PATCH bpf-next v2 5/9] bpf: preload: " Quentin Monnet 2021-10-01 11:08 ` [PATCH bpf-next v2 6/9] bpf: iterators: " Quentin Monnet 2021-10-01 23:19 ` Andrii Nakryiko 2021-10-02 20:27 ` Quentin Monnet 2021-10-02 22:11 ` Quentin Monnet 2021-10-04 19:10 ` Andrii Nakryiko 2021-10-04 21:30 ` Quentin Monnet 2021-10-05 20:03 ` Quentin Monnet [this message] 2021-10-06 17:44 ` Andrii Nakryiko 2021-10-01 11:08 ` [PATCH bpf-next v2 7/9] samples/bpf: " Quentin Monnet 2021-10-01 23:22 ` Andrii Nakryiko 2021-10-02 20:30 ` Quentin Monnet 2021-10-01 11:08 ` [PATCH bpf-next v2 8/9] samples/bpf: update .gitignore Quentin Monnet 2021-10-01 11:08 ` [PATCH bpf-next v2 9/9] selftests/bpf: better clean up for runqslower in test_bpftool_build.sh Quentin Monnet 2021-10-01 23:05 ` [PATCH bpf-next v2 0/9] install libbpf headers when using the library Andrii Nakryiko 2021-10-01 23:27 ` Andrii Nakryiko 2021-10-02 20:40 ` Quentin Monnet 2021-10-03 19:20 ` Quentin Monnet
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CACdoK4KC2Y69Sj2KmEFHtZctWeZfvUnckRY4Q1hpomHHr0CfDA@mail.gmail.com \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH bpf-next v2 6/9] bpf: iterators: install libbpf headers when building' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).