[PATCH RESEND v3 0/2] selftests: fix prepending $(OUTPUT) to $(TEST_PROGS)

[PATCH RESEND v3 0/2] selftests: fix prepending $(OUTPUT) to $(TEST_PROGS)

[Ilya Leoshkevich]

Hello,

Is there anything blocking this from getting merged?


This patch series fixes the following problem:

linux# make kselftest TARGETS=bpf O=/mnt/linux-build
# selftests: bpf: test_libbpf.sh
# ./test_libbpf.sh: line 23: ./test_libbpf_open: No such file or directory
# test_libbpf: failed at file test_l4lb.o
# selftests: test_libbpf [FAILED]

Patch 1 appends / to $(OUTPUT) in order to make it more uniform with the
rest of the tree.

Patch 2 fixes the problem by prepending $(OUTPUT) to all members of
$(TEST_PROGS).

v1->v2:
- Append / to $(OUTPUT).
- Use $(addprefix) instead of $(foreach).

v2->v3:
- Split the patch in two.
- Improve the commit message.

Ilya Leoshkevich (2):
  selftests: append / to $(OUTPUT)
  selftests: fix prepending $(OUTPUT) to $(TEST_PROGS)

 tools/testing/selftests/Makefile | 16 ++++++++--------
 tools/testing/selftests/lib.mk   |  3 ++-
 2 files changed, 10 insertions(+), 9 deletions(-)

-- 
2.23.0

[PATCH RESEND v3 1/2] selftests: append / to $(OUTPUT)

[Ilya Leoshkevich]

$(OUTPUT) is assumed to end with a / everywhere else in the tree, make
this the case for kselftest as well.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/Makefile | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 4cdbae6f4e61..f935d4738041 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -143,31 +143,31 @@ all: khdr
 	@for TARGET in $(TARGETS); do		\
 		BUILD_TARGET=$$BUILD/$$TARGET;	\
 		mkdir $$BUILD_TARGET  -p;	\
