* [PATCH RFC] selftests: do not use .ONESHELL
@ 2020-05-15 3:00 Yauheni Kaliuta
2020-05-15 8:28 ` Jiri Benc
0 siblings, 1 reply; 3+ messages in thread
From: Yauheni Kaliuta @ 2020-05-15 3:00 UTC (permalink / raw)
To: bpf; +Cc: Jiri Benc, Jiri Olsa, Shuah Khan
Using one shell for the whole recipe with long lists can cause
make[1]: execvp: /bin/sh: Argument list too long
with some shells. Triggered by commit 309b81f0fdc4 ("selftests/bpf:
Install generated test progs")
It requires to change the rule which rely on the one shell
behaviour (run_tests).
Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
Cc: Jiri Benc <jbenc@redhat.com>
Cc: Shuah Khan <shuah@kernel.org>
---
1) I'm wondering how commit c363eb48ada5 ("selftests: fix too long
argument") worked without the patch.
2) The code does not look working as expected for me:
2.1) "X$(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES)" != "X" is
always true sine the left part will be at least "X " (spaces);
2.2) according to manual in .ONESHELL case gmake checks only first
line for @, so @rsync is passed to the shell;
2.3) $(OUTPUT)/(TEST_PROGS) adds $(OUTPUT) only to the first prog;
Did I miss something?
---
tools/testing/selftests/lib.mk | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index b0556c752443..e9e5e33297cf 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -59,7 +59,6 @@ else
all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES)
endif
-.ONESHELL:
define RUN_TESTS
@BASE_DIR="$(selfdir)"; \
. $(selfdir)/kselftest/runner.sh; \
@@ -71,13 +70,13 @@ endef
run_tests: all
ifdef building_out_of_srctree
- @if [ "X$(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES)" != "X" ]; then
- @rsync -aq $(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES) $(OUTPUT)
+ @if [ "X$(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES)" != "X" ]; then \
+ 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))
- else
- $(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS))
+ @if [ "X$(TEST_PROGS)" != "X" ]; then \
+ $(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) $(OUTPUT)/$(TEST_PROGS)) ; \
+ else \
+ $(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS)); \
fi
else
$(call RUN_TESTS, $(TEST_GEN_PROGS) $(TEST_CUSTOM_PROGS) $(TEST_PROGS))
--
2.26.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH RFC] selftests: do not use .ONESHELL
2020-05-15 3:00 [PATCH RFC] selftests: do not use .ONESHELL Yauheni Kaliuta
@ 2020-05-15 8:28 ` Jiri Benc
2020-05-15 9:18 ` Yauheni Kaliuta
0 siblings, 1 reply; 3+ messages in thread
From: Jiri Benc @ 2020-05-15 8:28 UTC (permalink / raw)
To: Yauheni Kaliuta; +Cc: bpf, Jiri Olsa, Shuah Khan
On Fri, 15 May 2020 06:00:51 +0300, Yauheni Kaliuta wrote:
> 1) I'm wondering how commit c363eb48ada5 ("selftests: fix too long
> argument") worked without the patch.
I think it was because it reduced the list of files from three
replications to two. I did not notice the .ONESHELL; it also explains
the oddity that I saw with @ behavior.
With the .ONESHELL removed, we can further simplify INSTALL_SINGLE_RULE
by removing the @echo rsync and the at-sign before rsync.
> 2) The code does not look working as expected for me:
> 2.1) "X$(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES)" != "X" is
> always true sine the left part will be at least "X " (spaces);
> 2.2) according to manual in .ONESHELL case gmake checks only first
> line for @, so @rsync is passed to the shell;
> 2.3) $(OUTPUT)/(TEST_PROGS) adds $(OUTPUT) only to the first prog;
>
> Did I miss something?
I think you didn't miss anything and that you're right. Could you
submit a patch to remove the spaces? I can then submit a patch to
further simplify INSTALL_SINGLE_RULE if you don't want to do that, too.
Thanks!
Jiri
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH RFC] selftests: do not use .ONESHELL
2020-05-15 8:28 ` Jiri Benc
@ 2020-05-15 9:18 ` Yauheni Kaliuta
0 siblings, 0 replies; 3+ messages in thread
From: Yauheni Kaliuta @ 2020-05-15 9:18 UTC (permalink / raw)
To: Jiri Benc; +Cc: bpf, Jiri Olsa, Shuah Khan
Hi, Jiri!
>>>>> On Fri, 15 May 2020 10:28:41 +0200, Jiri Benc wrote:
> On Fri, 15 May 2020 06:00:51 +0300, Yauheni Kaliuta wrote:
>> 1) I'm wondering how commit c363eb48ada5 ("selftests: fix too long
>> argument") worked without the patch.
> I think it was because it reduced the list of files from three
> replications to two. I did not notice the .ONESHELL; it also
> explains the oddity that I saw with @ behavior.
> With the .ONESHELL removed, we can further simplify
> INSTALL_SINGLE_RULE by removing the @echo rsync and the
> at-sign before rsync.
Yeah.
>> 2) The code does not look working as expected for me:
>> 2.1) "X$(TEST_PROGS) $(TEST_PROGS_EXTENDED) $(TEST_FILES)" != "X" is
>> always true sine the left part will be at least "X " (spaces);
>> 2.2) according to manual in .ONESHELL case gmake checks only first
>> line for @, so @rsync is passed to the shell;
Actully, when I checked it in the `if` branch, @ worked as
expected, sounds strange for me. But well, without .ONESHELL it
will go away.
>> 2.3) $(OUTPUT)/(TEST_PROGS) adds $(OUTPUT) only to the first prog;
>>
>> Did I miss something?
> I think you didn't miss anything and that you're right. Could
> you submit a patch to remove the spaces? I can then submit a
> patch to further simplify INSTALL_SINGLE_RULE if you don't
> want to do that, too.
Just allow rsync command echoing, right? I can do it, no problem.
And RUN_TESTS' `@` does not work in the `if` branch, so the patch
should be fixed.
Also I noticed possible issue related to my previous patch:
lib.mk does TEST_GEN_FILES := $(patsubst %,$(OUTPUT)/%,$(TEST_GEN_FILES))
(Notice := ). But it's included (at least in the bpf/Makefile)
before TEST_GEN_FILES is constructed during rules generation so
basically it's skipped. BUT in the generated rules $(OUTPUT) is
taken into account. Sort of inconsistency. Did I miss something?
If any of the lists grows too much again the next modification in
my mind is to do $(foreach ...) on the lists and handle them
file-by-file.
Thanks for the review!
--
WBR,
Yauheni Kaliuta
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-05-15 9:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-15 3:00 [PATCH RFC] selftests: do not use .ONESHELL Yauheni Kaliuta
2020-05-15 8:28 ` Jiri Benc
2020-05-15 9:18 ` Yauheni Kaliuta
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.