diff mbox series

[bpf-next,v4,2/3] selftests/bpf: Make install target copy test_progs extra files

Message ID 20240129070916.1639008-3-bjorn@kernel.org (mailing list archive)
State New
Headers show
Series Fix make install target for BPF selftests | expand

Commit Message

Björn Töpel Jan. 29, 2024, 7:09 a.m. UTC 
From: Björn Töpel <bjorn@rivosinc.com>

Currently, "make install" does not install the required test_progs
"extra files" (e.g. kernel modules, helper shell scripts, etc.) for
the BPF machine flavors (e.g. cpuv4).

Add the missing "extra files" dependencies to rsync, called from the
install target.

Unfortunately, kselftest does not use bash as the default shell, so
the globbering is limited. Blindly enabling "SHELL:=/bin/bash" for the
Makefile breaks in other places. Workaround by explicitly call
"/bin/bash" to expand the file globbing.

Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
---
 tools/testing/selftests/bpf/Makefile | 29 +++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

Comments

Andrii Nakryiko Jan. 30, 2024, 12:59 a.m. UTC | #1 
On Sun, Jan 28, 2024 at 11:09 PM Björn Töpel <bjorn@kernel.org> wrote:
>
> From: Björn Töpel <bjorn@rivosinc.com>
>
> Currently, "make install" does not install the required test_progs
> "extra files" (e.g. kernel modules, helper shell scripts, etc.) for
> the BPF machine flavors (e.g. cpuv4).
>
> Add the missing "extra files" dependencies to rsync, called from the
> install target.
>
> Unfortunately, kselftest does not use bash as the default shell, so
> the globbering is limited. Blindly enabling "SHELL:=/bin/bash" for the
> Makefile breaks in other places. Workaround by explicitly call
> "/bin/bash" to expand the file globbing.
>
> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
> ---
>  tools/testing/selftests/bpf/Makefile | 29 +++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 830a34f0aa37..c3c5b85f7dae 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -605,14 +605,15 @@ TRUNNER_EXTRA_SOURCES := test_progs.c             \
>                          json_writer.c          \
>                          flow_dissector_load.h  \
>                          ip_check_defrag_frags.h
> -TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \
> -                      $(OUTPUT)/liburandom_read.so                     \
> -                      $(OUTPUT)/xdp_synproxy                           \
> -                      $(OUTPUT)/sign-file                              \
> -                      $(OUTPUT)/uprobe_multi                           \
> -                      ima_setup.sh                                     \
> -                      verify_sig_setup.sh                              \
> -                      $(wildcard progs/btf_dump_test_case_*.c)
> +TRUNNER_PROGS_EXTRA_FILES:= $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko    \
> +                           $(OUTPUT)/liburandom_read.so                        \
> +                           $(OUTPUT)/xdp_synproxy                              \
> +                           $(OUTPUT)/sign-file                                 \
> +                           $(OUTPUT)/uprobe_multi                              \
> +                           ima_setup.sh                                        \
> +                           verify_sig_setup.sh                                 \
> +                           $(wildcard progs/btf_dump_test_case_*.c)
> +TRUNNER_EXTRA_FILES := $(TRUNNER_PROGS_EXTRA_FILES)
>  TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
>  TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -DENABLE_ATOMICS_TESTS
>  $(eval $(call DEFINE_TEST_RUNNER,test_progs))
> @@ -740,11 +741,17 @@ EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)    \
>  # Delete partially updated (corrupted) files on error
>  .DELETE_ON_ERROR:
>
> +space := $(subst ,, )
> +comma := ,
> +EXTRA_FILES_GLOB := {$(subst $(space),$(comma),$(notdir $(TRUNNER_PROGS_EXTRA_FILES)))}
>  DEFAULT_INSTALL_RULE := $(INSTALL_RULE)
>  override define INSTALL_RULE
>         $(DEFAULT_INSTALL_RULE)
> -       @for DIR in $(TEST_INST_SUBDIRS); do              \
> -               mkdir -p $(INSTALL_PATH)/$$DIR;   \
> -               rsync -a $(OUTPUT)/$$DIR/*.bpf.o $(INSTALL_PATH)/$$DIR;\
> +       @for DIR in $(TEST_INST_SUBDIRS); do                                            \
> +               mkdir -p $(INSTALL_PATH)/$$DIR;                                         \
> +               rsync -a $(OUTPUT)/$$DIR/*.bpf.o $(INSTALL_PATH)/$$DIR;                 \
> +               rsync -a --copy-unsafe-links                                            \
> +                       $$(/bin/bash -c "echo $(OUTPUT)/$$DIR/$(EXTRA_FILES_GLOB)")     \

this feels quite hacky... have you tried using $(foreach) to go over
each element of TRUNNER_PROGS_EXTRA_FILES and append $(OUTPUT)/$$DIR/
to each one? Hopefully that will allow us to get rid of space and
comma hacks?

I'm also wondering if it would be ok to just combine two rsync calls
into one, so that $(INSTALL_PATH)/$$DIR is specified once?


> +                       $(INSTALL_PATH)/$$DIR;                                          \
>         done
>  endef
> --
> 2.40.1
>
Björn Töpel Jan. 30, 2024, 6:24 a.m. UTC | #2 
Andrii Nakryiko <andrii.nakryiko@gmail.com> writes:

> On Sun, Jan 28, 2024 at 11:09 PM Björn Töpel <bjorn@kernel.org> wrote:
>>
>> From: Björn Töpel <bjorn@rivosinc.com>
>>
>> Currently, "make install" does not install the required test_progs
>> "extra files" (e.g. kernel modules, helper shell scripts, etc.) for
>> the BPF machine flavors (e.g. cpuv4).
>>
>> Add the missing "extra files" dependencies to rsync, called from the
>> install target.
>>
>> Unfortunately, kselftest does not use bash as the default shell, so
>> the globbering is limited. Blindly enabling "SHELL:=/bin/bash" for the
>> Makefile breaks in other places. Workaround by explicitly call
>> "/bin/bash" to expand the file globbing.
>>
>> Signed-off-by: Björn Töpel <bjorn@rivosinc.com>
>> ---
>>  tools/testing/selftests/bpf/Makefile | 29 +++++++++++++++++-----------
>>  1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>> index 830a34f0aa37..c3c5b85f7dae 100644
>> --- a/tools/testing/selftests/bpf/Makefile
>> +++ b/tools/testing/selftests/bpf/Makefile
>> @@ -605,14 +605,15 @@ TRUNNER_EXTRA_SOURCES := test_progs.c             \
>>                          json_writer.c          \
>>                          flow_dissector_load.h  \
>>                          ip_check_defrag_frags.h
>> -TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko \
>> -                      $(OUTPUT)/liburandom_read.so                     \
>> -                      $(OUTPUT)/xdp_synproxy                           \
>> -                      $(OUTPUT)/sign-file                              \
>> -                      $(OUTPUT)/uprobe_multi                           \
>> -                      ima_setup.sh                                     \
>> -                      verify_sig_setup.sh                              \
>> -                      $(wildcard progs/btf_dump_test_case_*.c)
>> +TRUNNER_PROGS_EXTRA_FILES:= $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko    \
>> +                           $(OUTPUT)/liburandom_read.so                        \
>> +                           $(OUTPUT)/xdp_synproxy                              \
>> +                           $(OUTPUT)/sign-file                                 \
>> +                           $(OUTPUT)/uprobe_multi                              \
>> +                           ima_setup.sh                                        \
>> +                           verify_sig_setup.sh                                 \
>> +                           $(wildcard progs/btf_dump_test_case_*.c)
>> +TRUNNER_EXTRA_FILES := $(TRUNNER_PROGS_EXTRA_FILES)
>>  TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
>>  TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -DENABLE_ATOMICS_TESTS
>>  $(eval $(call DEFINE_TEST_RUNNER,test_progs))
>> @@ -740,11 +741,17 @@ EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)    \
>>  # Delete partially updated (corrupted) files on error
>>  .DELETE_ON_ERROR:
>>
>> +space := $(subst ,, )
>> +comma := ,
>> +EXTRA_FILES_GLOB := {$(subst $(space),$(comma),$(notdir $(TRUNNER_PROGS_EXTRA_FILES)))}
>>  DEFAULT_INSTALL_RULE := $(INSTALL_RULE)
>>  override define INSTALL_RULE
>>         $(DEFAULT_INSTALL_RULE)
>> -       @for DIR in $(TEST_INST_SUBDIRS); do              \
>> -               mkdir -p $(INSTALL_PATH)/$$DIR;   \
>> -               rsync -a $(OUTPUT)/$$DIR/*.bpf.o $(INSTALL_PATH)/$$DIR;\
>> +       @for DIR in $(TEST_INST_SUBDIRS); do                                            \
>> +               mkdir -p $(INSTALL_PATH)/$$DIR;                                         \
>> +               rsync -a $(OUTPUT)/$$DIR/*.bpf.o $(INSTALL_PATH)/$$DIR;                 \
>> +               rsync -a --copy-unsafe-links                                            \
>> +                       $$(/bin/bash -c "echo $(OUTPUT)/$$DIR/$(EXTRA_FILES_GLOB)")     \
>
> this feels quite hacky... have you tried using $(foreach) to go over
> each element of TRUNNER_PROGS_EXTRA_FILES and append $(OUTPUT)/$$DIR/
> to each one? Hopefully that will allow us to get rid of space and
> comma hacks?

I haven't -- I'll try that out. Another ugliness is that *.bpf.o and
bpftool has to be explicitly specified, which hints of unrobustness.

> I'm also wondering if it would be ok to just combine two rsync calls
> into one, so that $(INSTALL_PATH)/$$DIR is specified once?

FWIW, I fold the two calls in patch 3. Regardless; I'll try to make the
whole series more palatable.


Björn
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 830a34f0aa37..c3c5b85f7dae 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -605,14 +605,15 @@  TRUNNER_EXTRA_SOURCES := test_progs.c		\
 			 json_writer.c 		\
 			 flow_dissector_load.h	\
 			 ip_check_defrag_frags.h
-TRUNNER_EXTRA_FILES := $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko	\
-		       $(OUTPUT)/liburandom_read.so			\
-		       $(OUTPUT)/xdp_synproxy				\
-		       $(OUTPUT)/sign-file				\
-		       $(OUTPUT)/uprobe_multi				\
-		       ima_setup.sh 					\
-		       verify_sig_setup.sh				\
-		       $(wildcard progs/btf_dump_test_case_*.c)
+TRUNNER_PROGS_EXTRA_FILES:= $(OUTPUT)/urandom_read $(OUTPUT)/bpf_testmod.ko	\
+			    $(OUTPUT)/liburandom_read.so			\
+			    $(OUTPUT)/xdp_synproxy				\
+			    $(OUTPUT)/sign-file					\
+			    $(OUTPUT)/uprobe_multi				\
+			    ima_setup.sh					\
+			    verify_sig_setup.sh					\
+			    $(wildcard progs/btf_dump_test_case_*.c)
+TRUNNER_EXTRA_FILES := $(TRUNNER_PROGS_EXTRA_FILES)
 TRUNNER_BPF_BUILD_RULE := CLANG_BPF_BUILD_RULE
 TRUNNER_BPF_CFLAGS := $(BPF_CFLAGS) $(CLANG_CFLAGS) -DENABLE_ATOMICS_TESTS
 $(eval $(call DEFINE_TEST_RUNNER,test_progs))
@@ -740,11 +741,17 @@  EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(SCRATCH_DIR) $(HOST_SCRATCH_DIR)	\
 # Delete partially updated (corrupted) files on error
 .DELETE_ON_ERROR:
 
+space := $(subst ,, )
+comma := ,
+EXTRA_FILES_GLOB := {$(subst $(space),$(comma),$(notdir $(TRUNNER_PROGS_EXTRA_FILES)))}
 DEFAULT_INSTALL_RULE := $(INSTALL_RULE)
 override define INSTALL_RULE
 	$(DEFAULT_INSTALL_RULE)
-	@for DIR in $(TEST_INST_SUBDIRS); do		  \
-		mkdir -p $(INSTALL_PATH)/$$DIR;   \
-		rsync -a $(OUTPUT)/$$DIR/*.bpf.o $(INSTALL_PATH)/$$DIR;\
+	@for DIR in $(TEST_INST_SUBDIRS); do						\
+		mkdir -p $(INSTALL_PATH)/$$DIR;						\
+		rsync -a $(OUTPUT)/$$DIR/*.bpf.o $(INSTALL_PATH)/$$DIR;			\
+		rsync -a --copy-unsafe-links						\
+			$$(/bin/bash -c "echo $(OUTPUT)/$$DIR/$(EXTRA_FILES_GLOB)")	\
+			$(INSTALL_PATH)/$$DIR;						\
 	done
 endef