Re: [PATCH v1] tools/resolve_btfids: Install subcmd headers

[Ian Rogers <irogers@google.com>]

On Mon, Jan 16, 2023 at 3:22 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Jan 11, 2023 at 04:40:24PM -0800, Ian Rogers wrote:
> > Previously tools/lib/subcmd was added to the include path, switch to
> > installing the headers and then including from that directory. This
> > avoids dependencies on headers internal to tools/lib/subcmd. Add the
> > missing subcmd directory to the affected #include.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/bpf/resolve_btfids/Makefile | 19 ++++++++++++++-----
> >  tools/bpf/resolve_btfids/main.c   |  2 +-
> >  2 files changed, 15 insertions(+), 6 deletions(-)
>
> this depends on changes that went to Arnaldo's tree right?
> I can't apply this on bpf-next/master

Hmm.. sorry for that. I did the work on the master branch of
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

> >
> > diff --git a/tools/bpf/resolve_btfids/Makefile b/tools/bpf/resolve_btfids/Makefile
> > index 19a3112e271a..de7d29cf43d6 100644
> > --- a/tools/bpf/resolve_btfids/Makefile
> > +++ b/tools/bpf/resolve_btfids/Makefile
> > @@ -35,21 +35,29 @@ SUBCMD_SRC := $(srctree)/tools/lib/subcmd/
> >  BPFOBJ     := $(OUTPUT)/libbpf/libbpf.a
> >  LIBBPF_OUT := $(abspath $(dir $(BPFOBJ)))/
> >  SUBCMDOBJ  := $(OUTPUT)/libsubcmd/libsubcmd.a
> > +SUBCMD_OUT := $(abspath $(dir $(SUBCMDOBJ)))/
> >
> >  LIBBPF_DESTDIR := $(LIBBPF_OUT)
> >  LIBBPF_INCLUDE := $(LIBBPF_DESTDIR)include
> >
> > +SUBCMD_DESTDIR := $(SUBCMD_OUT)
> > +SUBCMD_INCLUDE := $(SUBCMD_DESTDIR)include
> > +
> >  BINARY     := $(OUTPUT)/resolve_btfids
> >  BINARY_IN  := $(BINARY)-in.o
> >
> >  all: $(BINARY)
> >
> > +prepare: $(SUBCMDOBJ)
>
> do we need special target for that? we already have BPFOBJ dependency
> placed in the BINARY_IN as prereq

BPFOBJ is $(OUTPUT)/libbpf/libbpf.a which is needed at link time. The
prepare step is one we have elsewhere and it creates things like the
header files necessary to compile the C code, so we need it earlier is
the answer.

> why not place both as BINARY_IN prereq, or is there some other reason
> for new 'prepare' target?

I was trying to follow the convention elsewhere in the tree of having
a prepare target that does things like get the necessary header files
ready. Having prepare is useful because if an additional dependency is
added, then it just needs to be added to prepare. It could be tedious
to list all the dependencies for every C file, although Makefile.build
handles most of that. It isn't clear to me why $(BPFOBJ) is a
dependency of $(BINARY_IN) as it is already a dependency of $(BINARY).

Thanks,
Ian

> jirka
>
> > +
> >  $(OUTPUT) $(OUTPUT)/libsubcmd $(LIBBPF_OUT):
> >       $(call msg,MKDIR,,$@)
> >       $(Q)mkdir -p $(@)
> >
> >  $(SUBCMDOBJ): fixdep FORCE | $(OUTPUT)/libsubcmd
> > -     $(Q)$(MAKE) -C $(SUBCMD_SRC) OUTPUT=$(abspath $(dir $@))/ $(abspath $@)
> > +     $(Q)$(MAKE) -C $(SUBCMD_SRC) OUTPUT=$(SUBCMD_OUT) \
> > +                 DESTDIR=$(SUBCMD_DESTDIR) prefix= \
> > +                 $(abspath $@) install_headers
> >
> >  $(BPFOBJ): $(wildcard $(LIBBPF_SRC)/*.[ch] $(LIBBPF_SRC)/Makefile) | $(LIBBPF_OUT)
> >       $(Q)$(MAKE) $(submake_extras) -C $(LIBBPF_SRC) OUTPUT=$(LIBBPF_OUT)    \
> > @@ -60,14 +68,14 @@ CFLAGS += -g \
> >            -I$(srctree)/tools/include \
> >            -I$(srctree)/tools/include/uapi \
> >            -I$(LIBBPF_INCLUDE) \
> > -          -I$(SUBCMD_SRC)
> > +          -I$(SUBCMD_INCLUDE)
> >
> >  LIBS = -lelf -lz
> >
> >  export srctree OUTPUT CFLAGS Q
> >  include $(srctree)/tools/build/Makefile.include
> >
> > -$(BINARY_IN): $(BPFOBJ) fixdep FORCE | $(OUTPUT)
> > +$(BINARY_IN): $(BPFOBJ) fixdep FORCE prepare | $(OUTPUT)
> >       $(Q)$(MAKE) $(build)=resolve_btfids
> >
> >  $(BINARY): $(BPFOBJ) $(SUBCMDOBJ) $(BINARY_IN)
> > @@ -79,7 +87,8 @@ clean_objects := $(wildcard $(OUTPUT)/*.o                \
> >                              $(OUTPUT)/.*.o.d             \
> >                              $(LIBBPF_OUT)                \
> >                              $(LIBBPF_DESTDIR)            \
> > -                            $(OUTPUT)/libsubcmd          \
> > +                            $(SUBCMD_OUT)                \
> > +                            $(SUBCMD_DESTDIR)            \
> >                              $(OUTPUT)/resolve_btfids)
> >
> >  ifneq ($(clean_objects),)
> > @@ -96,4 +105,4 @@ tags:
> >
> >  FORCE:
> >
> > -.PHONY: all FORCE clean tags
> > +.PHONY: all FORCE clean tags prepare
> > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> > index 80cd7843c677..77058174082d 100644
> > --- a/tools/bpf/resolve_btfids/main.c
> > +++ b/tools/bpf/resolve_btfids/main.c
> > @@ -75,7 +75,7 @@
> >  #include <linux/err.h>
> >  #include <bpf/btf.h>
> >  #include <bpf/libbpf.h>
> > -#include <parse-options.h>
> > +#include <subcmd/parse-options.h>
> >
> >  #define BTF_IDS_SECTION      ".BTF_ids"
> >  #define BTF_ID               "__BTF_ID__"
> > --
> > 2.39.0.314.g84b9a713c41-goog
> >