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

[Eric Blake]

On 12/13/18 9:34 AM, Roman Bolshakov wrote:
> $(VC_LIST_EXCEPT) is usually expanded into arguments for a command.
> When a project contains too many, some operating systems can't pass all
> the arguments because they hit the limit of arguments. FreeBSD and macOS
> are known to have the limit of 256k of arguments.
> 
> More on the issue:
> http://lists.gnu.org/archive/html/bug-gnulib/2015-08/msg00019.html
> https://www.redhat.com/archives/libvir-list/2015-August/msg00758.html
> 
> xargs without flags can be used to limit number of arguments.

This part is fine.

> The
> default number of arguments (max-args for "-n" flag) is
> platform-specific. If argument length exceeds default value for "-s"
> flag (max-chars), xargs will feed less arguments than max-args.

This was relevant to earlier versions, but is now just fluff; I'd drop
it.  What's more, you didn't mention that we are relying on 'echo
$(long_list) | xargs' working because the shell's built-in echo does not
use exec and can therefore process a larger list than what exec
enforces.  I don't mind tweaking that on commit.

> 
> Signed-off-by: Roman Bolshakov <address@hidden>
> ---
>  top/maint.mk | 135 ++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 86 insertions(+), 49 deletions(-)
> 
> diff --git a/top/maint.mk b/top/maint.mk
> index 4889ebacc..1152ad698 100644
> --- a/top/maint.mk
> +++ b/top/maint.mk
> @@ -302,32 +302,46 @@ define _sc_search_regexp
>     fi;                                                                       
> \
>                                                                       \
>     : Filter by content;                                                      
> \
> -   test -n "$$files" && test -n "$$containing"                               
> \
> -     && { files=$$(grep -l "$$containing" $$files); } || :;          \
> -   test -n "$$files" && test -n "$$non_containing"                   \
> -     && { files=$$(grep -vl "$$non_containing" $$files); } || :;     \
> +   test -n "$$files"                                                 \
> +     && test -n "$$containing"                                               
> \

It feels like extra churn to have split this line with no change to its
content, but I also don't mind keeping it split.

> +     && { files=$$(echo "$$files" | xargs grep -l "$$containing"); } \
> +     || :;                                                           \
> +   test -n "$$files"                                                 \
> +     && test -n "$$non_containing"                                   \
> +     && { files=$$(echo "$$files" | xargs grep -vl "$$non_containing"); } \
> +     || :;                                                           \
>                                                                       \
>     : Check for the construct;                                                
> \
>     if test -n "$$files"; then                                                
> \
>       if test -n "$$prohibit"; then                                   \
> -       grep $$with_grep_options $(_ignore_case) -nE "$$prohibit" $$files \
> +       echo "$$files" /dev/null                                              
> \
> +         | xargs                                                     \
> +             grep $$with_grep_options $(_ignore_case) -nE "$$prohibit"       
> \

Not quite canonical. That can cause xargs to invoke:
grep a b c
grep /dev/null

instead of the more typical:

grep /dev/null a b
grep /dev/null c

Remember, 'grep one' output differs from 'grep one two' - so the GOAL is
to ensure that grep always has at least two file, by always including
/dev/null in the list of arguments to EVERY grep invocation, rather than
only to the LAST grep invocation.

But, that said, your formulation "works" in the sense that it is
unlikely that any one single filename would push beyond exec limits, and
therefore xargs will either make a single call (no difference); or split
things so that the first call has more than one file (no /dev/null, but
since there are multiple files things work) and the second call has just
/dev/null (which will have no output, so it won't matter that grep was
called with one file instead of multiple); or split things so that the
first has more than one file, and the second has more than one file
(including /dev/null).

I'm still inclined to touch this up to use the canonical formulation:

'echo $(long_list) | xargs grep /dev/null'

rather than your formulation:

'echo $(long_list) /dev/null | xargs grep'

just so future readers don't have to revisit why yours happens to work.

>           | grep -vE "$${exclude:-^$$}"                                       
> \
> -         && { msg="$$halt" $(_sc_say_and_exit) } || :;                       
> \
> +         && { msg="$$halt" $(_sc_say_and_exit) }                     \
> +         || :;                                                               
> \

Again, this change is just churn.  I don't know that I would have done
it, but I'm also not in the mood to undo your effort.

> @@ -399,25 +413,31 @@ sc_error_exit_success:
>  # "FATAL:" should be fully upper-cased in error messages
>  # "WARNING:" should be fully upper-cased, or fully lower-cased
>  sc_error_message_warn_fatal:
> -     @grep -nEA2 '[^rp]error *\(' $$($(VC_LIST_EXCEPT))              \
> -         | grep -E '"Warning|"Fatal|"fatal' &&                       \
> -       { echo '$(ME): use FATAL, WARNING or warning' 1>&2;           \
> -         exit 1; } || :
> +     @$(VC_LIST_EXCEPT)                                              \
> +       | xargs grep -nEA2 '[^rp]error *\(' /dev/null                 \

For comparison to above, this conversion was canonical.

> @@ -845,9 +865,14 @@ 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))         \
> -         && { echo '$(ME): define the above via some gnulib .h file' \
> -               1>&2;  exit 1; } || :;                                \
> +       regex=`$(def_sym_regex)`; export regex;                       \

Using `` is odd, especially since...


> @@ -1033,7 +1065,8 @@ sc_prohibit_test_double_equal:
>  # definition of LDADD from the appropriate Makefile.am and exits 0
>  # when it contains "ICONV".
>  sc_proper_name_utf8_requires_ICONV:
> -     @progs=$$(grep -l 'proper_name_utf8 ''("' $$($(VC_LIST_EXCEPT)));\
> +     @progs=$$($(VC_LIST_EXCEPT)                                     \

...we are using $$() elsewhere.

I don't mind making those changes, if it will let me push today.

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

signature.asc
Description: OpenPGP digital signature