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
