On 07/16, Andrii Nakryiko wrote:
> On Tue, Jul 16, 2019 at 12:55 PM Stanislav Fomichev <sdf@xxxxxxxxxxx> wrote:
> >
> > On 07/16, Andrii Nakryiko wrote:
> > > e46fc22e60a4 ("selftests/bpf: make directory prerequisites order-only")
> > > exposed existing problem in Makefile for test_verifier and test_maps tests:
> > > their dependency on auto-generated header file with a list of all tests wasn't
> > > recorded explicitly. This patch fixes these issues.
> > Why adding it explicitly fixes it? At least for test_verifier, we have
> > the following rule:
> >
> > test_verifier.c: $(VERIFIER_TESTS_H)
> >
> > And there should be implicit/builtin test_verifier -> test_verifier.c
> > dependency rule.
> >
> > Same for maps, I guess:
> >
> > $(OUTPUT)/test_maps: map_tests/*.c
> > test_maps.c: $(MAP_TESTS_H)
> >
> > So why is it not working as is? What I'm I missing?
>
> I don't know exactly why it's not working, but it's clearly because of
> that. It's the only difference between how test_progs are set up,
> which didn't break, and test_maps/test_verifier, which did.
>
> Feel free to figure it out through a maze of Makefiles why it didn't
> work as expected, but this definitely fixed a breakage (at least for
> me).
Agreed on not wasting time. I took a brief look (with make -qp) and I
don't have any clue.
By default implicit matching doesn't work:
# makefile (from 'Makefile', line 261)
/linux/tools/testing/selftests/bpf/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS)
/linux/tools/testing/selftests/bpf/test_maps: map_tests/sk_storage_map.c /linux/tools/testing/selftests/bpf/test_stub.o /linux/tools/testing/selftests/bpf/libbpf.a
# Implicit rule search has not been done.
# File is an intermediate prerequisite.
# Modification time never checked.
# File has not been updated.
# variable set hash-table stats:
# Load=1/32=3%, Rehash=0, Collisions=0/2=0%
If I comment out the following line:
$(TEST_GEN_PROGS): $(OUTPUT)/test_stub.o $(BPFOBJ)
Then it works:
# makefile (from 'Makefile', line 261)
/linux/tools/testing/selftests/bpf/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS)
/linux/tools/testing/selftests/bpf/test_maps: test_maps.c map_tests/sk_storage_map.c
# Implicit rule search has been done.
# Implicit/static pattern stem: 'test_maps'
# File is an intermediate prerequisite.
# File does not exist.
# File has not been updated.
# variable set hash-table stats:
# Load=1/32=3%, Rehash=0, Collisions=0/2=0%
# recipe to execute (from '../lib.mk', line 138):
$(LINK.c) $^ $(LDLIBS) -o $@
It's because "File is an intermediate prerequisite.", but I
don't see how it's is a intermediate prerequisite for anything...
One other optional suggestion I have to your second patch: maybe drop all
those dependencies on the directories altogether? Why not do the following
instead, for example (same for test_progs/test_maps)?
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 1296253b3422..c2d087ce6d4b 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -277,12 +277,9 @@ VERIFIER_TESTS_H := $(OUTPUT)/verifier/tests.h
test_verifier.c: $(VERIFIER_TESTS_H)
$(OUTPUT)/test_verifier: CFLAGS += $(TEST_VERIFIER_CFLAGS)
-VERIFIER_TESTS_DIR = $(OUTPUT)/verifier
-$(VERIFIER_TESTS_DIR):
- mkdir -p $@
-
VERIFIER_TEST_FILES := $(wildcard verifier/*.c)
-$(OUTPUT)/verifier/tests.h: $(VERIFIER_TEST_FILES) | $(VERIFIER_TESTS_DIR)
+$(OUTPUT)/verifier/tests.h: $(VERIFIER_TEST_FILES)
+ mkdir -p $(dir $@)
$(shell ( cd verifier/; \
echo '/* Generated header, do not edit */'; \
echo '#ifdef FILL_ARRAY'; \
