[PATCH 0/8] selftests/bpf: installation and out of tree build fixes

[PATCH 0/8] selftests/bpf: installation and out of tree build fixes

[Yauheni Kaliuta]

I had a look, here are some fixes.

Yauheni Kaliuta (8):
  selftests/bpf: remove test_align from Makefile
  selftests/bpf: build bench.o for any $(OUTPUT)
  selftests/bpf: install btf .c files
  selftests/bpf: fix object files installation
  selftests/bpf: add output dir to include list
  selftests/bpf: fix urandom_read installation
  selftests/bpf: fix test.h placing for out of tree build
  selftests/bpf: factor out MKDIR rule

 tools/testing/selftests/bpf/Makefile | 77 ++++++++++++++++++++--------
 1 file changed, 55 insertions(+), 22 deletions(-)

-- 
2.26.2

[PATCH 1/8] selftests/bpf: remove test_align from Makefile

[Yauheni Kaliuta]

test_align has been moved under test_progs.

Fixes: 3b09d27cc93d ("selftests/bpf: Move test_align under
test_progs")

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 tools/testing/selftests/bpf/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index e716e931d0c9..09700db35c2d 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -30,7 +30,7 @@ LDLIBS += -lcap -lelf -lz -lrt -lpthread
 
 # Order correspond to 'make run_tests' order
 TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
-	test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
+	test_verifier_log test_dev_cgroup test_tcpbpf_user \
 	test_sock test_btf test_sockmap get_cgroup_id_user test_socket_cookie \
 	test_cgroup_storage \
 	test_netcnt test_tcpnotify_user test_sock_fields test_sysctl \
-- 
2.26.2

[PATCH 2/8] selftests/bpf: build bench.o for any $(OUTPUT)

[Yauheni Kaliuta]

bench.o is produced by implicit rule only if it's built in the same
directory where bench.c is located. If OUTPUT points somewhere else,
build fails.

Make an explicit rule for it (factor out common part).
Add bench.c as a dependency to make it source for CC.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 tools/testing/selftests/bpf/Makefile | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 09700db35c2d..f0b7d41ed6dd 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -243,6 +243,11 @@ define GCC_BPF_BUILD_RULE
 	$(BPF_GCC) $3 $4 -O2 -c $1 -o $2
 endef
 
+define COMPILE_C_RULE
+	$(call msg,CC,,$@)
+	$(CC) $(CFLAGS) -c $(filter %.c,$^) $(LDLIBS) -o $@
+endef
+
 SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
 
 # Set up extra TRUNNER_XXX "temporary" variables in the environment (relies on
@@ -409,11 +414,11 @@ $(OUTPUT)/test_cpp: test_cpp.cpp $(OUTPUT)/test_core_extern.skel.h $(BPFOBJ)
 
 # Benchmark runner
 $(OUTPUT)/bench_%.o: benchs/bench_%.c bench.h
-	$(call msg,CC,,$@)
-	$(CC) $(CFLAGS) -c $(filter %.c,$^) $(LDLIBS) -o $@
+	$(COMPILE_C_RULE)
 $(OUTPUT)/bench_rename.o: $(OUTPUT)/test_overhead.skel.h
 $(OUTPUT)/bench_trigger.o: $(OUTPUT)/trigger_bench.skel.h
-$(OUTPUT)/bench.o: bench.h testing_helpers.h
+$(OUTPUT)/bench.o: bench.c bench.h testing_helpers.h
+	$(COMPILE_C_RULE)
 $(OUTPUT)/bench: LDLIBS += -lm
 $(OUTPUT)/bench: $(OUTPUT)/bench.o $(OUTPUT)/testing_helpers.o \
 		 $(OUTPUT)/bench_count.o \
-- 
2.26.2

[PATCH 3/8] selftests/bpf: install btf .c files

[Yauheni Kaliuta]

Some .c files used by test_progs to check btf and they are missing
from installation after commit 74b5a5968fe8 ("selftests/bpf: Replace
test_progs and test_maps w/ general rule").

Take them back.

Reuse BTF_C_FILES for TRUNNER_EXTRA_FILES.

Fixes: 74b5a5968fe8 ("selftests/bpf: Replace test_progs and
test_maps w/ general rule")

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
Acked-by: Andrii Nakryiko <andriin@fb.com>
---
 tools/testing/selftests/bpf/Makefile | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index f0b7d41ed6dd..19091dbc8ca4 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -46,6 +46,9 @@ TEST_GEN_FILES =
 TEST_FILES = test_lwt_ip_encap.o \
 	test_tc_edt.o
 
+BTF_C_FILES = $(wildcard progs/btf_dump_test_case_*.c)
+TEST_FILES += $(BTF_C_FILES)
+
 # Order correspond to 'make run_tests' order
 TEST_PROGS := test_kmod.sh \
 	test_xdp_redirect.sh \
@@ -362,8 +365,7 @@ TRUNNER_BPF_PROGS_DIR := progs
 TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c	\
 			 network_helpers.c testing_helpers.c		\
 			 flow_dissector_load.h
-TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read				\
-		       $(wildcard progs/btf_dump_test_case_*.c)
+TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(BTF_C_FILES)
 TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
 TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS)
 TRUNNER_BPF_LDFLAGS := -mattr=+alu32
-- 
2.26.2

[PATCH 4/8] selftests/bpf: fix object files installation

[Yauheni Kaliuta]

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)))
 
 # Evaluate rules now with extra TRUNNER_XXX variables above already defined
 $$(eval $$(call DEFINE_TEST_RUNNER_RULES,$1,$2))
-- 
2.26.2

[PATCH 5/8] selftests/bpf: add output dir to include list

[Yauheni Kaliuta]

Some headers (skel) are generated in the output directory and used
for build (test_cpp.cpp), so add it to the list (as well as
CURDIR) otherwise out of tree build is broken.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 tools/testing/selftests/bpf/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 1ba3d72c3261..efab82151ce2 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -22,7 +22,8 @@ LLVM_OBJCOPY	?= llvm-objcopy
 BPF_GCC		?= $(shell command -v bpf-gcc;)
 SAN_CFLAGS	?=
 CFLAGS += -g -rdynamic -Wall -O2 $(GENFLAGS) $(SAN_CFLAGS)		\
-	  -I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR)		\
+	  -I$(CURDIR) -I$(abspath $(OUTPUT)) 				\
+	  -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR)			\
 	  -I$(TOOLSINCDIR) -I$(APIDIR)					\
 	  -Dbpf_prog_load=bpf_prog_test_load				\
 	  -Dbpf_load_program=bpf_test_load_program
-- 
2.26.2

[PATCH 6/8] selftests/bpf: fix urandom_read installation

[Yauheni Kaliuta]

selftests/lib.mk does not prepend TEST_CUSTOM_PROGS with OUTPUT (vs
TEST_GEN_PROGS, TEST_GEN_PROGS_EXTENDED, TEST_GEN_FILES). So do it
in the bpf Makefile. Otherwise make install fails to install it on
out of tree build.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 tools/testing/selftests/bpf/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index efab82151ce2..31598ca2d396 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -82,7 +82,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
 	flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
 	test_lirc_mode2_user xdping test_cpp runqslower bench
 
-TEST_CUSTOM_PROGS = urandom_read
+TEST_CUSTOM_PROGS = $(OUTPUT)/urandom_read
 
 # Emit succinct information message describing current building step
 # $1 - generic step name (e.g., CC, LINK, etc);
-- 
2.26.2

[PATCH 7/8] selftests/bpf: fix test.h placing for out of tree build

[Yauheni Kaliuta]

Flavors of test.h are generated in tree even for out of tree
build. Use OUTPUT directory for that.

It requires rules to make sure the directories exist.

Split EXTRA_CLEAN generation since existance of test.h files depends
of dynamic makefile generation.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 tools/testing/selftests/bpf/Makefile | 38 +++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 31598ca2d396..bade24e29a1a 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -83,6 +83,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
 	test_lirc_mode2_user xdping test_cpp runqslower bench
 
 TEST_CUSTOM_PROGS = $(OUTPUT)/urandom_read
+EXTRA_CLEAN += $(TEST_CUSTOM_PROGS)
 
 # Emit succinct information message describing current building step
 # $1 - generic step name (e.g., CC, LINK, etc);
@@ -267,7 +268,7 @@ TRUNNER_TEST_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.test.o,	\
 TRUNNER_EXTRA_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o,		\
 				 $$(filter %.c,$(TRUNNER_EXTRA_SOURCES)))
 TRUNNER_EXTRA_HDRS := $$(filter %.h,$(TRUNNER_EXTRA_SOURCES))
-TRUNNER_TESTS_HDR := $(TRUNNER_TESTS_DIR)/tests.h
+TRUNNER_TESTS_HDR := $(OUTPUT)/$(TRUNNER_TESTS_DIR)/tests.h
 TRUNNER_BPF_SRCS := $$(notdir $$(wildcard $(TRUNNER_BPF_PROGS_DIR)/*.c))
 TRUNNER_BPF_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o, $$(TRUNNER_BPF_SRCS))
 TRUNNER_BPF_SKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.skel.h,	\
@@ -295,6 +296,11 @@ $(TRUNNER_OUTPUT)-dir := y
 $(TRUNNER_OUTPUT):
 	$$(call msg,MKDIR,,$$@)
 	mkdir -p $$@
+
+ifneq ($2,)
+EXTRA_CLEAN +=$(TRUNNER_OUTPUT)
+endif
+
 endif
 
 # ensure we set up BPF objects generation rule just once for a given
@@ -320,13 +326,19 @@ endif
 # ensure we set up tests.h header generation rule just once
 ifeq ($($(TRUNNER_TESTS_DIR)-tests-hdr),)
 $(TRUNNER_TESTS_DIR)-tests-hdr := y
-$(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/*.c
+$(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/*.c | $(dir $(TRUNNER_TESTS_HDR))
 	$$(call msg,TEST-HDR,$(TRUNNER_BINARY),$$@)
 	$$(shell ( cd $(TRUNNER_TESTS_DIR);				\
 		  echo '/* Generated header, do not edit */';		\
 		  ls *.c 2> /dev/null |					\
 			sed -e 's@\([^\.]*\)\.c@DEFINE_TEST(\1)@';	\
 		 ) > $$@)
+
+EXTRA_CLEAN += $(TRUNNER_TESTS_HDR)
+
+$(dir $(TRUNNER_TESTS_HDR)):
+	$$(call msg,MKDIR,,$$@)
+	mkdir -p $$@
 endif
 
 # compile individual test files
@@ -402,14 +414,23 @@ $(eval $(call DEFINE_TEST_RUNNER,test_maps))
 # It is much simpler than test_maps/test_progs and sufficiently different from
 # them (e.g., test.h is using completely pattern), that it's worth just
 # explicitly defining all the rules explicitly.
-verifier/tests.h: verifier/*.c
+$(OUTPUT)/verifier/tests.h: verifier/*.c | $(OUTPUT)/verifier
 	$(shell ( cd verifier/; \
 		  echo '/* Generated header, do not edit */'; \
 		  echo '#ifdef FILL_ARRAY'; \
 		  ls *.c 2> /dev/null | sed -e 's@\(.*\)@#include \"\1\"@'; \
 		  echo '#endif' \
-		) > verifier/tests.h)
-$(OUTPUT)/test_verifier: test_verifier.c verifier/tests.h $(BPFOBJ) | $(OUTPUT)
+		) > $@)
+
+EXTRA_CLEAN += $(OUTPUT)/verifier/tests.h
+
+$(OUTPUT)/verifier:
+	$(call msg,MKDIR,,$@)
+	mkdir -p $@
+
+$(OUTPUT)/test_verifier: CFLAGS += -I$(abspath verifier)
+$(OUTPUT)/test_verifier: test_verifier.c $(OUTPUT)/verifier/tests.h $(BPFOBJ) \
+			| $(OUTPUT)
 	$(call msg,BINARY,,$@)
 	$(CC) $(CFLAGS) $(filter %.a %.o %.c,$^) $(LDLIBS) -o $@
 
@@ -433,7 +454,6 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o $(OUTPUT)/testing_helpers.o \
 	$(call msg,BINARY,,$@)
 	$(CC) $(LDFLAGS) -o $@ $(filter %.a %.o,$^) $(LDLIBS)
 
-EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR)			\
-	prog_tests/tests.h map_tests/tests.h verifier/tests.h		\
-	feature								\
-	$(addprefix $(OUTPUT)/,*.o *.skel.h no_alu32 bpf_gcc)
+EXTRA_CLEAN += $(SCRATCH_DIR)			\
+	feature					\
+	$(addprefix $(OUTPUT)/,*.o *.skel.h)
-- 
2.26.2

[PATCH 8/8] selftests/bpf: factor out MKDIR rule

[Yauheni Kaliuta]

Do not repeat youself, move common mkdir code (message and action)
to a variable.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 tools/testing/selftests/bpf/Makefile | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index bade24e29a1a..26497d8869ea 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -252,6 +252,11 @@ define COMPILE_C_RULE
 	$(CC) $(CFLAGS) -c $(filter %.c,$^) $(LDLIBS) -o $@
 endef
 
+define MKDIR_RULE
+	$(call msg,MKDIR,,$@)
+	mkdir -p $@
+endef
+
 SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
 
 # Set up extra TRUNNER_XXX "temporary" variables in the environment (relies on
@@ -294,8 +299,7 @@ define DEFINE_TEST_RUNNER_RULES
 ifeq ($($(TRUNNER_OUTPUT)-dir),)
 $(TRUNNER_OUTPUT)-dir := y
 $(TRUNNER_OUTPUT):
-	$$(call msg,MKDIR,,$$@)
-	mkdir -p $$@
+	$$(MKDIR_RULE)
 
 ifneq ($2,)
 EXTRA_CLEAN +=$(TRUNNER_OUTPUT)
