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

[Eric Blake]

On 12/3/18 9:00 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. 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.
>
> Signed-off-by: Roman Bolshakov <address@hidden>
> ---
>   top/maint.mk | 53 +++++++++++++++++++++++++++++++++-------------------
>   1 file changed, 34 insertions(+), 19 deletions(-)
>
> diff --git a/top/maint.mk b/top/maint.mk
> index 4889ebacc..36a5df262 100644
> --- a/top/maint.mk
> +++ b/top/maint.mk
> @@ -303,18 +303,22 @@ define _sc_search_regexp
>                                                                         \
>      : Filter by content;                                                       
> \
>      test -n "$$files" && test -n "$$containing"                            \
> -     && { files=$$(grep -l "$$containing" $$files); } || :;          \

If $$files is large enough to overflow the command-line limit for 
execv*() here...

> +     && { files=$$(echo "$$files"                                    \

...then why is it not large enough to overflow the limit for 'echo' 
here?  Or is it because you are implicitly relying on 'echo' being a 
shell-builtin that does not suffer from the same command-line limit 
because there is no execv*() involved?

> +       | xargs grep -l "$$containing" /dev/null); } || :;            \

At any rate, as long as the rewrite worked for you, it does look like 
you are benefitting from 'echo' being a shell builtin that does not 
suffer from the same limits as an external program.  If command-line 
arguments start affecting echo, we'd have to come up with something 
hairier like:

xargs <<EOF grep -l "$$containing" /dev/null
$$files
EOF

>      test -n "$$files" && test -n "$$non_containing"                        \
> -     && { files=$$(grep -vl "$$non_containing" $$files); } || :;     \
> +     && { files=$$(echo "$$files"                                    \
> +       | xargs grep -vl "$$non_containing" /dev/null); } || :;               \

Did you need /dev/null on this conversion?

> @@ -323,9 +327,10 @@ define _sc_search_regexp
>   endef
>   
>   sc_avoid_if_before_free:
> -       @$(srcdir)/$(_build-aux)/useless-if-before-free                 \
> -               $(useless_free_options)                                 \
> -           $$($(VC_LIST_EXCEPT) | grep -v useless-if-before-free) &&   \
> +       @$(VC_LIST_EXCEPT) | grep -v useless-if-before-free             \
> +            | xargs                                                    \
> +             $(srcdir)/$(_build-aux)/useless-if-before-free            \
> +             $(useless_free_options)            &&                     \

Spacing before the && looks odd here.

> @@ -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?

> +         $(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).

> @@ -927,7 +938,8 @@ require_exactly_one_NL_at_EOF_ =                            
>         \
>       }                                                                 \
>     END { exit defined $$fail }
>   sc_prohibit_empty_lines_at_EOF:
> -       @perl -le '$(require_exactly_one_NL_at_EOF_)' $$($(VC_LIST_EXCEPT)) \
> +       @$(VC_LIST_EXCEPT) | xargs -I{}                                 \
> +         perl -le '$(require_exactly_one_NL_at_EOF_)' {}               \

Why did you need -I{} here? If your only use of {} in the 'initial-args' 
to xargs is in the final position, this should be the same as:

$(VC_LIST_EXCEPT) | xargs perl -le '$(require_exactly_one_NL_at_EOF_)'

> @@ -1033,7 +1046,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) | xargs                             \
> +          grep -l 'proper_name_utf8 ''("' /dev/null);                     \

You don't need /dev/null here because of grep -l.

> @@ -1155,8 +1169,8 @@ sc_po_check:
>         @if test -f $(po_file); then                                    \
>           grep -E -v '^(#|$$)' $(po_file)                               \
>             | grep -v '^src/false\.c$$' | sort > address@hidden;                
>      \
> -         files=$$(perl $(perl_translatable_files_list_)                \
> -           $$($(VC_LIST_EXCEPT)) $(generated_files));                  \
> +         files=$$($(VC_LIST_EXCEPT) | xargs -I{}                       \
> +           perl $(perl_translatable_files_list_) {} $(generated_files)); \
>           grep -E -l '$(_gl_translatable_string_re)' $$files            \

This one is wrong; if xargs splits the list, it ends up running perl on 
the tail end of $(generated_files) more than once.  Better would be:

{ $(VC_LIST_EXCEPT); echo $(generated_files); } | \
  xargs perl $(perl_translatable_files_list_)) | \
  xargs grep -E -l '$(_gl_translatable_string_re)' \

>             | $(SED) 's|^$(_dot_escaped_srcdir)/||' | sort -u > address@hidden; 
>      \

Also, this points out that we tend to use the style:

   command 1 \
     | xargs command 2 \
     | command 3

rather than

   command 1 | xargs \
     command 2 | \
     command 3

so while I didn't point out many spots where your style is different (by 
splitting xargs from its arguments), it may be worth reformatting to 
include xargs on the same line as its arguments rather than separately.

> @@ -1230,7 +1244,8 @@ sc_cross_check_PATH_usage_in_tests:
>                  exit 1; };                                             \
>           good=$$(grep -E '$(_hv_regex_strong)' $(_hv_file));           \
>           grep -LFx "$$good"                                          \
> -               $$(grep -lE '$(_hv_regex_weak)' $$($(VC_LIST_EXCEPT)))  \
> +               $$($(VC_LIST_EXCEPT) | xargs                            \
> +                 grep -lE '$(_hv_regex_weak)' /dev/null)               \

Are you sure that the inner $$() won't produce too many arguments for 
the outer grep -LFx, or is this another case where you'll want to 
rewrite to:

$(VC_LIST_EXCEPT) | xargs grep -lE '$(_hv_regx_weak)' \
  | xargs grep -LFx "$$good"

Getting closer, but we'll need a v3.

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