On Tue, Jul 23, 2024 at 12:25 PM Ihor Solodrai <ihor.solodrai@xxxxx> 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 > >
