On Tue, Jul 16, 2019 at 3:57 PM Stanislav Fomichev <sdf@xxxxxxxxxxx> wrote:
>
> 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.
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'; \
