[PATCH 0/2] Fix syntax-check on macOS/FreeBSD

[PATCH 0/2] Fix syntax-check on macOS/FreeBSD

[Roman Bolshakov]

Hello,

There was an issue with syntax-check on FreeBSD reported a few years
ago:
https://www.redhat.com/archives/libvir-list/2015-August/msg00758.html
http://lists.gnu.org/archive/html/bug-gnulib/2015-08/msg00019.html

The patch series attempts to resolve the issue on gnulib side. With
related changes on libvirt side I can run make syntax-check on macOS.

I wasn't entirely sure on indentation in the long commands. I did my best,
sorry if something is wrong.

--
Best regards,
Roman

Roman Bolshakov (2):
  maint.mk: Split long argument lists
  maint.mk: Replace grep with $(GREP)

 top/maint.mk | 147 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 82 insertions(+), 65 deletions(-)

-- 
2.19.1

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

[Roman Bolshakov]

$(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

The workaround is to split argument list into chunks that operating
system can process. "getconf ARG_MAX" is used to determine size of the
chunk.

In-Reply-To: 55D5F55E.60503@redhat.com
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 top/maint.mk | 55 ++++++++++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 19 deletions(-)

diff --git a/top/maint.mk b/top/maint.mk
index 4889ebacc..c4f21f947 100644
--- a/top/maint.mk
+++ b/top/maint.mk
@@ -54,6 +54,8 @@ GIT = git
 VC = $(GIT)
 
 VC_LIST = $(srcdir)/$(_build-aux)/vc-list-files -C $(srcdir)
+# Most operating systems have a limit of arguments
+VC_ARG_MAX = $(shell getconf ARG_MAX)
 
 # You can override this variable in cfg.mk if your gnulib submodule lives
 # in a different location.
@@ -303,18 +305,22 @@ define _sc_search_regexp
 									\
    : Filter by content;							\
    test -n "$$files" && test -n "$$containing"				\
-     && { files=$$(grep -l "$$containing" $$files); } || :;		\
+     && { files=$$(echo "$$files"					\
+       | xargs -n $(VC_ARG_MAX) grep -l "$$containing"); } || :;	\
    test -n "$$files" && test -n "$$non_containing"			\
-     && { files=$$(grep -vl "$$non_containing" $$files); } || :;	\
+     && { files=$$(echo "$$files"					\
+       | xargs -n $(VC_ARG_MAX) 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" | xargs -n $(VC_ARG_MAX)				\
+         grep $$with_grep_options $(_ignore_case) -nE "$$prohibit"	\
          | grep -vE "$${exclude:-^$$}"					\
          && { msg="$$halt" $(_sc_say_and_exit) } || :;			\
      else								\
-       grep $$with_grep_options $(_ignore_case) -LE "$$require" $$files \
+       echo "$$files" | xargs -n $(VC_ARG_MAX)				\
+         grep $$with_grep_options $(_ignore_case) -LE "$$require"	\
            | grep .							\
          && { msg="$$halt" $(_sc_say_and_exit) } || :;			\
      fi									\
@@ -323,9 +329,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 -n $(VC_ARG_MAX)					\
+	      $(srcdir)/$(_build-aux)/useless-if-before-free		\
+	      $(useless_free_options)		 &&	                \
 	  { echo '$(ME): found useless "if" before "free" above' 1>&2;	\
 	    exit 1; } || :
 
@@ -399,14 +406,16 @@ 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))		\
+	@$(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX) 			\
+	    grep -nEA2 '[^rp]error *\('					\
 	    | grep -E '"Warning|"Fatal|"fatal' &&			\
 	  { echo '$(ME): use FATAL, WARNING or warning'	1>&2;		\
 	    exit 1; } || :
 
 # Error messages should not start with a capital letter
 sc_error_message_uppercase:
-	@grep -nEA2 '[^rp]error *\(' $$($(VC_LIST_EXCEPT))		\
+	@$(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX) 			\
+	    grep -nEA2 '[^rp]error *\('					\
 	    | grep -E '"[A-Z]'						\
 	    | grep -vE '"FATAL|"WARNING|"Java|"C#|PRIuMAX' &&		\
 	  { echo '$(ME): found capitalized error message' 1>&2;		\
@@ -414,7 +423,8 @@ sc_error_message_uppercase:
 
 # Error messages should not end with a period
 sc_error_message_period:
-	@grep -nEA2 '[^rp]error *\(' $$($(VC_LIST_EXCEPT))		\
+	@$(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX)			\
+            grep -nEA2 '[^rp]error *\(' 				\
 	    | grep -E '[^."]\."' &&					\
 	  { echo '$(ME): found error message ending in period' 1>&2;	\
 	    exit 1; } || :
@@ -845,7 +855,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);					\
+	  $(def_sym_regex) > $$sym_regexes;				\
+	  $(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX)			\
+	    grep -E -f $$sym_regexes					\
 	    && { echo '$(ME): define the above via some gnulib .h file'	\
 		  1>&2;  exit 1; } || :;				\
 	fi
@@ -927,7 +940,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{} -n $(VC_ARG_MAX)		\
+	  perl -le '$(require_exactly_one_NL_at_EOF_)' {}		\
 	  || { echo '$(ME): empty line(s) or no newline at EOF'		\
 		1>&2; exit 1; } || :
 
@@ -972,7 +986,8 @@ prohibit_doubled_word_ =						\
 ignore_doubled_word_match_RE_ ?= ^$$
 
 sc_prohibit_doubled_word:
-	@perl -n -0777 $(prohibit_doubled_word_) $$($(VC_LIST_EXCEPT))	\
+	@$(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX) 			\
+	  perl -n -0777 $(prohibit_doubled_word_)			\
 	  | grep -vE '$(ignore_doubled_word_match_RE_)'			\
 	  | grep . && { echo '$(ME): doubled words' 1>&2; exit 1; } || :
 
@@ -998,8 +1013,8 @@ prohibit_undesirable_word_seq_ =					\
 ignore_undesirable_word_sequence_RE_ ?= ^$$
 
 sc_prohibit_undesirable_word_seq:
-	@perl -n -0777 $(prohibit_undesirable_word_seq_)		\
-	     $$($(VC_LIST_EXCEPT))					\
+	@$(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX)			\
+	  perl -n -0777 $(prohibit_undesirable_word_seq_)		\
 	  | grep -vE '$(ignore_undesirable_word_sequence_RE_)' | grep .	\
 	  && { echo '$(ME): undesirable word sequence' >&2; exit 1; } || :
 
@@ -1033,7 +1048,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 -n $(VC_ARG_MAX)		\
+          grep -l 'proper_name_utf8 ''("'); 				\
 	if test "x$$progs" != x; then					\
 	  fail=0;							\
 	  for p in $$progs; do						\
