| Message ID | 20190712135631.91398-1-iii@linux.ibm.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] selftests/bpf: make directory prerequisites order-only | expand |
On Fri, Jul 12, 2019 at 6:57 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote: > > When directories are used as prerequisites in Makefiles, they can cause > a lot of unnecessary rebuilds, because a directory is considered changed > whenever a file in this directory is added, removed or modified. > > If the only thing a target is interested in is the existence of the > directory it depends on, which is the case for selftests/bpf, this > directory should be specified as an order-only prerequisite: it would > still be created in case it does not exist, but it would not trigger a > rebuild of a target in case it's considered changed. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > --- Thanks! Acked-by: Andrii Nakryiko <andriin@fb.com> > tools/testing/selftests/bpf/Makefile | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > [...]
On 7/12/19 3:56 PM, Ilya Leoshkevich wrote: > When directories are used as prerequisites in Makefiles, they can cause > a lot of unnecessary rebuilds, because a directory is considered changed > whenever a file in this directory is added, removed or modified. > > If the only thing a target is interested in is the existence of the > directory it depends on, which is the case for selftests/bpf, this > directory should be specified as an order-only prerequisite: it would > still be created in case it does not exist, but it would not trigger a > rebuild of a target in case it's considered changed. > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> Applied, thanks!
On Mon, Jul 15, 2019 at 3:22 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 7/12/19 3:56 PM, Ilya Leoshkevich wrote: > > When directories are used as prerequisites in Makefiles, they can cause > > a lot of unnecessary rebuilds, because a directory is considered changed > > whenever a file in this directory is added, removed or modified. > > > > If the only thing a target is interested in is the existence of the > > directory it depends on, which is the case for selftests/bpf, this > > directory should be specified as an order-only prerequisite: it would > > still be created in case it does not exist, but it would not trigger a > > rebuild of a target in case it's considered changed. > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > Applied, thanks! Hi Ilya, this commit breaks map_tests. To reproduce: rm map_tests/tests.h make tests.h will not be regenerated. Please provide a fix asap. We cannot ship bpf tree with such failure.
On Tue, Jul 16, 2019 at 10:49 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Mon, Jul 15, 2019 at 3:22 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > > > On 7/12/19 3:56 PM, Ilya Leoshkevich wrote: > > > When directories are used as prerequisites in Makefiles, they can cause > > > a lot of unnecessary rebuilds, because a directory is considered changed > > > whenever a file in this directory is added, removed or modified. > > > > > > If the only thing a target is interested in is the existence of the > > > directory it depends on, which is the case for selftests/bpf, this > > > directory should be specified as an order-only prerequisite: it would > > > still be created in case it does not exist, but it would not trigger a > > > rebuild of a target in case it's considered changed. > > > > > > Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> > > > > Applied, thanks! > > Hi Ilya, > > this commit breaks map_tests. This change just exposed existing problem with Makefile. Sent out fix. > To reproduce: > rm map_tests/tests.h > make > tests.h will not be regenerated. > Please provide a fix asap. > We cannot ship bpf tree with such failure.
> Am 16.07.2019 um 19:49 schrieb Alexei Starovoitov <alexei.starovoitov@gmail.com>: > > On Mon, Jul 15, 2019 at 3:22 PM Daniel Borkmann <daniel@iogearbox.net> wrote: >> >> On 7/12/19 3:56 PM, Ilya Leoshkevich wrote: >>> When directories are used as prerequisites in Makefiles, they can cause >>> a lot of unnecessary rebuilds, because a directory is considered changed >>> whenever a file in this directory is added, removed or modified. >>> >>> If the only thing a target is interested in is the existence of the >>> directory it depends on, which is the case for selftests/bpf, this >>> directory should be specified as an order-only prerequisite: it would >>> still be created in case it does not exist, but it would not trigger a >>> rebuild of a target in case it's considered changed. >>> >>> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> >> >> Applied, thanks! > > Hi Ilya, > > this commit breaks map_tests. > To reproduce: > rm map_tests/tests.h > make > tests.h will not be regenerated. > Please provide a fix asap. > We cannot ship bpf tree with such failure. Hi Alexei, Sorry about this! I actually had the following in my local tree: diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index f1f2b82b8fb8..95795cf5805c 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -231,7 +231,7 @@ ifeq ($(DWARF2BTF),y) endif PROG_TESTS_H := $(OUTPUT)/prog_tests/tests.h -test_progs.c: $(PROG_TESTS_H) +$(OUTPUT)/test_progs: $(PROG_TESTS_H) $(OUTPUT)/test_progs: CFLAGS += $(TEST_PROGS_CFLAGS) $(OUTPUT)/test_progs: prog_tests/*.c @@ -258,7 +258,7 @@ MAP_TESTS_DIR = $(OUTPUT)/map_tests $(MAP_TESTS_DIR): <------>mkdir -p $@ MAP_TESTS_H := $(MAP_TESTS_DIR)/tests.h -test_maps.c: $(MAP_TESTS_H) +$(OUTPUT)/test_maps: $(MAP_TESTS_H) $(OUTPUT)/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS) MAP_TESTS_FILES := $(wildcard map_tests/*.c) $(MAP_TESTS_H): $(MAP_TESTS_FILES) | $(MAP_TESTS_DIR) @@ -275,7 +275,7 @@ $(MAP_TESTS_H): $(MAP_TESTS_FILES) | $(MAP_TESTS_DIR) <------><------> ) > $(MAP_TESTS_H)) VERIFIER_TESTS_H := $(OUTPUT)/verifier/tests.h -test_verifier.c: $(VERIFIER_TESTS_H) +$(OUTPUT)/test_verifier: $(VERIFIER_TESTS_H) $(OUTPUT)/test_verifier: CFLAGS += $(TEST_VERIFIER_CFLAGS) VERIFIER_TESTS_DIR = $(OUTPUT)/verifier but did not realise that this is a pre-requisite for my directories change. I should have tested it separately, then I would have noticed. Andrii, Thanks for helping out and providing the fix! Best regards, Ilya
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index 277d8605e340..0e003fb6641b 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -183,12 +183,12 @@ TEST_CUSTOM_PROGS += $(ALU32_BUILD_DIR)/test_progs_32 $(ALU32_BUILD_DIR): mkdir -p $@ -$(ALU32_BUILD_DIR)/urandom_read: $(OUTPUT)/urandom_read +$(ALU32_BUILD_DIR)/urandom_read: $(OUTPUT)/urandom_read | $(ALU32_BUILD_DIR) cp $< $@ $(ALU32_BUILD_DIR)/test_progs_32: test_progs.c $(OUTPUT)/libbpf.a\ - $(ALU32_BUILD_DIR) \ - $(ALU32_BUILD_DIR)/urandom_read + $(ALU32_BUILD_DIR)/urandom_read \ + | $(ALU32_BUILD_DIR) $(CC) $(TEST_PROGS_CFLAGS) $(CFLAGS) \ -o $(ALU32_BUILD_DIR)/test_progs_32 \ test_progs.c test_stub.c trace_helpers.c prog_tests/*.c \ @@ -197,8 +197,8 @@ $(ALU32_BUILD_DIR)/test_progs_32: test_progs.c $(OUTPUT)/libbpf.a\ $(ALU32_BUILD_DIR)/test_progs_32: $(PROG_TESTS_H) $(ALU32_BUILD_DIR)/test_progs_32: prog_tests/*.c -$(ALU32_BUILD_DIR)/%.o: progs/%.c $(ALU32_BUILD_DIR) \ - $(ALU32_BUILD_DIR)/test_progs_32 +$(ALU32_BUILD_DIR)/%.o: progs/%.c $(ALU32_BUILD_DIR)/test_progs_32 \ + | $(ALU32_BUILD_DIR) ($(CLANG) $(CLANG_FLAGS) -O2 -target bpf -emit-llvm -c $< -o - || \ echo "clang failed") | \ $(LLC) -march=bpf -mattr=+alu32 -mcpu=$(CPU) $(LLC_FLAGS) \ @@ -236,7 +236,7 @@ $(PROG_TESTS_DIR): mkdir -p $@ PROG_TESTS_FILES := $(wildcard prog_tests/*.c) -$(PROG_TESTS_H): $(PROG_TESTS_DIR) $(PROG_TESTS_FILES) +$(PROG_TESTS_H): $(PROG_TESTS_FILES) | $(PROG_TESTS_DIR) $(shell ( cd prog_tests/; \ echo '/* Generated header, do not edit */'; \ echo '#ifdef DECLARE'; \ @@ -257,7 +257,7 @@ MAP_TESTS_H := $(MAP_TESTS_DIR)/tests.h test_maps.c: $(MAP_TESTS_H) $(OUTPUT)/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS) MAP_TESTS_FILES := $(wildcard map_tests/*.c) -$(MAP_TESTS_H): $(MAP_TESTS_DIR) $(MAP_TESTS_FILES) +$(MAP_TESTS_H): $(MAP_TESTS_FILES) | $(MAP_TESTS_DIR) $(shell ( cd map_tests/; \ echo '/* Generated header, do not edit */'; \ echo '#ifdef DECLARE'; \ @@ -279,7 +279,7 @@ $(VERIFIER_TESTS_DIR): mkdir -p $@ VERIFIER_TEST_FILES := $(wildcard verifier/*.c) -$(OUTPUT)/verifier/tests.h: $(VERIFIER_TESTS_DIR) $(VERIFIER_TEST_FILES) +$(OUTPUT)/verifier/tests.h: $(VERIFIER_TEST_FILES) | $(VERIFIER_TESTS_DIR) $(shell ( cd verifier/; \ echo '/* Generated header, do not edit */'; \ echo '#ifdef FILL_ARRAY'; \
When directories are used as prerequisites in Makefiles, they can cause a lot of unnecessary rebuilds, because a directory is considered changed whenever a file in this directory is added, removed or modified. If the only thing a target is interested in is the existence of the directory it depends on, which is the case for selftests/bpf, this directory should be specified as an order-only prerequisite: it would still be created in case it does not exist, but it would not trigger a rebuild of a target in case it's considered changed. Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com> --- tools/testing/selftests/bpf/Makefile | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)