diff mbox series

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

Message ID 20210205124020.683286-5-jolsa@kernel.org (mailing list archive)
State Accepted
Delegated to: BPF
Headers show
Series kbuild/resolve_btfids: Invoke resolve_btfids clean in root Makefile | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 2 maintainers not CCed: kpsingh@kernel.org andrii@kernel.org
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning WARNING: Possible repeated word: 'the'
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Jiri Olsa Feb. 5, 2021, 12:40 p.m. UTC 
The resolve_btfids tool is used during the kernel build,
so we should clean it on kernel's make clean.

Invoking the the resolve_btfids clean as part of root
'make clean'.

Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 Makefile | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Amy Parker Feb. 6, 2021, 5:11 a.m. UTC | #1 
On Fri, Feb 5, 2021 at 4:46 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The resolve_btfids tool is used during the kernel build,
> so we should clean it on kernel's make clean.
>
> Invoking the the resolve_btfids clean as part of root
> 'make clean'.
>
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  Makefile | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index b0e4767735dc..159d9592b587 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1086,6 +1086,11 @@ ifdef CONFIG_STACK_VALIDATION
>    endif
>  endif
>
> +PHONY += resolve_btfids_clean
> +
> +resolve_btfids_clean:
> +       $(Q)$(MAKE) -sC $(srctree)/tools/bpf/resolve_btfids O=$(abspath $(objtree))/tools/bpf/resolve_btfids clean
> +
>  ifdef CONFIG_BPF
>  ifdef CONFIG_DEBUG_INFO_BTF
>    ifeq ($(has_libelf),1)
> @@ -1495,7 +1500,7 @@ vmlinuxclean:
>         $(Q)$(CONFIG_SHELL) $(srctree)/scripts/link-vmlinux.sh clean
>         $(Q)$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) clean)
>
> -clean: archclean vmlinuxclean
> +clean: archclean vmlinuxclean resolve_btfids_clean
>
>  # mrproper - Delete all generated files, including .config
>  #
> --
> 2.26.2
>

It compiles, looks good to me.

Reviewed by: Amy Parker <enbyamy@gmail.com>

Have a great day, and thank you for this patch!

   -Amy IP
Nathan Chancellor Feb. 10, 2021, 5:44 p.m. UTC | #2 
On Fri, Feb 05, 2021 at 01:40:20PM +0100, Jiri Olsa wrote:
> The resolve_btfids tool is used during the kernel build,
> so we should clean it on kernel's make clean.
> 
> Invoking the the resolve_btfids clean as part of root
> 'make clean'.
> 
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  Makefile | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index b0e4767735dc..159d9592b587 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1086,6 +1086,11 @@ ifdef CONFIG_STACK_VALIDATION
>    endif
>  endif
>  
> +PHONY += resolve_btfids_clean
> +
> +resolve_btfids_clean:
> +	$(Q)$(MAKE) -sC $(srctree)/tools/bpf/resolve_btfids O=$(abspath $(objtree))/tools/bpf/resolve_btfids clean
> +
>  ifdef CONFIG_BPF
>  ifdef CONFIG_DEBUG_INFO_BTF
>    ifeq ($(has_libelf),1)
> @@ -1495,7 +1500,7 @@ vmlinuxclean:
>  	$(Q)$(CONFIG_SHELL) $(srctree)/scripts/link-vmlinux.sh clean
>  	$(Q)$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) clean)
>  
> -clean: archclean vmlinuxclean
> +clean: archclean vmlinuxclean resolve_btfids_clean
>  
>  # mrproper - Delete all generated files, including .config
>  #
> -- 
> 2.26.2
> 

This breaks running distclean on a clean tree (my script just
unconditionally runs distclean regardless of the tree state):

$ make -s O=build distclean
../../scripts/Makefile.include:4: *** O=/home/nathan/cbl/src/linux-next/build/tools/bpf/resolve_btfids does not exist.  Stop.

