Message ID | 20200515030051.60148-1-yauheni.kaliuta@redhat.com |
---|---|
State | RFC |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [RFC] selftests: do not use .ONESHELL | expand |
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
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!
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))
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(-)