@@ -337,8 +341,7 @@ $(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/*.c | $(dir $(TRUNNER_TESTS_HDR))
 EXTRA_CLEAN += $(TRUNNER_TESTS_HDR)
 
 $(dir $(TRUNNER_TESTS_HDR)):
-	$$(call msg,MKDIR,,$$@)
-	mkdir -p $$@
+	$$(MKDIR_RULE)
 endif
 
 # compile individual test files
@@ -425,8 +428,7 @@ $(OUTPUT)/verifier/tests.h: verifier/*.c | $(OUTPUT)/verifier
 EXTRA_CLEAN += $(OUTPUT)/verifier/tests.h
 
 $(OUTPUT)/verifier:
-	$(call msg,MKDIR,,$@)
-	mkdir -p $@
+	$(MKDIR_RULE)
 
 $(OUTPUT)/test_verifier: CFLAGS += -I$(abspath verifier)
 $(OUTPUT)/test_verifier: test_verifier.c $(OUTPUT)/verifier/tests.h $(BPFOBJ) \
-- 
2.26.2

Re: [PATCH 0/8] selftests/bpf: installation and out of tree build fixes

[Yauheni Kaliuta]

Actually, a bit more needed :)

>>>>> On Fri, 22 May 2020 07:13:02 +0300, Yauheni Kaliuta  wrote:

 > I had a look, here are some fixes.
 > Yauheni Kaliuta (8):
 >   selftests/bpf: remove test_align from Makefile
 >   selftests/bpf: build bench.o for any $(OUTPUT)
 >   selftests/bpf: install btf .c files
 >   selftests/bpf: fix object files installation
 >   selftests/bpf: add output dir to include list
 >   selftests/bpf: fix urandom_read installation
 >   selftests/bpf: fix test.h placing for out of tree build
 >   selftests/bpf: factor out MKDIR rule

 >  tools/testing/selftests/bpf/Makefile | 77 ++++++++++++++++++++--------
 >  1 file changed, 55 insertions(+), 22 deletions(-)

 > -- 
 > 2.26.2


-- 
WBR,
Yauheni Kaliuta

[PATCH] selftests/bpf: split -extras target to -static and -gen

[Yauheni Kaliuta]

There is difference in depoying static and generated extra resource
files between in/out of tree build and flavors:

- in case of unflavored out-of-tree build static files are not
available and must be copied as well as both static and generated
files for flavored build.

So split the rules and variables. The name TRUNNER_EXTRA_GEN_FILES
is chosen in analogy to TEST_GEN_* variants.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 tools/testing/selftests/bpf/Makefile | 26 ++++++++++++++++++++++----
 1 file changed, 22 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 26497d8869ea..c80c06272759 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -363,12 +363,28 @@ $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o:				\
 	$$(call msg,EXT-OBJ,$(TRUNNER_BINARY),$$@)
 	$$(CC) $$(CFLAGS) -c $$< $$(LDLIBS) -o $$@
 
-# only copy extra resources if in flavored build
-$(TRUNNER_BINARY)-extras: $(TRUNNER_EXTRA_FILES) | $(TRUNNER_OUTPUT)
-ifneq ($2,)
+# copy extra resources when needed.
+# Static files for both out of tree and flavored (so, not current dir).
+# Generated files for flavored only.
+$(TRUNNER_BINARY)-extras: $(TRUNNER_BINARY)-extras-static \
+			  $(TRUNNER_BINARY)-extras-gen
+
+$(TRUNNER_BINARY)-extras-static: $(TRUNNER_EXTRA_FILES) | $(TRUNNER_OUTPUT)
+ifneq ($(CURDIR)),$(realpath $(TRUNNER_OUTPUT)))
 	$$(call msg,EXT-COPY,$(TRUNNER_BINARY),$(TRUNNER_EXTRA_FILES))
+ifneq ($(TRUNNER_EXTRA_FILES),)
 	cp -a $$^ $(TRUNNER_OUTPUT)/
 endif
+endif
+
+$(TRUNNER_BINARY)-extras-gen: $(addprefix $(OUTPUT)/,$(TRUNNER_EXTRA_GEN_FILES)) \
+			    | $(TRUNNER_OUTPUT)
+ifneq ($2,)
+	$$(call msg,EXT-COPY,$(TRUNNER_BINARY),$(TRUNNER_EXTRA_GEN_FILES))
+ifneq ($(TRUNNER_EXTRA_GEN_FILES),)
+	cp -a $$^ $(TRUNNER_OUTPUT)/
+endif
+endif
 
 $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)			\
 			     $(TRUNNER_EXTRA_OBJS) $$(BPFOBJ)		\
@@ -384,7 +400,8 @@ TRUNNER_BPF_PROGS_DIR := progs
 TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c	\
 			 network_helpers.c testing_helpers.c		\
 			 flow_dissector_load.h
-TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(BTF_C_FILES)
+TRUNNER_EXTRA_FILES := $(BTF_C_FILES)
+TRUNNER_EXTRA_GEN_FILES := urandom_read
 TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
 TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS)
 TRUNNER_BPF_LDFLAGS := -mattr=+alu32
@@ -408,6 +425,7 @@ TRUNNER_TESTS_DIR := map_tests
 TRUNNER_BPF_PROGS_DIR := progs
 TRUNNER_EXTRA_SOURCES := test_maps.c
 TRUNNER_EXTRA_FILES :=
+TRUNNER_EXTRA_GEN_FILES :=
 TRUNNER_BPF_BUILD_RULE := $$(error no BPF objects should be built)
 TRUNNER_BPF_CFLAGS :=
 TRUNNER_BPF_LDFLAGS :=
-- 
2.26.2

Re: [PATCH 0/8] selftests/bpf: installation and out of tree build fixes

[Daniel Borkmann]

On 5/22/20 8:40 AM, Yauheni Kaliuta wrote:
> 
> Actually, a bit more needed :)

Not quite sure how to parse this, I presume you are intending to send a v2 of
this series with [0] folded in? Please also do not add line-breaks in the middle
of all your Fixes tags as otherwise it would break searching for commits in the
git log. For the v2 respin, please also add a better cover letter than just saying
nothing more than 'I had a look, here are some fixes.'. At least a minimal high
level summary of the selftest Makefile changes in this series.

Thanks,
Daniel

   [0] https://patchwork.ozlabs.org/project/netdev/patch/20200522081901.238516-1-yauheni.kaliuta@redhat.com/

>>>>>> On Fri, 22 May 2020 07:13:02 +0300, Yauheni Kaliuta  wrote:
> 
>   > I had a look, here are some fixes.
>   > Yauheni Kaliuta (8):
>   >   selftests/bpf: remove test_align from Makefile
>   >   selftests/bpf: build bench.o for any $(OUTPUT)
>   >   selftests/bpf: install btf .c files
>   >   selftests/bpf: fix object files installation
>   >   selftests/bpf: add output dir to include list
>   >   selftests/bpf: fix urandom_read installation
>   >   selftests/bpf: fix test.h placing for out of tree build
>   >   selftests/bpf: factor out MKDIR rule
> 
>   >  tools/testing/selftests/bpf/Makefile | 77 ++++++++++++++++++++--------
>   >  1 file changed, 55 insertions(+), 22 deletions(-)
> 
>   > --
>   > 2.26.2
> 
> 

Re: [PATCH 1/8] selftests/bpf: remove test_align from Makefile

[Andrii Nakryiko]

On Thu, May 21, 2020 at 9:14 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> test_align has been moved under test_progs.
>
> Fixes: 3b09d27cc93d ("selftests/bpf: Move test_align under
> test_progs")
>
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---

Fixes tag has to be on the single line, like this:

Fixes: 3b09d27cc93d ("selftests/bpf: Move test_align under test_progs")

Also no need for empty line between Fixes: and Signed-off-by: tags.

With that fixed, please add my ack:

Acked-by: Andrii Nakryiko <andriin@fb.com>



>  tools/testing/selftests/bpf/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index e716e931d0c9..09700db35c2d 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -30,7 +30,7 @@ LDLIBS += -lcap -lelf -lz -lrt -lpthread
>
>  # Order correspond to 'make run_tests' order
>  TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
> -       test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
> +       test_verifier_log test_dev_cgroup test_tcpbpf_user \
>         test_sock test_btf test_sockmap get_cgroup_id_user test_socket_cookie \
>         test_cgroup_storage \
>         test_netcnt test_tcpnotify_user test_sock_fields test_sysctl \
> --
> 2.26.2
>

Re: [PATCH 2/8] selftests/bpf: build bench.o for any $(OUTPUT)

[Andrii Nakryiko]

On Thu, May 21, 2020 at 9:14 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> bench.o is produced by implicit rule only if it's built in the same
> directory where bench.c is located. If OUTPUT points somewhere else,
> build fails.
>
> Make an explicit rule for it (factor out common part).
> Add bench.c as a dependency to make it source for CC.

If that's the case, then the similar problem would happen to
test_l4lb_noinline.o, test_xdp_noinline.o, and flow_dissector_load.o,
at least. Let's fix the implicit rule (or define our own, but generic
one), instead of ad-hoc fixing it for bench.o only.


>
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---
>  tools/testing/selftests/bpf/Makefile | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 09700db35c2d..f0b7d41ed6dd 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -243,6 +243,11 @@ define GCC_BPF_BUILD_RULE
>         $(BPF_GCC) $3 $4 -O2 -c $1 -o $2
>  endef
>
> +define COMPILE_C_RULE
> +       $(call msg,CC,,$@)
> +       $(CC) $(CFLAGS) -c $(filter %.c,$^) $(LDLIBS) -o $@
> +endef
> +
>  SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
>
>  # Set up extra TRUNNER_XXX "temporary" variables in the environment (relies on
> @@ -409,11 +414,11 @@ $(OUTPUT)/test_cpp: test_cpp.cpp $(OUTPUT)/test_core_extern.skel.h $(BPFOBJ)
>
>  # Benchmark runner
>  $(OUTPUT)/bench_%.o: benchs/bench_%.c bench.h
> -       $(call msg,CC,,$@)
> -       $(CC) $(CFLAGS) -c $(filter %.c,$^) $(LDLIBS) -o $@
> +       $(COMPILE_C_RULE)
>  $(OUTPUT)/bench_rename.o: $(OUTPUT)/test_overhead.skel.h
>  $(OUTPUT)/bench_trigger.o: $(OUTPUT)/trigger_bench.skel.h
> -$(OUTPUT)/bench.o: bench.h testing_helpers.h
> +$(OUTPUT)/bench.o: bench.c bench.h testing_helpers.h
> +       $(COMPILE_C_RULE)
>  $(OUTPUT)/bench: LDLIBS += -lm
>  $(OUTPUT)/bench: $(OUTPUT)/bench.o $(OUTPUT)/testing_helpers.o \
>                  $(OUTPUT)/bench_count.o \
> --
> 2.26.2
>

Re: [PATCH 5/8] selftests/bpf: add output dir to include list

[Andrii Nakryiko]

On Thu, May 21, 2020 at 9:14 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Some headers (skel) are generated in the output directory and used
> for build (test_cpp.cpp), so add it to the list (as well as
> CURDIR) otherwise out of tree build is broken.
>

So this is needed only for test_cpp, right? Because test_prog's tests
are built with a special rule changing their workdir to the one that
contains proper skeleton. Let's just add -I$(OUTPUT) to test_cpp rule
instead of adding it to every single .c build rule? If we later still
need this, then we should consider adding it widely.


> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---
>  tools/testing/selftests/bpf/Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 1ba3d72c3261..efab82151ce2 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -22,7 +22,8 @@ LLVM_OBJCOPY  ?= llvm-objcopy
>  BPF_GCC                ?= $(shell command -v bpf-gcc;)
>  SAN_CFLAGS     ?=
>  CFLAGS += -g -rdynamic -Wall -O2 $(GENFLAGS) $(SAN_CFLAGS)             \
> -         -I$(CURDIR) -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR)          \
> +         -I$(CURDIR) -I$(abspath $(OUTPUT))                            \
> +         -I$(INCLUDE_DIR) -I$(GENDIR) -I$(LIBDIR)                      \
>           -I$(TOOLSINCDIR) -I$(APIDIR)                                  \
>           -Dbpf_prog_load=bpf_prog_test_load                            \
>           -Dbpf_load_program=bpf_test_load_program
> --
> 2.26.2
>

Re: [PATCH 6/8] selftests/bpf: fix urandom_read installation

[Andrii Nakryiko]

On Thu, May 21, 2020 at 9:14 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> selftests/lib.mk does not prepend TEST_CUSTOM_PROGS with OUTPUT (vs
> TEST_GEN_PROGS, TEST_GEN_PROGS_EXTENDED, TEST_GEN_FILES). So do it
> in the bpf Makefile. Otherwise make install fails to install it on
> out of tree build.
>
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---

LGTM.

Acked-by: Andrii Nakryiko <andriin@fb.com>


>  tools/testing/selftests/bpf/Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index efab82151ce2..31598ca2d396 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -82,7 +82,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
>         flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
>         test_lirc_mode2_user xdping test_cpp runqslower bench
>
> -TEST_CUSTOM_PROGS = urandom_read
> +TEST_CUSTOM_PROGS = $(OUTPUT)/urandom_read
>
>  # Emit succinct information message describing current building step
>  # $1 - generic step name (e.g., CC, LINK, etc);
> --
> 2.26.2
>

Re: [PATCH 7/8] selftests/bpf: fix test.h placing for out of tree build

[Andrii Nakryiko]

On Thu, May 21, 2020 at 9:14 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Flavors of test.h are generated in tree even for out of tree
> build. Use OUTPUT directory for that.
>
> It requires rules to make sure the directories exist.
>
> Split EXTRA_CLEAN generation since existance of test.h files depends
> of dynamic makefile generation.
>
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---
>  tools/testing/selftests/bpf/Makefile | 38 +++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 31598ca2d396..bade24e29a1a 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -83,6 +83,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
>         test_lirc_mode2_user xdping test_cpp runqslower bench
>
>  TEST_CUSTOM_PROGS = $(OUTPUT)/urandom_read
> +EXTRA_CLEAN += $(TEST_CUSTOM_PROGS)

why += instead of := here?

>
>  # Emit succinct information message describing current building step
>  # $1 - generic step name (e.g., CC, LINK, etc);
> @@ -267,7 +268,7 @@ TRUNNER_TEST_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.test.o,   \
>  TRUNNER_EXTRA_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o,          \
>                                  $$(filter %.c,$(TRUNNER_EXTRA_SOURCES)))
>  TRUNNER_EXTRA_HDRS := $$(filter %.h,$(TRUNNER_EXTRA_SOURCES))
> -TRUNNER_TESTS_HDR := $(TRUNNER_TESTS_DIR)/tests.h
> +TRUNNER_TESTS_HDR := $(OUTPUT)/$(TRUNNER_TESTS_DIR)/tests.h
>  TRUNNER_BPF_SRCS := $$(notdir $$(wildcard $(TRUNNER_BPF_PROGS_DIR)/*.c))
>  TRUNNER_BPF_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o, $$(TRUNNER_BPF_SRCS))
>  TRUNNER_BPF_SKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.skel.h,      \
> @@ -295,6 +296,11 @@ $(TRUNNER_OUTPUT)-dir := y
>  $(TRUNNER_OUTPUT):
>         $$(call msg,MKDIR,,$$@)
>         mkdir -p $$@
> +
> +ifneq ($2,)
> +EXTRA_CLEAN +=$(TRUNNER_OUTPUT)
> +endif
> +
>  endif
>
>  # ensure we set up BPF objects generation rule just once for a given
> @@ -320,13 +326,19 @@ endif
>  # ensure we set up tests.h header generation rule just once
>  ifeq ($($(TRUNNER_TESTS_DIR)-tests-hdr),)
>  $(TRUNNER_TESTS_DIR)-tests-hdr := y
> -$(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/*.c
> +$(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/*.c | $(dir $(TRUNNER_TESTS_HDR))
>         $$(call msg,TEST-HDR,$(TRUNNER_BINARY),$$@)
>         $$(shell ( cd $(TRUNNER_TESTS_DIR);                             \
>                   echo '/* Generated header, do not edit */';           \
>                   ls *.c 2> /dev/null |                                 \
>                         sed -e 's@\([^\.]*\)\.c@DEFINE_TEST(\1)@';      \
>                  ) > $$@)
> +
> +EXTRA_CLEAN += $(TRUNNER_TESTS_HDR)
> +
> +$(dir $(TRUNNER_TESTS_HDR)):
> +       $$(call msg,MKDIR,,$$@)
> +       mkdir -p $$@
>  endif
>
>  # compile individual test files
> @@ -402,14 +414,23 @@ $(eval $(call DEFINE_TEST_RUNNER,test_maps))
>  # It is much simpler than test_maps/test_progs and sufficiently different from
>  # them (e.g., test.h is using completely pattern), that it's worth just
>  # explicitly defining all the rules explicitly.
> -verifier/tests.h: verifier/*.c
> +$(OUTPUT)/verifier/tests.h: verifier/*.c | $(OUTPUT)/verifier
>         $(shell ( cd verifier/; \
>                   echo '/* Generated header, do not edit */'; \
>                   echo '#ifdef FILL_ARRAY'; \
>                   ls *.c 2> /dev/null | sed -e 's@\(.*\)@#include \"\1\"@'; \
>                   echo '#endif' \
> -               ) > verifier/tests.h)
> -$(OUTPUT)/test_verifier: test_verifier.c verifier/tests.h $(BPFOBJ) | $(OUTPUT)
> +               ) > $@)
> +
> +EXTRA_CLEAN += $(OUTPUT)/verifier/tests.h
> +
> +$(OUTPUT)/verifier:
> +       $(call msg,MKDIR,,$@)
> +       mkdir -p $@

See below, given this directory is well-known and sort of static, can
you just add them to the list of pre-created directories at line 176?

> +
> +$(OUTPUT)/test_verifier: CFLAGS += -I$(abspath verifier)
> +$(OUTPUT)/test_verifier: test_verifier.c $(OUTPUT)/verifier/tests.h $(BPFOBJ) \
> +                       | $(OUTPUT)
>         $(call msg,BINARY,,$@)
>         $(CC) $(CFLAGS) $(filter %.a %.o %.c,$^) $(LDLIBS) -o $@
>
> @@ -433,7 +454,6 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o $(OUTPUT)/testing_helpers.o \
>         $(call msg,BINARY,,$@)
>         $(CC) $(LDFLAGS) -o $@ $(filter %.a %.o,$^) $(LDLIBS)
>
> -EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR)                     \
> -       prog_tests/tests.h map_tests/tests.h verifier/tests.h           \

Why not just append $(OUTPUT) to these three and keep TRUNNER rules
just a bit simpler, they don't need any extra complexity.

> -       feature                                                         \
> -       $(addprefix $(OUTPUT)/,*.o *.skel.h no_alu32 bpf_gcc)

same for no_alu32 and bpf_gcc, just append $(OUTPUT)/ to them?

> +EXTRA_CLEAN += $(SCRATCH_DIR)                  \
> +       feature                                 \
> +       $(addprefix $(OUTPUT)/,*.o *.skel.h)
> --
> 2.26.2
>

Re: [PATCH 8/8] selftests/bpf: factor out MKDIR rule

[Andrii Nakryiko]

On Thu, May 21, 2020 at 9:14 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Do not repeat youself, move common mkdir code (message and action)
> to a variable.
>
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---
>  tools/testing/selftests/bpf/Makefile | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index bade24e29a1a..26497d8869ea 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -252,6 +252,11 @@ define COMPILE_C_RULE
>         $(CC) $(CFLAGS) -c $(filter %.c,$^) $(LDLIBS) -o $@
>  endef
>
> +define MKDIR_RULE
> +       $(call msg,MKDIR,,$@)
> +       mkdir -p $@
> +endef

I don't think you save much with this, especially by combining dir
creation rules together. Let's not do this, just adds extra level of
"rule nestedness", if I may say so...

> +
>  SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
>
>  # Set up extra TRUNNER_XXX "temporary" variables in the environment (relies on
> @@ -294,8 +299,7 @@ define DEFINE_TEST_RUNNER_RULES
>  ifeq ($($(TRUNNER_OUTPUT)-dir),)
>  $(TRUNNER_OUTPUT)-dir := y
>  $(TRUNNER_OUTPUT):
> -       $$(call msg,MKDIR,,$$@)
> -       mkdir -p $$@
> +       $$(MKDIR_RULE)
>
>  ifneq ($2,)
>  EXTRA_CLEAN +=$(TRUNNER_OUTPUT)
> @@ -337,8 +341,7 @@ $(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/*.c | $(dir $(TRUNNER_TESTS_HDR))
>  EXTRA_CLEAN += $(TRUNNER_TESTS_HDR)
>
>  $(dir $(TRUNNER_TESTS_HDR)):

combine this rule with $(TRUNNER_OUTPUT) above?

> -       $$(call msg,MKDIR,,$$@)
> -       mkdir -p $$@
> +       $$(MKDIR_RULE)
>  endif
>
>  # compile individual test files
> @@ -425,8 +428,7 @@ $(OUTPUT)/verifier/tests.h: verifier/*.c | $(OUTPUT)/verifier
>  EXTRA_CLEAN += $(OUTPUT)/verifier/tests.h
>
>  $(OUTPUT)/verifier:
> -       $(call msg,MKDIR,,$@)
> -       mkdir -p $@
> +       $(MKDIR_RULE)

This should go together with libbpf, bpftool and $(INCLUDE_DIR) rule
at line 176.

>
>  $(OUTPUT)/test_verifier: CFLAGS += -I$(abspath verifier)
>  $(OUTPUT)/test_verifier: test_verifier.c $(OUTPUT)/verifier/tests.h $(BPFOBJ) \
> --
> 2.26.2
>

Re: [PATCH 4/8] selftests/bpf: fix object files installation

[Andrii Nakryiko]

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
>

Re: [PATCH 0/8] selftests/bpf: installation and out of tree build fixes

[Andrii Nakryiko]

On Thu, May 21, 2020 at 11:41 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
>
> Actually, a bit more needed :)

From the other kselftest thread, it seems like selftests are not
supporting builds out-of-tree. With that, wouldn't it be simpler to
build in tree and then just copy selftests/bpf directory to wherever
you need to run tests from? It would be simple and reliable. Given I
and probably everyone else never build and run tests out-of-tree, it's
just too easy to break this and you'll be constantly chasing some
non-obvious breakages...

Is there some problem with such approach?

>
> >>>>> On Fri, 22 May 2020 07:13:02 +0300, Yauheni Kaliuta  wrote:
>
>  > I had a look, here are some fixes.
>  > Yauheni Kaliuta (8):
>  >   selftests/bpf: remove test_align from Makefile
>  >   selftests/bpf: build bench.o for any $(OUTPUT)
>  >   selftests/bpf: install btf .c files
>  >   selftests/bpf: fix object files installation
>  >   selftests/bpf: add output dir to include list
>  >   selftests/bpf: fix urandom_read installation
>  >   selftests/bpf: fix test.h placing for out of tree build
>  >   selftests/bpf: factor out MKDIR rule
>
>  >  tools/testing/selftests/bpf/Makefile | 77 ++++++++++++++++++++--------
>  >  1 file changed, 55 insertions(+), 22 deletions(-)
>
>  > --
>  > 2.26.2
>
>
> --
> WBR,
> Yauheni Kaliuta
>

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Andrii Nakryiko]

On Fri, May 22, 2020 at 1:19 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> There is difference in depoying static and generated extra resource
> files between in/out of tree build and flavors:
>
> - in case of unflavored out-of-tree build static files are not
> available and must be copied as well as both static and generated
> files for flavored build.
>
> So split the rules and variables. The name TRUNNER_EXTRA_GEN_FILES
> is chosen in analogy to TEST_GEN_* variants.
>

Can we keep them together but be smarter about what needs to be copied
based on source/target directories? I would really like to not blow up
all these rules.

> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---
>  tools/testing/selftests/bpf/Makefile | 26 ++++++++++++++++++++++----
>  1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 26497d8869ea..c80c06272759 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -363,12 +363,28 @@ $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o:                             \
>         $$(call msg,EXT-OBJ,$(TRUNNER_BINARY),$$@)
>         $$(CC) $$(CFLAGS) -c $$< $$(LDLIBS) -o $$@
>
> -# only copy extra resources if in flavored build
> -$(TRUNNER_BINARY)-extras: $(TRUNNER_EXTRA_FILES) | $(TRUNNER_OUTPUT)
> -ifneq ($2,)
> +# copy extra resources when needed.
> +# Static files for both out of tree and flavored (so, not current dir).
> +# Generated files for flavored only.
> +$(TRUNNER_BINARY)-extras: $(TRUNNER_BINARY)-extras-static \
> +                         $(TRUNNER_BINARY)-extras-gen
> +
> +$(TRUNNER_BINARY)-extras-static: $(TRUNNER_EXTRA_FILES) | $(TRUNNER_OUTPUT)
> +ifneq ($(CURDIR)),$(realpath $(TRUNNER_OUTPUT)))
>         $$(call msg,EXT-COPY,$(TRUNNER_BINARY),$(TRUNNER_EXTRA_FILES))
> +ifneq ($(TRUNNER_EXTRA_FILES),)
>         cp -a $$^ $(TRUNNER_OUTPUT)/
>  endif
> +endif
> +
> +$(TRUNNER_BINARY)-extras-gen: $(addprefix $(OUTPUT)/,$(TRUNNER_EXTRA_GEN_FILES)) \
> +                           | $(TRUNNER_OUTPUT)
> +ifneq ($2,)
> +       $$(call msg,EXT-COPY,$(TRUNNER_BINARY),$(TRUNNER_EXTRA_GEN_FILES))
> +ifneq ($(TRUNNER_EXTRA_GEN_FILES),)
> +       cp -a $$^ $(TRUNNER_OUTPUT)/
> +endif
> +endif
>
>  $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)                      \
>                              $(TRUNNER_EXTRA_OBJS) $$(BPFOBJ)           \
> @@ -384,7 +400,8 @@ TRUNNER_BPF_PROGS_DIR := progs
>  TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c \
>                          network_helpers.c testing_helpers.c            \
>                          flow_dissector_load.h
> -TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(BTF_C_FILES)
> +TRUNNER_EXTRA_FILES := $(BTF_C_FILES)
> +TRUNNER_EXTRA_GEN_FILES := urandom_read
>  TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
>  TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS)
>  TRUNNER_BPF_LDFLAGS := -mattr=+alu32
> @@ -408,6 +425,7 @@ TRUNNER_TESTS_DIR := map_tests
>  TRUNNER_BPF_PROGS_DIR := progs
>  TRUNNER_EXTRA_SOURCES := test_maps.c
>  TRUNNER_EXTRA_FILES :=
> +TRUNNER_EXTRA_GEN_FILES :=
>  TRUNNER_BPF_BUILD_RULE := $$(error no BPF objects should be built)
>  TRUNNER_BPF_CFLAGS :=
>  TRUNNER_BPF_LDFLAGS :=
> --
> 2.26.2
>

Re: [PATCH 0/8] selftests/bpf: installation and out of tree build fixes

[Yauheni Kaliuta]

Hi, Daniel!

>>>>> On Tue, 26 May 2020 23:48:01 +0200, Daniel Borkmann  wrote:

 > On 5/22/20 8:40 AM, Yauheni Kaliuta wrote:
 >> 
 >> Actually, a bit more needed :)

 > Not quite sure how to parse this, I presume you are intending
 > to send a v2 of this series with [0] folded in? Please also do
 > not add line-breaks in the middle of all your Fixes tags as
 > otherwise it would break searching for commits in the git
 > log. For the v2 respin, please also add a better cover letter
 > than just saying nothing more than 'I had a look, here are
 > some fixes.'. At least a minimal high level summary of the
 > selftest Makefile changes in this series.

Thanks! That was part of thread with Andrii, but I should have
sent separated. Anyway (see Andrii's comments) it's not coming as
is, so I'll do v2. Sorry for that. 

 > Thanks,
 > Daniel

 >   [0]
 > https://patchwork.ozlabs.org/project/netdev/patch/20200522081901.238516-1-yauheni.kaliuta@redhat.com/

 >>>>>>> On Fri, 22 May 2020 07:13:02 +0300, Yauheni Kaliuta  wrote:
 >> 
 >> > I had a look, here are some fixes.
 >> > Yauheni Kaliuta (8):
 >> >   selftests/bpf: remove test_align from Makefile
 >> >   selftests/bpf: build bench.o for any $(OUTPUT)
 >> >   selftests/bpf: install btf .c files
 >> >   selftests/bpf: fix object files installation
 >> >   selftests/bpf: add output dir to include list
 >> >   selftests/bpf: fix urandom_read installation
 >> >   selftests/bpf: fix test.h placing for out of tree build
 >> >   selftests/bpf: factor out MKDIR rule
 >> 
 >> >  tools/testing/selftests/bpf/Makefile | 77 ++++++++++++++++++++--------
 >> >  1 file changed, 55 insertions(+), 22 deletions(-)
 >> 
 >> > --
 >> > 2.26.2
 >> 
 >> 


-- 
WBR,
Yauheni Kaliuta

Re: [PATCH 0/8] selftests/bpf: installation and out of tree build fixes

[Yauheni Kaliuta]

Hi, Andrii!

>>>>> On Tue, 26 May 2020 15:32:10 -0700, Andrii Nakryiko  wrote:

 > On Thu, May 21, 2020 at 11:41 PM Yauheni Kaliuta
 > <yauheni.kaliuta@redhat.com> wrote:
 >> 
 >> 
 >> Actually, a bit more needed :)

 > From the other kselftest thread, it seems like selftests are not
 > supporting builds out-of-tree. With that, wouldn't it be simpler to
 > build in tree and then just copy selftests/bpf directory to wherever
 > you need to run tests from? It would be simple and reliable. Given I
 > and probably everyone else never build and run tests out-of-tree, it's
 > just too easy to break this and you'll be constantly chasing some
 > non-obvious breakages...

 > Is there some problem with such approach?

This is `make install` ;).

I personally do not need OOT build, but since it's in the code,
I'd prefer either fix it or remove it, otherwise it's
misleading. But I have not got reply from kselftest.

 >> 
 >> >>>>> On Fri, 22 May 2020 07:13:02 +0300, Yauheni Kaliuta  wrote:
 >> 
 >> > I had a look, here are some fixes.
 >> > Yauheni Kaliuta (8):
 >> >   selftests/bpf: remove test_align from Makefile
 >> >   selftests/bpf: build bench.o for any $(OUTPUT)
 >> >   selftests/bpf: install btf .c files
 >> >   selftests/bpf: fix object files installation
 >> >   selftests/bpf: add output dir to include list
 >> >   selftests/bpf: fix urandom_read installation
 >> >   selftests/bpf: fix test.h placing for out of tree build
 >> >   selftests/bpf: factor out MKDIR rule
 >> 
 >> >  tools/testing/selftests/bpf/Makefile | 77 ++++++++++++++++++++--------
 >> >  1 file changed, 55 insertions(+), 22 deletions(-)
 >> 
 >> > --
 >> > 2.26.2
 >> 
 >> 
 >> --
 >> WBR,
 >> Yauheni Kaliuta
 >> 


-- 
WBR,
Yauheni Kaliuta

Re: [PATCH 2/8] selftests/bpf: build bench.o for any $(OUTPUT)

[Yauheni Kaliuta]

Hi, Andrii!

>>>>> On Tue, 26 May 2020 15:13:36 -0700, Andrii Nakryiko  wrote:

 > On Thu, May 21, 2020 at 9:14 PM Yauheni Kaliuta
 > <yauheni.kaliuta@redhat.com> wrote:
 >> 
 >> bench.o is produced by implicit rule only if it's built in the same
 >> directory where bench.c is located. If OUTPUT points somewhere else,
 >> build fails.
 >> 
 >> Make an explicit rule for it (factor out common part).
 >> Add bench.c as a dependency to make it source for CC.

 > If that's the case, then the similar problem would happen to
 > test_l4lb_noinline.o, test_xdp_noinline.o, and
 > flow_dissector_load.o, at least. Let's fix the implicit rule
 > (or define our own, but generic one), instead of ad-hoc fixing
 > it for bench.o only.

I'll check why they did not cause problems.


 >> 
 >> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
 >> ---
 >> tools/testing/selftests/bpf/Makefile | 11 ++++++++---
 >> 1 file changed, 8 insertions(+), 3 deletions(-)
 >> 
 >> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
 >> index 09700db35c2d..f0b7d41ed6dd 100644
 >> --- a/tools/testing/selftests/bpf/Makefile
 >> +++ b/tools/testing/selftests/bpf/Makefile
 >> @@ -243,6 +243,11 @@ define GCC_BPF_BUILD_RULE
 >> $(BPF_GCC) $3 $4 -O2 -c $1 -o $2
 >> endef
 >> 
 >> +define COMPILE_C_RULE
 >> +       $(call msg,CC,,$@)
 >> +       $(CC) $(CFLAGS) -c $(filter %.c,$^) $(LDLIBS) -o $@
 >> +endef
 >> +
 >> SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
 >> 
 >> # Set up extra TRUNNER_XXX "temporary" variables in the environment (relies on
 >> @@ -409,11 +414,11 @@ $(OUTPUT)/test_cpp: test_cpp.cpp $(OUTPUT)/test_core_extern.skel.h $(BPFOBJ)
 >> 
 >> # Benchmark runner
 >> $(OUTPUT)/bench_%.o: benchs/bench_%.c bench.h
 >> -       $(call msg,CC,,$@)
 >> -       $(CC) $(CFLAGS) -c $(filter %.c,$^) $(LDLIBS) -o $@
 >> +       $(COMPILE_C_RULE)
 >> $(OUTPUT)/bench_rename.o: $(OUTPUT)/test_overhead.skel.h
 >> $(OUTPUT)/bench_trigger.o: $(OUTPUT)/trigger_bench.skel.h
 >> -$(OUTPUT)/bench.o: bench.h testing_helpers.h
 >> +$(OUTPUT)/bench.o: bench.c bench.h testing_helpers.h
 >> +       $(COMPILE_C_RULE)
 >> $(OUTPUT)/bench: LDLIBS += -lm
 >> $(OUTPUT)/bench: $(OUTPUT)/bench.o $(OUTPUT)/testing_helpers.o \
 >> $(OUTPUT)/bench_count.o \
 >> --
 >> 2.26.2
 >> 


-- 
WBR,
Yauheni Kaliuta

Re: [PATCH 0/8] selftests/bpf: installation and out of tree build fixes

[Andrii Nakryiko]

On Tue, May 26, 2020 at 9:52 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi, Andrii!
>
> >>>>> On Tue, 26 May 2020 15:32:10 -0700, Andrii Nakryiko  wrote:
>
>  > On Thu, May 21, 2020 at 11:41 PM Yauheni Kaliuta
>  > <yauheni.kaliuta@redhat.com> wrote:
>  >>
>  >>
>  >> Actually, a bit more needed :)
>
>  > From the other kselftest thread, it seems like selftests are not
>  > supporting builds out-of-tree. With that, wouldn't it be simpler to
>  > build in tree and then just copy selftests/bpf directory to wherever
>  > you need to run tests from? It would be simple and reliable. Given I
>  > and probably everyone else never build and run tests out-of-tree, it's
>  > just too easy to break this and you'll be constantly chasing some
>  > non-obvious breakages...
>
>  > Is there some problem with such approach?
>
> This is `make install` ;).

So patch #2, #5, and #7 is solving just `make install` problem?..

My point is that by building in tree and then just copying everything
under selftests/bpf directory to wherever you want to "install" it
would just work. And won't require complicating already complicated
Makefile. Any problem with such approach?


>
> I personally do not need OOT build, but since it's in the code,
> I'd prefer either fix it or remove it, otherwise it's
> misleading. But I have not got reply from kselftest.
>
>  >>
>  >> >>>>> On Fri, 22 May 2020 07:13:02 +0300, Yauheni Kaliuta  wrote:
>  >>
>  >> > I had a look, here are some fixes.
>  >> > Yauheni Kaliuta (8):
>  >> >   selftests/bpf: remove test_align from Makefile
>  >> >   selftests/bpf: build bench.o for any $(OUTPUT)
>  >> >   selftests/bpf: install btf .c files
>  >> >   selftests/bpf: fix object files installation
>  >> >   selftests/bpf: add output dir to include list
>  >> >   selftests/bpf: fix urandom_read installation
>  >> >   selftests/bpf: fix test.h placing for out of tree build
>  >> >   selftests/bpf: factor out MKDIR rule
>  >>
>  >> >  tools/testing/selftests/bpf/Makefile | 77 ++++++++++++++++++++--------
>  >> >  1 file changed, 55 insertions(+), 22 deletions(-)
>  >>
>  >> > --
>  >> > 2.26.2
>  >>
>  >>
>  >> --
>  >> WBR,
>  >> Yauheni Kaliuta
>  >>
>
>
> --
> WBR,
> Yauheni Kaliuta
>

Re: [PATCH 7/8] selftests/bpf: fix test.h placing for out of tree build

[Yauheni Kaliuta]

Hi, Andrii!

>>>>> On Tue, 26 May 2020 15:26:43 -0700, Andrii Nakryiko  wrote:

 > On Thu, May 21, 2020 at 9:14 PM Yauheni Kaliuta
 > <yauheni.kaliuta@redhat.com> wrote:
 >> 
 >> Flavors of test.h are generated in tree even for out of tree
 >> build. Use OUTPUT directory for that.
 >> 
 >> It requires rules to make sure the directories exist.
 >> 
 >> Split EXTRA_CLEAN generation since existance of test.h files depends
 >> of dynamic makefile generation.
 >> 
 >> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
 >> ---
 >> tools/testing/selftests/bpf/Makefile | 38 +++++++++++++++++++++-------
 >> 1 file changed, 29 insertions(+), 9 deletions(-)
 >> 
 >> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
 >> index 31598ca2d396..bade24e29a1a 100644
 >> --- a/tools/testing/selftests/bpf/Makefile
 >> +++ b/tools/testing/selftests/bpf/Makefile
 >> @@ -83,6 +83,7 @@ TEST_GEN_PROGS_EXTENDED = test_sock_addr test_skb_cgroup_id_user \
 >> test_lirc_mode2_user xdping test_cpp runqslower bench
 >> 
 >> TEST_CUSTOM_PROGS = $(OUTPUT)/urandom_read
 >> +EXTRA_CLEAN += $(TEST_CUSTOM_PROGS)

 > why += instead of := here?

Well, in this particular case there is no difference, but in
general it looks better for me

1) unified +=, no need to track which is first;
2) for first time it makes the variable deffered evaluated which
sound more appropriate (if TEST_CUSTOM_PROGS was constructed);

But if you insist, I'll change it.

 >> 
 >> # Emit succinct information message describing current building step
 >> # $1 - generic step name (e.g., CC, LINK, etc);
 >> @@ -267,7 +268,7 @@ TRUNNER_TEST_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.test.o,   \
 >> TRUNNER_EXTRA_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o,          \
 >> $$(filter %.c,$(TRUNNER_EXTRA_SOURCES)))
 >> TRUNNER_EXTRA_HDRS := $$(filter %.h,$(TRUNNER_EXTRA_SOURCES))
 >> -TRUNNER_TESTS_HDR := $(TRUNNER_TESTS_DIR)/tests.h
 >> +TRUNNER_TESTS_HDR := $(OUTPUT)/$(TRUNNER_TESTS_DIR)/tests.h
 >> TRUNNER_BPF_SRCS := $$(notdir $$(wildcard $(TRUNNER_BPF_PROGS_DIR)/*.c))
 >> TRUNNER_BPF_OBJS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.o, $$(TRUNNER_BPF_SRCS))
 >> TRUNNER_BPF_SKELS := $$(patsubst %.c,$$(TRUNNER_OUTPUT)/%.skel.h,      \
 >> @@ -295,6 +296,11 @@ $(TRUNNER_OUTPUT)-dir := y
 >> $(TRUNNER_OUTPUT):
 >> $$(call msg,MKDIR,,$$@)
 >> mkdir -p $$@
 >> +
 >> +ifneq ($2,)
 >> +EXTRA_CLEAN +=$(TRUNNER_OUTPUT)
 >> +endif
 >> +
 >> endif
 >> 
 >> # ensure we set up BPF objects generation rule just once for a given
 >> @@ -320,13 +326,19 @@ endif
 >> # ensure we set up tests.h header generation rule just once
 >> ifeq ($($(TRUNNER_TESTS_DIR)-tests-hdr),)
 >> $(TRUNNER_TESTS_DIR)-tests-hdr := y
 >> -$(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/*.c
 >> +$(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/*.c | $(dir $(TRUNNER_TESTS_HDR))
 >> $$(call msg,TEST-HDR,$(TRUNNER_BINARY),$$@)
 >> $$(shell ( cd $(TRUNNER_TESTS_DIR);                             \
 >> echo '/* Generated header, do not edit */';           \
 >> ls *.c 2> /dev/null |                                 \
 >> sed -e 's@\([^\.]*\)\.c@DEFINE_TEST(\1)@';      \
 >> ) > $$@)
 >> +
 >> +EXTRA_CLEAN += $(TRUNNER_TESTS_HDR)
 >> +
 >> +$(dir $(TRUNNER_TESTS_HDR)):
 >> +       $$(call msg,MKDIR,,$$@)
 >> +       mkdir -p $$@
 >> endif
 >> 
 >> # compile individual test files
 >> @@ -402,14 +414,23 @@ $(eval $(call DEFINE_TEST_RUNNER,test_maps))
 >> # It is much simpler than test_maps/test_progs and sufficiently different from
 >> # them (e.g., test.h is using completely pattern), that it's worth just
 >> # explicitly defining all the rules explicitly.
 >> -verifier/tests.h: verifier/*.c
 >> +$(OUTPUT)/verifier/tests.h: verifier/*.c | $(OUTPUT)/verifier
 >> $(shell ( cd verifier/; \
 >> echo '/* Generated header, do not edit */'; \
 >> echo '#ifdef FILL_ARRAY'; \
 >> ls *.c 2> /dev/null | sed -e 's@\(.*\)@#include \"\1\"@'; \
 >> echo '#endif' \
 >> -               ) > verifier/tests.h)
 >> -$(OUTPUT)/test_verifier: test_verifier.c verifier/tests.h $(BPFOBJ) | $(OUTPUT)
 >> +               ) > $@)
 >> +
 >> +EXTRA_CLEAN += $(OUTPUT)/verifier/tests.h
 >> +
 >> +$(OUTPUT)/verifier:
 >> +       $(call msg,MKDIR,,$@)
 >> +       mkdir -p $@

 > See below, given this directory is well-known and sort of
 > static, can you just add them to the list of pre-created
 > directories at line 176?

