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

Ihor Solodrai
2024-07-23T19:25:46Z

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.

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)
> $$(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.

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