@@ -1155,8 +1171,8 @@ sc_po_check:
 	@if test -f $(po_file); then					\
 	  grep -E -v '^(#|$$)' $(po_file)				\
 	    | grep -v '^src/false\.c$$' | sort > $@-1;			\
-	  files=$$(perl $(perl_translatable_files_list_)		\
-	    $$($(VC_LIST_EXCEPT)) $(generated_files));			\
+	  files=$$($(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX) -I{}	\
+	    perl $(perl_translatable_files_list_) {} $(generated_files)); \
 	  grep -E -l '$(_gl_translatable_string_re)' $$files		\
 	    | $(SED) 's|^$(_dot_escaped_srcdir)/||' | sort -u > $@-2;	\
 	  diff -u -L $(po_file) -L $(po_file) $@-1 $@-2			\
@@ -1230,7 +1246,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 -n $(VC_ARG_MAX)		\
+		  grep -lE '$(_hv_regex_weak)')				\
 	      | grep . &&						\
 	    { echo "$(ME): the above files use path_prepend_ inconsistently" \
 		1>&2; exit 1; } || :;					\
-- 
2.19.1

[PATCH 2/2] maint.mk: Replace grep with $(GREP)

[Roman Bolshakov]

A project that uses maint.mk can specify regular expressions that are
not supported in system grep. Autoconf can discover an alias for GNU
grep and set it in GREP but it takes no effect for maint.mk

The patch provides an ability to use GNU grep if it was discovered by
autoconf and by calling GNU grep we don't get the messages in syntax-check:
  prohibit_diagnostic_without_format
  grep: empty (sub)expression
  grep: empty (sub)expression
  grep: empty (sub)expression
  grep: empty (sub)expression
  grep: empty (sub)expression
  grep: empty (sub)expression

Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 top/maint.mk | 114 +++++++++++++++++++++++++--------------------------
 1 file changed, 57 insertions(+), 57 deletions(-)

diff --git a/top/maint.mk b/top/maint.mk
index c4f21f947..fc29631f5 100644
--- a/top/maint.mk
+++ b/top/maint.mk
@@ -46,7 +46,7 @@ member-check =								\
 # Do not save the original name or timestamp in the .tar.gz file.
 # Use --rsyncable if available.
 gzip_rsyncable := \
-  $(shell gzip --help 2>/dev/null|grep rsyncable >/dev/null \
+  $(shell gzip --help 2>/dev/null|$(GREP) rsyncable >/dev/null \
     && printf %s --rsyncable)
 GZIP_ENV = '--no-name --best $(gzip_rsyncable)'
 
@@ -85,9 +85,9 @@ _sc_excl = \
   $(or $(exclude_file_name_regexp--$@),^$$)
 VC_LIST_EXCEPT = \
   $(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))' \
+	| 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)
 
 ifeq ($(origin prev_version_file), undefined)
@@ -294,34 +294,34 @@ define _sc_search_regexp
 									\
    : Filter by file name;						\
    if test -n "$$in_files"; then					\
-     files=$$(find $(srcdir) | grep -E "$$in_files"			\
-              | grep -Ev '$(_sc_excl)');				\
+     files=$$(find $(srcdir) | $(GREP) -E "$$in_files"			\
+              | $(GREP) -Ev '$(_sc_excl)');				\
    else									\
      files=$$($(VC_LIST_EXCEPT));					\
      if test -n "$$in_vc_files"; then					\
-       files=$$(echo "$$files" | grep -E "$$in_vc_files");		\
+       files=$$(echo "$$files" | $(GREP) -E "$$in_vc_files");		\
      fi;								\
    fi;									\
 									\
    : Filter by content;							\
    test -n "$$files" && test -n "$$containing"				\
      && { files=$$(echo "$$files"					\
-       | xargs -n $(VC_ARG_MAX) grep -l "$$containing"); } || :;	\
+       | xargs -n $(VC_ARG_MAX) $(GREP) -l "$$containing"); } || :;	\
    test -n "$$files" && test -n "$$non_containing"			\
      && { files=$$(echo "$$files"					\
-       | xargs -n $(VC_ARG_MAX) grep -vl "$$non_containing"); } || :;	\
+       | xargs -n $(VC_ARG_MAX) $(GREP) -vl "$$non_containing"); } || :;\
 									\
    : Check for the construct;						\
    if test -n "$$files"; then						\
      if test -n "$$prohibit"; then					\
        echo "$$files" | xargs -n $(VC_ARG_MAX)				\
-         grep $$with_grep_options $(_ignore_case) -nE "$$prohibit"	\
-         | grep -vE "$${exclude:-^$$}"					\
+         $(GREP) $$with_grep_options $(_ignore_case) -nE "$$prohibit"	\
+         | $(GREP) -vE "$${exclude:-^$$}"				\
          && { msg="$$halt" $(_sc_say_and_exit) } || :;			\
      else								\
        echo "$$files" | xargs -n $(VC_ARG_MAX)				\
-         grep $$with_grep_options $(_ignore_case) -LE "$$require"	\
-           | grep .							\
+         $(GREP) $$with_grep_options $(_ignore_case) -LE "$$require"	\
+           | $(GREP) .							\
          && { msg="$$halt" $(_sc_say_and_exit) } || :;			\
      fi									\
    else :;								\
@@ -329,7 +329,7 @@ define _sc_search_regexp
 endef
 
 sc_avoid_if_before_free:
-	@$(VC_LIST_EXCEPT) | grep -v useless-if-before-free		\
+	@$(VC_LIST_EXCEPT) | $(GREP) -v useless-if-before-free		\
             | xargs -n $(VC_ARG_MAX)					\
 	      $(srcdir)/$(_build-aux)/useless-if-before-free		\
 	      $(useless_free_options)		 &&	                \
@@ -407,25 +407,25 @@ sc_error_exit_success:
 # "WARNING:" should be fully upper-cased, or fully lower-cased
 sc_error_message_warn_fatal:
 	@$(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX) 			\
-	    grep -nEA2 '[^rp]error *\('					\
-	    | grep -E '"Warning|"Fatal|"fatal' &&			\
+	    $(GREP) -nEA2 '[^rp]error *\('				\
+	    | $(GREP) -E '"Warning|"Fatal|"fatal' &&			\
 	  { echo '$(ME): use FATAL, WARNING or warning'	1>&2;		\
 	    exit 1; } || :
 
 # Error messages should not start with a capital letter
 sc_error_message_uppercase:
 	@$(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX) 			\