Agree.

 >> +
 >> +$(OUTPUT)/test_verifier: CFLAGS += -I$(abspath verifier)
 >> +$(OUTPUT)/test_verifier: test_verifier.c $(OUTPUT)/verifier/tests.h $(BPFOBJ) \
 >> +                       | $(OUTPUT)
 >> $(call msg,BINARY,,$@)
 >> $(CC) $(CFLAGS) $(filter %.a %.o %.c,$^) $(LDLIBS) -o $@
 >> 
 >> @@ -433,7 +454,6 @@ $(OUTPUT)/bench: $(OUTPUT)/bench.o
 >> $(OUTPUT)/testing_helpers.o \
 >> $(call msg,BINARY,,$@)
 >> $(CC) $(LDFLAGS) -o $@ $(filter %.a %.o,$^) $(LDLIBS)
 >> 
 >> -EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR)                     \
 >> -       prog_tests/tests.h map_tests/tests.h verifier/tests.h           \

 > Why not just append $(OUTPUT) to these three and keep TRUNNER
 > rules just a bit simpler, they don't need any extra
 > complexity.

 >> -       feature                                                         \
 >> -       $(addprefix $(OUTPUT)/,*.o *.skel.h no_alu32 bpf_gcc)

 > same for no_alu32 and bpf_gcc, just append $(OUTPUT)/ to them?

Well, it's possible, but for me it looks a bit wrong. It sort of
creates 2 points of sync -- the calls to dynamic rule creation
and here (skip/add dynamic call -- change clean rule), and having
it in the place were all the code handling flavors located sounds
a bit more correct for me.

But since there are not a lot of them if you find it nicer, I'll
do that.

 >> +EXTRA_CLEAN += $(SCRATCH_DIR)                  \
 >> +       feature                                 \
 >> +       $(addprefix $(OUTPUT)/,*.o *.skel.h)
 >> --
 >> 2.26.2
 >> 


-- 
WBR,
Yauheni Kaliuta

Re: [PATCH 8/8] selftests/bpf: factor out MKDIR rule

[Yauheni Kaliuta]

Hi, Andrii!

>>>>> On Tue, 26 May 2020 15:29:04 -0700, Andrii Nakryiko  wrote:

 > On Thu, May 21, 2020 at 9:14 PM Yauheni Kaliuta
 > <yauheni.kaliuta@redhat.com> wrote:
 >> 
 >> Do not repeat youself, move common mkdir code (message and action)
 >> to a variable.
 >> 
 >> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
 >> ---
 >> tools/testing/selftests/bpf/Makefile | 14 ++++++++------
 >> 1 file changed, 8 insertions(+), 6 deletions(-)
 >> 
 >> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
 >> index bade24e29a1a..26497d8869ea 100644
 >> --- a/tools/testing/selftests/bpf/Makefile
 >> +++ b/tools/testing/selftests/bpf/Makefile
 >> @@ -252,6 +252,11 @@ define COMPILE_C_RULE
 >> $(CC) $(CFLAGS) -c $(filter %.c,$^) $(LDLIBS) -o $@
 >> endef
 >> 
 >> +define MKDIR_RULE
 >> +       $(call msg,MKDIR,,$@)
 >> +       mkdir -p $@
 >> +endef

 > I don't think you save much with this, especially by combining
 > dir creation rules together. Let's not do this, just adds
 > extra level of "rule nestedness", if I may say so...

