* [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies
@ 2019-07-16 19:38 Andrii Nakryiko
2019-07-16 19:38 ` [PATCH bpf 2/2] selftests/bpf: structure test_{progs,maps,verifier} test runners uniformly Andrii Nakryiko
2019-07-16 19:55 ` [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies Stanislav Fomichev
0 siblings, 2 replies; 9+ messages in thread
From: Andrii Nakryiko @ 2019-07-16 19:38 UTC (permalink / raw)
To: bpf, netdev, daniel, ast
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko, Ilya Leoshkevich,
Stanislav Fomichev, Martin KaFai Lau
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.
Fixes: 51a0e301a563 ("bpf: Add BPF_MAP_TYPE_SK_STORAGE test to test_maps")
Fixes: 6b7b6995c43e ("selftests: bpf: tests.h should depend on .c files, not the output")
Cc: Ilya Leoshkevich <iii@linux.ibm.com>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/Makefile | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 1296253b3422..9bc68d8abc5f 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -86,8 +86,6 @@ $(OUTPUT)/urandom_read: $(OUTPUT)/%: %.c
$(OUTPUT)/test_stub.o: test_stub.c
$(CC) $(TEST_PROGS_CFLAGS) $(CFLAGS) -c -o $@ $<
-$(OUTPUT)/test_maps: map_tests/*.c
-
BPFOBJ := $(OUTPUT)/libbpf.a
$(TEST_GEN_PROGS): $(OUTPUT)/test_stub.o $(BPFOBJ)
@@ -257,9 +255,10 @@ MAP_TESTS_DIR = $(OUTPUT)/map_tests
$(MAP_TESTS_DIR):
mkdir -p $@
MAP_TESTS_H := $(MAP_TESTS_DIR)/tests.h
+MAP_TESTS_FILES := $(wildcard map_tests/*.c)
test_maps.c: $(MAP_TESTS_H)
$(OUTPUT)/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS)
-MAP_TESTS_FILES := $(wildcard map_tests/*.c)
+$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_H) $(MAP_TESTS_FILES)
$(MAP_TESTS_H): $(MAP_TESTS_FILES) | $(MAP_TESTS_DIR)
$(shell ( cd map_tests/; \
echo '/* Generated header, do not edit */'; \
@@ -276,6 +275,7 @@ $(MAP_TESTS_H): $(MAP_TESTS_FILES) | $(MAP_TESTS_DIR)
VERIFIER_TESTS_H := $(OUTPUT)/verifier/tests.h
test_verifier.c: $(VERIFIER_TESTS_H)
$(OUTPUT)/test_verifier: CFLAGS += $(TEST_VERIFIER_CFLAGS)
+$(OUTPUT)/test_verifier: test_verifier.c $(VERIFIER_TESTS_H)
VERIFIER_TESTS_DIR = $(OUTPUT)/verifier
$(VERIFIER_TESTS_DIR):
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH bpf 2/2] selftests/bpf: structure test_{progs,maps,verifier} test runners uniformly 2019-07-16 19:38 [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies Andrii Nakryiko @ 2019-07-16 19:38 ` Andrii Nakryiko 2019-07-16 19:55 ` [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies Stanislav Fomichev 1 sibling, 0 replies; 9+ messages in thread From: Andrii Nakryiko @ 2019-07-16 19:38 UTC (permalink / raw) To: bpf, netdev, daniel, ast; +Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko It's easier to follow the logic if it's structured the same. There is just slight difference between test_progs/test_maps and test_verifier. test_verifier's verifier/*.c files are not really compilable C files (they are more of include headers), so they can't be specified as explicit dependencies of test_verifier. Cc: Alexei Starovoitov <ast@fb.com> Signed-off-by: Andrii Nakryiko <andriin@fb.com> --- tools/testing/selftests/bpf/Makefile | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index 9bc68d8abc5f..11c9c62c3362 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -176,6 +176,7 @@ endif endif TEST_PROGS_CFLAGS := -I. -I$(OUTPUT) +TEST_MAPS_CFLAGS := -I. -I$(OUTPUT) TEST_VERIFIER_CFLAGS := -I. -I$(OUTPUT) -Iverifier ifneq ($(SUBREG_CODEGEN),) @@ -227,16 +228,14 @@ ifeq ($(DWARF2BTF),y) $(BTF_PAHOLE) -J $@ endif -PROG_TESTS_H := $(OUTPUT)/prog_tests/tests.h -test_progs.c: $(PROG_TESTS_H) -$(OUTPUT)/test_progs: CFLAGS += $(TEST_PROGS_CFLAGS) -$(OUTPUT)/test_progs: prog_tests/*.c - PROG_TESTS_DIR = $(OUTPUT)/prog_tests $(PROG_TESTS_DIR): mkdir -p $@ - +PROG_TESTS_H := $(PROG_TESTS_DIR)/tests.h PROG_TESTS_FILES := $(wildcard prog_tests/*.c) +test_progs.c: $(PROG_TESTS_H) +$(OUTPUT)/test_progs: CFLAGS += $(TEST_PROGS_CFLAGS) +$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_H) $(PROG_TESTS_FILES) $(PROG_TESTS_H): $(PROG_TESTS_FILES) | $(PROG_TESTS_DIR) $(shell ( cd prog_tests/; \ echo '/* Generated header, do not edit */'; \ @@ -250,7 +249,6 @@ $(PROG_TESTS_H): $(PROG_TESTS_FILES) | $(PROG_TESTS_DIR) echo '#endif' \ ) > $(PROG_TESTS_H)) -TEST_MAPS_CFLAGS := -I. -I$(OUTPUT) MAP_TESTS_DIR = $(OUTPUT)/map_tests $(MAP_TESTS_DIR): mkdir -p $@ @@ -272,17 +270,15 @@ $(MAP_TESTS_H): $(MAP_TESTS_FILES) | $(MAP_TESTS_DIR) echo '#endif' \ ) > $(MAP_TESTS_H)) -VERIFIER_TESTS_H := $(OUTPUT)/verifier/tests.h -test_verifier.c: $(VERIFIER_TESTS_H) -$(OUTPUT)/test_verifier: CFLAGS += $(TEST_VERIFIER_CFLAGS) -$(OUTPUT)/test_verifier: test_verifier.c $(VERIFIER_TESTS_H) - VERIFIER_TESTS_DIR = $(OUTPUT)/verifier $(VERIFIER_TESTS_DIR): mkdir -p $@ - +VERIFIER_TESTS_H := $(VERIFIER_TESTS_DIR)/tests.h VERIFIER_TEST_FILES := $(wildcard verifier/*.c) -$(OUTPUT)/verifier/tests.h: $(VERIFIER_TEST_FILES) | $(VERIFIER_TESTS_DIR) +test_verifier.c: $(VERIFIER_TESTS_H) +$(OUTPUT)/test_verifier: CFLAGS += $(TEST_VERIFIER_CFLAGS) +$(OUTPUT)/test_verifier: test_verifier.c $(VERIFIER_TESTS_H) +$(VERIFIER_TESTS_H): $(VERIFIER_TEST_FILES) | $(VERIFIER_TESTS_DIR) $(shell ( cd verifier/; \ echo '/* Generated header, do not edit */'; \ echo '#ifdef FILL_ARRAY'; \ -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies 2019-07-16 19:38 [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies Andrii Nakryiko 2019-07-16 19:38 ` [PATCH bpf 2/2] selftests/bpf: structure test_{progs,maps,verifier} test runners uniformly Andrii Nakryiko @ 2019-07-16 19:55 ` Stanislav Fomichev 2019-07-16 21:40 ` Andrii Nakryiko 1 sibling, 1 reply; 9+ messages in thread From: Stanislav Fomichev @ 2019-07-16 19:55 UTC (permalink / raw) To: Andrii Nakryiko Cc: bpf, netdev, daniel, ast, andrii.nakryiko, kernel-team, Ilya Leoshkevich, Stanislav Fomichev, Martin KaFai Lau 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies 2019-07-16 19:55 ` [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies Stanislav Fomichev @ 2019-07-16 21:40 ` Andrii Nakryiko 2019-07-16 22:57 ` Stanislav Fomichev 0 siblings, 1 reply; 9+ messages in thread From: Andrii Nakryiko @ 2019-07-16 21:40 UTC (permalink / raw) To: Stanislav Fomichev Cc: Andrii Nakryiko, bpf, Networking, Daniel Borkmann, Alexei Starovoitov, Kernel Team, Ilya Leoshkevich, Stanislav Fomichev, Martin KaFai Lau On Tue, Jul 16, 2019 at 12:55 PM Stanislav Fomichev <sdf@fomichev.me> 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). ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies 2019-07-16 21:40 ` Andrii Nakryiko @ 2019-07-16 22:57 ` Stanislav Fomichev 2019-07-16 23:37 ` Andrii Nakryiko 0 siblings, 1 reply; 9+ messages in thread From: Stanislav Fomichev @ 2019-07-16 22:57 UTC (permalink / raw) To: Andrii Nakryiko Cc: Andrii Nakryiko, bpf, Networking, Daniel Borkmann, Alexei Starovoitov, Kernel Team, Ilya Leoshkevich, Stanislav Fomichev, Martin KaFai Lau On 07/16, Andrii Nakryiko wrote: > On Tue, Jul 16, 2019 at 12:55 PM Stanislav Fomichev <sdf@fomichev.me> 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'; \ ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies 2019-07-16 22:57 ` Stanislav Fomichev @ 2019-07-16 23:37 ` Andrii Nakryiko 2019-07-17 0:14 ` Stanislav Fomichev 0 siblings, 1 reply; 9+ messages in thread From: Andrii Nakryiko @ 2019-07-16 23:37 UTC (permalink / raw) To: Stanislav Fomichev Cc: Andrii Nakryiko, bpf, Networking, Daniel Borkmann, Alexei Starovoitov, Kernel Team, Ilya Leoshkevich, Stanislav Fomichev, Martin KaFai Lau On Tue, Jul 16, 2019 at 3:57 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > On 07/16, Andrii Nakryiko wrote: > > On Tue, Jul 16, 2019 at 12:55 PM Stanislav Fomichev <sdf@fomichev.me> 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. Oh, -qp is cool, noted :) > > 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... Well, it's "File is an intermediate prerequisite." in both cases, so I don't know if that's it. Makefiles is not my strong suit, honestly, and definitely not an area of passion, so no idea > > > 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)? Some of those _TEST_DIR's are dependencies of multiple targets, so you'd need to replicate that `mkdir -p $@` in multiple places, which is annoying. But I also don't think we need to worry about creating them, because there is always at least one test (otherwise those tests are useless anyways) in those directories, so we might as well just remove those dependencies, I guess. TBH, those explicit dependencies are good to specify anyways, so I think this fix is good. Second patch just makes the layout of dependencies similar, so it's easier to spot differences like this one. As usual, I'll let Alexei and Daniel decide which one to apply (or if we need to take some other approach to fixing this). > > 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'; \ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies 2019-07-16 23:37 ` Andrii Nakryiko @ 2019-07-17 0:14 ` Stanislav Fomichev 2019-07-17 0:22 ` Andrii Nakryiko 0 siblings, 1 reply; 9+ messages in thread From: Stanislav Fomichev @ 2019-07-17 0:14 UTC (permalink / raw) To: Andrii Nakryiko Cc: Andrii Nakryiko, bpf, Networking, Daniel Borkmann, Alexei Starovoitov, Kernel Team, Ilya Leoshkevich, Stanislav Fomichev, Martin KaFai Lau On 07/16, Andrii Nakryiko wrote: > On Tue, Jul 16, 2019 at 3:57 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > On 07/16, Andrii Nakryiko wrote: > > > On Tue, Jul 16, 2019 at 12:55 PM Stanislav Fomichev <sdf@fomichev.me> 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. > > Oh, -qp is cool, noted :) > > > > > 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... > > Well, it's "File is an intermediate prerequisite." in both cases, so I > don't know if that's it. > Makefiles is not my strong suit, honestly, and definitely not an area > of passion, so no idea > > > > > > > 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)? > > Some of those _TEST_DIR's are dependencies of multiple targets, so > you'd need to replicate that `mkdir -p $@` in multiple places, which > is annoying. Agreed, but one single "mkdir -p $@" might be better than having three lines that define XXX_TEST_DIR and then add a target that mkdir's it. Up to you, I can give it a try once your changes are in; the Makefile becomes hairier by the day, thx for cleaning it up a bit :-) > But I also don't think we need to worry about creating them, because > there is always at least one test (otherwise those tests are useless > anyways) in those directories, so we might as well just remove those > dependencies, I guess. We probably care about them for "make -C tools/selftests O=$PWD/xyz" case where OUTPUT would point to a fresh/clean directory. > TBH, those explicit dependencies are good to specify anyways, so I > think this fix is good. Second patch just makes the layout of > dependencies similar, so it's easier to spot differences like this > one. > > As usual, I'll let Alexei and Daniel decide which one to apply (or if > we need to take some other approach to fixing this). I agree, both of your changes look good. I was just trying to understand what's happening because I was assuming implicit rules to always kick in. > > 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'; \ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies 2019-07-17 0:14 ` Stanislav Fomichev @ 2019-07-17 0:22 ` Andrii Nakryiko 2019-07-17 1:35 ` Alexei Starovoitov 0 siblings, 1 reply; 9+ messages in thread From: Andrii Nakryiko @ 2019-07-17 0:22 UTC (permalink / raw) To: Stanislav Fomichev Cc: Andrii Nakryiko, bpf, Networking, Daniel Borkmann, Alexei Starovoitov, Kernel Team, Ilya Leoshkevich, Stanislav Fomichev, Martin KaFai Lau On Tue, Jul 16, 2019 at 5:15 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > On 07/16, Andrii Nakryiko wrote: > > On Tue, Jul 16, 2019 at 3:57 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > > > > > On 07/16, Andrii Nakryiko wrote: > > > > On Tue, Jul 16, 2019 at 12:55 PM Stanislav Fomichev <sdf@fomichev.me> 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. > > > > Oh, -qp is cool, noted :) > > > > > > > > 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... > > > > Well, it's "File is an intermediate prerequisite." in both cases, so I > > don't know if that's it. > > Makefiles is not my strong suit, honestly, and definitely not an area > > of passion, so no idea > > > > > > > > > > > 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)? > > > > Some of those _TEST_DIR's are dependencies of multiple targets, so > > you'd need to replicate that `mkdir -p $@` in multiple places, which > > is annoying. > Agreed, but one single "mkdir -p $@" might be better than having three > lines that define XXX_TEST_DIR and then add a target that mkdir's it. > Up to you, I can give it a try once your changes are in; the > Makefile becomes hairier by the day, thx for cleaning it up a bit :-) > > > But I also don't think we need to worry about creating them, because > > there is always at least one test (otherwise those tests are useless > > anyways) in those directories, so we might as well just remove those > > dependencies, I guess. > We probably care about them for "make -C tools/selftests O=$PWD/xyz" case > where OUTPUT would point to a fresh/clean directory. Oh, yes, out-of-tree builds, forgot about that, so yeah, we need that. > > > TBH, those explicit dependencies are good to specify anyways, so I > > think this fix is good. Second patch just makes the layout of > > dependencies similar, so it's easier to spot differences like this > > one. > > > > As usual, I'll let Alexei and Daniel decide which one to apply (or if > > we need to take some other approach to fixing this). > I agree, both of your changes look good. I was just trying to understand > what's happening because I was assuming implicit rules to always kick > in. > > > > 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'; \ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies 2019-07-17 0:22 ` Andrii Nakryiko @ 2019-07-17 1:35 ` Alexei Starovoitov 0 siblings, 0 replies; 9+ messages in thread From: Alexei Starovoitov @ 2019-07-17 1:35 UTC (permalink / raw) To: Andrii Nakryiko Cc: Stanislav Fomichev, Andrii Nakryiko, bpf, Networking, Daniel Borkmann, Alexei Starovoitov, Kernel Team, Ilya Leoshkevich, Stanislav Fomichev, Martin KaFai Lau On Tue, Jul 16, 2019 at 5:22 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > Makefile becomes hairier by the day, thx for cleaning it up a bit :-) > > > > > But I also don't think we need to worry about creating them, because > > > there is always at least one test (otherwise those tests are useless > > > anyways) in those directories, so we might as well just remove those > > > dependencies, I guess. > > We probably care about them for "make -C tools/selftests O=$PWD/xyz" case > > where OUTPUT would point to a fresh/clean directory. > > Oh, yes, out-of-tree builds, forgot about that, so yeah, we need that. Anyhow the patches fixed the issue I was seeing. Hence both applied. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-17 1:36 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-16 19:38 [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies Andrii Nakryiko
2019-07-16 19:38 ` [PATCH bpf 2/2] selftests/bpf: structure test_{progs,maps,verifier} test runners uniformly Andrii Nakryiko
2019-07-16 19:55 ` [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies Stanislav Fomichev
2019-07-16 21:40 ` Andrii Nakryiko
2019-07-16 22:57 ` Stanislav Fomichev
2019-07-16 23:37 ` Andrii Nakryiko
2019-07-17 0:14 ` Stanislav Fomichev
2019-07-17 0:22 ` Andrii Nakryiko
2019-07-17 1:35 ` Alexei Starovoitov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).