-	    grep -nEA2 '[^rp]error *\('					\
-	    | grep -E '"[A-Z]'						\
-	    | grep -vE '"FATAL|"WARNING|"Java|"C#|PRIuMAX' &&		\
+	    $(GREP) -nEA2 '[^rp]error *\('				\
+	    | $(GREP) -E '"[A-Z]'					\
+	    | $(GREP) -vE '"FATAL|"WARNING|"Java|"C#|PRIuMAX' &&	\
 	  { echo '$(ME): found capitalized error message' 1>&2;		\
 	    exit 1; } || :
 
 # Error messages should not end with a period
 sc_error_message_period:
 	@$(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX)			\
-            grep -nEA2 '[^rp]error *\(' 				\
-	    | grep -E '[^."]\."' &&					\
+            $(GREP) -nEA2 '[^rp]error *\(' 				\
+	    | $(GREP) -E '[^."]\."' &&					\
 	  { echo '$(ME): found error message ending in period' 1>&2;	\
 	    exit 1; } || :
 
@@ -469,8 +469,8 @@ perl_config_h_first_ =							\
 # You must include <config.h> before including any other header file.
 # This can possibly be via a package-specific header, if given by cfg.mk.
 sc_require_config_h_first:
-	@if $(VC_LIST_EXCEPT) | grep '\.c$$' > /dev/null; then		\
-	  files=$$($(VC_LIST_EXCEPT) | grep '\.c$$') &&			\
+	@if $(VC_LIST_EXCEPT) | $(GREP) '\.c$$' > /dev/null; then	\
+	  files=$$($(VC_LIST_EXCEPT) | $(GREP) '\.c$$') &&		\
 	  perl -n $(perl_config_h_first_) $$files ||			\
 	    { echo '$(ME): the above files include some other header'	\
 		'before <config.h>' 1>&2; exit 1; } || :;		\
@@ -488,10 +488,10 @@ sc_prohibit_HAVE_MBRTOWC:
 define _sc_header_without_use
   dummy=; : so we do not need a semicolon before each use;		\
   h_esc=`echo '[<"]'"$$h"'[">]'|$(SED) 's/\./\\\\./g'`;			\
-  if $(VC_LIST_EXCEPT) | grep '\.c$$' > /dev/null; then			\
-    files=$$(grep -l '^# *include '"$$h_esc"				\
-	     $$($(VC_LIST_EXCEPT) | grep '\.c$$')) &&			\
-    grep -LE "$$re" $$files | grep . &&					\
+  if $(VC_LIST_EXCEPT) | $(GREP) '\.c$$' > /dev/null; then		\
+    files=$$($(GREP) -l '^# *include '"$$h_esc"				\
+	     $$($(VC_LIST_EXCEPT) | $(GREP) '\.c$$')) &&		\
+    $(GREP) -LE "$$re" $$files | $(GREP) . &&				\
       { echo "$(ME): the above files include $$h but don't use it"	\
 	1>&2; exit 1; } || :;						\
   else :;								\
@@ -745,9 +745,9 @@ Exit_base := $(notdir $(Exit_witness_file))
 sc_require_test_exit_idiom:
 	@if test -f $(srcdir)/$(Exit_witness_file); then		\
 	  die=0;							\
-	  for i in $$(grep -l -F 'srcdir/$(Exit_base)'			\
+	  for i in $$($(GREP) -l -F 'srcdir/$(Exit_base)'		\
 		$$($(VC_LIST) tests)); do				\
-	    tail -n1 $$i | grep '^Exit .' > /dev/null			\
+	    tail -n1 $$i | $(GREP) '^Exit .' > /dev/null		\
 	      && : || { die=1; echo $$i; }				\
 	  done;								\
 	  test $$die = 1 &&						\
@@ -852,13 +852,13 @@ endef
 # Don't define macros that we already get from gnulib header files.
 sc_prohibit_always-defined_macros:
 	@if test -d $(gnulib_dir); then					\
-	  case $$(echo all: | grep -l -f - Makefile) in Makefile);; *)	\
+	  case $$(echo all: | $(GREP) -l -f - Makefile) in Makefile);; *) \
 	    echo '$(ME): skipping $@: you lack GNU grep' 1>&2; exit 0;;	\
 	  esac;								\
 	  sym_regexes=$$(mktemp);					\
 	  $(def_sym_regex) > $$sym_regexes;				\
 	  $(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX)			\
-	    grep -E -f $$sym_regexes					\
+	    $(GREP) -E -f $$sym_regexes					\
 	    && { echo '$(ME): define the above via some gnulib .h file'	\
 		  1>&2;  exit 1; } || :;				\
 	fi
@@ -866,7 +866,7 @@ sc_prohibit_always-defined_macros:
 
 # Prohibit checked in backup files.
 sc_prohibit_backup_files:
-	@$(VC_LIST) | grep '~$$' &&				\
+	@$(VC_LIST) | $(GREP) '~$$' &&					\
 	  { echo '$(ME): found version controlled backup file' 1>&2;	\
 	    exit 1; } || :
 
@@ -988,8 +988,8 @@ ignore_doubled_word_match_RE_ ?= ^$$
 sc_prohibit_doubled_word:
 	@$(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX) 			\
 	  perl -n -0777 $(prohibit_doubled_word_)			\
-	  | grep -vE '$(ignore_doubled_word_match_RE_)'			\
-	  | grep . && { echo '$(ME): doubled words' 1>&2; exit 1; } || :
+	  | $(GREP) -vE '$(ignore_doubled_word_match_RE_)'		\
+	  | $(GREP) . && { echo '$(ME): doubled words' 1>&2; exit 1; } || :
 
 # A regular expression matching undesirable combinations of words like
 # "can not"; this matches them even when the two words appear on different
@@ -1015,7 +1015,7 @@ ignore_undesirable_word_sequence_RE_ ?= ^$$
 sc_prohibit_undesirable_word_seq:
 	@$(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX)			\
 	  perl -n -0777 $(prohibit_undesirable_word_seq_)		\
-	  | grep -vE '$(ignore_undesirable_word_sequence_RE_)' | grep .	\
+	  | $(GREP) -vE '$(ignore_undesirable_word_sequence_RE_)' | $(GREP) . \
 	  && { echo '$(ME): undesirable word sequence' >&2; exit 1; } || :
 
 # Except for shell files and for loops, double semicolon is probably a mistake
@@ -1049,7 +1049,7 @@ sc_prohibit_test_double_equal:
 # when it contains "ICONV".
 sc_proper_name_utf8_requires_ICONV:
 	@progs=$$($(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX)		\
-          grep -l 'proper_name_utf8 ''("'); 				\
+          $(GREP) -l 'proper_name_utf8 ''("'); 				\
 	if test "x$$progs" != x; then					\
 	  fail=0;							\
 	  for p in $$progs; do						\
@@ -1058,7 +1058,7 @@ sc_proper_name_utf8_requires_ICONV:
 	      -ne 'exit !(/^LDADD =(.+?[^\\]\n)/ms && $$1 =~ /ICONV/)'	\
 	      $$dir/Makefile.am && continue;				\
 	    base=$$(basename "$$p" .c);					\
-	    grep "$${base}_LDADD.*ICONV)" $$dir/Makefile.am > /dev/null	\
+	    $(GREP) "$${base}_LDADD.*ICONV)" $$dir/Makefile.am > /dev/null	\
 	      || { fail=1; echo 1>&2 "$(ME): $$p uses proper_name_utf8"; }; \
 	  done;								\
 	  test $$fail = 1 &&						\
@@ -1119,12 +1119,12 @@ sc_makefile_at_at_check:
           -e ' && !/(\w+)\s+=.*\@\1\@$$/'				\
           -e ''$(_makefile_at_at_check_exceptions)			\
 	  -e 'and (print "$$ARGV:$$.: $$_"), $$m=1; END {exit !$$m}'	\
-	    $$($(VC_LIST_EXCEPT) | grep -E '(^|/)(Makefile\.am|[^/]+\.mk)$$') \
+	    $$($(VC_LIST_EXCEPT) | $(GREP) -E '(^|/)(Makefile\.am|[^/]+\.mk)$$') \
 	  && { echo '$(ME): use $$(...), not @...@' 1>&2; exit 1; } || :
 
 news-check: NEWS
 	$(AM_V_GEN)if $(SED) -n $(news-check-lines-spec)p $<		\
-	    | grep -E $(news-check-regexp) >/dev/null; then		\
+	    | $(GREP) -E $(news-check-regexp) >/dev/null; then		\
 	  :;								\
 	else								\
 	  echo 'NEWS: $$(news-check-regexp) failed to match' 1>&2;	\
@@ -1169,11 +1169,11 @@ generated_files ?= $(srcdir)/lib/*.[ch]
 _gl_translatable_string_re ?= \b(N?_|gettext *)\([^)"]*("|$$)
 sc_po_check:
 	@if test -f $(po_file); then					\
-	  grep -E -v '^(#|$$)' $(po_file)				\
-	    | grep -v '^src/false\.c$$' | sort > $@-1;			\
+	  $(GREP) -E -v '^(#|$$)' $(po_file)				\
+	    | $(GREP) -v '^src/false\.c$$' | sort > $@-1;		\
 	  files=$$($(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX) -I{}	\
 	    perl $(perl_translatable_files_list_) {} $(generated_files)); \
-	  grep -E -l '$(_gl_translatable_string_re)' $$files		\
+	  $(GREP) -E -l '$(_gl_translatable_string_re)' $$files		\
 	    | $(SED) 's|^$(_dot_escaped_srcdir)/||' | sort -u > $@-2;	\
 	  diff -u -L $(po_file) -L $(po_file) $@-1 $@-2			\
 	    || { printf '$(ME): '$(fix_po_file_diag) 1>&2; exit 1; };	\
@@ -1238,17 +1238,17 @@ _hv_regex_weak ?= ^ *\. .*/init\.sh"
 _hv_regex_strong ?= ^ *\. "\$${srcdir=\.}/init\.sh"
 sc_cross_check_PATH_usage_in_tests:
 	@if test -f $(_hv_file); then					\
-	  grep -l 'VERSION mismatch' $(_hv_file) >/dev/null		\
+	  $(GREP) -l 'VERSION mismatch' $(_hv_file) >/dev/null		\
 	    || { echo "$@: skipped: no such file: $(_hv_file)" 1>&2;	\
 		 exit 0; };						\
-	  grep -lE '$(_hv_regex_strong)' $(_hv_file) >/dev/null		\
+	  $(GREP) -lE '$(_hv_regex_strong)' $(_hv_file) >/dev/null	\
 	    || { echo "$@: $(_hv_file) lacks conforming use of init.sh" 1>&2; \
 		 exit 1; };						\
-	  good=$$(grep -E '$(_hv_regex_strong)' $(_hv_file));		\
-	  grep -LFx "$$good"						\
+	  good=$$($(GREP) -E '$(_hv_regex_strong)' $(_hv_file));	\
+	  $(GREP) -LFx "$$good"						\
 		$$($(VC_LIST_EXCEPT) | xargs -n $(VC_ARG_MAX)		\
-		  grep -lE '$(_hv_regex_weak)')				\
-	      | grep . &&						\
+		  $(GREP) -lE '$(_hv_regex_weak)')			\
+	      | $(GREP) . &&						\
 	    { echo "$(ME): the above files use path_prepend_ inconsistently" \
 		1>&2; exit 1; } || :;					\
 	fi
@@ -1443,7 +1443,7 @@ check: $(gl_public_submodule_commit)
 ALL_RECURSIVE_TARGETS += alpha beta stable
 alpha beta stable: $(local-check) writable-files $(submodule-checks)
 	$(AM_V_GEN)test $@ = stable					\
-	  && { echo $(VERSION) | grep -E '^[0-9]+(\.[0-9]+)+$$'		\
+	  && { echo $(VERSION) | $(GREP) -E '^[0-9]+(\.[0-9]+)+$$'	\
 	       || { echo "invalid version string: $(VERSION)" 1>&2; exit 1;};}\
 	  || :
 	$(AM_V_at)$(MAKE) vc-diff-check
@@ -1541,7 +1541,7 @@ refresh-gnulib-patches:
 	       -e 'END{defined $$d and print $$d}' bootstrap.conf);	\
 	  test -n "$$t" && gl=$$t;					\
 	fi;								\
-	for diff in $$(cd $$gl; git ls-files | grep '\.diff$$'); do	\
+	for diff in $$(cd $$gl; git ls-files | $(GREP) '\.diff$$'); do	\
 	  b=$$(printf %s "$$diff"|$(SED) 's/\.diff$$//');		\
 	  VERSION_CONTROL=none						\
 	    patch "$(gnulib_dir)/$$b" "$$gl/$$diff" || exit 1;		\
@@ -1584,7 +1584,7 @@ update-copyright-env ?=
 # in the file .x-update-copyright.
 .PHONY: update-copyright
 update-copyright:
-	$(AM_V_GEN)grep -l -w Copyright                                  \
+	$(AM_V_GEN)$(GREP) -l -w Copyright                               \
 	  $$(export VC_LIST_EXCEPT_DEFAULT=COPYING && $(VC_LIST_EXCEPT)) \
 	  | $(update-copyright-env) xargs $(srcdir)/$(_build-aux)/$@
 
@@ -1598,9 +1598,9 @@ _gl_TS_dir ?= src
 ALL_RECURSIVE_TARGETS += sc_tight_scope
 sc_tight_scope: tight-scope.mk
 	@fail=0;							\
-	if ! grep '^ *export _gl_TS_headers *=' $(srcdir)/cfg.mk	\
+	if ! $(GREP) '^ *export _gl_TS_headers *=' $(srcdir)/cfg.mk	\
 		> /dev/null						\
-	   && ! grep -w noinst_HEADERS $(srcdir)/$(_gl_TS_dir)/Makefile.am \
+	   && ! $(GREP) -w noinst_HEADERS $(srcdir)/$(_gl_TS_dir)/Makefile.am \
 		> /dev/null 2>&1; then					\
 	    echo '$(ME): skipping $@';					\
 	else								\
@@ -1672,12 +1672,12 @@ _gl_tight_scope: $(bin_PROGRAMS)
 	hdr=`for f in $(_gl_TS_headers); do				\
 	       test -f $$f && d= || d=$(srcdir)/; echo $$d$$f; done`;	\
 	( printf '%s\n' '__.*' $(_gl_TS_unmarked_extern_functions);	\
-	  grep -h -A1 '^extern .*[^;]$$' $$src				\
-	    | grep -vE '^(extern |--|#)' | $(SED) 's/ .*//; /^$$/d';	\
+	  $(GREP) -h -A1 '^extern .*[^;]$$' $$src			\
+	    | $(GREP) -vE '^(extern |--|#)' | $(SED) 's/ .*//; /^$$/d';	\
 	  perl -lne							\
 	     '$(_gl_TS_function_match) and print $$1' $$hdr;		\
 	) | sort -u | $(SED) "$$sed_wrap" > $$t;			\
-	nm -g $(_gl_TS_obj_files)|$(SED) -n 's/.* T //p'|grep -Ev -f $$t \
+	nm -g $(_gl_TS_obj_files)|$(SED) -n 's/.* T //p'|$(GREP) -Ev -f $$t \
 	  && { echo the above functions should have static scope >&2;	\
 	       exit 1; } || : ;						\
 	( printf '%s\n' '__.*' main $(_gl_TS_unmarked_extern_vars);	\
@@ -1685,7 +1685,7 @@ _gl_tight_scope: $(bin_PROGRAMS)
 		$$hdr $(_gl_TS_other_headers)				\
 	) | sort -u | $(SED) "$$sed_wrap" > $$t;			\
 	nm -g $(_gl_TS_obj_files) | $(SED) -n 's/.* [BCDGRS] //p'	\
-            | sort -u | grep -Ev -f $$t					\
+            | sort -u | $(GREP) -Ev -f $$t				\
 	  && { echo the above variables should have static scope >&2;	\
 	       exit 1; } || :
 # TS-end
-- 
2.19.1

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

[Bruno Haible]

Hi,

> The workaround is to split argument list into chunks that operating
> system can process. "getconf ARG_MAX" is used to determine size of the
> chunk.

Two questions on this:

1) People say that 'getconf ARG_MAX' returns the appromixate number
   of bytes in a command line. [1]
   But you use it with 'xargs -n', which gives a limit on the number of
   arguments. Shouldn't the patch use 'xargs -s' instead?

2) The really available values are slightly smaller.

   On Linux:
   $ getconf ARG_MAX
   2097152
   $ LC_ALL=C xargs --show-limits
   Your environment variables take up 4744 bytes
   POSIX upper limit on argument length (this system): 2090360
   POSIX smallest allowable upper limit on argument length (all systems): 4096
   Maximum length of command we could actually use: 2085616
   Size of command buffer we are actually using: 131072

   On FreeBSD/x86_64:
   $ getconf ARG_MAX
   262144
   $ LC_ALL=C xargs --show-limits
   Your environment variables take up 353 bytes
   POSIX upper limit on argument length (this system): 259743
   POSIX smallest allowable upper limit on argument length (all systems): 4096
   Maximum length of command we could actually use: 259390
   Size of command buffer we are actually using: 131072

   On macOS:
   $ getconf ARG_MAX
   262144
   $ LC_ALL=C xargs --show-limits
   Your environment variables take up 1262 bytes
   POSIX upper limit on argument length (this system): 258834
   POSIX smallest allowable upper limit on argument length (all systems): 4096
   Maximum length of command we could actually use: 257572
   Size of command buffer we are actually using: 131072

   How about being conservative and dividing the limit by 2, to avoid
   this margin error?

Could it be that your patch works only because xargs uses a command buffer
of length 131072, regardless of the value you pass to '-n'?

Bruno

[1] https://www.cyberciti.biz/faq/linux-unix-arg_max-maximum-length-of-arguments/

Re: [PATCH 2/2] maint.mk: Replace grep with $(GREP)

[Bruno Haible]

Roman Bolshakov wrote:
> Autoconf can discover an alias for GNU
> grep and set it in GREP but it takes no effect for maint.mk

Does Automake always invoke this Autoconf macro which sets GREP?

In other words, can you guarantee that $(GREP) will never expand to empty?

Bruno

Re: [PATCH 2/2] maint.mk: Replace grep with $(GREP)

[Eric Blake]

On 11/27/18 12:21 PM, Bruno Haible wrote:
> Roman Bolshakov wrote:
>> Autoconf can discover an alias for GNU
>> grep and set it in GREP but it takes no effect for maint.mk
> 
> Does Automake always invoke this Autoconf macro which sets GREP?
> 
> In other words, can you guarantee that $(GREP) will never expand to empty?

If we change gl_INIT() to AC_REQUIRE([AC_PROG_GREP]), then that should 
be sufficient to ensure $(GREP) is set by the time Makefile is parsed, 
which in turn will propagate to maint.mk.

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

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

[Bruno Haible]

Roman Bolshakov wrote:
>     if test -n "$$files"; then						\
>       if test -n "$$prohibit"; then					\
> -       grep $$with_grep_options $(_ignore_case) -nE "$$prohibit" $$files \
> +       echo "$$files" | xargs -n $(VC_ARG_MAX)				\
> +         grep $$with_grep_options $(_ignore_case) -nE "$$prohibit"	\
>           | grep -vE "$${exclude:-^$$}"					\
>           && { msg="$$halt" $(_sc_say_and_exit) } || :;			\

It is incorrect to transform

  grep OPTIONS FILES