Cheers,
Nathan
Andrii Nakryiko Feb. 10, 2021, 5:52 p.m. UTC | #3 
On Wed, Feb 10, 2021 at 9:47 AM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Fri, Feb 05, 2021 at 01:40:20PM +0100, Jiri Olsa wrote:
> > The resolve_btfids tool is used during the kernel build,
> > so we should clean it on kernel's make clean.
> >
> > Invoking the the resolve_btfids clean as part of root
> > 'make clean'.
> >
> > Acked-by: Song Liu <songliubraving@fb.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  Makefile | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index b0e4767735dc..159d9592b587 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1086,6 +1086,11 @@ ifdef CONFIG_STACK_VALIDATION
> >    endif
> >  endif
> >
> > +PHONY += resolve_btfids_clean
> > +
> > +resolve_btfids_clean:
> > +     $(Q)$(MAKE) -sC $(srctree)/tools/bpf/resolve_btfids O=$(abspath $(objtree))/tools/bpf/resolve_btfids clean
> > +
> >  ifdef CONFIG_BPF
> >  ifdef CONFIG_DEBUG_INFO_BTF
> >    ifeq ($(has_libelf),1)
> > @@ -1495,7 +1500,7 @@ vmlinuxclean:
> >       $(Q)$(CONFIG_SHELL) $(srctree)/scripts/link-vmlinux.sh clean
> >       $(Q)$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) clean)
> >
> > -clean: archclean vmlinuxclean
> > +clean: archclean vmlinuxclean resolve_btfids_clean
> >
> >  # mrproper - Delete all generated files, including .config
> >  #
> > --
> > 2.26.2
> >
>
> This breaks running distclean on a clean tree (my script just
> unconditionally runs distclean regardless of the tree state):
>
> $ make -s O=build distclean
> ../../scripts/Makefile.include:4: *** O=/home/nathan/cbl/src/linux-next/build/tools/bpf/resolve_btfids does not exist.  Stop.
>

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.

> Cheers,
> Nathan
Nathan Chancellor Feb. 10, 2021, 6:02 p.m. UTC | #4 
On Wed, Feb 10, 2021 at 09:52:42AM -0800, Andrii Nakryiko wrote:
> On Wed, Feb 10, 2021 at 9:47 AM Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > On Fri, Feb 05, 2021 at 01:40:20PM +0100, Jiri Olsa wrote:
> > > The resolve_btfids tool is used during the kernel build,
> > > so we should clean it on kernel's make clean.
> > >
> > > Invoking the the resolve_btfids clean as part of root
> > > 'make clean'.
> > >
> > > Acked-by: Song Liu <songliubraving@fb.com>
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  Makefile | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index b0e4767735dc..159d9592b587 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -1086,6 +1086,11 @@ ifdef CONFIG_STACK_VALIDATION
> > >    endif
> > >  endif
> > >
> > > +PHONY += resolve_btfids_clean
> > > +
> > > +resolve_btfids_clean:
> > > +     $(Q)$(MAKE) -sC $(srctree)/tools/bpf/resolve_btfids O=$(abspath $(objtree))/tools/bpf/resolve_btfids clean
> > > +
> > >  ifdef CONFIG_BPF
> > >  ifdef CONFIG_DEBUG_INFO_BTF
> > >    ifeq ($(has_libelf),1)
> > > @@ -1495,7 +1500,7 @@ vmlinuxclean:
> > >       $(Q)$(CONFIG_SHELL) $(srctree)/scripts/link-vmlinux.sh clean
> > >       $(Q)$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) clean)
> > >
> > > -clean: archclean vmlinuxclean
> > > +clean: archclean vmlinuxclean resolve_btfids_clean
> > >
> > >  # mrproper - Delete all generated files, including .config
> > >  #
> > > --
> > > 2.26.2
> > >
> >
> > This breaks running distclean on a clean tree (my script just
> > unconditionally runs distclean regardless of the tree state):
> >
> > $ make -s O=build distclean
> > ../../scripts/Makefile.include:4: *** O=/home/nathan/cbl/src/linux-next/build/tools/bpf/resolve_btfids does not exist.  Stop.
> >
> 
> 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?

