From: Toon Claes <toon@iotcl.com>
Date: Fri, 18 Oct 2024 15:21:17 GMT
Subject: Re: [PATCH v2 1/3] Makefile: extract script to generate clar declarations
Message-ID: <871q0dcu8i.fsf@iotcl.com>
In-Reply-To: <7a619677c7af6ba8213a36208e20ab75c4318e38.1728985514.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> Extract the script to generate function declarations for the clar unit
> testing framework into a standalone script. This is done such that we
> can reuse it in other build systems.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  Makefile                            |  4 +---
>  t/unit-tests/generate-clar-decls.sh | 16 ++++++++++++++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
>  create mode 100755 t/unit-tests/generate-clar-decls.sh
>
> diff --git a/Makefile b/Makefile
> index feeed6f9321..87b86c5be1a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -3904,9 +3904,7 @@ GIT-TEST-SUITES: FORCE
>              fi
>  
>  $(UNIT_TEST_DIR)/clar-decls.h: $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUITES)) GIT-TEST-SUITES
> -	$(QUIET_GEN)for suite in $(CLAR_TEST_SUITES); do \
> -		sed -ne "s/^\(void test_$${suite}__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$$\)/extern \1;/p" $(UNIT_TEST_DIR)/$$suite.c; \
> -	done >$@
> +	$(QUIET_GEN)$(SHELL_PATH) $(UNIT_TEST_DIR)/generate-clar-decls.sh "$@" $(patsubst %,$(UNIT_TEST_DIR)/%.c,$(CLAR_TEST_SUITES))
>  $(UNIT_TEST_DIR)/clar.suite: $(UNIT_TEST_DIR)/clar-decls.h
>  	$(QUIET_GEN)awk -f $(UNIT_TEST_DIR)/clar-generate.awk $< >$(UNIT_TEST_DIR)/clar.suite
>  $(CLAR_TEST_OBJS): $(UNIT_TEST_DIR)/clar-decls.h
> diff --git a/t/unit-tests/generate-clar-decls.sh b/t/unit-tests/generate-clar-decls.sh
> new file mode 100755
> index 00000000000..81da732917a
> --- /dev/null
> +++ b/t/unit-tests/generate-clar-decls.sh
> @@ -0,0 +1,16 @@
> +#!/bin/sh
> +
> +if test $# -lt 2
> +then
> +	echo "USAGE: $0 <OUTPUT> <SUITE>..." 2>&1
> +	exit 1
> +fi
> +
> +OUTPUT="$1"
> +shift
> +
> +for suite in "$@"
> +do
> +	sed -ne "s/^\(void test_$suite__[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p" "$suite" ||

In the Makefile the first `suite` was wrapped in curly braces. And I
think we need to keep them in this script as well. I noticed because I
was reviewing this code in my editor I've noticed it highlights
"source__" as the variable name. You can see what happens if you add
`set -x` to the top of the script:

    $ make t/unit-tests/clar-decls.h V=1
    /bin/sh t/unit-tests/generate-clar-decls.sh "t/unit-tests/clar-decls.h" t/unit-tests/ctype.c t/unit-tests/strvec.c
    + test 3 -lt 2
    + OUTPUT=t/unit-tests/clar-decls.h
    + shift
    + for suite in "$@"
    + sed -ne 's/^\(void test_[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p' t/unit-tests/ctype.c
    + for suite in "$@"
    + sed -ne 's/^\(void test_[a-zA-Z_0-9][a-zA-Z_0-9]*(void)$\)/extern \1;/p' t/unit-tests/strvec.c

So it seems the script currently works "by accident".

You should replace the first $suite with something like:

    $(basename $suite .c)

One other suggestion, and feel free to disagree. What do you think about
replacing the `$(patsubst ...)` in the recipe to `$(filter %.c,$^)`?

--
Toon