bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Eric Blake
Subject: Re: [PATCH v2 1/2] maint.mk: Split long argument lists
Date: Wed, 12 Dec 2018 19:42:44 -0600
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.3.1

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.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



reply via email to

[Prev in Thread] Current Thread [Next in Thread]