Re: [PATCH bpf-next 4/4] kbuild: Add resolve_btfids clean to root clean target

From: Masahiro Yamada <masahiroy@kernel.org>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	Nathan Chancellor <nathan@kernel.org>,
	Jiri Olsa <jolsa@kernel.org>, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andriin@fb.com>,
	Song Liu <songliubraving@fb.com>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Yonghong Song <yhs@fb.com>,
	John Fastabend <john.fastabend@gmail.com>,
	KP Singh <kpsingh@chromium.org>,
	Michal Marek <michal.lkml@markovi.net>,
	Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>
Subject: Re: [PATCH bpf-next 4/4] kbuild: Add resolve_btfids clean to root clean target
Date: Fri, 12 Feb 2021 12:30:45 +0900	[thread overview]
Message-ID: <CAK7LNAT8oTvLJ9FRsrRB5GUS2K+y2QY36Wshb9x1YE5d=ZyA5g@mail.gmail.com> (raw)
In-Reply-To: <YCUgUlCDGTS85MCO@krava>

On Thu, Feb 11, 2021 at 9:17 PM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Feb 10, 2021 at 11:26:28AM -0800, Andrii Nakryiko wrote:
>
> SNIP
>
> > > > > Can't reproduce it. It works in all kinds of variants (relative and
> > > > > absolute O=, clean and not clean trees, etc). Jiri, please check as
> > > > > well.
> > > > >
> > > >
> > > > Odd, this reproduces for me on a completely clean checkout of bpf-next:
> > > >
> > > > $ git clone --depth=1 https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/
> > > >
> > > > $ cd bpf-next
> > > >
> > > > $ make -s O=build distclean
> > > > ../../scripts/Makefile.include:4: *** O=/tmp/bpf-next/build/tools/bpf/resolve_btfids does not exist.  Stop.
> > > >
> > > > I do not really see how this could be environment related. It seems like
> > > > this comes from tools/scripts/Makefile.include, where there is no
> > > > guarantee that $(O) is created before being used like in the main
> > > > Makefile?
> > >
> > > right, we need to handle the case where tools/bpf/resolve_btfids
> > > does not exist, patch below fixes it for me
> > >
> > > jirka
> > >
> >
> > Looks good to me, please send it as a proper patch to bpf-next.
> >
> > But I'm curious, why is objtool not doing something like that? Is it
> > not doing clean at all? Or does it do it in some different way?
>
> yes, it's not connected to global make clean
>
> >
> > >
> > > ---
> > > diff --git a/Makefile b/Makefile
> > > index 159d9592b587..ce9685961abe 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -1088,8 +1088,14 @@ endif
> > >
> > >  PHONY += resolve_btfids_clean
> > >
> > > +resolve_btfids_O = $(abspath $(objtree))/tools/bpf/resolve_btfids
> > > +
> > > +# tools/bpf/resolve_btfids directory might not exist
> > > +# in output directory, skip its clean in that case
> > >  resolve_btfids_clean:
> > > -       $(Q)$(MAKE) -sC $(srctree)/tools/bpf/resolve_btfids O=$(abspath $(objtree))/tools/bpf/resolve_btfids clean
> > > +ifneq (,$(wildcard $(resolve_btfids_O)))
> >
> > nit: kind of backwards, usually it's in a `ifneq($var,)` form
>
> ok
>
> thanks,
> jirka
>

I expected this kind of mess
when I saw 33a57ce0a54d498275f432db04850001175dfdfa

The tools/ directory is a completely different world
governed by a different build system
(no, not a build system, but a collection of adhoc makefile code)

All the other programs used during the kernel build
are located under scripts/, and can be built with
a simple syntax, and cleaned up correctly.
It is simple, clean and robust.

objtool is the first alien that opt out Kbuild,
and this is the second one.

It is scary to mix up two different things,
which run in different working directories.

See, this is wired up in the top Makefile
in an ugly way, and you are struggling
in suppressing issues, where you can never
do it in the right way.

-- 
Best Regards
Masahiro Yamada