Ok

 >> +
 >> SKEL_BLACKLIST := btf__% test_pinning_invalid.c test_sk_assign.c
 >> 
 >> # Set up extra TRUNNER_XXX "temporary" variables in the environment (relies on
 >> @@ -294,8 +299,7 @@ define DEFINE_TEST_RUNNER_RULES
 >> ifeq ($($(TRUNNER_OUTPUT)-dir),)
 >> $(TRUNNER_OUTPUT)-dir := y
 >> $(TRUNNER_OUTPUT):
 >> -       $$(call msg,MKDIR,,$$@)
 >> -       mkdir -p $$@
 >> +       $$(MKDIR_RULE)
 >> 
 >> ifneq ($2,)
 >> EXTRA_CLEAN +=$(TRUNNER_OUTPUT)
 >> @@ -337,8 +341,7 @@ $(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/*.c |
 >> $(dir $(TRUNNER_TESTS_HDR))
 >> EXTRA_CLEAN += $(TRUNNER_TESTS_HDR)
 >> 
 >> $(dir $(TRUNNER_TESTS_HDR)):

 > combine this rule with $(TRUNNER_OUTPUT) above?

 >> -       $$(call msg,MKDIR,,$$@)
 >> -       mkdir -p $$@
 >> +       $$(MKDIR_RULE)
 >> endif
 >> 
 >> # compile individual test files
 >> @@ -425,8 +428,7 @@ $(OUTPUT)/verifier/tests.h: verifier/*.c | $(OUTPUT)/verifier
 >> EXTRA_CLEAN += $(OUTPUT)/verifier/tests.h
 >> 
 >> $(OUTPUT)/verifier:
 >> -       $(call msg,MKDIR,,$@)
 >> -       mkdir -p $@
 >> +       $(MKDIR_RULE)

 > This should go together with libbpf, bpftool and $(INCLUDE_DIR) rule
 > at line 176.

 >> 
 >> $(OUTPUT)/test_verifier: CFLAGS += -I$(abspath verifier)
 >> $(OUTPUT)/test_verifier: test_verifier.c $(OUTPUT)/verifier/tests.h $(BPFOBJ) \
 >> --
 >> 2.26.2
 >> 


-- 
WBR,
Yauheni Kaliuta

Re: [PATCH 4/8] selftests/bpf: fix object files installation

[Yauheni Kaliuta]

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


-- 
WBR,
Yauheni Kaliuta

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Yauheni Kaliuta]

Hi, Andrii!

>>>>> On Tue, 26 May 2020 17:19:18 -0700, Andrii Nakryiko  wrote:

 > On Fri, May 22, 2020 at 1:19 AM Yauheni Kaliuta
 > <yauheni.kaliuta@redhat.com> wrote:
 >> 
 >> There is difference in depoying static and generated extra resource
 >> files between in/out of tree build and flavors:
 >> 
 >> - in case of unflavored out-of-tree build static files are not
 >> available and must be copied as well as both static and generated
 >> files for flavored build.
 >> 
 >> So split the rules and variables. The name TRUNNER_EXTRA_GEN_FILES
 >> is chosen in analogy to TEST_GEN_* variants.
 >> 

 > Can we keep them together but be smarter about what needs to
 > be copied based on source/target directories? I would really
 > like to not blow up all these rules.

I can try, ok, I just find it a bit more clear. But it's good to
get some input from kselftest about OOT build in general.

 >> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
 >> ---
 >> tools/testing/selftests/bpf/Makefile | 26 ++++++++++++++++++++++----
 >> 1 file changed, 22 insertions(+), 4 deletions(-)
 >> 
 >> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
 >> index 26497d8869ea..c80c06272759 100644
 >> --- a/tools/testing/selftests/bpf/Makefile
 >> +++ b/tools/testing/selftests/bpf/Makefile
 >> @@ -363,12 +363,28 @@ $(TRUNNER_EXTRA_OBJS): $(TRUNNER_OUTPUT)/%.o:                             \
 >> $$(call msg,EXT-OBJ,$(TRUNNER_BINARY),$$@)
 >> $$(CC) $$(CFLAGS) -c $$< $$(LDLIBS) -o $$@
 >> 
 >> -# only copy extra resources if in flavored build
 >> -$(TRUNNER_BINARY)-extras: $(TRUNNER_EXTRA_FILES) | $(TRUNNER_OUTPUT)
 >> -ifneq ($2,)
 >> +# copy extra resources when needed.
 >> +# Static files for both out of tree and flavored (so, not current dir).
 >> +# Generated files for flavored only.
 >> +$(TRUNNER_BINARY)-extras: $(TRUNNER_BINARY)-extras-static \
 >> +                         $(TRUNNER_BINARY)-extras-gen
 >> +
 >> +$(TRUNNER_BINARY)-extras-static: $(TRUNNER_EXTRA_FILES) | $(TRUNNER_OUTPUT)
 >> +ifneq ($(CURDIR)),$(realpath $(TRUNNER_OUTPUT)))
 >> $$(call msg,EXT-COPY,$(TRUNNER_BINARY),$(TRUNNER_EXTRA_FILES))
 >> +ifneq ($(TRUNNER_EXTRA_FILES),)
 >> cp -a $$^ $(TRUNNER_OUTPUT)/
 >> endif
 >> +endif
 >> +
 >> +$(TRUNNER_BINARY)-extras-gen: $(addprefix $(OUTPUT)/,$(TRUNNER_EXTRA_GEN_FILES)) \
 >> +                           | $(TRUNNER_OUTPUT)
 >> +ifneq ($2,)
 >> +       $$(call msg,EXT-COPY,$(TRUNNER_BINARY),$(TRUNNER_EXTRA_GEN_FILES))
 >> +ifneq ($(TRUNNER_EXTRA_GEN_FILES),)
 >> +       cp -a $$^ $(TRUNNER_OUTPUT)/
 >> +endif
 >> +endif
 >> 
 >> $(OUTPUT)/$(TRUNNER_BINARY): $(TRUNNER_TEST_OBJS)                      \
 >> $(TRUNNER_EXTRA_OBJS) $$(BPFOBJ)           \
 >> @@ -384,7 +400,8 @@ TRUNNER_BPF_PROGS_DIR := progs
 >> TRUNNER_EXTRA_SOURCES := test_progs.c cgroup_helpers.c trace_helpers.c \
 >> network_helpers.c testing_helpers.c            \
 >> flow_dissector_load.h
 >> -TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(BTF_C_FILES)
 >> +TRUNNER_EXTRA_FILES := $(BTF_C_FILES)
 >> +TRUNNER_EXTRA_GEN_FILES := urandom_read
 >> TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
 >> TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS)
 >> TRUNNER_BPF_LDFLAGS := -mattr=+alu32
 >> @@ -408,6 +425,7 @@ TRUNNER_TESTS_DIR := map_tests
 >> TRUNNER_BPF_PROGS_DIR := progs
 >> TRUNNER_EXTRA_SOURCES := test_maps.c
 >> TRUNNER_EXTRA_FILES :=
 >> +TRUNNER_EXTRA_GEN_FILES :=
 >> TRUNNER_BPF_BUILD_RULE := $$(error no BPF objects should be built)
 >> TRUNNER_BPF_CFLAGS :=
 >> TRUNNER_BPF_LDFLAGS :=
 >> --
 >> 2.26.2
 >> 


-- 
WBR,
Yauheni Kaliuta

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Alexei Starovoitov]

On Tue, May 26, 2020 at 10:31 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi, Andrii!
>
> >>>>> On Tue, 26 May 2020 17:19:18 -0700, Andrii Nakryiko  wrote:
>
>  > On Fri, May 22, 2020 at 1:19 AM Yauheni Kaliuta
>  > <yauheni.kaliuta@redhat.com> wrote:
>  >>
>  >> There is difference in depoying static and generated extra resource
>  >> files between in/out of tree build and flavors:
>  >>
>  >> - in case of unflavored out-of-tree build static files are not
>  >> available and must be copied as well as both static and generated
>  >> files for flavored build.
>  >>
>  >> So split the rules and variables. The name TRUNNER_EXTRA_GEN_FILES
>  >> is chosen in analogy to TEST_GEN_* variants.
>  >>
>
>  > Can we keep them together but be smarter about what needs to
>  > be copied based on source/target directories? I would really
>  > like to not blow up all these rules.
>
> I can try, ok, I just find it a bit more clear. But it's good to
> get some input from kselftest about OOT build in general.

I see no value in 'make install' of selftests/bpf
and since it's broken just remove that makefile target.

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Yauheni Kaliuta]

Hi, Alexei!

>>>>> On Tue, 26 May 2020 22:37:39 -0700, Alexei Starovoitov  wrote:

 > On Tue, May 26, 2020 at 10:31 PM Yauheni Kaliuta
 > <yauheni.kaliuta@redhat.com> wrote:
 >> 
 >> Hi, Andrii!
 >> 
 >> >>>>> On Tue, 26 May 2020 17:19:18 -0700, Andrii Nakryiko  wrote:
 >> 
 >> > On Fri, May 22, 2020 at 1:19 AM Yauheni Kaliuta
 >> > <yauheni.kaliuta@redhat.com> wrote:
 >> >>
 >> >> There is difference in depoying static and generated extra resource
 >> >> files between in/out of tree build and flavors:
 >> >>
 >> >> - in case of unflavored out-of-tree build static files are not
 >> >> available and must be copied as well as both static and generated
 >> >> files for flavored build.
 >> >>
 >> >> So split the rules and variables. The name TRUNNER_EXTRA_GEN_FILES
 >> >> is chosen in analogy to TEST_GEN_* variants.
 >> >>
 >> 
 >> > Can we keep them together but be smarter about what needs to
 >> > be copied based on source/target directories? I would really
 >> > like to not blow up all these rules.
 >> 
 >> I can try, ok, I just find it a bit more clear. But it's good to
 >> get some input from kselftest about OOT build in general.

 > I see no value in 'make install' of selftests/bpf
 > and since it's broken just remove that makefile target.

Some CI systems perform testing next stage after building were
build tree is not available anymore. So it's in use at the
moment.

-- 
WBR,
Yauheni Kaliuta

Re: [PATCH 0/8] selftests/bpf: installation and out of tree build fixes

[Yauheni Kaliuta]

Hi, Andrii!

>>>>> On Tue, 26 May 2020 22:04:35 -0700, Andrii Nakryiko  wrote:

 > On Tue, May 26, 2020 at 9:52 PM Yauheni Kaliuta
 > <yauheni.kaliuta@redhat.com> wrote:
 >> 
 >> Hi, Andrii!
 >> 
 >> >>>>> On Tue, 26 May 2020 15:32:10 -0700, Andrii Nakryiko  wrote:
 >> 
 >> > On Thu, May 21, 2020 at 11:41 PM Yauheni Kaliuta
 >> > <yauheni.kaliuta@redhat.com> wrote:
 >> >>
 >> >>
 >> >> Actually, a bit more needed :)
 >> 
 >> > From the other kselftest thread, it seems like selftests are not
 >> > supporting builds out-of-tree. With that, wouldn't it be simpler to
 >> > build in tree and then just copy selftests/bpf directory to wherever
 >> > you need to run tests from? It would be simple and reliable. Given I
 >> > and probably everyone else never build and run tests out-of-tree, it's
 >> > just too easy to break this and you'll be constantly chasing some
 >> > non-obvious breakages...
 >> 
 >> > Is there some problem with such approach?
 >> 
 >> This is `make install` ;).

 > So patch #2, #5, and #7 is solving just `make install` problem?..

No, they are fixing OOT build problems. I should have probably
split the series, oot and install fixes.

 > My point is that by building in tree and then just copying
 > everything under selftests/bpf directory to wherever you want
 > to "install" it would just work. And won't require
 > complicating already complicated Makefile. Any problem with
 > such approach?

I understand. I see only wasting of space as a problem, but
should check.

 >> 
 >> I personally do not need OOT build, but since it's in the code,
 >> I'd prefer either fix it or remove it, otherwise it's
 >> misleading. But I have not got reply from kselftest.
 >> 
 >> >>
 >> >> >>>>> On Fri, 22 May 2020 07:13:02 +0300, Yauheni Kaliuta  wrote:
 >> >>
 >> >> > I had a look, here are some fixes.
 >> >> > Yauheni Kaliuta (8):
 >> >> >   selftests/bpf: remove test_align from Makefile
 >> >> >   selftests/bpf: build bench.o for any $(OUTPUT)
 >> >> >   selftests/bpf: install btf .c files
 >> >> >   selftests/bpf: fix object files installation
 >> >> >   selftests/bpf: add output dir to include list
 >> >> >   selftests/bpf: fix urandom_read installation
 >> >> >   selftests/bpf: fix test.h placing for out of tree build
 >> >> >   selftests/bpf: factor out MKDIR rule
 >> >>
 >> >> >  tools/testing/selftests/bpf/Makefile | 77 ++++++++++++++++++++--------
 >> >> >  1 file changed, 55 insertions(+), 22 deletions(-)
 >> >>
 >> >> > --
 >> >> > 2.26.2
 >> >>
 >> >>
 >> >> --
 >> >> WBR,
 >> >> Yauheni Kaliuta
 >> >>
 >> 
 >> 
 >> --
 >> WBR,
 >> Yauheni Kaliuta
 >> 


-- 
WBR,
Yauheni Kaliuta

Re: [PATCH 0/8] selftests/bpf: installation and out of tree build fixes

[Yauheni Kaliuta]

On Wed, May 27, 2020 at 10:25 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:

[...]

>  > My point is that by building in tree and then just copying
>  > everything under selftests/bpf directory to wherever you want
>  > to "install" it would just work. And won't require
>  > complicating already complicated Makefile. Any problem with
>  > such approach?
>
> I understand. I see only wasting of space as a problem, but
> should check.
>

Well, it messes up with the lib.mk functionality. There must be
explicit was for customization, like it's done with OVERRIDE_TARGETS.

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Alexei Starovoitov]

On Wed, May 27, 2020 at 12:19 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi, Alexei!
>
> >>>>> On Tue, 26 May 2020 22:37:39 -0700, Alexei Starovoitov  wrote:
>
>  > On Tue, May 26, 2020 at 10:31 PM Yauheni Kaliuta
>  > <yauheni.kaliuta@redhat.com> wrote:
>  >>
>  >> Hi, Andrii!
>  >>
>  >> >>>>> On Tue, 26 May 2020 17:19:18 -0700, Andrii Nakryiko  wrote:
>  >>
>  >> > On Fri, May 22, 2020 at 1:19 AM Yauheni Kaliuta
>  >> > <yauheni.kaliuta@redhat.com> wrote:
>  >> >>
>  >> >> There is difference in depoying static and generated extra resource
>  >> >> files between in/out of tree build and flavors:
>  >> >>
>  >> >> - in case of unflavored out-of-tree build static files are not
>  >> >> available and must be copied as well as both static and generated
>  >> >> files for flavored build.
>  >> >>
>  >> >> So split the rules and variables. The name TRUNNER_EXTRA_GEN_FILES
>  >> >> is chosen in analogy to TEST_GEN_* variants.
>  >> >>
>  >>
>  >> > Can we keep them together but be smarter about what needs to
>  >> > be copied based on source/target directories? I would really
>  >> > like to not blow up all these rules.
>  >>
>  >> I can try, ok, I just find it a bit more clear. But it's good to
>  >> get some input from kselftest about OOT build in general.
>
>  > I see no value in 'make install' of selftests/bpf
>  > and since it's broken just remove that makefile target.
>
> Some CI systems perform testing next stage after building were
> build tree is not available anymore. So it's in use at the
> moment.

such CI systems can do 'cp -r' then

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Yauheni Kaliuta]

Hi, Alexei!

>>>>> On Wed, 27 May 2020 09:48:04 -0700, Alexei Starovoitov  wrote:

 > On Wed, May 27, 2020 at 12:19 AM Yauheni Kaliuta
 > <yauheni.kaliuta@redhat.com> wrote:
 >> 
 >> Hi, Alexei!
 >> 
 >> >>>>> On Tue, 26 May 2020 22:37:39 -0700, Alexei Starovoitov  wrote:
 >> 
 >> > On Tue, May 26, 2020 at 10:31 PM Yauheni Kaliuta
 >> > <yauheni.kaliuta@redhat.com> wrote:
 >> >>
 >> >> Hi, Andrii!
 >> >>
 >> >> >>>>> On Tue, 26 May 2020 17:19:18 -0700, Andrii Nakryiko  wrote:
 >> >>
 >> >> > On Fri, May 22, 2020 at 1:19 AM Yauheni Kaliuta
 >> >> > <yauheni.kaliuta@redhat.com> wrote:
 >> >> >>
 >> >> >> There is difference in depoying static and generated extra resource
 >> >> >> files between in/out of tree build and flavors:
 >> >> >>
 >> >> >> - in case of unflavored out-of-tree build static files are not
 >> >> >> available and must be copied as well as both static and generated
 >> >> >> files for flavored build.
 >> >> >>
 >> >> >> So split the rules and variables. The name TRUNNER_EXTRA_GEN_FILES
 >> >> >> is chosen in analogy to TEST_GEN_* variants.
 >> >> >>
 >> >>
 >> >> > Can we keep them together but be smarter about what needs to
 >> >> > be copied based on source/target directories? I would really
 >> >> > like to not blow up all these rules.
 >> >>
 >> >> I can try, ok, I just find it a bit more clear. But it's good to
 >> >> get some input from kselftest about OOT build in general.
 >> 
 >> > I see no value in 'make install' of selftests/bpf
 >> > and since it's broken just remove that makefile target.
 >> 
 >> Some CI systems perform testing next stage after building were
 >> build tree is not available anymore. So it's in use at the
 >> moment.

 > such CI systems can do 'cp -r' then

It's a discussion for linux-kselftest@ (added).

At the moment `make install` is generic kselftest functionality
and since bpf is part of that infra it looks a bit strange to
break it intentionally.

-- 
WBR,
Yauheni Kaliuta

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Alexei Starovoitov]

On Wed, May 27, 2020 at 10:02 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi, Alexei!
>
> >>>>> On Wed, 27 May 2020 09:48:04 -0700, Alexei Starovoitov  wrote:
>
>  > On Wed, May 27, 2020 at 12:19 AM Yauheni Kaliuta
>  > <yauheni.kaliuta@redhat.com> wrote:
>  >>
>  >> Hi, Alexei!
>  >>
>  >> >>>>> On Tue, 26 May 2020 22:37:39 -0700, Alexei Starovoitov  wrote:
>  >>
>  >> > On Tue, May 26, 2020 at 10:31 PM Yauheni Kaliuta
>  >> > <yauheni.kaliuta@redhat.com> wrote:
>  >> >>
>  >> >> Hi, Andrii!
>  >> >>
>  >> >> >>>>> On Tue, 26 May 2020 17:19:18 -0700, Andrii Nakryiko  wrote:
>  >> >>
>  >> >> > On Fri, May 22, 2020 at 1:19 AM Yauheni Kaliuta
>  >> >> > <yauheni.kaliuta@redhat.com> wrote:
>  >> >> >>
>  >> >> >> There is difference in depoying static and generated extra resource
>  >> >> >> files between in/out of tree build and flavors:
>  >> >> >>
>  >> >> >> - in case of unflavored out-of-tree build static files are not
>  >> >> >> available and must be copied as well as both static and generated
>  >> >> >> files for flavored build.
>  >> >> >>
>  >> >> >> So split the rules and variables. The name TRUNNER_EXTRA_GEN_FILES
>  >> >> >> is chosen in analogy to TEST_GEN_* variants.
>  >> >> >>
>  >> >>
>  >> >> > Can we keep them together but be smarter about what needs to
>  >> >> > be copied based on source/target directories? I would really
>  >> >> > like to not blow up all these rules.
>  >> >>
>  >> >> I can try, ok, I just find it a bit more clear. But it's good to
>  >> >> get some input from kselftest about OOT build in general.
>  >>
>  >> > I see no value in 'make install' of selftests/bpf
>  >> > and since it's broken just remove that makefile target.
>  >>
>  >> Some CI systems perform testing next stage after building were
>  >> build tree is not available anymore. So it's in use at the
>  >> moment.
>
>  > such CI systems can do 'cp -r' then
>
> It's a discussion for linux-kselftest@ (added).
>
> At the moment `make install` is generic kselftest functionality
> and since bpf is part of that infra it looks a bit strange to
> break it intentionally.

selftests/bpf is only historically part of selftests.
It probably should stop using kselftest build infra all together.
We had breakages in selftests/bpf in the past only because
of changes in kselftests bits.

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[shuah]

On 5/27/20 11:05 AM, Alexei Starovoitov wrote:
> On Wed, May 27, 2020 at 10:02 AM Yauheni Kaliuta
> <yauheni.kaliuta@redhat.com> wrote:
>>
>> Hi, Alexei!
>>
>>>>>>> On Wed, 27 May 2020 09:48:04 -0700, Alexei Starovoitov  wrote:
>>
>>   > On Wed, May 27, 2020 at 12:19 AM Yauheni Kaliuta
>>   > <yauheni.kaliuta@redhat.com> wrote:
>>   >>
>>   >> Hi, Alexei!
>>   >>
>>   >> >>>>> On Tue, 26 May 2020 22:37:39 -0700, Alexei Starovoitov  wrote:
>>   >>
>>   >> > On Tue, May 26, 2020 at 10:31 PM Yauheni Kaliuta
>>   >> > <yauheni.kaliuta@redhat.com> wrote:
>>   >> >>
>>   >> >> Hi, Andrii!
>>   >> >>
>>   >> >> >>>>> On Tue, 26 May 2020 17:19:18 -0700, Andrii Nakryiko  wrote:
>>   >> >>
>>   >> >> > On Fri, May 22, 2020 at 1:19 AM Yauheni Kaliuta
>>   >> >> > <yauheni.kaliuta@redhat.com> wrote:
>>   >> >> >>
>>   >> >> >> There is difference in depoying static and generated extra resource
>>   >> >> >> files between in/out of tree build and flavors:
>>   >> >> >>
>>   >> >> >> - in case of unflavored out-of-tree build static files are not
>>   >> >> >> available and must be copied as well as both static and generated
>>   >> >> >> files for flavored build.
>>   >> >> >>
>>   >> >> >> So split the rules and variables. The name TRUNNER_EXTRA_GEN_FILES
>>   >> >> >> is chosen in analogy to TEST_GEN_* variants.
>>   >> >> >>
>>   >> >>
>>   >> >> > Can we keep them together but be smarter about what needs to
>>   >> >> > be copied based on source/target directories? I would really
>>   >> >> > like to not blow up all these rules.
>>   >> >>
>>   >> >> I can try, ok, I just find it a bit more clear. But it's good to
>>   >> >> get some input from kselftest about OOT build in general.
>>   >>
>>   >> > I see no value in 'make install' of selftests/bpf
>>   >> > and since it's broken just remove that makefile target.
>>   >>
>>   >> Some CI systems perform testing next stage after building were
>>   >> build tree is not available anymore. So it's in use at the
>>   >> moment.
>>
>>   > such CI systems can do 'cp -r' then
>> >> It's a discussion for linux-kselftest@ (added).
>>
>> At the moment `make install` is generic kselftest functionality
>> and since bpf is part of that infra it looks a bit strange to
>> break it intentionally.
> 
> selftests/bpf is only historically part of selftests.
> It probably should stop using kselftest build infra all together.
> We had breakages in selftests/bpf in the past only because
> of changes in kselftests bits.
> 

The question is whether or not the breakages addresses quickly.
Also, bpf keels breaking ksleftest builds and installs because
it has dependencies on bleeding edge tools and causes problems
for kselftest users.

You are pulling me into the discussion midway and I am missing the
context for the discussion. There is another thread on this topic
where Yauheni and I have been talking about bpf install.

I would say bpf install has never really worked from kselftest
install mechanism.

Ideally all tests use kselftest common install rule to leverage
the install and not have users do individual test installs.
It isn't productive and cooperative to say let's have bpf test
do its thing. It is part of selftests and we have to figure out
how to have it consistently build and run.

It isn't building for me on Linux 5.7-rc7 at the moment, leave
alone install.

The test Makefile has to handle OUTPUT directory. Please add me
to the entire patch series especially if it changes selftests
Makefile and lib.mk. I will review and try to see if we can make
bpf install work under kselftest common infrastructure.

I recently fixed bugs in kvm test Makefile to address the install
problems for cross-builds.

thanks,
-- Shuah

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Alexei Starovoitov]

On Wed, May 27, 2020 at 3:01 PM shuah <shuah@kernel.org> wrote:
>
> On 5/27/20 11:05 AM, Alexei Starovoitov wrote:
> > On Wed, May 27, 2020 at 10:02 AM Yauheni Kaliuta
> > <yauheni.kaliuta@redhat.com> wrote:
> >>
> >> Hi, Alexei!
> >>
> >>>>>>> On Wed, 27 May 2020 09:48:04 -0700, Alexei Starovoitov  wrote:
> >>
> >>   > On Wed, May 27, 2020 at 12:19 AM Yauheni Kaliuta
> >>   > <yauheni.kaliuta@redhat.com> wrote:
> >>   >>
> >>   >> Hi, Alexei!
> >>   >>
> >>   >> >>>>> On Tue, 26 May 2020 22:37:39 -0700, Alexei Starovoitov  wrote:
> >>   >>
> >>   >> > On Tue, May 26, 2020 at 10:31 PM Yauheni Kaliuta
> >>   >> > <yauheni.kaliuta@redhat.com> wrote:
> >>   >> >>
> >>   >> >> Hi, Andrii!
> >>   >> >>
> >>   >> >> >>>>> On Tue, 26 May 2020 17:19:18 -0700, Andrii Nakryiko  wrote:
> >>   >> >>
> >>   >> >> > On Fri, May 22, 2020 at 1:19 AM Yauheni Kaliuta
> >>   >> >> > <yauheni.kaliuta@redhat.com> wrote:
> >>   >> >> >>
> >>   >> >> >> There is difference in depoying static and generated extra resource
> >>   >> >> >> files between in/out of tree build and flavors:
> >>   >> >> >>
> >>   >> >> >> - in case of unflavored out-of-tree build static files are not
> >>   >> >> >> available and must be copied as well as both static and generated
> >>   >> >> >> files for flavored build.
> >>   >> >> >>
> >>   >> >> >> So split the rules and variables. The name TRUNNER_EXTRA_GEN_FILES
> >>   >> >> >> is chosen in analogy to TEST_GEN_* variants.
> >>   >> >> >>
> >>   >> >>
> >>   >> >> > Can we keep them together but be smarter about what needs to
> >>   >> >> > be copied based on source/target directories? I would really
> >>   >> >> > like to not blow up all these rules.
> >>   >> >>
> >>   >> >> I can try, ok, I just find it a bit more clear. But it's good to
> >>   >> >> get some input from kselftest about OOT build in general.
> >>   >>
> >>   >> > I see no value in 'make install' of selftests/bpf
> >>   >> > and since it's broken just remove that makefile target.
> >>   >>
> >>   >> Some CI systems perform testing next stage after building were
> >>   >> build tree is not available anymore. So it's in use at the
> >>   >> moment.
> >>
> >>   > such CI systems can do 'cp -r' then
> >> >> It's a discussion for linux-kselftest@ (added).
> >>
> >> At the moment `make install` is generic kselftest functionality
> >> and since bpf is part of that infra it looks a bit strange to
> >> break it intentionally.
> >
> > selftests/bpf is only historically part of selftests.
> > It probably should stop using kselftest build infra all together.
> > We had breakages in selftests/bpf in the past only because
> > of changes in kselftests bits.
> >
>
> The question is whether or not the breakages addresses quickly.
> Also, bpf keels breaking ksleftest builds and installs because
> it has dependencies on bleeding edge tools and causes problems
> for kselftest users.
>
> You are pulling me into the discussion midway and I am missing the
> context for the discussion. There is another thread on this topic
> where Yauheni and I have been talking about bpf install.
>
> I would say bpf install has never really worked from kselftest
> install mechanism.
>
> Ideally all tests use kselftest common install rule to leverage
> the install and not have users do individual test installs.
> It isn't productive and cooperative to say let's have bpf test
> do its thing. It is part of selftests and we have to figure out
> how to have it consistently build and run.
>
> It isn't building for me on Linux 5.7-rc7 at the moment, leave
> alone install.
>
> The test Makefile has to handle OUTPUT directory. Please add me
> to the entire patch series especially if it changes selftests
> Makefile and lib.mk. I will review and try to see if we can make
> bpf install work under kselftest common infrastructure.

I prefer to keep selftests/bpf install broken.
This forced marriage between kselftests and selftests/bpf
never worked well. I think it's a time to free them up from each other.

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Jiri Benc]

On Wed, 27 May 2020 15:23:13 -0700, Alexei Starovoitov wrote:
> I prefer to keep selftests/bpf install broken.
> This forced marriage between kselftests and selftests/bpf
> never worked well. I think it's a time to free them up from each other.

Alexei, it would be great if you could cooperate with other people
instead of pushing your own way. The selftests infrastructure was put
to the kernel to have one place for testing. Inventing yet another way
to add tests does not help anyone. You don't own the kernel. We're
community, we should cooperate.

 Jiri

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Greg KH]

On Thu, May 28, 2020 at 10:05:57AM +0200, Jiri Benc wrote:
> On Wed, 27 May 2020 15:23:13 -0700, Alexei Starovoitov wrote:
> > I prefer to keep selftests/bpf install broken.
> > This forced marriage between kselftests and selftests/bpf
> > never worked well. I think it's a time to free them up from each other.
> 
> Alexei, it would be great if you could cooperate with other people
> instead of pushing your own way. The selftests infrastructure was put
> to the kernel to have one place for testing. Inventing yet another way
> to add tests does not help anyone. You don't own the kernel. We're
> community, we should cooperate.

I agree, we rely on the infrastructure of the kselftests framework so
that testing systems do not have to create "custom" frameworks to handle
all of the individual variants that could easily crop up here.

Let's keep it easy for people to run and use these tests, to not do so
is to ensure that they are not used, which is the exact opposite goal of
creating tests.

thanks,

greg k-h

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Alexei Starovoitov]

On Thu, May 28, 2020 at 12:56:31PM +0200, Greg KH wrote:
> On Thu, May 28, 2020 at 10:05:57AM +0200, Jiri Benc wrote:
> > On Wed, 27 May 2020 15:23:13 -0700, Alexei Starovoitov wrote:
> > > I prefer to keep selftests/bpf install broken.
> > > This forced marriage between kselftests and selftests/bpf
> > > never worked well. I think it's a time to free them up from each other.
> > 
> > Alexei, it would be great if you could cooperate with other people
> > instead of pushing your own way. The selftests infrastructure was put
> > to the kernel to have one place for testing. Inventing yet another way
> > to add tests does not help anyone. You don't own the kernel. We're
> > community, we should cooperate.
> 
> I agree, we rely on the infrastructure of the kselftests framework so
> that testing systems do not have to create "custom" frameworks to handle
> all of the individual variants that could easily crop up here.
> 
> Let's keep it easy for people to run and use these tests, to not do so
> is to ensure that they are not used, which is the exact opposite goal of
> creating tests.

Greg,

It is easy for people (bpf developers) to run and use the tests.
Every developer runs them before submitting patches.
New tests is a hard requirement for any new features.
Maintainers run them for every push.

What I was and will push back hard is when other people (not bpf developers)
come back with an excuse that some CI system has a hard time running these
tests. It's the problem of weak CI. That CI needs to be fixed. Not the tests.
The example of this is that we already have github/libbpf CI that runs
selftests/bpf just fine. Anyone who wants to do another CI are welcome to copy
paste what already works instead of burdening people (bpf developers) who run
and use existing tests. I frankly have no sympathy to folks who put their own
interest of their CI development in front of bpf community of developers.
The main job of CI is to help developers and maintainers.
Where helping means to not impose new dumb rules on developers because CI
framework is dumb. Fix CI instead.

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Shuah Khan]

On 5/28/20 10:14 AM, Alexei Starovoitov wrote:
> On Thu, May 28, 2020 at 12:56:31PM +0200, Greg KH wrote:
>> On Thu, May 28, 2020 at 10:05:57AM +0200, Jiri Benc wrote:
>>> On Wed, 27 May 2020 15:23:13 -0700, Alexei Starovoitov wrote:
>>>> I prefer to keep selftests/bpf install broken.
>>>> This forced marriage between kselftests and selftests/bpf
>>>> never worked well. I think it's a time to free them up from each other.
>>>
>>> Alexei, it would be great if you could cooperate with other people
>>> instead of pushing your own way. The selftests infrastructure was put
>>> to the kernel to have one place for testing. Inventing yet another way
>>> to add tests does not help anyone. You don't own the kernel. We're
>>> community, we should cooperate.
>>
>> I agree, we rely on the infrastructure of the kselftests framework so
>> that testing systems do not have to create "custom" frameworks to handle
>> all of the individual variants that could easily crop up here.
>>
>> Let's keep it easy for people to run and use these tests, to not do so
>> is to ensure that they are not used, which is the exact opposite goal of
>> creating tests.
> 
> Greg,
> 
> It is easy for people (bpf developers) to run and use the tests.
> Every developer runs them before submitting patches.
> New tests is a hard requirement for any new features.
> Maintainers run them for every push.
> 
> What I was and will push back hard is when other people (not bpf developers)
> come back with an excuse that some CI system has a hard time running these
> tests. It's the problem of weak CI. That CI needs to be fixed. Not the tests.
> The example of this is that we already have github/libbpf CI that runs
> selftests/bpf just fine. Anyone who wants to do another CI are welcome to copy
> paste what already works instead of burdening people (bpf developers) who run
> and use existing tests. I frankly have no sympathy to folks who put their own
> interest of their CI development in front of bpf community of developers.
> The main job of CI is to help developers and maintainers.
> Where helping means to not impose new dumb rules on developers because CI
> framework is dumb. Fix CI instead.
> 

Here is what CI users are requesting:

- ability to install bpf test with other selftests using kselftest
   install. The common framework is in place and with minor changes
   to bpf test Makefile, we can make this happen. Others and myself
   are willing to work on this, so we can get bpf test coverage in
   test rings.

- be able to build and run existing tests without breaking the test
   build when new tests are added that have hard dependency on new
   versions of tools (llvm etc.). This isn't such a novel idea. We
   don't break kernel builds every single release and even when we
   require newer compiler releases. Plan the new tests with the intent
   to not break existing users and add new tests at the same time.
   We use min rev and not bleeding edge as the requirement for kernel
   build.

Requiring test rings upgrade to new versions of llvm is unreasonable.
It places undue burden on the admins to do this every single release
(may be even every rc cycle)

What is dumb about these requests and why is it not acceptable to just
bpf when all other sub-systems keep adding tests continuously using the
selftests framework so we can test the kernel better and our releases
are of better quality.

If you check the volume of tests that get added every release, you can
easily see it isn't hard.

Calling the needs of CI dumb is detrimental to kernel quality as these
rings provide a very important function. Addressing their use-case helps
get better test coverage for bpf and kernel areas that use bpf.

thanks,
-- Shuah

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Yauheni Kaliuta]

Hi, Alexei,

On Thu, May 28, 2020 at 7:14 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, May 28, 2020 at 12:56:31PM +0200, Greg KH wrote:
> > On Thu, May 28, 2020 at 10:05:57AM +0200, Jiri Benc wrote:
> > > On Wed, 27 May 2020 15:23:13 -0700, Alexei Starovoitov wrote:
> > > > I prefer to keep selftests/bpf install broken.
> > > > This forced marriage between kselftests and selftests/bpf
> > > > never worked well. I think it's a time to free them up from each other.
> > >
> > > Alexei, it would be great if you could cooperate with other people
> > > instead of pushing your own way. The selftests infrastructure was put
> > > to the kernel to have one place for testing. Inventing yet another way
> > > to add tests does not help anyone. You don't own the kernel. We're
> > > community, we should cooperate.
> >
> > I agree, we rely on the infrastructure of the kselftests framework so
> > that testing systems do not have to create "custom" frameworks to handle
> > all of the individual variants that could easily crop up here.
> >
> > Let's keep it easy for people to run and use these tests, to not do so
> > is to ensure that they are not used, which is the exact opposite goal of
> > creating tests.
>
> Greg,
>
> It is easy for people (bpf developers) to run and use the tests.
> Every developer runs them before submitting patches.
> New tests is a hard requirement for any new features.
> Maintainers run them for every push.
>
> What I was and will push back hard is when other people (not bpf developers)
> come back with an excuse that some CI system has a hard time running these
> tests. It's the problem of weak CI. That CI needs to be fixed. Not the tests.
> The example of this is that we already have github/libbpf CI that runs
> selftests/bpf just fine. Anyone who wants to do another CI are welcome to copy
> paste what already works instead of burdening people (bpf developers) who run
> and use existing tests. I frankly have no sympathy to folks who put their own
> interest of their CI development in front of bpf community of developers.
> The main job of CI is to help developers and maintainers.
> Where helping means to not impose new dumb rules on developers because CI
> framework is dumb. Fix CI instead.
>

Any good reason why bpf selftests, residing under selftests/, should
be an exception?
"Breakages" is not, breakages are fixable.

-- 
WBR, Yauheni

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Alexei Starovoitov]

On Thu, May 28, 2020 at 11:07:09AM -0600, Shuah Khan wrote:
> On 5/28/20 10:14 AM, Alexei Starovoitov wrote:
> > On Thu, May 28, 2020 at 12:56:31PM +0200, Greg KH wrote:
> > > On Thu, May 28, 2020 at 10:05:57AM +0200, Jiri Benc wrote:
> > > > On Wed, 27 May 2020 15:23:13 -0700, Alexei Starovoitov wrote:
> > > > > I prefer to keep selftests/bpf install broken.
> > > > > This forced marriage between kselftests and selftests/bpf
> > > > > never worked well. I think it's a time to free them up from each other.
> > > > 
> > > > Alexei, it would be great if you could cooperate with other people
> > > > instead of pushing your own way. The selftests infrastructure was put
> > > > to the kernel to have one place for testing. Inventing yet another way
> > > > to add tests does not help anyone. You don't own the kernel. We're
> > > > community, we should cooperate.
> > > 
> > > I agree, we rely on the infrastructure of the kselftests framework so
> > > that testing systems do not have to create "custom" frameworks to handle
> > > all of the individual variants that could easily crop up here.
> > > 
> > > Let's keep it easy for people to run and use these tests, to not do so
> > > is to ensure that they are not used, which is the exact opposite goal of
> > > creating tests.
> > 
> > Greg,
> > 
> > It is easy for people (bpf developers) to run and use the tests.
> > Every developer runs them before submitting patches.
> > New tests is a hard requirement for any new features.
> > Maintainers run them for every push.
> > 
> > What I was and will push back hard is when other people (not bpf developers)
> > come back with an excuse that some CI system has a hard time running these
> > tests. It's the problem of weak CI. That CI needs to be fixed. Not the tests.
> > The example of this is that we already have github/libbpf CI that runs
> > selftests/bpf just fine. Anyone who wants to do another CI are welcome to copy
> > paste what already works instead of burdening people (bpf developers) who run
> > and use existing tests. I frankly have no sympathy to folks who put their own
> > interest of their CI development in front of bpf community of developers.
> > The main job of CI is to help developers and maintainers.
> > Where helping means to not impose new dumb rules on developers because CI
> > framework is dumb. Fix CI instead.
> > 
> 
> Here is what CI users are requesting:
> 
> - ability to install bpf test with other selftests using kselftest
>   install. The common framework is in place and with minor changes
>   to bpf test Makefile, we can make this happen. Others and myself
>   are willing to work on this, so we can get bpf test coverage in
>   test rings.

so you're saying that bpf maintainers and all bpf developers now
would need to incorporate new 'make install' step to their workflow
because some unknown CI system that is not even functional decided
to do 'make install' ?
That's exactly my point about selfish CI developers who put their
needs in front of bpf community of developers.

> - be able to build and run existing tests without breaking the test
>   build when new tests are added that have hard dependency on new
>   versions of tools (llvm etc.). This isn't such a novel idea. We
>   don't break kernel builds every single release and even when we
>   require newer compiler releases. Plan the new tests with the intent
>   to not break existing users and add new tests at the same time.
>   We use min rev and not bleeding edge as the requirement for kernel
>   build.

'existing users'? CI is not a user. CI is machine that should _help_
developers. Above two things (forcing install on humans and
breaking day-to-day tests for bpf maintainers and developers)
is the opposite of helping developers.
Please do NOT use such useless CI as an excuse.
Such CI should not be built in the first place when it slows down
the development instead of helping it.

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Alexei Starovoitov]

On Thu, May 28, 2020 at 08:10:57PM +0300, Yauheni Kaliuta wrote:
> Hi, Alexei,
> 
> On Thu, May 28, 2020 at 7:14 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, May 28, 2020 at 12:56:31PM +0200, Greg KH wrote:
> > > On Thu, May 28, 2020 at 10:05:57AM +0200, Jiri Benc wrote:
> > > > On Wed, 27 May 2020 15:23:13 -0700, Alexei Starovoitov wrote:
> > > > > I prefer to keep selftests/bpf install broken.
> > > > > This forced marriage between kselftests and selftests/bpf
> > > > > never worked well. I think it's a time to free them up from each other.
> > > >
> > > > Alexei, it would be great if you could cooperate with other people
> > > > instead of pushing your own way. The selftests infrastructure was put
> > > > to the kernel to have one place for testing. Inventing yet another way
> > > > to add tests does not help anyone. You don't own the kernel. We're
> > > > community, we should cooperate.
> > >
> > > I agree, we rely on the infrastructure of the kselftests framework so
> > > that testing systems do not have to create "custom" frameworks to handle
> > > all of the individual variants that could easily crop up here.
> > >
> > > Let's keep it easy for people to run and use these tests, to not do so
> > > is to ensure that they are not used, which is the exact opposite goal of
> > > creating tests.
> >
> > Greg,
> >
> > It is easy for people (bpf developers) to run and use the tests.
> > Every developer runs them before submitting patches.
> > New tests is a hard requirement for any new features.
> > Maintainers run them for every push.
> >
> > What I was and will push back hard is when other people (not bpf developers)
> > come back with an excuse that some CI system has a hard time running these
> > tests. It's the problem of weak CI. That CI needs to be fixed. Not the tests.
> > The example of this is that we already have github/libbpf CI that runs
> > selftests/bpf just fine. Anyone who wants to do another CI are welcome to copy
> > paste what already works instead of burdening people (bpf developers) who run
> > and use existing tests. I frankly have no sympathy to folks who put their own
> > interest of their CI development in front of bpf community of developers.
> > The main job of CI is to help developers and maintainers.
> > Where helping means to not impose new dumb rules on developers because CI
> > framework is dumb. Fix CI instead.
> >
> 
> Any good reason why bpf selftests, residing under selftests/, should
> be an exception?
> "Breakages" is not, breakages are fixable.

As I said early the location of bpf selftests in tools/testing/selftests/
was a historical mistake that needs to be corrected.
There is no value in residing in that directory.
kselftest are aiming to test the kernel whereas selftests/bpf are testing
kernel, libbpf, bpftool, llvm, pahole. These are the tests for bpf ecosystem.

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Yauheni Kaliuta]

Hi!

On Thu, May 28, 2020 at 9:16 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, May 28, 2020 at 11:07:09AM -0600, Shuah Khan wrote:

[...]
> >
> > Here is what CI users are requesting:
> >
> > - ability to install bpf test with other selftests using kselftest
> >   install. The common framework is in place and with minor changes
> >   to bpf test Makefile, we can make this happen. Others and myself
> >   are willing to work on this, so we can get bpf test coverage in
> >   test rings.
>
> so you're saying that bpf maintainers and all bpf developers now
> would need to incorporate new 'make install' step to their workflow
> because some unknown CI system that is not even functional decided
> to do 'make install' ?
> That's exactly my point about selfish CI developers who put their
> needs in front of bpf community of developers.

May be, it can work both ways to make everybody happy :) (I haven't
seen yet fundamental problems why not).

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Alexei Starovoitov]

On Thu, May 28, 2020 at 11:29 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi!
>
> On Thu, May 28, 2020 at 9:16 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, May 28, 2020 at 11:07:09AM -0600, Shuah Khan wrote:
>
> [...]
> > >
> > > Here is what CI users are requesting:
> > >
> > > - ability to install bpf test with other selftests using kselftest
> > >   install. The common framework is in place and with minor changes
> > >   to bpf test Makefile, we can make this happen. Others and myself
> > >   are willing to work on this, so we can get bpf test coverage in
> > >   test rings.
> >
> > so you're saying that bpf maintainers and all bpf developers now
> > would need to incorporate new 'make install' step to their workflow
> > because some unknown CI system that is not even functional decided
> > to do 'make install' ?
> > That's exactly my point about selfish CI developers who put their
> > needs in front of bpf community of developers.
>
> May be, it can work both ways to make everybody happy :) (I haven't
> seen yet fundamental problems why not).

then stop pretending and do 'cp -r' for your CI as you were suggested
several times already.
It works just fine for libbpf CI. Feel free to copy those scripts.

Re: [PATCH 4/8] selftests/bpf: fix object files installation

[Andrii Nakryiko]

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
>

Re: [PATCH 4/8] selftests/bpf: fix object files installation

[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
> >
>


-- 
WBR, Yauheni

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Shuah Khan]

On 5/28/20 12:15 PM, Alexei Starovoitov wrote:
> On Thu, May 28, 2020 at 11:07:09AM -0600, Shuah Khan wrote:
>> On 5/28/20 10:14 AM, Alexei Starovoitov wrote:
>>> On Thu, May 28, 2020 at 12:56:31PM +0200, Greg KH wrote:
>>>> On Thu, May 28, 2020 at 10:05:57AM +0200, Jiri Benc wrote:
>>>>> On Wed, 27 May 2020 15:23:13 -0700, Alexei Starovoitov wrote:
>>>>>> I prefer to keep selftests/bpf install broken.
>>>>>> This forced marriage between kselftests and selftests/bpf
>>>>>> never worked well. I think it's a time to free them up from each other.
>>>>>
>>>>> Alexei, it would be great if you could cooperate with other people
>>>>> instead of pushing your own way. The selftests infrastructure was put
>>>>> to the kernel to have one place for testing. Inventing yet another way
>>>>> to add tests does not help anyone. You don't own the kernel. We're
>>>>> community, we should cooperate.
>>>>
>>>> I agree, we rely on the infrastructure of the kselftests framework so
>>>> that testing systems do not have to create "custom" frameworks to handle
>>>> all of the individual variants that could easily crop up here.
>>>>
>>>> Let's keep it easy for people to run and use these tests, to not do so
>>>> is to ensure that they are not used, which is the exact opposite goal of
>>>> creating tests.
>>>
>>> Greg,
>>>
>>> It is easy for people (bpf developers) to run and use the tests.
>>> Every developer runs them before submitting patches.
>>> New tests is a hard requirement for any new features.
>>> Maintainers run them for every push.
>>>
>>> What I was and will push back hard is when other people (not bpf developers)
>>> come back with an excuse that some CI system has a hard time running these
>>> tests. It's the problem of weak CI. That CI needs to be fixed. Not the tests.
>>> The example of this is that we already have github/libbpf CI that runs
>>> selftests/bpf just fine. Anyone who wants to do another CI are welcome to copy
>>> paste what already works instead of burdening people (bpf developers) who run
>>> and use existing tests. I frankly have no sympathy to folks who put their own
>>> interest of their CI development in front of bpf community of developers.
>>> The main job of CI is to help developers and maintainers.
>>> Where helping means to not impose new dumb rules on developers because CI
>>> framework is dumb. Fix CI instead.
>>>
>>
>> Here is what CI users are requesting:
>>
>> - ability to install bpf test with other selftests using kselftest
>>    install. The common framework is in place and with minor changes
>>    to bpf test Makefile, we can make this happen. Others and myself
>>    are willing to work on this, so we can get bpf test coverage in
>>    test rings.
> 
> so you're saying that bpf maintainers and all bpf developers now
> would need to incorporate new 'make install' step to their workflow
> because some unknown CI system that is not even functional decided
> to do 'make install' ?
> That's exactly my point about selfish CI developers who put their
> needs in front of bpf community of developers.
> 

There is no need change bpf maintainer and developer workflow. You
don't have to use install option. Kselftest framework doesn't
require a specific workflow and you can:

1. Build and run your tests from bpf directory if you choose to
2. Install to run on different target.

Adding install install option requires a change to bpf Makefile
only to copy test that are built to install directory.

make kselftest-install from the main kernel Makefile in conjunction
with selftests Makefile and lib.mk will handle all of that.

Sounds like there is a misunderstanding that bpf maintainer/developer
workflow will have to change to support install. That is not the case.
The reason kselftest exists on the first place is to have common
framework to take care of build/run/install as a common layer so
individual test writers don't have to worry about these details
and write tests instead.

>> - be able to build and run existing tests without breaking the test
>>    build when new tests are added that have hard dependency on new
>>    versions of tools (llvm etc.). This isn't such a novel idea. We
>>    don't break kernel builds every single release and even when we
>>    require newer compiler releases. Plan the new tests with the intent
>>    to not break existing users and add new tests at the same time.
>>    We use min rev and not bleeding edge as the requirement for kernel
>>    build.
> 
> 'existing users'? 

I said existing tests not users. When you add new bpf tests, existing
tests should continue to build and run without dependency on new revs
of llvm.

CI is not a user. CI is machine that should _help_
> developers. Above two things (forcing install on humans and
> breaking day-to-day tests for bpf maintainers and developers)
> is the opposite of helping developers.
> Please do NOT use such useless CI as an excuse.
> Such CI should not be built in the first place when it slows down
> the development instead of helping it.
> 
LKFT test ring runs selftests now and Kernel CI will be soon. Various
users run selftests in their own environments. LKFT ring admins have
to build and install tests whether it is automated or not and look at
the results. If bpf test doesn't build and/or installed, it won't run
on test rings that qualify stable/next/main releases.

I don't understand why CI use-case is useless.

thanks,
-- Shuah

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Shuah Khan]

On 5/28/20 12:34 PM, Alexei Starovoitov wrote:
> On Thu, May 28, 2020 at 11:29 AM Yauheni Kaliuta
> <yauheni.kaliuta@redhat.com> wrote:
>>
>> Hi!
>>
>> On Thu, May 28, 2020 at 9:16 PM Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>>>
>>> On Thu, May 28, 2020 at 11:07:09AM -0600, Shuah Khan wrote:
>>
>> [...]
>>>>
>>>> Here is what CI users are requesting:
>>>>
>>>> - ability to install bpf test with other selftests using kselftest
>>>>    install. The common framework is in place and with minor changes
>>>>    to bpf test Makefile, we can make this happen. Others and myself
>>>>    are willing to work on this, so we can get bpf test coverage in
>>>>    test rings.
>>>
>>> so you're saying that bpf maintainers and all bpf developers now
>>> would need to incorporate new 'make install' step to their workflow
>>> because some unknown CI system that is not even functional decided
>>> to do 'make install' ?
>>> That's exactly my point about selfish CI developers who put their
>>> needs in front of bpf community of developers.
>>
>> May be, it can work both ways to make everybody happy :) (I haven't
>> seen yet fundamental problems why not).
> 
> then stop pretending and do 'cp -r' for your CI as you were suggested
> several times already.
> It works just fine for libbpf CI. Feel free to copy those scripts.
> 

Yauheni,

Let's drop discussing this patch set. I don't have all the patches
and changes in this to weigh in. There is no need to add cp -r at all.

I want to take the discussion one level up to the use-cases.

thanks,
-- Shuah

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Shuah Khan]

On 5/28/20 11:10 AM, Yauheni Kaliuta wrote:
> Hi, Alexei,
> 
> On Thu, May 28, 2020 at 7:14 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
>>
>> On Thu, May 28, 2020 at 12:56:31PM +0200, Greg KH wrote:
>>> On Thu, May 28, 2020 at 10:05:57AM +0200, Jiri Benc wrote:
>>>> On Wed, 27 May 2020 15:23:13 -0700, Alexei Starovoitov wrote:
>>>>> I prefer to keep selftests/bpf install broken.
>>>>> This forced marriage between kselftests and selftests/bpf
>>>>> never worked well. I think it's a time to free them up from each other.
>>>>
>>>> Alexei, it would be great if you could cooperate with other people
>>>> instead of pushing your own way. The selftests infrastructure was put
>>>> to the kernel to have one place for testing. Inventing yet another way
>>>> to add tests does not help anyone. You don't own the kernel. We're
>>>> community, we should cooperate.
>>>
>>> I agree, we rely on the infrastructure of the kselftests framework so
>>> that testing systems do not have to create "custom" frameworks to handle
>>> all of the individual variants that could easily crop up here.
>>>
>>> Let's keep it easy for people to run and use these tests, to not do so
>>> is to ensure that they are not used, which is the exact opposite goal of
>>> creating tests.
>>
>> Greg,
>>
>> It is easy for people (bpf developers) to run and use the tests.
>> Every developer runs them before submitting patches.
>> New tests is a hard requirement for any new features.
>> Maintainers run them for every push.
>>
>> What I was and will push back hard is when other people (not bpf developers)
>> come back with an excuse that some CI system has a hard time running these
>> tests. It's the problem of weak CI. That CI needs to be fixed. Not the tests.
>> The example of this is that we already have github/libbpf CI that runs
>> selftests/bpf just fine. Anyone who wants to do another CI are welcome to copy
>> paste what already works instead of burdening people (bpf developers) who run
>> and use existing tests. I frankly have no sympathy to folks who put their own
>> interest of their CI development in front of bpf community of developers.
>> The main job of CI is to help developers and maintainers.
>> Where helping means to not impose new dumb rules on developers because CI
>> framework is dumb. Fix CI instead.
>>
> 
> Any good reason why bpf selftests, residing under selftests/, should
> be an exception?
> "Breakages" is not, breakages are fixable.
> 

Let's not talk about moving tests. I don't want to discuss that until
we are all on the same page on what is the problem in adding install
support to bpf Makefile.

It is possible that there is a misunderstanding that bpf maintainer
and developer workflow will change. Which is definitely not needed.
If this patch series requires it, it isn't correct and needs to be
reworked.

thanks,
-- Shuah

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Alexei Starovoitov]

On Thu, May 28, 2020 at 12:59:06PM -0600, Shuah Khan wrote:
> On 5/28/20 12:15 PM, Alexei Starovoitov wrote:
> > On Thu, May 28, 2020 at 11:07:09AM -0600, Shuah Khan wrote:
> > > On 5/28/20 10:14 AM, Alexei Starovoitov wrote:
> > > > On Thu, May 28, 2020 at 12:56:31PM +0200, Greg KH wrote:
> > > > > On Thu, May 28, 2020 at 10:05:57AM +0200, Jiri Benc wrote:
> > > > > > On Wed, 27 May 2020 15:23:13 -0700, Alexei Starovoitov wrote:
> > > > > > > I prefer to keep selftests/bpf install broken.
> > > > > > > This forced marriage between kselftests and selftests/bpf
> > > > > > > never worked well. I think it's a time to free them up from each other.
> > > > > > 
> > > > > > Alexei, it would be great if you could cooperate with other people
> > > > > > instead of pushing your own way. The selftests infrastructure was put
> > > > > > to the kernel to have one place for testing. Inventing yet another way
> > > > > > to add tests does not help anyone. You don't own the kernel. We're
> > > > > > community, we should cooperate.
> > > > > 
> > > > > I agree, we rely on the infrastructure of the kselftests framework so
> > > > > that testing systems do not have to create "custom" frameworks to handle
> > > > > all of the individual variants that could easily crop up here.
> > > > > 
> > > > > Let's keep it easy for people to run and use these tests, to not do so
> > > > > is to ensure that they are not used, which is the exact opposite goal of
> > > > > creating tests.
> > > > 
> > > > Greg,
> > > > 
> > > > It is easy for people (bpf developers) to run and use the tests.
> > > > Every developer runs them before submitting patches.
> > > > New tests is a hard requirement for any new features.
> > > > Maintainers run them for every push.
> > > > 
> > > > What I was and will push back hard is when other people (not bpf developers)
> > > > come back with an excuse that some CI system has a hard time running these
> > > > tests. It's the problem of weak CI. That CI needs to be fixed. Not the tests.
> > > > The example of this is that we already have github/libbpf CI that runs
> > > > selftests/bpf just fine. Anyone who wants to do another CI are welcome to copy
> > > > paste what already works instead of burdening people (bpf developers) who run
> > > > and use existing tests. I frankly have no sympathy to folks who put their own
> > > > interest of their CI development in front of bpf community of developers.
> > > > The main job of CI is to help developers and maintainers.
> > > > Where helping means to not impose new dumb rules on developers because CI
> > > > framework is dumb. Fix CI instead.
> > > > 
> > > 
> > > Here is what CI users are requesting:
> > > 
> > > - ability to install bpf test with other selftests using kselftest
> > >    install. The common framework is in place and with minor changes
> > >    to bpf test Makefile, we can make this happen. Others and myself
> > >    are willing to work on this, so we can get bpf test coverage in
> > >    test rings.
> > 
> > so you're saying that bpf maintainers and all bpf developers now
> > would need to incorporate new 'make install' step to their workflow
> > because some unknown CI system that is not even functional decided
> > to do 'make install' ?
> > That's exactly my point about selfish CI developers who put their
> > needs in front of bpf community of developers.
> > 
> 
> There is no need change bpf maintainer and developer workflow. You
> don't have to use install option. Kselftest framework doesn't
> require a specific workflow and you can:
> 
> 1. Build and run your tests from bpf directory if you choose to
> 2. Install to run on different target.
> 
> Adding install install option requires a change to bpf Makefile
> only to copy test that are built to install directory.
> 
> make kselftest-install from the main kernel Makefile in conjunction
> with selftests Makefile and lib.mk will handle all of that.
> 
> Sounds like there is a misunderstanding that bpf maintainer/developer
> workflow will have to change to support install. That is not the case.
> The reason kselftest exists on the first place is to have common
> framework to take care of build/run/install as a common layer so
> individual test writers don't have to worry about these details
> and write tests instead.

I don't think you understand the 'make install' implications.
Not doing 'make install' means that all the Makefile changes that
Yauheni is proposing will immediately bit rot.
People are constantly adding new tests and changing makefile.
'make install' _will_ be broken instantly unless _humans_ incorporate
it in their patch development process and maintainer incorporate
that step into their workflow as well.

Ohh, but don't worry about this broken 'make install' is not an answer.
It's broken now and I don't want to see patches that move it
from one broken state into another broken state and at the same time
add complexity to it.

That's very different from 'make install' doing 'cp -r' of the whole dir.
In such case the chances of it going stale and broken are much lower.

> > > - be able to build and run existing tests without breaking the test
> > >    build when new tests are added that have hard dependency on new
> > >    versions of tools (llvm etc.). This isn't such a novel idea. We
> > >    don't break kernel builds every single release and even when we
> > >    require newer compiler releases. Plan the new tests with the intent
> > >    to not break existing users and add new tests at the same time.
> > >    We use min rev and not bleeding edge as the requirement for kernel
> > >    build.
> > 
> > 'existing users'?
> 
> I said existing tests not users. When you add new bpf tests, existing
> tests should continue to build and run without dependency on new revs
> of llvm.

Who said that existing test stop building with new llvm ?
Please check your facts.

> If bpf test doesn't build and/or installed, it won't run
> on test rings that qualify stable/next/main releases.

That's the case today and I prefer to keep it this way.
stable folks ignored our recommendations on how selftests/bpf should
be run, so please don't come up with your ways of doing it and
complain that something doesn't work.

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Yauheni Kaliuta]

On Thu, May 28, 2020 at 10:09 PM Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 5/28/20 11:10 AM, Yauheni Kaliuta wrote:
> > Hi, Alexei,
> >
> > On Thu, May 28, 2020 at 7:14 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> >>
> >> On Thu, May 28, 2020 at 12:56:31PM +0200, Greg KH wrote:
> >>> On Thu, May 28, 2020 at 10:05:57AM +0200, Jiri Benc wrote:
> >>>> On Wed, 27 May 2020 15:23:13 -0700, Alexei Starovoitov wrote:
> >>>>> I prefer to keep selftests/bpf install broken.
> >>>>> This forced marriage between kselftests and selftests/bpf
> >>>>> never worked well. I think it's a time to free them up from each other.
> >>>>
> >>>> Alexei, it would be great if you could cooperate with other people
> >>>> instead of pushing your own way. The selftests infrastructure was put
> >>>> to the kernel to have one place for testing. Inventing yet another way
> >>>> to add tests does not help anyone. You don't own the kernel. We're
> >>>> community, we should cooperate.
> >>>
> >>> I agree, we rely on the infrastructure of the kselftests framework so
> >>> that testing systems do not have to create "custom" frameworks to handle
> >>> all of the individual variants that could easily crop up here.
> >>>
> >>> Let's keep it easy for people to run and use these tests, to not do so
> >>> is to ensure that they are not used, which is the exact opposite goal of
> >>> creating tests.
> >>
> >> Greg,
> >>
> >> It is easy for people (bpf developers) to run and use the tests.
> >> Every developer runs them before submitting patches.
> >> New tests is a hard requirement for any new features.
> >> Maintainers run them for every push.
> >>
> >> What I was and will push back hard is when other people (not bpf developers)
> >> come back with an excuse that some CI system has a hard time running these
> >> tests. It's the problem of weak CI. That CI needs to be fixed. Not the tests.
> >> The example of this is that we already have github/libbpf CI that runs
> >> selftests/bpf just fine. Anyone who wants to do another CI are welcome to copy
> >> paste what already works instead of burdening people (bpf developers) who run
> >> and use existing tests. I frankly have no sympathy to folks who put their own
> >> interest of their CI development in front of bpf community of developers.
> >> The main job of CI is to help developers and maintainers.
> >> Where helping means to not impose new dumb rules on developers because CI
> >> framework is dumb. Fix CI instead.
> >>
> >
> > Any good reason why bpf selftests, residing under selftests/, should
> > be an exception?
> > "Breakages" is not, breakages are fixable.
> >
>
> Let's not talk about moving tests. I don't want to discuss that until
> we are all on the same page on what is the problem in adding install
> support to bpf Makefile.
>
> It is possible that there is a misunderstanding that bpf maintainer
> and developer workflow will change. Which is definitely not needed.
> If this patch series requires it, it isn't correct and needs to be
> reworked.

patch series does not change it. Running bpf tests in-place is one of
my own usecases, I'm not going to break it :)
The series tried to fix what does not work (install and build in
$(OUTPUT) directory) but exists in the code. I'll include you in Cc
for the respin(s) if you are interested.

The discussion is pretty much not connected to the patchset, but about
bpf selftests and the infra in general.

-- 
WBR, Yauheni

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Shuah Khan]

On 5/28/20 1:20 PM, Yauheni Kaliuta wrote:
> On Thu, May 28, 2020 at 10:09 PM Shuah Khan <skhan@linuxfoundation.org> wrote:
>>
>> On 5/28/20 11:10 AM, Yauheni Kaliuta wrote:
>>> Hi, Alexei,
>>>
>>> On Thu, May 28, 2020 at 7:14 PM Alexei Starovoitov
>>> <alexei.starovoitov@gmail.com> wrote:
>>>>
>>>> On Thu, May 28, 2020 at 12:56:31PM +0200, Greg KH wrote:
>>>>> On Thu, May 28, 2020 at 10:05:57AM +0200, Jiri Benc wrote:
>>>>>> On Wed, 27 May 2020 15:23:13 -0700, Alexei Starovoitov wrote:
>>>>>>> I prefer to keep selftests/bpf install broken.
>>>>>>> This forced marriage between kselftests and selftests/bpf
>>>>>>> never worked well. I think it's a time to free them up from each other.
>>>>>>
>>>>>> Alexei, it would be great if you could cooperate with other people
>>>>>> instead of pushing your own way. The selftests infrastructure was put
>>>>>> to the kernel to have one place for testing. Inventing yet another way
>>>>>> to add tests does not help anyone. You don't own the kernel. We're
>>>>>> community, we should cooperate.
>>>>>
>>>>> I agree, we rely on the infrastructure of the kselftests framework so
>>>>> that testing systems do not have to create "custom" frameworks to handle
>>>>> all of the individual variants that could easily crop up here.
>>>>>
>>>>> Let's keep it easy for people to run and use these tests, to not do so
>>>>> is to ensure that they are not used, which is the exact opposite goal of
>>>>> creating tests.
>>>>
>>>> Greg,
>>>>
>>>> It is easy for people (bpf developers) to run and use the tests.
>>>> Every developer runs them before submitting patches.
>>>> New tests is a hard requirement for any new features.
>>>> Maintainers run them for every push.
>>>>
>>>> What I was and will push back hard is when other people (not bpf developers)
>>>> come back with an excuse that some CI system has a hard time running these
>>>> tests. It's the problem of weak CI. That CI needs to be fixed. Not the tests.
>>>> The example of this is that we already have github/libbpf CI that runs
>>>> selftests/bpf just fine. Anyone who wants to do another CI are welcome to copy
>>>> paste what already works instead of burdening people (bpf developers) who run
>>>> and use existing tests. I frankly have no sympathy to folks who put their own
>>>> interest of their CI development in front of bpf community of developers.
>>>> The main job of CI is to help developers and maintainers.
>>>> Where helping means to not impose new dumb rules on developers because CI
>>>> framework is dumb. Fix CI instead.
>>>>
>>>
>>> Any good reason why bpf selftests, residing under selftests/, should
>>> be an exception?
>>> "Breakages" is not, breakages are fixable.
>>>
>>
>> Let's not talk about moving tests. I don't want to discuss that until
>> we are all on the same page on what is the problem in adding install
>> support to bpf Makefile.
>>
>> It is possible that there is a misunderstanding that bpf maintainer
>> and developer workflow will change. Which is definitely not needed.
>> If this patch series requires it, it isn't correct and needs to be
>> reworked.
> 
> patch series does not change it. Running bpf tests in-place is one of
> my own usecases, I'm not going to break it :)

Great. I wasn't suggesting you would. So there is no need for the
concern that bpf developer/maintainer workflow will have to change
to add install support.

> The series tried to fix what does not work (install and build in
> $(OUTPUT) directory) but exists in the code. I'll include you in Cc
> for the respin(s) if you are interested.

Please do.

> 
> The discussion is pretty much not connected to the patchset, but about
> bpf selftests and the infra in general.
> 

Yeah. We have to get a better understanding and be on the same page on
this anyway.

thanks,
-- Shuah

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Shuah Khan]

On 5/28/20 1:18 PM, Alexei Starovoitov wrote:
> On Thu, May 28, 2020 at 12:59:06PM -0600, Shuah Khan wrote:
>> On 5/28/20 12:15 PM, Alexei Starovoitov wrote:
>>> On Thu, May 28, 2020 at 11:07:09AM -0600, Shuah Khan wrote:
>>>> On 5/28/20 10:14 AM, Alexei Starovoitov wrote:
>>>>> On Thu, May 28, 2020 at 12:56:31PM +0200, Greg KH wrote:
>>>>>> On Thu, May 28, 2020 at 10:05:57AM +0200, Jiri Benc wrote:
>>>>>>> On Wed, 27 May 2020 15:23:13 -0700, Alexei Starovoitov wrote:
>>>>>>>> I prefer to keep selftests/bpf install broken.
>>>>>>>> This forced marriage between kselftests and selftests/bpf
>>>>>>>> never worked well. I think it's a time to free them up from each other.
>>>>>>>
>>>>>>> Alexei, it would be great if you could cooperate with other people
>>>>>>> instead of pushing your own way. The selftests infrastructure was put
>>>>>>> to the kernel to have one place for testing. Inventing yet another way
>>>>>>> to add tests does not help anyone. You don't own the kernel. We're
>>>>>>> community, we should cooperate.
>>>>>>
>>>>>> I agree, we rely on the infrastructure of the kselftests framework so
>>>>>> that testing systems do not have to create "custom" frameworks to handle
>>>>>> all of the individual variants that could easily crop up here.
>>>>>>
>>>>>> Let's keep it easy for people to run and use these tests, to not do so
>>>>>> is to ensure that they are not used, which is the exact opposite goal of
>>>>>> creating tests.
>>>>>
>>>>> Greg,
>>>>>
>>>>> It is easy for people (bpf developers) to run and use the tests.
>>>>> Every developer runs them before submitting patches.
>>>>> New tests is a hard requirement for any new features.
>>>>> Maintainers run them for every push.
>>>>>
>>>>> What I was and will push back hard is when other people (not bpf developers)
>>>>> come back with an excuse that some CI system has a hard time running these
>>>>> tests. It's the problem of weak CI. That CI needs to be fixed. Not the tests.
>>>>> The example of this is that we already have github/libbpf CI that runs
>>>>> selftests/bpf just fine. Anyone who wants to do another CI are welcome to copy
>>>>> paste what already works instead of burdening people (bpf developers) who run
>>>>> and use existing tests. I frankly have no sympathy to folks who put their own
>>>>> interest of their CI development in front of bpf community of developers.
>>>>> The main job of CI is to help developers and maintainers.
>>>>> Where helping means to not impose new dumb rules on developers because CI
>>>>> framework is dumb. Fix CI instead.
>>>>>
>>>>
>>>> Here is what CI users are requesting:
>>>>
>>>> - ability to install bpf test with other selftests using kselftest
>>>>     install. The common framework is in place and with minor changes
>>>>     to bpf test Makefile, we can make this happen. Others and myself
>>>>     are willing to work on this, so we can get bpf test coverage in
>>>>     test rings.
>>>
>>> so you're saying that bpf maintainers and all bpf developers now
>>> would need to incorporate new 'make install' step to their workflow
>>> because some unknown CI system that is not even functional decided
>>> to do 'make install' ?
>>> That's exactly my point about selfish CI developers who put their
>>> needs in front of bpf community of developers.
>>>
>>
>> There is no need change bpf maintainer and developer workflow. You
>> don't have to use install option. Kselftest framework doesn't
>> require a specific workflow and you can:
>>
>> 1. Build and run your tests from bpf directory if you choose to
>> 2. Install to run on different target.
>>
>> Adding install install option requires a change to bpf Makefile
>> only to copy test that are built to install directory.
>>
>> make kselftest-install from the main kernel Makefile in conjunction
>> with selftests Makefile and lib.mk will handle all of that.
>>
>> Sounds like there is a misunderstanding that bpf maintainer/developer
>> workflow will have to change to support install. That is not the case.
>> The reason kselftest exists on the first place is to have common
>> framework to take care of build/run/install as a common layer so
>> individual test writers don't have to worry about these details
>> and write tests instead.
> 
> I don't think you understand the 'make install' implications.
> Not doing 'make install' means that all the Makefile changes that
> Yauheni is proposing will immediately bit rot.
> People are constantly adding new tests and changing makefile.
> 'make install' _will_ be broken instantly unless _humans_ incorporate
> it in their patch development process and maintainer incorporate
> that step into their workflow as well.
> 

I don't think so. I think if you want to work with us on this, we can
find a way. bpf isn't such a unique test that adding install will break
it.

> Ohh, but don't worry about this broken 'make install' is not an answer.
> It's broken now and I don't want to see patches that move it
> from one broken state into another broken state and at the same time
> add complexity to it.
> 

Well! What you are saying "I don;t want to collaborate to find a
solution to the problem".

> That's very different from 'make install' doing 'cp -r' of the whole dir.
> In such case the chances of it going stale and broken are much lower.
> 
>>>> - be able to build and run existing tests without breaking the test
>>>>     build when new tests are added that have hard dependency on new
>>>>     versions of tools (llvm etc.). This isn't such a novel idea. We
>>>>     don't break kernel builds every single release and even when we
>>>>     require newer compiler releases. Plan the new tests with the intent
>>>>     to not break existing users and add new tests at the same time.
>>>>     We use min rev and not bleeding edge as the requirement for kernel
>>>>     build.
>>>
>>> 'existing users'?
>>
>> I said existing tests not users. When you add new bpf tests, existing
>> tests should continue to build and run without dependency on new revs
>> of llvm.
> 
> Who said that existing test stop building with new llvm ?
> Please check your facts.
> 

I am basing this on a previous discussion on this topic. bpf test(s)
that build stop building and the solution always has been upgrade
to new tool chain. If this changed now and there is no hard tie to
bpf test building and llvm release, great.

>> If bpf test doesn't build and/or installed, it won't run
>> on test rings that qualify stable/next/main releases.
> 
> That's the case today and I prefer to keep it this way.

Why is that? Are you making a decision for the rest of the kernel
with this approach? If bpf doesn't get tested in stable test rings,
this isn't bpf problem alone. This is a kernel release issue.

> stable folks ignored our recommendations on how selftests/bpf should
> be run, so please don't come up with your ways of doing it and
> complain that something doesn't work.
> 

If you want to talk about history, bpf test was added in Oct 2016 and
by then kselftest was in use with its run_tests and install support.
So it is misleading to suggest that the framework came after the bpf
test. bpf test was added to existing kselftest framework used  by all
other tests. Expecting the existing framework to change and adapt to
a new test isn't reasonable.

When a new test is added, it has to work within the framework. Framework
can be improved and changed, however not the way you are attempting to
do in this thread. You can do this like others in the community to do
by making changes and improvements.

Sadly, it doesn't appear we are getting anywhere productive with this
thread. :(


thanks,
-- Shuah

Re: [PATCH] selftests/bpf: split -extras target to -static and -gen

[Shuah Khan]

On 5/28/20 2:09 PM, Shuah Khan wrote:
> On 5/28/20 1:18 PM, Alexei Starovoitov wrote:
>> On Thu, May 28, 2020 at 12:59:06PM -0600, Shuah Khan wrote:
>>> On 5/28/20 12:15 PM, Alexei Starovoitov wrote:
>>>> On Thu, May 28, 2020 at 11:07:09AM -0600, Shuah Khan wrote:
>>>>> On 5/28/20 10:14 AM, Alexei Starovoitov wrote:
>>>>>> On Thu, May 28, 2020 at 12:56:31PM +0200, Greg KH wrote:
>>>>>>> On Thu, May 28, 2020 at 10:05:57AM +0200, Jiri Benc wrote:
>>>>>>>> On Wed, 27 May 2020 15:23:13 -0700, Alexei Starovoitov wrote:
>>>>>>>>> I prefer to keep selftests/bpf install broken.
>>>>>>>>> This forced marriage between kselftests and selftests/bpf
>>>>>>>>> never worked well. I think it's a time to free them up from 
>>>>>>>>> each other.
>>>>>>>>
>>>>>>>> Alexei, it would be great if you could cooperate with other people
>>>>>>>> instead of pushing your own way. The selftests infrastructure 
>>>>>>>> was put
>>>>>>>> to the kernel to have one place for testing. Inventing yet 
>>>>>>>> another way
>>>>>>>> to add tests does not help anyone. You don't own the kernel. We're
>>>>>>>> community, we should cooperate.
>>>>>>>
>>>>>>> I agree, we rely on the infrastructure of the kselftests 
>>>>>>> framework so
>>>>>>> that testing systems do not have to create "custom" frameworks to 
>>>>>>> handle
>>>>>>> all of the individual variants that could easily crop up here.
>>>>>>>
>>>>>>> Let's keep it easy for people to run and use these tests, to not 
>>>>>>> do so
>>>>>>> is to ensure that they are not used, which is the exact opposite 
>>>>>>> goal of
>>>>>>> creating tests.
>>>>>>
>>>>>> Greg,
>>>>>>
>>>>>> It is easy for people (bpf developers) to run and use the tests.
>>>>>> Every developer runs them before submitting patches.
>>>>>> New tests is a hard requirement for any new features.
>>>>>> Maintainers run them for every push.
>>>>>>
>>>>>> What I was and will push back hard is when other people (not bpf 
>>>>>> developers)
>>>>>> come back with an excuse that some CI system has a hard time 
>>>>>> running these
>>>>>> tests. It's the problem of weak CI. That CI needs to be fixed. Not 
>>>>>> the tests.
>>>>>> The example of this is that we already have github/libbpf CI that 
>>>>>> runs
>>>>>> selftests/bpf just fine. Anyone who wants to do another CI are 
>>>>>> welcome to copy
>>>>>> paste what already works instead of burdening people (bpf 
>>>>>> developers) who run
>>>>>> and use existing tests. I frankly have no sympathy to folks who 
>>>>>> put their own
>>>>>> interest of their CI development in front of bpf community of 
>>>>>> developers.
>>>>>> The main job of CI is to help developers and maintainers.
>>>>>> Where helping means to not impose new dumb rules on developers 
>>>>>> because CI
>>>>>> framework is dumb. Fix CI instead.
>>>>>>
>>>>>
>>>>> Here is what CI users are requesting:
>>>>>
>>>>> - ability to install bpf test with other selftests using kselftest
>>>>>     install. The common framework is in place and with minor changes
>>>>>     to bpf test Makefile, we can make this happen. Others and myself
>>>>>     are willing to work on this, so we can get bpf test coverage in
>>>>>     test rings.
>>>>
>>>> so you're saying that bpf maintainers and all bpf developers now
>>>> would need to incorporate new 'make install' step to their workflow
>>>> because some unknown CI system that is not even functional decided
>>>> to do 'make install' ?
>>>> That's exactly my point about selfish CI developers who put their
>>>> needs in front of bpf community of developers.
>>>>
>>>
>>> There is no need change bpf maintainer and developer workflow. You
>>> don't have to use install option. Kselftest framework doesn't
>>> require a specific workflow and you can:
>>>
>>> 1. Build and run your tests from bpf directory if you choose to
>>> 2. Install to run on different target.
>>>
>>> Adding install install option requires a change to bpf Makefile
>>> only to copy test that are built to install directory.
>>>
>>> make kselftest-install from the main kernel Makefile in conjunction
>>> with selftests Makefile and lib.mk will handle all of that.
>>>
>>> Sounds like there is a misunderstanding that bpf maintainer/developer
>>> workflow will have to change to support install. That is not the case.
>>> The reason kselftest exists on the first place is to have common
>>> framework to take care of build/run/install as a common layer so
>>> individual test writers don't have to worry about these details
>>> and write tests instead.
>>
>> I don't think you understand the 'make install' implications.
>> Not doing 'make install' means that all the Makefile changes that
>> Yauheni is proposing will immediately bit rot.
>> People are constantly adding new tests and changing makefile.
>> 'make install' _will_ be broken instantly unless _humans_ incorporate
>> it in their patch development process and maintainer incorporate
>> that step into their workflow as well.
>>
> 
> I don't think so. I think if you want to work with us on this, we can
> find a way. bpf isn't such a unique test that adding install will break
> it.
> 
>> Ohh, but don't worry about this broken 'make install' is not an answer.
>> It's broken now and I don't want to see patches that move it
>> from one broken state into another broken state and at the same time
>> add complexity to it.
>>
> 
> Well! What you are saying "I don;t want to collaborate to find a
> solution to the problem".
> 
>> That's very different from 'make install' doing 'cp -r' of the whole dir.
>> In such case the chances of it going stale and broken are much lower.
>>
>>>>> - be able to build and run existing tests without breaking the test
>>>>>     build when new tests are added that have hard dependency on new
>>>>>     versions of tools (llvm etc.). This isn't such a novel idea. We
>>>>>     don't break kernel builds every single release and even when we
>>>>>     require newer compiler releases. Plan the new tests with the 
>>>>> intent
>>>>>     to not break existing users and add new tests at the same time.
>>>>>     We use min rev and not bleeding edge as the requirement for kernel
>>>>>     build.
>>>>
>>>> 'existing users'?
>>>
>>> I said existing tests not users. When you add new bpf tests, existing
>>> tests should continue to build and run without dependency on new revs
>>> of llvm.
>>
>> Who said that existing test stop building with new llvm ?
>> Please check your facts.
>>
> 
> I am basing this on a previous discussion on this topic. bpf test(s)
> that build stop building and the solution always has been upgrade
> to new tool chain. If this changed now and there is no hard tie to
> bpf test building and llvm release, great.
> 
>>> If bpf test doesn't build and/or installed, it won't run
>>> on test rings that qualify stable/next/main releases.
>>
>> That's the case today and I prefer to keep it this way.
> 
> Why is that? Are you making a decision for the rest of the kernel
> with this approach? If bpf doesn't get tested in stable test rings,
> this isn't bpf problem alone. This is a kernel release issue.
> 
>> stable folks ignored our recommendations on how selftests/bpf should
>> be run, so please don't come up with your ways of doing it and
>> complain that something doesn't work.
>>
> 
> If you want to talk about history, bpf test was added in Oct 2016 and
> by then kselftest was in use with its run_tests and install support.
> So it is misleading to suggest that the framework came after the bpf
> test. bpf test was added to existing kselftest framework used  by all
> other tests. Expecting the existing framework to change and adapt to
> a new test isn't reasonable.
> 
> When a new test is added, it has to work within the framework. Framework
> can be improved and changed, however not the way you are attempting to
> do in this thread. You can do this like others in the community to do
> by making changes and improvements.
> 

In any case, kselftest framework lets you override INSTALL_RULE and
define your own. Same is true for other rules: RUN_TESTS, EMIT_TESTS,
CLEAN, INSTALL

bpf does this already for CLEAN.

# override lib.mk's default rules
OVERRIDE_TARGETS := 1
override define CLEAN

If generic install rule doesn't work for you do override so bpf so
users can install it.

thanks,
-- Shuah