diff mbox series

[v2,02/14] selftests/nolibc: add macros to enhance maintainability

Message ID 0415392c9c2b0a7249563abd79599a475019b508.1689759351.git.falcon@tinylab.org
State New
Headers show
Series selftests/nolibc: add minimal kernel config support - part1 | expand

Commit Message

Zhangjin Wu July 19, 2023, 1:19 p.m. UTC
The kernel targets share the same kernel make operations, the same
.config file, the same kernel image, add MAKE_KERNEL, KERNEL_CONFIG and
KERNEL_IMAGE for them.

Many targets share the same logging related settings, let's add common
variables RUN_OUT, LOG_OUT and REPORT_RUN_OUT for them.

The qemu run/rerun targets share the same qemu system run command, add
QEMU_SYSTEM_RUN for them.

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/testing/selftests/nolibc/Makefile | 41 ++++++++++++++++---------
 1 file changed, 27 insertions(+), 14 deletions(-)

Comments

Willy Tarreau July 22, 2023, 12:20 p.m. UTC | #1
On Wed, Jul 19, 2023 at 09:19:10PM +0800, Zhangjin Wu wrote:
> The kernel targets share the same kernel make operations, the same
> .config file, the same kernel image, add MAKE_KERNEL, KERNEL_CONFIG and
> KERNEL_IMAGE for them.
> 
> Many targets share the same logging related settings, let's add common
> variables RUN_OUT, LOG_OUT and REPORT_RUN_OUT for them.
> 
> The qemu run/rerun targets share the same qemu system run command, add
> QEMU_SYSTEM_RUN for them.
> 
> Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> ---
>  tools/testing/selftests/nolibc/Makefile | 41 ++++++++++++++++---------
>  1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> index 0cd17de2062c..8c531518bb9f 100644
> --- a/tools/testing/selftests/nolibc/Makefile
> +++ b/tools/testing/selftests/nolibc/Makefile
> @@ -166,45 +166,58 @@ endif
>  libc-test: nolibc-test.c
>  	$(QUIET_CC)$(CC) -o $@ $<
>  
> +# common macros for logging
> +RUN_OUT = $(CURDIR)/run.out
> +LOG_OUT = > "$(RUN_OUT)"
> +REPORT_RUN_OUT = $(REPORT) "$(RUN_OUT)"
> +
>  # local libc-test
>  run-libc-test: libc-test
> -	$(Q)./libc-test > "$(CURDIR)/run.out" || :
> -	$(Q)$(REPORT) $(CURDIR)/run.out
> +	$(Q)./libc-test $(LOG_OUT) || :
> +	$(Q)$(REPORT_RUN_OUT)

Sorry but I don't find that this improves maintainability, quite the
opposite in fact. One reason is that you never visually expect that
some shell indirection delimiters are hidden in a macro that seems
to only convey a name. Sure there are valid use cases for this, but
I think that here it's just adding too much abstraction and it makes
it quite hard to unfold all of this mentally.

Please try to keep the number of macros to the minimum that needs to
be replaced or forced by the user. Here I'm not seeing a compelling
reason for a user to want to force LOG_OUT to something else. Also
makefile macros are generally a pain to debug, which is another
reason for not putting too many of them.

I noticed that your next patch changes LOG_OUT to tee, it could have
done it everywhere and wouldn't affect readability as much.

Thanks,
Willy
Zhangjin Wu July 25, 2023, 12:37 p.m. UTC | #2
Hi, Willy

> On Wed, Jul 19, 2023 at 09:19:10PM +0800, Zhangjin Wu wrote:
> > The kernel targets share the same kernel make operations, the same
> > .config file, the same kernel image, add MAKE_KERNEL, KERNEL_CONFIG and
> > KERNEL_IMAGE for them.
> > 
> > Many targets share the same logging related settings, let's add common
> > variables RUN_OUT, LOG_OUT and REPORT_RUN_OUT for them.
> > 
> > The qemu run/rerun targets share the same qemu system run command, add
> > QEMU_SYSTEM_RUN for them.
> > 
> > Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
> > ---
> >  tools/testing/selftests/nolibc/Makefile | 41 ++++++++++++++++---------
> >  1 file changed, 27 insertions(+), 14 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
> > index 0cd17de2062c..8c531518bb9f 100644
> > --- a/tools/testing/selftests/nolibc/Makefile
> > +++ b/tools/testing/selftests/nolibc/Makefile
> > @@ -166,45 +166,58 @@ endif
> >  libc-test: nolibc-test.c
> >  	$(QUIET_CC)$(CC) -o $@ $<
> >  
> > +# common macros for logging
> > +RUN_OUT = $(CURDIR)/run.out
> > +LOG_OUT = > "$(RUN_OUT)"
> > +REPORT_RUN_OUT = $(REPORT) "$(RUN_OUT)"
> > +
> >  # local libc-test
> >  run-libc-test: libc-test
> > -	$(Q)./libc-test > "$(CURDIR)/run.out" || :
> > -	$(Q)$(REPORT) $(CURDIR)/run.out
> > +	$(Q)./libc-test $(LOG_OUT) || :
> > +	$(Q)$(REPORT_RUN_OUT)
> 
> Sorry but I don't find that this improves maintainability, quite the
> opposite in fact. One reason is that you never visually expect that
> some shell indirection delimiters are hidden in a macro that seems
> to only convey a name. Sure there are valid use cases for this, but
> I think that here it's just adding too much abstraction and it makes
> it quite hard to unfold all of this mentally.
>

