Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Zhangjin Wu <falcon@tinylab.org>
To:     w@1wt.eu
Cc:     arnd@arndb.de, falcon@tinylab.org, linux-kernel@vger.kernel.org,
        linux-kselftest@vger.kernel.org, thomas@t-8ch.de
Subject: Re: [PATCH v2 02/14] selftests/nolibc: add macros to enhance maintainability
Date:   Tue, 25 Jul 2023 20:37:25 +0800
Message-Id: <20230725123725.35508-1-falcon@tinylab.org>
In-Reply-To: <20230722122009.GE17311@1wt.eu>
References: <20230722122009.GE17311@1wt.eu>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Feedback-ID: bizesmtp:tinylab.org:qybglogicsvrgz:qybglogicsvrgz5a-1
Precedence: bulk

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