[PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies

[PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies

[Andrii Nakryiko]

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

[PATCH bpf 2/2] selftests/bpf: structure test_{progs,maps,verifier} test runners uniformly

[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

Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies

[Stanislav Fomichev]

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?

Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies

[Andrii Nakryiko]

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).

Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies

[Stanislav Fomichev]

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'; \

Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies

[Andrii Nakryiko]

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'; \

Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies

[Stanislav Fomichev]

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'; \

Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies

[Andrii Nakryiko]

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'; \

Re: [PATCH bpf 1/2] selftests/bpf: fix test_verifier/test_maps make dependencies

[Alexei Starovoitov]

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.