Ok, will reserve less ones as possible as we can.

- RUN_OUT is required for architecture specific output
- REPORT_RUN_OUT is not necessary, will remove it

> Please try to keep the number of macros to the minimum that needs to
> be replaced or forced by the user. Here I'm not seeing a compelling
> reason for a user to want to force LOG_OUT to something else. Also
> makefile macros are generally a pain to debug, which is another
> reason for not putting too many of them.
>
> I noticed that your next patch changes LOG_OUT to tee, it could have
> done it everywhere and wouldn't affect readability as much.
>

I have forgetten to pick an old patch to silence the running log like
this:

    ifeq ($(QUIET_RUN),1)
    LOG_OUT = > "$(RUN_OUT)"
    else
    LOG_OUT = | tee "$(RUN_OUT)"
    endif

Without QUIET_RUN, we can silence the running log with:

    $ make run LOG_OUT="> /dev/null"

It is not meaningful like QUIET_RUN, seems the QUIET_RUN is not
necessary for we have 'grep status' now, so, let's remove this RUN_OUT
too.

Thanks,
Zhangjin

> Thanks,
> Willy
diff mbox series

Patch

diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index 0cd17de2062c..8c531518bb9f 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -166,45 +166,58 @@  endif
 libc-test: nolibc-test.c
 	$(QUIET_CC)$(CC) -o $@ $<
 
+# common macros for logging
+RUN_OUT = $(CURDIR)/run.out
+LOG_OUT = > "$(RUN_OUT)"
+REPORT_RUN_OUT = $(REPORT) "$(RUN_OUT)"
+
 # local libc-test
 run-libc-test: libc-test
-	$(Q)./libc-test > "$(CURDIR)/run.out" || :
-	$(Q)$(REPORT) $(CURDIR)/run.out
+	$(Q)./libc-test $(LOG_OUT) || :
+	$(Q)$(REPORT_RUN_OUT)
 
 # local nolibc-test
 run-nolibc-test: nolibc-test
-	$(Q)./nolibc-test > "$(CURDIR)/run.out" || :
-	$(Q)$(REPORT) $(CURDIR)/run.out
+	$(Q)./nolibc-test $(LOG_OUT) || :
+	$(Q)$(REPORT_RUN_OUT)
 
 # qemu user-land test
 run-user: nolibc-test
-	$(Q)qemu-$(QEMU_ARCH) ./nolibc-test > "$(CURDIR)/run.out" || :
-	$(Q)$(REPORT) $(CURDIR)/run.out
+	$(Q)qemu-$(QEMU_ARCH) ./nolibc-test $(LOG_OUT) || :
+	$(Q)$(REPORT_RUN_OUT)
 
 initramfs: nolibc-test
 	$(QUIET_MKDIR)mkdir -p initramfs
 	$(call QUIET_INSTALL, initramfs/init)
 	$(Q)cp nolibc-test initramfs/init
 
+# common macros for kernel targets
+MAKE_KERNEL   = $(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE)
+KERNEL_CONFIG = $(srctree)/.config
+KERNEL_IMAGE  = $(srctree)/$(IMAGE)
+
 defconfig:
-	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) mrproper $(DEFCONFIG) prepare
+	$(Q)$(MAKE_KERNEL) mrproper $(DEFCONFIG) prepare
 
 extconfig:
-	$(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(srctree)" -m "$(srctree)/.config" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c))
-	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) KCONFIG_ALLCONFIG="$(srctree)/.config" allnoconfig
+	$(Q)$(srctree)/scripts/kconfig/merge_config.sh -O "$(srctree)" -m "$(KERNEL_CONFIG)" $(foreach c,$(EXTCONFIG),$(wildcard $(CURDIR)/configs/$c))
+	$(Q)$(MAKE_KERNEL) KCONFIG_ALLCONFIG="$(KERNEL_CONFIG)" allnoconfig
 
 kernel: initramfs
-	$(Q)$(MAKE) -C $(srctree) ARCH=$(ARCH) CC=$(CC) CROSS_COMPILE=$(CROSS_COMPILE) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
+	$(Q)$(MAKE_KERNEL) $(IMAGE_NAME) CONFIG_INITRAMFS_SOURCE=$(CURDIR)/initramfs
+
+# common macros for qemu run/rerun targets
+QEMU_SYSTEM_RUN = qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(KERNEL_IMAGE)" -serial stdio $(QEMU_ARGS)
 
 # run the tests after building the kernel
 run: kernel
-	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
-	$(Q)$(REPORT) $(CURDIR)/run.out
+	$(Q)$(QEMU_SYSTEM_RUN) $(LOG_OUT)
+	$(Q)$(REPORT_RUN_OUT)
 
 # re-run the tests from an existing kernel
 rerun:
-	$(Q)qemu-system-$(QEMU_ARCH) -display none -no-reboot -kernel "$(srctree)/$(IMAGE)" -serial stdio $(QEMU_ARGS) > "$(CURDIR)/run.out"
-	$(Q)$(REPORT) $(CURDIR)/run.out
+	$(Q)$(QEMU_SYSTEM_RUN) $(LOG_OUT)
+	$(Q)$(REPORT_RUN_OUT)
 
 # report with existing test log
 report: