Re: [XEN PATCH v2 1/4] build,include: rework shell script for headers++.chk

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.06.2022 18:22, Anthony PERARD wrote:
> The command line generated for headers++.chk by make is quite long,
> and in some environment it is too long. This issue have been seen in
> Yocto build environment.
> 
> Error messages:
>     make[9]: execvp: /bin/sh: Argument list too long
>     make[9]: *** [include/Makefile:181: include/headers++.chk] Error 127
> 
> Rework so that we do the foreach loop in shell rather that make to
> reduce the command line size by a lot. We also need a way to get
> headers prerequisite for some public headers so we use a switch "case"
> in shell to be able to do some simple pattern matching. Variables
> alone in POSIX shell don't allow to work with associative array or
> variables with "/".
> 
> Reported-by: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>
> Fixes: 28e13c7f43 ("build: xen/include: use if_changed")
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> ---
> 
> Notes:
>     v2:
>     - fix typo in commit message
>     - fix out-of-tree build
>     
>     v1:
>     - was sent as a reply to v1 of the series
> 
>  xen/include/Makefile | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/include/Makefile b/xen/include/Makefile
> index 6d9bcc19b0..49c75a78f9 100644
> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -158,13 +158,22 @@ define cmd_headerscxx_chk
>           touch $@.new;                                                     \
>           exit 0;                                                           \
>       fi;                                                                   \
> -     $(foreach i, $(filter %.h,$^),                                        \
> -         echo "#include "\"$(i)\"                                          \
> +     get_prereq() {                                                        \
> +         case $$1 in                                                       \
> +         $(foreach i, $(filter %.h,$^),                                    \
> +         $(if $($(patsubst $(srctree)/%,%,$(i))-prereq),                   \
> +             $(i)$(close)                                                  \
> +             echo "$(foreach j, $($(patsubst $(srctree)/%,%,$(i))-prereq), \
> +                     -include c$(j))";;))                                  \
> +         esac;                                                             \

If I'm reading this right (indentation looks to be a little misleading
and hence one needs to count parentheses) the case statement could (in
theory) remain without any "body". As per the command language spec I'm
looking at this (if it works in the first place) is an extension, and
formally there's always at least one label required. Since we aim to be
portable in such regards, I'd like to request that there be a final
otherwise empty *) line.

> +     };                                                                    \
> +     for i in $(filter %.h,$^); do                                         \
> +         echo "#include "\"$$i\"                                           \
>           | $(CXX) -x c++ -std=gnu++98 -Wall -Werror -D__XEN_TOOLS__        \
>             -include stdint.h -include $(srcdir)/public/xen.h               \
> -           $(foreach j, $($(patsubst $(srctree)/%,%,$i)-prereq), -include 
> c$(j)) \
> +           `get_prereq $$i`                                                \

While I know we use back-ticked quoting elsewhere, I'd generally
recommend to use $() for readability. But maybe others view this
exactly the other way around ...

And a question without good context to put at: Isn't headers99.chk in
similar need of converting? It looks only slightly less involved than
the C++ one.

Jan

>             -S -o /dev/null -                                               \
> -         || exit $$?; echo $(i) >> $@.new;) \
> +         || exit $$?; echo $$i >> $@.new; done;                            \
>       mv $@.new $@
>  endef
>