Re: [PATCH bpf-next v4] selftests/bpf: use auto-dependencies for test objects

Andrii Nakryiko
2024-07-23T20:03:03Z

On Tue, Jul 23, 2024 at 12:25 PM Ihor Solodrai <ihor.solodrai@pm.me> wrote:
>
> Andrii,
>
> I looked over the v4 of the patch, and apparently I messed it up by
> losing the v1 -> v2 change. So the issue with dump order of %.test.d
> relative to %.test.o files is present on the master branch right now.
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 74f829952..4bcb1d1ce 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -596,7 +596,7 @@ endif
> # Note: we cd into output directory to ensure embedded BPF object is found
> $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o: \
> $(TRUNNER_TESTS_DIR)/%.c \
> - $(TRUNNER_OUTPUT)/%.test.d
> + | $(TRUNNER_OUTPUT)/%.test.d
> $$(call msg,TEST-OBJ,$(TRUNNER_BINARY),$$@)
> $(Q)cd $$(@D) && $$(CC) -I. $$(CFLAGS) -MMD -MT $$@ -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F)
>
> I can send this fix together with the condition for the clean targets
> (so [1] can be discarded); or I can submit a separate change. Let me
> know what you'd prefer.

Send it separately, if that fix is good, I'll just apply it as is?

>
>
> I also had a discussion with Eduard off-list, he suggested trying to
> remove explicit %.test.d targets altogether like this:
>
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 05b234248b38..f01dc1cc8af8 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -596,18 +596,12 @@ endif
> > # Note: we cd into output directory to ensure embedded BPF object is found
> > $(TRUNNER_TEST_OBJS): $(TRUNNER_OUTPUT)/%.test.o: \
> > $(TRUNNER_TESTS_DIR)/%.c \
> > - $(TRUNNER_OUTPUT)/%.test.d
> > + | $(TRUNNER_BPF_SKELS) \
> > + $(TRUNNER_BPF_LSKELS) \
> > + $(TRUNNER_BPF_SKELS_LINKED)

shouldn't we also have a dependency on libbpf.a here as well, then? So
that all the auto-generated headers are autogenerated.

> > $$(call msg,TEST-OBJ,$(TRUNNER_BINARY),$$@)
> > $(Q)cd $$(@D) && $$(CC) -I. $$(CFLAGS) -MMD -MT $$@ -c $(CURDIR)/$$< $$(LDLIBS) -o $$(@F)
> >
> > -$(TRUNNER_TEST_OBJS:.o=.d): $(TRUNNER_OUTPUT)/%.test.d: \
> > - $(TRUNNER_TESTS_DIR)/%.c \
> > - $(TRUNNER_EXTRA_HDRS) \
> > - $(TRUNNER_BPF_SKELS) \
> > - $(TRUNNER_BPF_LSKELS) \
> > - $(TRUNNER_BPF_SKELS_LINKED) \
> > - $$(BPFOBJ) | $(TRUNNER_OUTPUT)
> > -
> > include $(wildcard $(TRUNNER_TEST_OBJS:.o=.d))
> >
> > $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o: \
> > --
> > 2.45.2
>
> This works almost as we want it, except for a situation when any
> %.test.d gets deleted (say, due to local branch switch). In such case,
> if one forgets to run `make clean`, there is no dependency of the
> %.test.o on skels, and so they won't be properly updated.
>
> After some discussion, me and Ed concluded that we shouldn't expect
> people to remember to do clean in particular situations, especially if
> consequences are not obvious. So the state after the suggested fixes
> would be good enough.
>

ok

> [1] http://lore.kernel.org/K69Y8OKMLXBWR0dtOfsC4J46-HxeQfvqoFx1CysCm7u19HRx4MB6yAKOFkM6X-KAx2EFuCcCh_9vYWpsgQXnAer8oQ8PMeDEuiRMYECuGH4=@pm.me
>
>