Cheers,
Nathan
Jiri Olsa Feb. 10, 2021, 6:14 p.m. UTC | #5 
On Wed, Feb 10, 2021 at 09:52:42AM -0800, Andrii Nakryiko wrote:
> On Wed, Feb 10, 2021 at 9:47 AM Nathan Chancellor <nathan@kernel.org> wrote:
> >
> > On Fri, Feb 05, 2021 at 01:40:20PM +0100, Jiri Olsa wrote:
> > > The resolve_btfids tool is used during the kernel build,
> > > so we should clean it on kernel's make clean.
> > >
> > > Invoking the the resolve_btfids clean as part of root
> > > 'make clean'.
> > >
> > > Acked-by: Song Liu <songliubraving@fb.com>
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  Makefile | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index b0e4767735dc..159d9592b587 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -1086,6 +1086,11 @@ ifdef CONFIG_STACK_VALIDATION
> > >    endif
> > >  endif
> > >
> > > +PHONY += resolve_btfids_clean
> > > +
> > > +resolve_btfids_clean:
> > > +     $(Q)$(MAKE) -sC $(srctree)/tools/bpf/resolve_btfids O=$(abspath $(objtree))/tools/bpf/resolve_btfids clean
> > > +
> > >  ifdef CONFIG_BPF
> > >  ifdef CONFIG_DEBUG_INFO_BTF
> > >    ifeq ($(has_libelf),1)
> > > @@ -1495,7 +1500,7 @@ vmlinuxclean:
> > >       $(Q)$(CONFIG_SHELL) $(srctree)/scripts/link-vmlinux.sh clean
> > >       $(Q)$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) clean)
> > >
> > > -clean: archclean vmlinuxclean
> > > +clean: archclean vmlinuxclean resolve_btfids_clean
> > >
> > >  # mrproper - Delete all generated files, including .config
> > >  #
> > > --
> > > 2.26.2
> > >
> >
> > This breaks running distclean on a clean tree (my script just
> > unconditionally runs distclean regardless of the tree state):
> >
> > $ make -s O=build distclean
> > ../../scripts/Makefile.include:4: *** O=/home/nathan/cbl/src/linux-next/build/tools/bpf/resolve_btfids does not exist.  Stop.
> >
> 
> 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.

I can reproduce if I set O=XXX to directory that does not contain kernel build

	$ mkdir /tmp/krava
	$ make O=/tmp/krava distclean
	make[1]: Entering directory '/tmp/krava'
	../../scripts/Makefile.include:4: *** O=/tmp/krava/tools/bpf/resolve_btfids does not exist.  Stop.
	make[1]: *** [/home/jolsa/linux/Makefile:1092: resolve_btfids_clean] Error 2
	make[1]: Leaving directory '/tmp/krava'
	make: *** [Makefile:185: __sub-make] Error 2

will check on fix

jirka
Jiri Olsa Feb. 10, 2021, 6:29 p.m. UTC | #6 
On Wed, Feb 10, 2021 at 11:02:15AM -0700, Nathan Chancellor wrote:
> On Wed, Feb 10, 2021 at 09:52:42AM -0800, Andrii Nakryiko wrote:
> > On Wed, Feb 10, 2021 at 9:47 AM Nathan Chancellor <nathan@kernel.org> wrote:
> > >
> > > On Fri, Feb 05, 2021 at 01:40:20PM +0100, Jiri Olsa wrote:
> > > > The resolve_btfids tool is used during the kernel build,
> > > > so we should clean it on kernel's make clean.
> > > >
> > > > Invoking the the resolve_btfids clean as part of root
> > > > 'make clean'.
> > > >
> > > > Acked-by: Song Liu <songliubraving@fb.com>
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  Makefile | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Makefile b/Makefile
> > > > index b0e4767735dc..159d9592b587 100644
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -1086,6 +1086,11 @@ ifdef CONFIG_STACK_VALIDATION
> > > >    endif
> > > >  endif
> > > >
> > > > +PHONY += resolve_btfids_clean
> > > > +
> > > > +resolve_btfids_clean:
> > > > +     $(Q)$(MAKE) -sC $(srctree)/tools/bpf/resolve_btfids O=$(abspath $(objtree))/tools/bpf/resolve_btfids clean
> > > > +
> > > >  ifdef CONFIG_BPF
> > > >  ifdef CONFIG_DEBUG_INFO_BTF
> > > >    ifeq ($(has_libelf),1)
> > > > @@ -1495,7 +1500,7 @@ vmlinuxclean:
> > > >       $(Q)$(CONFIG_SHELL) $(srctree)/scripts/link-vmlinux.sh clean
> > > >       $(Q)$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) clean)
> > > >
> > > > -clean: archclean vmlinuxclean
> > > > +clean: archclean vmlinuxclean resolve_btfids_clean
> > > >
> > > >  # mrproper - Delete all generated files, including .config
> > > >  #
> > > > --
> > > > 2.26.2
> > > >
> > >
> > > This breaks running distclean on a clean tree (my script just
> > > unconditionally runs distclean regardless of the tree state):
> > >
> > > $ make -s O=build distclean
> > > ../../scripts/Makefile.include:4: *** O=/home/nathan/cbl/src/linux-next/build/tools/bpf/resolve_btfids does not exist.  Stop.
> > >
> > 
> > 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


