| Message ID | 20240709204203.1481851-4-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tools build: Incorrect fixdep dependencies | expand |
On Tue, Jul 9, 2024 at 1:43 PM Brian Norris <briannorris@chromium.org> wrote: > > The dependencies in tools/lib/bpf/Makefile are incorrect. Before we > recurse to build $(BPF_IN_STATIC), we need to build its 'fixdep' > executable. > > I can't use the usual shortcut from Makefile.include: > > <target>: <sources> fixdep > > because its 'fixdep' target relies on $(OUTPUT), and $(OUTPUT) differs > in the parent 'make' versus the child 'make' -- so I imitate it via > open-coding. > > I tweak a few $(MAKE) invocations while I'm at it, because > 1. I'm adding a new recursive make; and > 2. these recursive 'make's print spurious lines about files that are "up > to date" (which isn't normally a feature in Kbuild subtargets) or > "jobserver not available" (see [1]) > > I also need to tweak the assignment of the OUTPUT variable, so that > relative path builds work. For example, for 'make tools/lib/bpf', OUTPUT > is unset, and is usually treated as "cwd" -- but recursive make will > change cwd and so OUTPUT has a new meaning. For consistency, I ensure > OUTPUT is always an absolute path. > > And $(Q) gets a backup definition in tools/build/Makefile.include, > because Makefile.include is sometimes included without > tools/build/Makefile, so the "quiet command" stuff doesn't actually work > consistently without it. > > After this change, top-level builds result in an empty grep result from: > > $ grep 'cannot find fixdep' $(find tools/ -name '*.cmd') > > [1] https://www.gnu.org/software/make/manual/html_node/MAKE-Variable.html > If we're not using $(MAKE) directly, then we need to use more '+'. > > Signed-off-by: Brian Norris <briannorris@chromium.org> > Acked-by: Jiri Olsa <jolsa@kernel.org> > --- > I almost gave my acked-by and tested-by, but then I noticed that this leaves fixdep, staticobjs and sharedobjs directories as to-be-committed files. Please check, something is off with .gitignore or where those are put: $ cd ~/linux/tools/lib/bpf $ make -j90 $ git st On branch master Your branch is ahead of 'bpf-next/master' by 4 commits. (use "git push" to publish your local commits) Untracked files: (use "git add <file>..." to include in what will be committed) fixdep sharedobjs/ staticobjs/ nothing added to commit but untracked files present (use "git add" to track) Other than that the changes look good, but we should be leaving uncommitted (and unignored) files around. Also (in it's less the question to the author, but rather all the maintainers involved), which kernel tree is this intended to go through, seems like it was marked as "Not Applicable" for bpf, so I'm wondering where is the proper destination? > Changes in v3: > - add Jiri's Acked-by > > Changes in v2: > - also fix libbpf shared library rules > - ensure OUTPUT is always set, and always an absolute path > - add backup $(Q) definition in tools/build/Makefile.include > > tools/build/Makefile.include | 12 +++++++++++- > tools/lib/bpf/Makefile | 14 ++++++++++++-- > 2 files changed, 23 insertions(+), 3 deletions(-) > [...] > -$(BPF_IN_SHARED): force $(BPF_GENERATED) > +$(SHARED_OBJDIR): > + $(Q)mkdir -p $@ > + > +$(STATIC_OBJDIR): > + $(Q)mkdir -p $@ I'd probably combine the above two rules into one, but it's minor [...]
Hi Andrii, On Fri, Jul 12, 2024 at 12:38:28PM -0700, Andrii Nakryiko wrote: > I almost gave my acked-by and tested-by, but then I noticed that this > leaves fixdep, staticobjs and sharedobjs directories as > to-be-committed files. Please check, something is off with .gitignore > or where those are put: > > $ cd ~/linux/tools/lib/bpf > $ make -j90 > $ git st > On branch master > Your branch is ahead of 'bpf-next/master' by 4 commits. > (use "git push" to publish your local commits) > > Untracked files: > (use "git add <file>..." to include in what will be committed) > fixdep > sharedobjs/ > staticobjs/ > > nothing added to commit but untracked files present (use "git add" to track) > > > Other than that the changes look good, but we should be leaving > uncommitted (and unignored) files around. Thanks for looking and for the diligence. At first I thought I moved the dirs by accident, but that's not the case. The problem is that I'm now leaving a 'fixdep' artifact in these dirs (they already had a variety of *.o, etc., files, which were already ignored), so the containing dirs now show up in the untracked list. I've added a 'fixdep' .gitignore in my upcoming v4, as well as proper cleaning (fixdep-clean) for it too. > On Tue, Jul 9, 2024 at 1:43 PM Brian Norris <briannorris@chromium.org> wrote: > > -$(BPF_IN_SHARED): force $(BPF_GENERATED) > > +$(SHARED_OBJDIR): > > + $(Q)mkdir -p $@ > > + > > +$(STATIC_OBJDIR): > > + $(Q)mkdir -p $@ > > I'd probably combine the above two rules into one, but it's minor Ack. I forgot some Makefile-language details when writing this part. I'll update in v4. I'll probably send v4 next week. Thanks, Brian
diff --git a/tools/build/Makefile.include b/tools/build/Makefile.include index 8dadaa0fbb43..0e4de83400ac 100644 --- a/tools/build/Makefile.include +++ b/tools/build/Makefile.include @@ -1,8 +1,18 @@ # SPDX-License-Identifier: GPL-2.0-only build := -f $(srctree)/tools/build/Makefile.build dir=. obj +# More than just $(Q), we sometimes want to suppress all command output from a +# recursive make -- even the 'up to date' printout. +ifeq ($(V),1) + Q ?= + SILENT_MAKE = +$(Q)$(MAKE) +else + Q ?= @ + SILENT_MAKE = +$(Q)$(MAKE) --silent +endif + fixdep: - $(Q)$(MAKE) -C $(srctree)/tools/build CFLAGS= LDFLAGS= $(OUTPUT)fixdep + $(SILENT_MAKE) -C $(srctree)/tools/build CFLAGS= LDFLAGS= $(OUTPUT)fixdep fixdep-clean: $(Q)$(MAKE) -C $(srctree)/tools/build clean diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile index 2cf892774346..630369c0091e 100644 --- a/tools/lib/bpf/Makefile +++ b/tools/lib/bpf/Makefile @@ -108,6 +108,8 @@ MAKEOVERRIDES= all: +OUTPUT ?= ./ +OUTPUT := $(abspath $(OUTPUT))/ export srctree OUTPUT CC LD CFLAGS V include $(srctree)/tools/build/Makefile.include @@ -141,7 +143,13 @@ all: fixdep all_cmd: $(CMD_TARGETS) check -$(BPF_IN_SHARED): force $(BPF_GENERATED) +$(SHARED_OBJDIR): + $(Q)mkdir -p $@ + +$(STATIC_OBJDIR): + $(Q)mkdir -p $@ + +$(BPF_IN_SHARED): force $(BPF_GENERATED) | $(SHARED_OBJDIR) @(test -f ../../include/uapi/linux/bpf.h -a -f ../../../include/uapi/linux/bpf.h && ( \ (diff -B ../../include/uapi/linux/bpf.h ../../../include/uapi/linux/bpf.h >/dev/null) || \ echo "Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs from latest version at 'include/uapi/linux/bpf.h'" >&2 )) || true @@ -151,9 +159,11 @@ $(BPF_IN_SHARED): force $(BPF_GENERATED) @(test -f ../../include/uapi/linux/if_xdp.h -a -f ../../../include/uapi/linux/if_xdp.h && ( \ (diff -B ../../include/uapi/linux/if_xdp.h ../../../include/uapi/linux/if_xdp.h >/dev/null) || \ echo "Warning: Kernel ABI header at 'tools/include/uapi/linux/if_xdp.h' differs from latest version at 'include/uapi/linux/if_xdp.h'" >&2 )) || true + $(SILENT_MAKE) -C $(srctree)/tools/build CFLAGS= LDFLAGS= OUTPUT=$(SHARED_OBJDIR) $(SHARED_OBJDIR)fixdep $(Q)$(MAKE) $(build)=libbpf OUTPUT=$(SHARED_OBJDIR) CFLAGS="$(CFLAGS) $(SHLIB_FLAGS)" -$(BPF_IN_STATIC): force $(BPF_GENERATED) +$(BPF_IN_STATIC): force $(BPF_GENERATED) | $(STATIC_OBJDIR) + $(SILENT_MAKE) -C $(srctree)/tools/build CFLAGS= LDFLAGS= OUTPUT=$(STATIC_OBJDIR) $(STATIC_OBJDIR)fixdep $(Q)$(MAKE) $(build)=libbpf OUTPUT=$(STATIC_OBJDIR) $(BPF_HELPER_DEFS): $(srctree)/tools/include/uapi/linux/bpf.h