| Message ID | 20220614162248.40278-2-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: rework compat headers generation | expand |
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@arm.com> > Fixes: 28e13c7f43 ("build: xen/include: use if_changed") > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> > --- > > 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 >
On Wed, Jun 15, 2022 at 08:37:42AM +0200, Jan Beulich wrote: > On 14.06.2022 18:22, Anthony PERARD wrote: > > 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. When looking at the shell grammar at [1], an empty body seems to be allowed. But I can add "*)" at the end for peace of mind. [1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_10_02 As for misleading indentation, I've got my $EDITOR to show me matching parentheses, so I don't need to count them. But if I rewrite that function as following, would it be easier to follow? + 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; \ + }; \ > > + }; \ > > + 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 ... Well, in Makefile it's `` vs $$(). At least, we don't have to write $$$(open)$(close) here :-). I guess $$(get_prereq $$i) isn't too bad here, I'll replace the backquote. > 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. It doesn't really need converting at the moment, because there's only two headers to check, so the resulting command line is small. But converting it at the same time is probably a good idea to avoid having two different implementations of the header check. Thanks,
On 15.06.2022 12:31, Anthony PERARD wrote: > On Wed, Jun 15, 2022 at 08:37:42AM +0200, Jan Beulich wrote: >> On 14.06.2022 18:22, Anthony PERARD wrote: >>> 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. > > When looking at the shell grammar at [1], an empty body seems to be > allowed. But I can add "*)" at the end for peace of mind. > > [1] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_10_02 Hmm, indeed. As opposed to https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_04 > As for misleading indentation, I've got my $EDITOR to show me matching > parentheses, so I don't need to count them. But if I rewrite that > function as following, would it be easier to follow? > > + 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; \ > + }; \ Hmm, not really, no (and it may be more obvious looking at the reply context above): My primary concern is the use of hard tabs beyond the leading one (which is uniform across all lines and hence doesn't alter apparent alignment even with the + or > prefixed). Jan
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; \ + }; \ + 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` \ -S -o /dev/null - \ - || exit $$?; echo $(i) >> $@.new;) \ + || exit $$?; echo $$i >> $@.new; done; \ mv $@.new $@ endef
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@arm.com> Fixes: 28e13c7f43 ("build: xen/include: use if_changed") Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com> --- 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(-)