-		$(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET;\
+		$(MAKE) OUTPUT=$$BUILD_TARGET/ -C $$TARGET;\
 	done;
 
 run_tests: all
 	@for TARGET in $(TARGETS); do \
 		BUILD_TARGET=$$BUILD/$$TARGET;	\
-		$(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET run_tests;\
+		$(MAKE) OUTPUT=$$BUILD_TARGET/ -C $$TARGET run_tests;\
 	done;
 
 hotplug:
 	@for TARGET in $(TARGETS_HOTPLUG); do \
 		BUILD_TARGET=$$BUILD/$$TARGET;	\
-		$(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET;\
+		$(MAKE) OUTPUT=$$BUILD_TARGET/ -C $$TARGET;\
 	done;
 
 run_hotplug: hotplug
 	@for TARGET in $(TARGETS_HOTPLUG); do \
 		BUILD_TARGET=$$BUILD/$$TARGET;	\
-		$(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET run_full_test;\
+		$(MAKE) OUTPUT=$$BUILD_TARGET/ -C $$TARGET run_full_test;\
 	done;
 
 clean_hotplug:
 	@for TARGET in $(TARGETS_HOTPLUG); do \
 		BUILD_TARGET=$$BUILD/$$TARGET;	\
-		$(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET clean;\
+		$(MAKE) OUTPUT=$$BUILD_TARGET/ -C $$TARGET clean;\
 	done;
 
 run_pstore_crash:
@@ -194,7 +194,7 @@ ifdef INSTALL_PATH
 	install -m 744 kselftest/prefix.pl $(INSTALL_PATH)/kselftest/
 	@for TARGET in $(TARGETS); do \
 		BUILD_TARGET=$$BUILD/$$TARGET;	\
-		$(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET INSTALL_PATH=$(INSTALL_PATH)/$$TARGET install; \
+		$(MAKE) OUTPUT=$$BUILD_TARGET/ -C $$TARGET INSTALL_PATH=$(INSTALL_PATH)/$$TARGET install; \
 	done;
 
 	@# Ask all targets to emit their test scripts
@@ -218,7 +218,7 @@ ifdef INSTALL_PATH
 		echo "cd $$TARGET" >> $(ALL_SCRIPT); \
 		echo -n "run_many" >> $(ALL_SCRIPT); \
 		echo -n "Emit Tests for $$TARGET\n"; \
-		$(MAKE) -s --no-print-directory OUTPUT=$$BUILD_TARGET -C $$TARGET emit_tests >> $(ALL_SCRIPT); \
+		$(MAKE) -s --no-print-directory OUTPUT=$$BUILD_TARGET/ -C $$TARGET emit_tests >> $(ALL_SCRIPT); \
 		echo "" >> $(ALL_SCRIPT);	    \
 		echo "cd \$$ROOT" >> $(ALL_SCRIPT); \
 	done;
@@ -231,7 +231,7 @@ endif
 clean:
 	@for TARGET in $(TARGETS); do \
 		BUILD_TARGET=$$BUILD/$$TARGET;	\
-		$(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET clean;\
+		$(MAKE) OUTPUT=$$BUILD_TARGET/ -C $$TARGET clean;\
 	done;
 
 .PHONY: khdr all run_tests hotplug run_hotplug clean_hotplug run_pstore_crash install clean
-- 
2.23.0

[PATCH RESEND v3 2/2] selftests: fix prepending $(OUTPUT) to $(TEST_PROGS)

[Ilya Leoshkevich]

Currently the following command fails:

    linux# make kselftest TARGETS=bpf O=/mnt/linux-build
    # selftests: bpf: test_libbpf.sh
    # ./test_libbpf.sh: line 23: ./test_libbpf_open: No such file or directory
    # test_libbpf: failed at file test_l4lb.o
    # selftests: test_libbpf [FAILED]

The current logic prepends $(OUTPUT) only to the first member of
$(TEST_PROGS). After that, run_one() does

   cd `dirname $TEST`

For all tests except the first one, `dirname $TEST` is ., which means
they cannot access the files generated in $(OUTPUT).

Fix by using $(addprefix) to prepend $(OUTPUT) to each member of
$(TEST_PROGS).

Fixes: 1a940687e424 ("selftests: lib.mk: copy test scripts and test files for make O=dir run")
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tools/testing/selftests/lib.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 1c8a1963d03f..27a97a1b4e9e 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -75,7 +75,8 @@ ifdef building_out_of_srctree
 		@rsync -aq $(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES) $(OUTPUT)
 	fi
 	@if [ "X$(TEST_PROGS)" != "X" ]; then
-		$(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) $(OUTPUT)/$(TEST_PROGS))
+		$(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) \
+				  $(addprefix $(OUTPUT),$(TEST_PROGS)))
 	else
 		$(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS))
 	fi
-- 
2.23.0

Re: [PATCH RESEND v3 0/2] selftests: fix prepending $(OUTPUT) to $(TEST_PROGS)

[shuah]

On 10/24/19 6:13 AM, Ilya Leoshkevich wrote:
> Hello,
> 
> Is there anything blocking this from getting merged?
> 
> 

Please see below:

> This patch series fixes the following problem:
> 
> linux# make kselftest TARGETS=bpf O=/mnt/linux-build
> # selftests: bpf: test_libbpf.sh
> # ./test_libbpf.sh: line 23: ./test_libbpf_open: No such file or directory
> # test_libbpf: failed at file test_l4lb.o
> # selftests: test_libbpf [FAILED]
> 
> Patch 1 appends / to $(OUTPUT) in order to make it more uniform with the
> rest of the tree.
> 

It isn't clear what this fixes. In addition, this patch appends an extra
"/"

Excerpts from make kselftest-all with this patch applied:

linux_5.4/tools/testing/selftests/bpf//test_tcpnotify_user

I am not seeing any reason to take this patch.

> Patch 2 fixes the problem by prepending $(OUTPUT) to all members of
> $(TEST_PROGS).
> 
> v1->v2:
> - Append / to $(OUTPUT).
> - Use $(addprefix) instead of $(foreach).

I can't reproduce this problem - I ran

make kselftest O=/tmp/linux-build

This might be fixing the problem in bpf case. However, I don't think
this is the right fix.

I would like to see this problem reproduced on another test first.

thanks,
-- Shuah

Re: [PATCH RESEND v3 0/2] selftests: fix prepending $(OUTPUT) to $(TEST_PROGS)

[Ilya Leoshkevich]

> Am 07.11.2019 um 23:52 schrieb shuah <shuah@kernel.org>:
> 
> On 10/24/19 6:13 AM, Ilya Leoshkevich wrote:
>> Hello,
>> Is there anything blocking this from getting merged?
> 
> Please see below:
> 
>> This patch series fixes the following problem:
>> linux# make kselftest TARGETS=bpf O=/mnt/linux-build
>> # selftests: bpf: test_libbpf.sh
>> # ./test_libbpf.sh: line 23: ./test_libbpf_open: No such file or directory
>> # test_libbpf: failed at file test_l4lb.o
>> # selftests: test_libbpf [FAILED]
>> Patch 1 appends / to $(OUTPUT) in order to make it more uniform with the
>> rest of the tree.
> 
> It isn't clear what this fixes. In addition, this patch appends an extra
> "/"
> 
> Excerpts from make kselftest-all with this patch applied:
> 
> linux_5.4/tools/testing/selftests/bpf//test_tcpnotify_user
> 
> I am not seeing any reason to take this patch.

The goal of patch 1 was not to fix a practical problem, but rather make
kselftest code follow the same convention as the rest of the build code
in tools/, namely: that $(OUTPUT) is assumed to always end with /.
However, if this commonality is not valuable, I don't think I should
pursue this. So I'd like to withdraw patch 1.

>> Patch 2 fixes the problem by prepending $(OUTPUT) to all members of
>> $(TEST_PROGS).
>> v1->v2:
>> - Append / to $(OUTPUT).
>> - Use $(addprefix) instead of $(foreach).
> 
> I can't reproduce this problem - I ran
> 
> make kselftest O=/tmp/linux-build

May I ask you, how did you check the test results? I'm asking because
some test failures, including this one, may not affect make exit code.
So you need to grep for e.g. test_libbpf.sh in make output to see the
problem. For example, in my setup a lot of eBPF tests fail without this
patch, and yet:

[root@bpf linux]# make kselftest TARGETS=bpf O=/mnt/linux-build
<snip>
ok 28 selftests: bpf: test_kmod.sh
# selftests: bpf: test_libbpf.sh
# ./test_libbpf.sh: line 23: ./test_libbpf_open: No such file or directory
# test_libbpf: failed at file test_l4lb.o
# selftests: test_libbpf [FAILED]
not ok 29 selftests: bpf: test_libbpf.sh # exit=127
<snip>
make[1]: Leaving directory '/mnt/linux-build'

[root@bpf linux]# echo $?
0

> This might be fixing the problem in bpf case. However, I don't think
> this is the right fix.

It is weird in general that the existing code prepends $(OUTPUT) only to
the first test prog. The first test prog is by no means special. The
commit message of patch 2 explains in more detail how this discrepancy
causes a practical problem:

> The current logic prepends $(OUTPUT) only to the first member of
> $(TEST_PROGS). After that, run_one() does
>
>   cd `dirname $TEST`
>
> For all tests except the first one, `dirname $TEST` is ., which means
> they cannot access the files generated in $(OUTPUT).

Is there a problem with this reasoning?

> I would like to see this problem reproduced on another test first.

Best regards,
Ilya