Re: [PATCH v2 1/2] maint.mk: Split long argument lists

[Roman Bolshakov]

On Wed, Dec 12, 2018 at 07:42:44PM -0600, Eric Blake wrote:
> On 12/12/18 3:13 PM, Roman Bolshakov wrote:
> 
> > > > @@ -845,7 +853,10 @@ sc_prohibit_always-defined_macros:
> > > >           case $$(echo all: | grep -l -f - Makefile) in Makefile);; *)  
> > > > \
> > > >             echo '$(ME): skipping $@: you lack GNU grep' 1>&2; exit 0;; 
> > > > \
> > > >           esac;                                                         
> > > > \
> > > > -         $(def_sym_regex) | grep -E -f - $$($(VC_LIST_EXCEPT))         
> > > > \
> > > > +         sym_regexes=$$(mktemp);                                       
> > > > \
> > > 
> > > Are we guaranteed that 'mktemp' is portably present on all developer's
> > > machines?
> > > 
> > 
> > That's a good question. It's not present on Solaris 9. What platforms
> > gnulib should cover?
> > 
> > > > +         $(def_sym_regex) > $$sym_regexes;                             
> > > > \
> > > > +         $(VC_LIST_EXCEPT) | xargs                                     
> > > > \
> > > > +           grep -E -f $$sym_regexes /dev/null                          
> > > > \
> > > >             && { echo '$(ME): define the above via some gnulib .h file' 
> > > > \
> > > >                   1>&2;  exit 1; } || :;                                
> > > > \
> > > 
> > > Even worse, you aren't cleaning up your temporary file.  This conversion
> > > needs to be rewritten in a better manner.  I suggest:
> > > 
> > > $(VC_LIST_EXCEPT) | xargs sh -c \
> > >     '$(def_sym_regex) | grep -E -f - "$$@"' dummy /dev/null
> > > 
> > > which uses 'sh' as an intermediary to let you still feed $(def_sym_regex) 
> > > as
> > > the stdin to the grep process (the dummy argument supplies $0 to sh, then
> > > the /dev/null is the usual placeholder to ensure grep sees more than one
> > > file at a time to avoid its output changing styles).
> > > 
> > 
> > Ok, I was struggling to find a portable solution. Thank you for
> > proposing this one (and explaining it). The macro can't be expanded
> > without breaking shell parser though. I don't know... the approach with
> > mktemp is more readable but I'm not sure if that's what we want to use
> > because of portability concerns.
> 
> My initial problem with mktemp is its portability, the bigger problem is
> that you don't clean the temp file up. But rewriting to an alternative form
> that avoids the temp file means we don't have to even think about mktemp
> problems.
> 
> So, all that remains is your comment about $(def_sym_regex) being parsed by
> sh.  And I see what you mean: what I wrote won't work, because make would
> expand it to something like this (well, there are more levels of make
> variables substituted before the actual shell command, but this is enough
> expansion to show the problem):
> 
> $(VC_LIST) | $(SED) 's|^$(_dot_escaped_srcdir)/||' \
>         | if test -f $(srcdir)/.x-$@; then grep -vEf $(srcdir)/.x-$@; \
>           else grep -Ev -e "$${VC_LIST_EXCEPT_DEFAULT-ChangeLog}"; fi \
>         | grep -Ev -e '($(VC_LIST_ALWAYS_EXCLUDE_REGEX)|$(_sc_excl))' \
>         $(_prepend_srcdir_prefix) \
> xargs -sh -c '      \
>     gen_h=$(gl_generated_headers_);                                 \
>         (cd $(gnulib_dir)/lib;                                      \
>           for f in *.in.h $(gl_other_headers_); do                  \
>             test -f $$f                                             \
>               && perl -lne '$(gl_extract_significant_defines_)' $$f;\
>           done;                                                     \
>         ) | sort -u                                                 \
>           | $(SED) 's/^/^ *# *(define|undef)  */;s/$$/\\>/ \
>   | grep -E -f - "$$@"' dummy /dev/null
> 
> where the embedded ' in the expansion of $(def_sym_regex) don't do well
> through a second round of sh parsing.  Obviously, I didn't test things, and
> was acting more like I had a shell variable than a make variable. But it
> seems like a shell variable would do the trick (unless you hit other command
> line limits?):
> 
>   regex=`$(def_sym_regex)`; export regex; \
>   $(VC_LIST_EXCEPT) \
>     | xargs sh -c 'echo $$regex | grep -E -f - "$$@"' dummy /dev/null
> 
> I'll have to rely on you to test it, since I'm not running into the limits
> on my machine, but hopefully that helps you with some ideas.
> 

This one is good, I've tested it with libvirt's syntax-check. regex
variable is around 28KB but the rule doesn't hit any limits for me. And
I don't see how it could because the regular expressions are passed
through a pipe with a shell built-in rather than as arguments to execve.

Thank you,
Roman