---
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)))
+	$(Q)$(MAKE) -sC $(srctree)/tools/bpf/resolve_btfids O=$(resolve_btfids_O) clean
+endif
 
 ifdef CONFIG_BPF
 ifdef CONFIG_DEBUG_INFO_BTF
Nathan Chancellor Feb. 10, 2021, 6:37 p.m. UTC | #7 
On Wed, Feb 10, 2021 at 07:29:31PM +0100, Jiri Olsa wrote:
> On Wed, Feb 10, 2021 at 11:02:15AM -0700, Nathan Chancellor wrote:
> > On Wed, Feb 10, 2021 at 09:52:42AM -0800, Andrii Nakryiko wrote:
> > > On Wed, Feb 10, 2021 at 9:47 AM Nathan Chancellor <nathan@kernel.org> wrote:
> > > >
> > > > On Fri, Feb 05, 2021 at 01:40:20PM +0100, Jiri Olsa wrote:
> > > > > The resolve_btfids tool is used during the kernel build,
> > > > > so we should clean it on kernel's make clean.
> > > > >
> > > > > Invoking the the resolve_btfids clean as part of root
> > > > > 'make clean'.
> > > > >
> > > > > Acked-by: Song Liu <songliubraving@fb.com>
> > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > > ---
> > > > >  Makefile | 7 ++++++-
> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Makefile b/Makefile
> > > > > index b0e4767735dc..159d9592b587 100644
> > > > > --- a/Makefile
> > > > > +++ b/Makefile
> > > > > @@ -1086,6 +1086,11 @@ ifdef CONFIG_STACK_VALIDATION
> > > > >    endif
> > > > >  endif
> > > > >
> > > > > +PHONY += resolve_btfids_clean
> > > > > +
> > > > > +resolve_btfids_clean:
> > > > > +     $(Q)$(MAKE) -sC $(srctree)/tools/bpf/resolve_btfids O=$(abspath $(objtree))/tools/bpf/resolve_btfids clean
> > > > > +
> > > > >  ifdef CONFIG_BPF
> > > > >  ifdef CONFIG_DEBUG_INFO_BTF
> > > > >    ifeq ($(has_libelf),1)
> > > > > @@ -1495,7 +1500,7 @@ vmlinuxclean:
> > > > >       $(Q)$(CONFIG_SHELL) $(srctree)/scripts/link-vmlinux.sh clean
> > > > >       $(Q)$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) clean)
> > > > >
> > > > > -clean: archclean vmlinuxclean
> > > > > +clean: archclean vmlinuxclean resolve_btfids_clean
> > > > >
> > > > >  # mrproper - Delete all generated files, including .config
> > > > >  #
> > > > > --
> > > > > 2.26.2
> > > > >
> > > >
> > > > This breaks running distclean on a clean tree (my script just
> > > > unconditionally runs distclean regardless of the tree state):
> > > >
> > > > $ make -s O=build distclean
> > > > ../../scripts/Makefile.include:4: *** O=/home/nathan/cbl/src/linux-next/build/tools/bpf/resolve_btfids does not exist.  Stop.
> > > >
> > > 
> > > 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