to

  echo FILES | xargs -n N grep OPTIONS

because when the last chunk of FILES consists of just 1 file, 'grep'
produces different output. Instead, you need to transform it to

  echo FILES | xargs -n N grep OPTIONS /dev/null

See:

$ cd gnulib/modules

$ grep xalloc *-tests
acl-tests:xalloc
copy-file-tests:xalloc
c-xvasprintf-tests:xalloc
obstack-printf-tests:xalloc
regex-quote-tests:xalloc
userspec-tests:xalloc
xalloc-die-tests:tests/test-xalloc-die.c
xalloc-die-tests:tests/test-xalloc-die.sh
xalloc-die-tests:TESTS += test-xalloc-die.sh
xalloc-die-tests:check_PROGRAMS += test-xalloc-die
xalloc-die-tests:test_xalloc_die_LDADD = $(LDADD) @LIBINTL@

$ echo *-tests | xargs -n 1 grep xalloc
xalloc
xalloc
xalloc
xalloc
xalloc
xalloc
tests/test-xalloc-die.c
tests/test-xalloc-die.sh
TESTS += test-xalloc-die.sh
check_PROGRAMS += test-xalloc-die
test_xalloc_die_LDADD = $(LDADD) @LIBINTL@

$ echo *-tests | xargs -n 1 grep xalloc /dev/null
acl-tests:xalloc
copy-file-tests:xalloc
c-xvasprintf-tests:xalloc
obstack-printf-tests:xalloc
regex-quote-tests:xalloc
userspec-tests:xalloc
xalloc-die-tests:tests/test-xalloc-die.c
xalloc-die-tests:tests/test-xalloc-die.sh
xalloc-die-tests:TESTS += test-xalloc-die.sh
xalloc-die-tests:check_PROGRAMS += test-xalloc-die
xalloc-die-tests:test_xalloc_die_LDADD = $(LDADD) @LIBINTL@

Bruno

Re: [PATCH 2/2] maint.mk: Replace grep with $(GREP)

[Bruno Haible]

Eric Blake wrote:
> > In other words, can you guarantee that $(GREP) will never expand to empty?
> 
> If we change gl_INIT() to AC_REQUIRE([AC_PROG_GREP]), then that should 
> be sufficient to ensure $(GREP) is set by the time Makefile is parsed, 
> which in turn will propagate to maint.mk.

Yes. And (question to Roman): what the proper place to change, so that
gl_INIT() contains AC_REQUIRE([AC_PROG_GREP]) ?

Hint: The answer is contained in this part of the Gnulib documentation:
https://www.gnu.org/software/gnulib/manual/html_node/Writing-modules.html

Bruno

Re: [PATCH 2/2] maint.mk: Replace grep with $(GREP)

[Eric Blake]

On 11/27/18 12:47 PM, Bruno Haible wrote:

> Yes. And (question to Roman): what the proper place to change, so that
> gl_INIT() contains AC_REQUIRE([AC_PROG_GREP]) ?
> 
> Hint: The answer is contained in this part of the Gnulib documentation:
> https://www.gnu.org/software/gnulib/manual/html_node/Writing-modules.html

Another hint - modules/maintainer-makefile already ensures that $(SED) 
is non-empty :)

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

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

[Roman Bolshakov]

On Tue, Nov 27, 2018 at 07:19:43PM +0100, Bruno Haible wrote:
> Hi,
> 
> > The workaround is to split argument list into chunks that operating
> > system can process. "getconf ARG_MAX" is used to determine size of the
> > chunk.
> 
> Two questions on this:
> 
> 1) People say that 'getconf ARG_MAX' returns the appromixate number
>    of bytes in a command line. [1]
>    But you use it with 'xargs -n', which gives a limit on the number of
>    arguments. Shouldn't the patch use 'xargs -s' instead?
> 

Hi Bruno,

You're right about "-s", the patch should probably use it. I started
from "-n" with a reasonably low value of arguments (IIRC it was 8000
arguments), then tried to lookup if there's system limit of arguments. I
haven't found one, used ARG_MAX instead.

> 2) The really available values are slightly smaller.
> 
>    On Linux:
>    $ getconf ARG_MAX
>    2097152
>    $ LC_ALL=C xargs --show-limits
>    Your environment variables take up 4744 bytes
>    POSIX upper limit on argument length (this system): 2090360
>    POSIX smallest allowable upper limit on argument length (all systems): 4096
>    Maximum length of command we could actually use: 2085616
>    Size of command buffer we are actually using: 131072
> 
>    On FreeBSD/x86_64:
>    $ getconf ARG_MAX
>    262144
>    $ LC_ALL=C xargs --show-limits
>    Your environment variables take up 353 bytes
>    POSIX upper limit on argument length (this system): 259743
>    POSIX smallest allowable upper limit on argument length (all systems): 4096
>    Maximum length of command we could actually use: 259390
>    Size of command buffer we are actually using: 131072
> 
>    On macOS:
>    $ getconf ARG_MAX
>    262144
>    $ LC_ALL=C xargs --show-limits
>    Your environment variables take up 1262 bytes
>    POSIX upper limit on argument length (this system): 258834
>    POSIX smallest allowable upper limit on argument length (all systems): 4096
>    Maximum length of command we could actually use: 257572
>    Size of command buffer we are actually using: 131072
> 
>    How about being conservative and dividing the limit by 2, to avoid
>    this margin error?
> 
> Could it be that your patch works only because xargs uses a command buffer
> of length 131072, regardless of the value you pass to '-n'?
> 
> Bruno
> 
> [1] https://www.cyberciti.biz/faq/linux-unix-arg_max-maximum-length-of-arguments/
> 

This is an interesting coincidence. If we pick big enough value for "-n"
that drains command buffer length, the number of arguments is going
to be limited by default value of "-s" flag. And ARG_MAX of arguments
will always drain command buffer length up to the limit.

Here are related excerpts from macOS xargs man page:
  -n _number_
               Set the maximum number of arguments taken from standard
               input for each invocation of utility.  An invocation of
               utility will use less than _number_ standard input arguments
               if the number of bytes accumulated (see the -s option)
               exceeds the specified _size_ or there are fewer than _number_
               arguments remaining for the last invocation of utility.
               The current default value for _number_ is 5000.
  -s _size_
               Set the maximum number of bytes for the command line length
               provided to utility.  The sum of the length of the utility
               name, the arguments passed to utility (including NULL
               terminators) and the current environment will be less than
               or equal to this number.  The current default value for
               _size_ is ARG_MAX - 4096.

