Re: [PATCH 0/2] tools: Unify top-level quiet infrastructure

[Andrii Nakryiko]

On Mon, Feb 10, 2025 at 10:31 AM Charlie Jenkins <charlie@xxxxxxxxxxxx> wrote:
>
> On Wed, Feb 05, 2025 at 05:28:19PM -0800, Andrii Nakryiko wrote:
> > On Tue, Feb 4, 2025 at 3:24 PM Charlie Jenkins <charlie@xxxxxxxxxxxx> wrote:
> > >
> > > On Tue, Feb 04, 2025 at 11:02:42PM +0000, Alexei Starovoitov wrote:
> > > > On Tue, Feb 4, 2025 at 7:04 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > On Tue, Feb 04, 2025 at 10:34:41AM -0800, Charlie Jenkins wrote:
> > > > > > On Tue, Feb 04, 2025 at 05:18:42PM +0000, Alexei Starovoitov wrote:
> > > > > > > On Tue, Feb 4, 2025 at 12:10 AM Charlie Jenkins <charlie@xxxxxxxxxxxx> wrote:
> > > > > > > >
> > > > > > > > The quiet infrastructure was moved out of Makefile.build to accomidate
> > > > > > > > the new syscall table generation scripts in perf. Syscall table
> > > > > > > > generation wanted to also be able to be quiet, so instead of again
> > > > > > > > copying the code to set the quiet variables, the code was moved into
> > > > > > > > Makefile.perf to be used globally. This was not the right solution. It
> > > > > > > > should have been moved even further upwards in the call chain.
> > > > > > > > Makefile.include is imported in many files so this seems like a proper
> > > > > > > > place to put it.
> > > > > > > >
> > > > > > > > To:
> > > > > > > >
> > > > > > > > Signed-off-by: Charlie Jenkins <charlie@xxxxxxxxxxxx>
> > > > > > > > ---
> > > > > > > > Charlie Jenkins (2):
> > > > > > > >       tools: Unify top-level quiet infrastructure
> > > > > > > >       tools: Remove redundant quiet setup
> > > > > > > >
> > > > > > > >  tools/arch/arm64/tools/Makefile           |  6 -----
> > > > > > > >  tools/bpf/Makefile                        |  6 -----
> > > > > > > >  tools/bpf/bpftool/Documentation/Makefile  |  6 -----
> > > > > > > >  tools/bpf/bpftool/Makefile                |  6 -----
> > > > > > > >  tools/bpf/resolve_btfids/Makefile         |  2 --
> > > > > > > >  tools/bpf/runqslower/Makefile             |  5 +---
> > > > > > > >  tools/build/Makefile                      |  8 +-----
> > > > > > > >  tools/lib/bpf/Makefile                    | 13 ----------
> > > > > > >
> > > > > > > Nack.
> > > > > > > libbpf and bpftool are synced independently to github
> > > > > > > and released from there.
> > > > > > > This change breaks it.
> > > > >
> > > > > Sorry, I overlooked this part and merged a change that touched the
> > > > > common files into the perf tree.
> > > > >
> > > > > f2868b1a66d4f40f ("perf tools: Expose quiet/verbose variables in Makefile.perf")
> > > > >
> > > > > Unfortunately, it's already in v6.14-rc1.
> > > > >
> > > > > >
> > > > > > Can you explain how it breaks it? Currently bpftool and resolve_btfids
> > > > > > don't build quietly so this was an attempt to fix that.
> > > > >
> > > > > So I think you will need something like this for v6.14.  Again, sorry
> > > > > about the trouble.
> > > >
> > > > Just revert f2868b1a66d4f40f that created this mess.
> > >
> > > Why are you opposed to unifying this helpers among the various projects
> > > in tools? Can you explain what about this breaks the Github syncing flow
> > > and why it cannot be resolved? It doesn't make sense to duplicate "Q="
> > > in every Makefile anybody ever wants to add to tools just because bpf
> > > syncing isn't robust.
> >
> > Alexei's concern about Github mirrors of bpftool and libbpf isn't
> > valid. Github versions of those projects use their own independent
> > Makefiles anyways, so your change doesn't break that aspect.
> >
> > But your change *does* break both libbpf's and bpftool's make output
> > *in the kernel repo*. With this patch we basically don't have "quiet"
> > mode anymore:
> >
> > $ git co f2868b1a66d4f40f07e985b0beead606b2753602
> > HEAD is now at f2868b1a66d4 perf tools: Expose quiet/verbose variables
> > in Makefile.perf
> > $ git log --oneline -n1
> > f2868b1a66d4 (HEAD) perf tools: Expose quiet/verbose variables in Makefile.perf
> > $ pwd
> > /home/andriin/linux/tools/lib/bpf
> > $ make
> >   gcc -Wp,-MD,/data/users/andriin/linux/tools/lib/bpf/staticobjs/.libbpf.o.d
> > -Wp,-MT,/data/users/andriin/linux/tools/lib/bpf/staticobjs/libbpf.o -g
> > -O2 -std=gnu89 -Wbad-function-cast -Wdeclaration-after-statement
> > -Wformat-security -Wformat-y2k -Winit-self -Wmissing-declarations
> > -Wmissing-prototypes -Wnested-externs -Wno-system-headers
> > -Wold-style-definition -Wpacked -Wredundant-decls -Wstrict-prototypes
> > -Wswitch-default -Wswitch-enum -Wundef -Wwrite-strings -Wformat
> > -Wno-type-limits -Wstrict-aliasing=3 -Wshadow -Wno-switch-enum -Werror
> > -Wall -I/data/users/andriin/linux/tools/lib/bpf/
> > -I/data/users/andriin/linux/tools/include
> > -I/data/users/andriin/linux/tools/include/uapi
> > -I/data/users/andriin/linux/tools/arch/x86/include -fvisibility=hidden
> > -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -D"BUILD_STR(s)=#s" -c -o
> > /data/users/andriin/linux/tools/lib/bpf/staticobjs/libbpf.o libbpf.c
> > ^Cmake[2]: *** [/data/users/andriin/linux/tools/build/Makefile.build:86:
> > /data/users/andriin/linux/tools/lib/bpf/staticobjs/libbpf.o] Interrupt
> > make[1]: *** [Makefile:165:
> > /data/users/andriin/linux/tools/lib/bpf/staticobjs/libbpf-in.o]
> > Interrupt
> > make: *** [Makefile:143: all] Interrupt
> >
> > $ git co HEAD~
> > Previous HEAD position was f2868b1a66d4 perf tools: Expose
> > quiet/verbose variables in Makefile.perf
> > HEAD is now at e9cbc854d8b1 perf config: Add a function to set one
> > variable in .perfconfig
> > $ make
> >   CC      /data/users/andriin/linux/tools/lib/bpf/staticobjs/libbpf.o
> > ^C
> >
> > So, can you please check and fix?
>
> I think I am misunderstanding what you are saying. The patch that we are
> discussing on is the patch to fix this? You are showing the output from
> the patch that is being fixed in this series.

Ah, it's me getting confused. It was the earlier perf commit that
broke all this, makes sense. I just double-checked with your patches
applied locally. It indeed fixes the issue, LGTM.

Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>

>
> >
> > Also, looking at your patch:
> >
> > a) you removed the `"$(origin V)", "command line"` check from both
> > perf and libbpf, so that's not really an equivalent change/behavior
> > now
>
> I don't believe it is required to keep that check.
>
> >
> > b) a bit sloppy on assignment:
> >
> > +ifeq ($(V),1)
> > +  quiet =
> > +  Q =
> > +else
> > +  quiet=quiet_
> > +  Q=@
> > +endif
> >
> > note the spaces around '=', try to keep stuff like this consistent
> > (and if this was shell, it would bite you as well)
>
> Yes, I can make that consistent.
>
> >
> > >
> > > - Charlie
> > >