Yes, this seems to work fine for me as well.

Tested-by: Nathan Chancellor <nathan@kernel.org>

> ---
> 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)))
> +	$(Q)$(MAKE) -sC $(srctree)/tools/bpf/resolve_btfids O=$(resolve_btfids_O) clean
> +endif
>  
>  ifdef CONFIG_BPF
>  ifdef CONFIG_DEBUG_INFO_BTF
>
Andrii Nakryiko Feb. 10, 2021, 7:23 p.m. UTC | #8 
On Wed, Feb 10, 2021 at 10:02 AM Nathan Chancellor <nathan@kernel.org> wrote:
>
> On Wed, Feb 10, 2021 at 09:52:42AM -0800, Andrii Nakryiko wrote:
> > On Wed, Feb 10, 2021 at 9:47 AM Nathan Chancellor <nathan@kernel.org> wrote:
> > >
> > > On Fri, Feb 05, 2021 at 01:40:20PM +0100, Jiri Olsa wrote:
> > > > The resolve_btfids tool is used during the kernel build,
> > > > so we should clean it on kernel's make clean.
> > > >
> > > > Invoking the the resolve_btfids clean as part of root
> > > > 'make clean'.
> > > >
> > > > Acked-by: Song Liu <songliubraving@fb.com>
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  Makefile | 7 ++++++-
> > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Makefile b/Makefile
> > > > index b0e4767735dc..159d9592b587 100644
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > > > @@ -1086,6 +1086,11 @@ ifdef CONFIG_STACK_VALIDATION
> > > >    endif
> > > >  endif
> > > >
> > > > +PHONY += resolve_btfids_clean
> > > > +
> > > > +resolve_btfids_clean:
> > > > +     $(Q)$(MAKE) -sC $(srctree)/tools/bpf/resolve_btfids O=$(abspath $(objtree))/tools/bpf/resolve_btfids clean
> > > > +
> > > >  ifdef CONFIG_BPF
> > > >  ifdef CONFIG_DEBUG_INFO_BTF
> > > >    ifeq ($(has_libelf),1)
> > > > @@ -1495,7 +1500,7 @@ vmlinuxclean:
> > > >       $(Q)$(CONFIG_SHELL) $(srctree)/scripts/link-vmlinux.sh clean
> > > >       $(Q)$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) clean)
> > > >
> > > > -clean: archclean vmlinuxclean
> > > > +clean: archclean vmlinuxclean resolve_btfids_clean
> > > >
> > > >  # mrproper - Delete all generated files, including .config
> > > >  #
> > > > --
> > > > 2.26.2
> > > >
> > >
> > > This breaks running distclean on a clean tree (my script just
> > > unconditionally runs distclean regardless of the tree state):
> > >
> > > $ make -s O=build distclean
> > > ../../scripts/Makefile.include:4: *** O=/home/nathan/cbl/src/linux-next/build/tools/bpf/resolve_btfids does not exist.  Stop.
> > >
> >
> > 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:

my bad, I was trying it on a branch that didn't have Jiri's patches,
sorry about that.