And from GNU xargs:
  -n _max-args_, --max-args=_max-args_
                Use  at most _max-args_ arguments per command line.
                Fewer than _max-args_ arguments will be used if the size
                (see the -s option) is exceeded, unless the -x option is
                given, in which case xargs will exit
  -s _max-chars_, --max-chars=_max-chars_
                Use  at  most _max-chars_ characters per command line,
                including the command and initial-arguments and the
                terminating nulls at the ends of the argument strings.
                The largest allowed value is system-dependent, and is
                calculated as the argument length limit for exec, less
                the size of your environment, less 2048 bytes of
                headroom.  If this value is more than 128KiB, 128Kib is
                used as the default value; otherwise, the default value
                is the maximum.  1KiB is 1024 bytes.  xargs
                automatically adapts to tighter constraints.


Given that even if the patch is kept as is it should work properly on
macOS, FreeBSD (the docs are very close to macOS xargs) and all systems
with GNU xargs. I can correct commit message to note the observation.

Alternatively, we can replace "-n" with "-s", as you pointed out in 1).
But then we will need to correct calculation of VC_ARG_MAX. We can take
formulae from [2]:
expr `getconf ARG_MAX` - `env|wc -c` - `env|egrep '^[^ ]+='|wc -l` \* 4 - 2048

But it's a bit higher than effective limit of command line buffer on
macOS/FreeBSD. To fix that we need to replace 2048 with 4096 in the
formulae (according to man page above), so I think the final VC_ARG_MAX
should be:
expr `getconf ARG_MAX` - `env|wc -c` - `env|egrep '^[^ ]+='|wc -l` \* 4 - 4096

[2] https://www.in-ulm.de/~mascheck/various/argmax/

Thank you,
Roman

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

[Roman Bolshakov]

On Tue, Nov 27, 2018 at 07:40:24PM +0100, Bruno Haible wrote:
> Roman Bolshakov wrote:
> >     if test -n "$$files"; then						\
> >       if test -n "$$prohibit"; then					\
> > -       grep $$with_grep_options $(_ignore_case) -nE "$$prohibit" $$files \
> > +       echo "$$files" | xargs -n $(VC_ARG_MAX)				\
> > +         grep $$with_grep_options $(_ignore_case) -nE "$$prohibit"	\
> >           | grep -vE "$${exclude:-^$$}"					\
> >           && { msg="$$halt" $(_sc_say_and_exit) } || :;			\
> 
> It is incorrect to transform
> 
>   grep OPTIONS FILES
> 
> to
> 
>   echo FILES | xargs -n N grep OPTIONS
> 
> because when the last chunk of FILES consists of just 1 file, 'grep'
> produces different output. Instead, you need to transform it to
> 
>   echo FILES | xargs -n N grep OPTIONS /dev/null
> 
> See:
> 
> $ cd gnulib/modules
> 
> $ grep xalloc *-tests
> acl-tests:xalloc
> copy-file-tests:xalloc
> c-xvasprintf-tests:xalloc
> obstack-printf-tests:xalloc
> regex-quote-tests:xalloc
> userspec-tests:xalloc
> xalloc-die-tests:tests/test-xalloc-die.c
> xalloc-die-tests:tests/test-xalloc-die.sh
> xalloc-die-tests:TESTS += test-xalloc-die.sh
> xalloc-die-tests:check_PROGRAMS += test-xalloc-die
> xalloc-die-tests:test_xalloc_die_LDADD = $(LDADD) @LIBINTL@
> 
> $ echo *-tests | xargs -n 1 grep xalloc
> xalloc
> xalloc
> xalloc
> xalloc
> xalloc
> xalloc
> tests/test-xalloc-die.c
> tests/test-xalloc-die.sh
> TESTS += test-xalloc-die.sh
> check_PROGRAMS += test-xalloc-die
> test_xalloc_die_LDADD = $(LDADD) @LIBINTL@
> 
> $ echo *-tests | xargs -n 1 grep xalloc /dev/null
> acl-tests:xalloc
> copy-file-tests:xalloc
> c-xvasprintf-tests:xalloc
> obstack-printf-tests:xalloc
> regex-quote-tests:xalloc
> userspec-tests:xalloc
> xalloc-die-tests:tests/test-xalloc-die.c
> xalloc-die-tests:tests/test-xalloc-die.sh
> xalloc-die-tests:TESTS += test-xalloc-die.sh
> xalloc-die-tests:check_PROGRAMS += test-xalloc-die
> xalloc-die-tests:test_xalloc_die_LDADD = $(LDADD) @LIBINTL@
> 
> Bruno
> 

Understood, I will add /dev/null as an extra argument.

Thank you,
Roman

Re: [PATCH 2/2] maint.mk: Replace grep with $(GREP)

[Roman Bolshakov]

On Tue, Nov 27, 2018 at 07:47:12PM +0100, Bruno Haible wrote:
> Eric Blake wrote:
> > > In other words, can you guarantee that $(GREP) will never expand to empty?
> > 
> > If we change gl_INIT() to AC_REQUIRE([AC_PROG_GREP]), then that should 
> > be sufficient to ensure $(GREP) is set by the time Makefile is parsed, 
> > which in turn will propagate to maint.mk.
> 
> Yes. And (question to Roman): what the proper place to change, so that
> gl_INIT() contains AC_REQUIRE([AC_PROG_GREP]) ?
> 
> Hint: The answer is contained in this part of the Gnulib documentation:
> https://www.gnu.org/software/gnulib/manual/html_node/Writing-modules.html
> 
> Bruno
> 

I'm quite new to gnulib but thanks to Eric and your comments that should do it:

diff --git a/modules/maintainer-makefile b/modules/maintainer-makefile
index 39b51583c..13b8c546a 100644
--- a/modules/maintainer-makefile
+++ b/modules/maintainer-makefile
@@ -14,6 +14,7 @@ configure.ac:
 AC_CONFIG_COMMANDS_PRE([m4_ifdef([AH_HEADER],
   [AC_SUBST([CONFIG_INCLUDE], m4_defn([AH_HEADER]))])])
 AC_REQUIRE([AC_PROG_SED])
+AC_REQUIRE([AC_PROG_GREP])

 Makefile.am:
 EXTRA_DIST += $(top_srcdir)/maint.mk

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

[Roman Bolshakov]

Hi Bruno,

May I ask you to review what way we should go with ARG_MAX?

I'm okay with both ways whether it's:
 * computing effective argument length and passing it to "-s" option;
 * or exploiting behaviour of GNU/BSD xargs and specifying "-n" beyond
   the limit.

Thank you,
Roman

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

[Bruno Haible]

