| Message ID | 20200522041310.233185-5-yauheni.kaliuta@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | selftests/bpf: installation and out of tree build fixes | expand |
On Thu, May 21, 2020 at 9:14 PM Yauheni Kaliuta <yauheni.kaliuta@redhat.com> wrote: > > There are problems with bpf test programs object files: > > 1) some of them are build for flavored test runner and should be > installed in the subdirectory; > 2) it's possible that the same file mentioned several times (added > for every different unflavored test runner); > 3) some generated files are not treated properly. > > Fix 1) by adding subdirectory to the list. rsync -a in the install > target will handle it. > > Fix 2) by filtering the list. Performance should not matter for such > amount of files. > > Fix 3) by use proper (TEST_GEN_FILES) variable for the list. > > Fixes: 309b81f0fdc4 ("selftests/bpf: Install generated test progs") > Fixes: e47a179997ce ("bpf, testing: Add missing object file to > TEST_FILES") > > Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com> > --- > tools/testing/selftests/bpf/Makefile | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > index 19091dbc8ca4..1ba3d72c3261 100644 > --- a/tools/testing/selftests/bpf/Makefile > +++ b/tools/testing/selftests/bpf/Makefile > @@ -42,8 +42,7 @@ ifneq ($(BPF_GCC),) > TEST_GEN_PROGS += test_progs-bpf_gcc > endif > > -TEST_GEN_FILES = > -TEST_FILES = test_lwt_ip_encap.o \ > +TEST_GEN_FILES = test_lwt_ip_encap.o \ > test_tc_edt.o > > BTF_C_FILES = $(wildcard progs/btf_dump_test_case_*.c) > @@ -273,7 +272,11 @@ TRUNNER_BPF_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o, $$(TRUNNER_BPF_SRCS) > TRUNNER_BPF_SKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.skel.h, \ > $$(filter-out $(SKEL_BLACKLIST), \ > $$(TRUNNER_BPF_SRCS))) > -TEST_GEN_FILES += $$(TRUNNER_BPF_OBJS) > + > +TO_ADD := $(if $2,$$(TRUNNER_OUTPUT),$$(TRUNNER_BPF_OBJS)) > +$$(foreach i,$$(TO_ADD),\ > + $$(eval \ > + TEST_GEN_FILES += $$(if $$(filter $$i,$$(TEST_GEN_FILES)),,$$i))) This makes me cringe. Can we not have three levels of nested evals, please? I also didn't get exactly what's the problem you are trying to solve, could you give some example, please? > > # Evaluate rules now with extra TRUNNER_XXX variables above already defined > $$(eval $$(call DEFINE_TEST_RUNNER_RULES,$1,$2)) > -- > 2.26.2 >
Hi, Andrii! >>>>> On Tue, 26 May 2020 15:30:19 -0700, Andrii Nakryiko wrote: > On Thu, May 21, 2020 at 9:14 PM Yauheni Kaliuta > <yauheni.kaliuta@redhat.com> wrote: >> >> There are problems with bpf test programs object files: >> >> 1) some of them are build for flavored test runner and should be >> installed in the subdirectory; >> 2) it's possible that the same file mentioned several times (added >> for every different unflavored test runner); >> 3) some generated files are not treated properly. >> >> Fix 1) by adding subdirectory to the list. rsync -a in the install >> target will handle it. >> >> Fix 2) by filtering the list. Performance should not matter for such >> amount of files. >> >> Fix 3) by use proper (TEST_GEN_FILES) variable for the list. >> >> Fixes: 309b81f0fdc4 ("selftests/bpf: Install generated test progs") >> Fixes: e47a179997ce ("bpf, testing: Add missing object file to >> TEST_FILES") >> >> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com> >> --- >> tools/testing/selftests/bpf/Makefile | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile >> index 19091dbc8ca4..1ba3d72c3261 100644 >> --- a/tools/testing/selftests/bpf/Makefile >> +++ b/tools/testing/selftests/bpf/Makefile >> @@ -42,8 +42,7 @@ ifneq ($(BPF_GCC),) >> TEST_GEN_PROGS += test_progs-bpf_gcc >> endif >> >> -TEST_GEN_FILES = >> -TEST_FILES = test_lwt_ip_encap.o \ >> +TEST_GEN_FILES = test_lwt_ip_encap.o \ >> test_tc_edt.o >> >> BTF_C_FILES = $(wildcard progs/btf_dump_test_case_*.c) >> @@ -273,7 +272,11 @@ TRUNNER_BPF_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o, $$(TRUNNER_BPF_SRCS) >> TRUNNER_BPF_SKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.skel.h, \ >> $$(filter-out $(SKEL_BLACKLIST), \ >> $$(TRUNNER_BPF_SRCS))) >> -TEST_GEN_FILES += $$(TRUNNER_BPF_OBJS) >> + >> +TO_ADD := $(if $2,$$(TRUNNER_OUTPUT),$$(TRUNNER_BPF_OBJS)) >> +$$(foreach i,$$(TO_ADD),\ >> + $$(eval \ >> + TEST_GEN_FILES += $$(if $$(filter $$i,$$(TEST_GEN_FILES)),,$$i))) > This makes me cringe. Can we not have three levels of nested > evals, please? I also didn't get exactly what's the problem > you are trying to solve, could you give some example, please? It's sort of `unique` functionality. With the current approach TEST_GEN_FILES has at least 2 copies of an object file (for call test_progs and test_maps) which is both inaccurate and increasing the length of the variable (even if copying the same file should not cause problems). (Without sub-directory handling it's even overwritten by flavoured binaries in between). BTW, how would you like to change $(call ...) with $(value ...)? It will get rid of one level of indirection but requires rule-specific variables for rule generation, since some evaluations are done in recipies. >> >> # Evaluate rules now with extra TRUNNER_XXX variables above already defined >> $$(eval $$(call DEFINE_TEST_RUNNER_RULES,$1,$2)) >> -- >> 2.26.2 >>
On Tue, May 26, 2020 at 10:17 PM Yauheni Kaliuta <yauheni.kaliuta@redhat.com> wrote: > > Hi, Andrii! > > >>>>> On Tue, 26 May 2020 15:30:19 -0700, Andrii Nakryiko wrote: > > > On Thu, May 21, 2020 at 9:14 PM Yauheni Kaliuta > > <yauheni.kaliuta@redhat.com> wrote: > >> > >> There are problems with bpf test programs object files: > >> > >> 1) some of them are build for flavored test runner and should be > >> installed in the subdirectory; > >> 2) it's possible that the same file mentioned several times (added > >> for every different unflavored test runner); > >> 3) some generated files are not treated properly. > >> > >> Fix 1) by adding subdirectory to the list. rsync -a in the install > >> target will handle it. > >> > >> Fix 2) by filtering the list. Performance should not matter for such > >> amount of files. > >> > >> Fix 3) by use proper (TEST_GEN_FILES) variable for the list. > >> > >> Fixes: 309b81f0fdc4 ("selftests/bpf: Install generated test progs") > >> Fixes: e47a179997ce ("bpf, testing: Add missing object file to > >> TEST_FILES") > >> > >> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com> > >> --- > >> tools/testing/selftests/bpf/Makefile | 9 ++++++--- > >> 1 file changed, 6 insertions(+), 3 deletions(-) > >> > >> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > >> index 19091dbc8ca4..1ba3d72c3261 100644 > >> --- a/tools/testing/selftests/bpf/Makefile > >> +++ b/tools/testing/selftests/bpf/Makefile > >> @@ -42,8 +42,7 @@ ifneq ($(BPF_GCC),) > >> TEST_GEN_PROGS += test_progs-bpf_gcc > >> endif > >> > >> -TEST_GEN_FILES = > >> -TEST_FILES = test_lwt_ip_encap.o \ > >> +TEST_GEN_FILES = test_lwt_ip_encap.o \ > >> test_tc_edt.o > >> > >> BTF_C_FILES = $(wildcard progs/btf_dump_test_case_*.c) > >> @@ -273,7 +272,11 @@ TRUNNER_BPF_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o, $$(TRUNNER_BPF_SRCS) > >> TRUNNER_BPF_SKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.skel.h, \ > >> $$(filter-out $(SKEL_BLACKLIST), \ > >> $$(TRUNNER_BPF_SRCS))) > >> -TEST_GEN_FILES += $$(TRUNNER_BPF_OBJS) > >> + > >> +TO_ADD := $(if $2,$$(TRUNNER_OUTPUT),$$(TRUNNER_BPF_OBJS)) > >> +$$(foreach i,$$(TO_ADD),\ > >> + $$(eval \ > >> + TEST_GEN_FILES += $$(if $$(filter $$i,$$(TEST_GEN_FILES)),,$$i))) > > > This makes me cringe. Can we not have three levels of nested > > evals, please? I also didn't get exactly what's the problem > > you are trying to solve, could you give some example, please? > > It's sort of `unique` functionality. `unique` in make world is just $(sort $VAR). Isn't that a more light-weight and generic way to avoid duplicates in lib.mk? > > With the current approach TEST_GEN_FILES has at least 2 copies of > an object file (for call test_progs and test_maps) which is both > inaccurate and increasing the length of the variable (even if > copying the same file should not cause problems). > > > (Without sub-directory handling it's even overwritten by > flavoured binaries in between). > > BTW, how would you like to change $(call ...) with $(value ...)? > It will get rid of one level of indirection but requires > rule-specific variables for rule generation, since some > evaluations are done in recipies. I don't exactly understand the implications, so don't know. But the less changes to this Makefile, the happier I am, so... :) > > >> > >> # Evaluate rules now with extra TRUNNER_XXX variables above already defined > >> $$(eval $$(call DEFINE_TEST_RUNNER_RULES,$1,$2)) > >> -- > >> 2.26.2 > >> > > > -- > WBR, > Yauheni Kaliuta >
On Thu, May 28, 2020 at 9:40 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, May 26, 2020 at 10:17 PM Yauheni Kaliuta > <yauheni.kaliuta@redhat.com> wrote: > > > > Hi, Andrii! > > > > >>>>> On Tue, 26 May 2020 15:30:19 -0700, Andrii Nakryiko wrote: > > > > > On Thu, May 21, 2020 at 9:14 PM Yauheni Kaliuta > > > <yauheni.kaliuta@redhat.com> wrote: > > >> > > >> There are problems with bpf test programs object files: > > >> > > >> 1) some of them are build for flavored test runner and should be > > >> installed in the subdirectory; > > >> 2) it's possible that the same file mentioned several times (added > > >> for every different unflavored test runner); > > >> 3) some generated files are not treated properly. > > >> > > >> Fix 1) by adding subdirectory to the list. rsync -a in the install > > >> target will handle it. > > >> > > >> Fix 2) by filtering the list. Performance should not matter for such > > >> amount of files. > > >> > > >> Fix 3) by use proper (TEST_GEN_FILES) variable for the list. > > >> > > >> Fixes: 309b81f0fdc4 ("selftests/bpf: Install generated test progs") > > >> Fixes: e47a179997ce ("bpf, testing: Add missing object file to > > >> TEST_FILES") > > >> > > >> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com> > > >> --- > > >> tools/testing/selftests/bpf/Makefile | 9 ++++++--- > > >> 1 file changed, 6 insertions(+), 3 deletions(-) > > >> > > >> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile > > >> index 19091dbc8ca4..1ba3d72c3261 100644 > > >> --- a/tools/testing/selftests/bpf/Makefile > > >> +++ b/tools/testing/selftests/bpf/Makefile > > >> @@ -42,8 +42,7 @@ ifneq ($(BPF_GCC),) > > >> TEST_GEN_PROGS += test_progs-bpf_gcc > > >> endif > > >> > > >> -TEST_GEN_FILES = > > >> -TEST_FILES = test_lwt_ip_encap.o \ > > >> +TEST_GEN_FILES = test_lwt_ip_encap.o \ > > >> test_tc_edt.o > > >> > > >> BTF_C_FILES = $(wildcard progs/btf_dump_test_case_*.c) > > >> @@ -273,7 +272,11 @@ TRUNNER_BPF_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o, $$(TRUNNER_BPF_SRCS) > > >> TRUNNER_BPF_SKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.skel.h, \ > > >> $$(filter-out $(SKEL_BLACKLIST), \ > > >> $$(TRUNNER_BPF_SRCS))) > > >> -TEST_GEN_FILES += $$(TRUNNER_BPF_OBJS) > > >> + > > >> +TO_ADD := $(if $2,$$(TRUNNER_OUTPUT),$$(TRUNNER_BPF_OBJS)) > > >> +$$(foreach i,$$(TO_ADD),\ > > >> + $$(eval \ > > >> + TEST_GEN_FILES += $$(if $$(filter $$i,$$(TEST_GEN_FILES)),,$$i))) > > > > > This makes me cringe. Can we not have three levels of nested > > > evals, please? I also didn't get exactly what's the problem > > > you are trying to solve, could you give some example, please? > > > > It's sort of `unique` functionality. > > `unique` in make world is just $(sort $VAR). Isn't that a more > light-weight and generic way to avoid duplicates in lib.mk? Oh, my bad, totally forgot it. Sure! Thanks! > > > > > With the current approach TEST_GEN_FILES has at least 2 copies of > > an object file (for call test_progs and test_maps) which is both > > inaccurate and increasing the length of the variable (even if > > copying the same file should not cause problems). > > > > > > (Without sub-directory handling it's even overwritten by > > flavoured binaries in between). > > > > BTW, how would you like to change $(call ...) with $(value ...)? > > It will get rid of one level of indirection but requires > > rule-specific variables for rule generation, since some > > evaluations are done in recipies. > > I don't exactly understand the implications, so don't know. But the > less changes to this Makefile, the happier I am, so... :) So, no way ;) It would be relatively many changes, but more simple code without extra $$. > > > > > >> > > >> # Evaluate rules now with extra TRUNNER_XXX variables above already defined > > >> $$(eval $$(call DEFINE_TEST_RUNNER_RULES,$1,$2)) > > >> -- > > >> 2.26.2 > > >> > > > > > > -- > > WBR, > > Yauheni Kaliuta > > >
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index 19091dbc8ca4..1ba3d72c3261 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -42,8 +42,7 @@ ifneq ($(BPF_GCC),) TEST_GEN_PROGS += test_progs-bpf_gcc endif -TEST_GEN_FILES = -TEST_FILES = test_lwt_ip_encap.o \ +TEST_GEN_FILES = test_lwt_ip_encap.o \ test_tc_edt.o BTF_C_FILES = $(wildcard progs/btf_dump_test_case_*.c) @@ -273,7 +272,11 @@ TRUNNER_BPF_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o, $$(TRUNNER_BPF_SRCS) TRUNNER_BPF_SKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.skel.h, \ $$(filter-out $(SKEL_BLACKLIST), \ $$(TRUNNER_BPF_SRCS))) -TEST_GEN_FILES += $$(TRUNNER_BPF_OBJS) + +TO_ADD := $(if $2,$$(TRUNNER_OUTPUT),$$(TRUNNER_BPF_OBJS)) +$$(foreach i,$$(TO_ADD),\ + $$(eval \ + TEST_GEN_FILES += $$(if $$(filter $$i,$$(TEST_GEN_FILES)),,$$i))) # Evaluate rules now with extra TRUNNER_XXX variables above already defined $$(eval $$(call DEFINE_TEST_RUNNER_RULES,$1,$2))
There are problems with bpf test programs object files: 1) some of them are build for flavored test runner and should be installed in the subdirectory; 2) it's possible that the same file mentioned several times (added for every different unflavored test runner); 3) some generated files are not treated properly. Fix 1) by adding subdirectory to the list. rsync -a in the install target will handle it. Fix 2) by filtering the list. Performance should not matter for such amount of files. Fix 3) by use proper (TEST_GEN_FILES) variable for the list. Fixes: 309b81f0fdc4 ("selftests/bpf: Install generated test progs") Fixes: e47a179997ce ("bpf, testing: Add missing object file to TEST_FILES") Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com> --- tools/testing/selftests/bpf/Makefile | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)