>
> $ 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?
>
> Cheers,
> Nathan
Andrii Nakryiko Feb. 10, 2021, 7:26 p.m. UTC | #9 
On Wed, Feb 10, 2021 at 10:29 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Wed, Feb 10, 2021 at 11:02:15AM -0700, Nathan Chancellor wrote:
> > On Wed, Feb 10, 2021 at 09:52:42AM -0800, Andrii Nakryiko wrote:
> > > On Wed, Feb 10, 2021 at 9:47 AM Nathan Chancellor <nathan@kernel.org> wrote:
> > > >
> > > > On Fri, Feb 05, 2021 at 01:40:20PM +0100, Jiri Olsa wrote:
> > > > > The resolve_btfids tool is used during the kernel build,
> > > > > so we should clean it on kernel's make clean.
> > > > >
> > > > > Invoking the the resolve_btfids clean as part of root
> > > > > 'make clean'.
> > > > >
> > > > > Acked-by: Song Liu <songliubraving@fb.com>
> > > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > > ---
> > > > >  Makefile | 7 ++++++-
> > > > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Makefile b/Makefile
> > > > > index b0e4767735dc..159d9592b587 100644
> > > > > --- a/Makefile
> > > > > +++ b/Makefile
> > > > > @@ -1086,6 +1086,11 @@ ifdef CONFIG_STACK_VALIDATION
> > > > >    endif
> > > > >  endif
> > > > >
> > > > > +PHONY += resolve_btfids_clean
> > > > > +
> > > > > +resolve_btfids_clean:
> > > > > +     $(Q)$(MAKE) -sC $(srctree)/tools/bpf/resolve_btfids O=$(abspath $(objtree))/tools/bpf/resolve_btfids clean
> > > > > +
> > > > >  ifdef CONFIG_BPF
> > > > >  ifdef CONFIG_DEBUG_INFO_BTF
> > > > >    ifeq ($(has_libelf),1)
> > > > > @@ -1495,7 +1500,7 @@ vmlinuxclean:
> > > > >       $(Q)$(CONFIG_SHELL) $(srctree)/scripts/link-vmlinux.sh clean
> > > > >       $(Q)$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) clean)
> > > > >
> > > > > -clean: archclean vmlinuxclean
> > > > > +clean: archclean vmlinuxclean resolve_btfids_clean
> > > > >
> > > > >  # mrproper - Delete all generated files, including .config
> > > > >  #
> > > > > --
> > > > > 2.26.2
> > > > >
> > > >
> > > > This breaks running distclean on a clean tree (my script just
> > > > unconditionally runs distclean regardless of the tree state):
> > > >
> > > > $ make -s O=build distclean
> > > > ../../scripts/Makefile.include:4: *** O=/home/nathan/cbl/src/linux-next/build/tools/bpf/resolve_btfids does not exist.  Stop.
> > > >
> > >
> > > 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?

>
> ---
> 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

> +       $(Q)$(MAKE) -sC $(srctree)/tools/bpf/resolve_btfids O=$(resolve_btfids_O) clean
> +endif
>
>  ifdef CONFIG_BPF
>  ifdef CONFIG_DEBUG_INFO_BTF
>
Jiri Olsa Feb. 11, 2021, 12:17 p.m. UTC | #10 
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
Masahiro Yamada Feb. 12, 2021, 3:30 a.m. UTC | #11 
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.
Jiri Olsa Feb. 12, 2021, 1:35 p.m. UTC | #12 
On Fri, Feb 12, 2021 at 12:30:45PM +0900, Masahiro Yamada wrote:

SNIP

> 
> 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.

would you see any way out? apart from changing resolve_btfids
to use Kbuild.. there are some dependencies we'd need to change
as well and they are used by other tools.. probably it'd end up
with all or nothing scenario

> 
> 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.

maybe we could move it out of top makefile into separate one,
that would handle all the related mess

jirka
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index b0e4767735dc..159d9592b587 100644
--- a/Makefile
+++ b/Makefile
@@ -1086,6 +1086,11 @@  ifdef CONFIG_STACK_VALIDATION
   endif
 endif
 
+PHONY += resolve_btfids_clean
+
+resolve_btfids_clean:
+	$(Q)$(MAKE) -sC $(srctree)/tools/bpf/resolve_btfids O=$(abspath $(objtree))/tools/bpf/resolve_btfids clean
+
 ifdef CONFIG_BPF
 ifdef CONFIG_DEBUG_INFO_BTF
   ifeq ($(has_libelf),1)
@@ -1495,7 +1500,7 @@  vmlinuxclean:
 	$(Q)$(CONFIG_SHELL) $(srctree)/scripts/link-vmlinux.sh clean
 	$(Q)$(if $(ARCH_POSTLINK), $(MAKE) -f $(ARCH_POSTLINK) clean)
 
-clean: archclean vmlinuxclean
+clean: archclean vmlinuxclean resolve_btfids_clean
 
 # mrproper - Delete all generated files, including .config
 #