Roman Bolshakov wrote:
> But then we will need to correct calculation of VC_ARG_MAX. We can take
> formulae from [2]:
> expr `getconf ARG_MAX` - `env|wc -c` - `env|egrep '^[^ ]+='|wc -l` \* 4 - 2048

This formula assumes that a pointer in the 'environ' array is 4 bytes long.
On 64-bit platforms it surely is 8 bytes long.

More generally, I find this formula too fragile. It assumes so many things.
I would prefer a formula which does not attempt to produce the highest possible
value, but makes less assumptions. How about
  expr `getconf ARG_MAX` / 2
?

Bruno

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

[Bruno Haible]

Hi Roman,

> May I ask you to review what way we should go with ARG_MAX?
> 
> I'm okay with both ways whether it's:
>  * computing effective argument length and passing it to "-s" option;
>  * or exploiting behaviour of GNU/BSD xargs and specifying "-n" beyond
>    the limit.

Use the approach that makes the least undocumented assumptions. In other
words, rely on what the documentation says and on nothing else. The
relevant documentation here is POSIX [1].

Bruno

[1] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/xargs.html

Re: [PATCH 2/2] maint.mk: Replace grep with $(GREP)

[Bruno Haible]

Roman Bolshakov wrote:
> I'm quite new to gnulib but thanks to Eric and your comments that should do it:
> 
> diff --git a/modules/maintainer-makefile b/modules/maintainer-makefile
> index 39b51583c..13b8c546a 100644
> --- a/modules/maintainer-makefile
> +++ b/modules/maintainer-makefile
> @@ -14,6 +14,7 @@ configure.ac:
>  AC_CONFIG_COMMANDS_PRE([m4_ifdef([AH_HEADER],
>    [AC_SUBST([CONFIG_INCLUDE], m4_defn([AH_HEADER]))])])
>  AC_REQUIRE([AC_PROG_SED])
> +AC_REQUIRE([AC_PROG_GREP])
> 
>  Makefile.am:
>  EXTRA_DIST += $(top_srcdir)/maint.mk
> 

Yes, this will do it.

Can you please resubmit the entire patch 'maint.mk: Replace grep with $(GREP)'
as a whole?

Bruno

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

[Bernhard Voelker]

On 11/30/18 12:14 PM, Roman Bolshakov wrote:
> May I ask you to review what way we should go with ARG_MAX?
> 
> I'm okay with both ways whether it's:
>  * computing effective argument length and passing it to "-s" option;
>  * or exploiting behaviour of GNU/BSD xargs and specifying "-n" beyond
>    the limit.

Actually, xargs (and any implementation of it) cares about the limit
itself.  That's what it is made for.

You would limit the number of args with "-n" if the executed program
can only handle up to that number, or if the logic requires it, e.g.
when input comes in as pairs:
  $ seq 6 | xargs -n2 echo diff -u
  diff -u 1 2
  diff -u 3 4
  diff -u 5 6

There's no need to worry about the other end of the range.
So in your patch, just omit the -n (and getconf).

Have a nice day,
Berny

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

[Roman Bolshakov]

On Mon, Dec 03, 2018 at 08:16:22AM +0100, Bernhard Voelker wrote:
> On 11/30/18 12:14 PM, Roman Bolshakov wrote:
> > May I ask you to review what way we should go with ARG_MAX?
> > 
> > I'm okay with both ways whether it's:
> >  * computing effective argument length and passing it to "-s" option;
> >  * or exploiting behaviour of GNU/BSD xargs and specifying "-n" beyond
> >    the limit.
> 
> Actually, xargs (and any implementation of it) cares about the limit
> itself.  That's what it is made for.
> 
> You would limit the number of args with "-n" if the executed program
> can only handle up to that number, or if the logic requires it, e.g.
> when input comes in as pairs:
>   $ seq 6 | xargs -n2 echo diff -u
>   diff -u 1 2
>   diff -u 3 4
>   diff -u 5 6
> 
> There's no need to worry about the other end of the range.
> So in your patch, just omit the -n (and getconf).
> 

Hi Berny,

I like the approach. I was thinking of it while reading xargs manual but
forgot to write it down. Since we need to compute the limit only for
xargs we can omit it altogether.

Thank you,
Roman

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

[Roman Bolshakov]

On Sun, Dec 02, 2018 at 01:26:52PM +0100, Bruno Haible wrote:
> Roman Bolshakov wrote:
> > But then we will need to correct calculation of VC_ARG_MAX. We can take
> > formulae from [2]:
> > expr `getconf ARG_MAX` - `env|wc -c` - `env|egrep '^[^ ]+='|wc -l` \* 4 - 2048
> 
> This formula assumes that a pointer in the 'environ' array is 4 bytes long.
> On 64-bit platforms it surely is 8 bytes long.
> 
> More generally, I find this formula too fragile. It assumes so many things.
> I would prefer a formula which does not attempt to produce the highest possible
> value, but makes less assumptions. How about
>   expr `getconf ARG_MAX` / 2
> ?
> 

Hi Bruno,

I agree the command knows too much. I wasn't sure if it's important for
someone if we use maximum possible value.

I've found some prior art in computing max effective argument length in
libtool (3/4 of ARG_MAX on GNU/BSD):
http://git.savannah.gnu.org/cgit/libtool.git/tree/m4/libtool.m4#n1682

If someone needs the value, probably it's worth to move the computation
from libtool to gnulib.

Thank you,
Roman

Re: [PATCH 2/2] maint.mk: Replace grep with $(GREP)

[Roman Bolshakov]

On Sun, Dec 02, 2018 at 01:31:03PM +0100, Bruno Haible wrote:
> Roman Bolshakov wrote:
> > I'm quite new to gnulib but thanks to Eric and your comments that should do it:
> > 
> > diff --git a/modules/maintainer-makefile b/modules/maintainer-makefile
> > index 39b51583c..13b8c546a 100644
> > --- a/modules/maintainer-makefile
> > +++ b/modules/maintainer-makefile
> > @@ -14,6 +14,7 @@ configure.ac:
> >  AC_CONFIG_COMMANDS_PRE([m4_ifdef([AH_HEADER],
> >    [AC_SUBST([CONFIG_INCLUDE], m4_defn([AH_HEADER]))])])
> >  AC_REQUIRE([AC_PROG_SED])
> > +AC_REQUIRE([AC_PROG_GREP])
> > 
> >  Makefile.am:
> >  EXTRA_DIST += $(top_srcdir)/maint.mk
> > 
> 
> Yes, this will do it.
> 
> Can you please resubmit the entire patch 'maint.mk: Replace grep with $(GREP)'
> as a whole?
> 

I will send v2 for the patchset shortly.

Best regards,
Roman