[libvirt] [PATCH v2 0/3] Better syntax-check on BSD
Since v1: fix the gnulib bug I accidentally introduced, then copy the gist of Roman's gnulib changes to also apply to our cfg.mk syntax checks. Eric Blake (3): maint: update gnulib for syntax-check on BSD maint: prefer $(GREP) in cfg.mk maint: split long lines for BSD syntax-check .gnulib | 2 +- cfg.mk | 95 +++++++++++++++++++++++++++++---------------------------- 2 files changed, 49 insertions(+), 48 deletions(-) -- 2.20.1
In particular, this incorporates Roman's patches to allow 'make syntax-check' to work on BSD with its exec argv limitations that previously failed when trying to grep the large number of files present in libvirt. cfg.mk needs similar changes, but that will be tackled separately. Signed-off-by: Eric Blake <eblake@redhat.com> --- .gnulib | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gnulib b/.gnulib index 4652c7bafa..70eb4687b1 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit 4652c7bafa60332145f1e05a7de5f48e1bc56226 +Subproject commit 70eb4687b1d84ae780bbf2b8141bee07c85e6d3b -- 2.20.1
On Thu, Jan 03, 2019 at 01:41:57PM -0600, Eric Blake wrote:
In particular, this incorporates Roman's patches to allow 'make syntax-check' to work on BSD with its exec argv limitations that previously failed when trying to grep the large number of files present in libvirt.
cfg.mk needs similar changes, but that will be tackled separately.
Signed-off-by: Eric Blake <eblake@redhat.com> --- .gnulib | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
We already used $(GREP) in some places, but might as well use it everywhere during syntax check, in line with similar recent gnulib changes. --- cfg.mk | 54 +++++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/cfg.mk b/cfg.mk index 90ec8dfc60..9956d20034 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1,5 +1,5 @@ # Customize Makefile.maint. -*- makefile -*- -# Copyright (C) 2008-2015 Red Hat, Inc. +# Copyright (C) 2008-2019 Red Hat, Inc. # Copyright (C) 2003-2008 Free Software Foundation, Inc. # This program is free software: you can redistribute it and/or modify @@ -305,7 +305,7 @@ sc_flags_usage: $(srcdir)/include/libvirt/libvirt-qemu.h \ $(srcdir)/include/libvirt/libvirt-lxc.h \ $(srcdir)/include/libvirt/libvirt-admin.h \ - | grep -c '\(long\|unsigned\) flags')" != 4 && \ + | $(GREP) -c '\(long\|unsigned\) flags')" != 4 && \ { echo '$(ME): new API should use "unsigned int flags"' 1>&2; \ exit 1; } || : @prohibit=' flags ATTRIBUTE_UNUSED' \ @@ -639,10 +639,10 @@ sc_libvirt_unmarked_diagnostics: exclude='_\(' \ halt='found unmarked diagnostic(s)' \ $(_sc_search_regexp) - @{ grep -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ - grep -A1 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ + @{ $(GREP) -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ + $(GREP) -A1 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ | $(SED) 's/_("\([^\"]\|\\.\)\+"//;s/[ ]"%s"//' \ - | grep '[ ]"' && \ + | $(GREP) '[ ]"' && \ { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \ exit 1; } || : @@ -654,9 +654,9 @@ sc_libvirt_unmarked_diagnostics: # there are functions to which this one applies but that do not get marked # diagnostics. sc_prohibit_newline_at_end_of_diagnostic: - @grep -A2 -nE \ + @$(GREP) -A2 -nE \ '\<$(func_re) *\(' $$($(VC_LIST_EXCEPT)) \ - | grep '\\n"' \ + | $(GREP) '\\n"' \ && { echo '$(ME): newline at end of message(s)' 1>&2; \ exit 1; } || : @@ -664,12 +664,12 @@ sc_prohibit_newline_at_end_of_diagnostic: # allow VIR_ERROR to do this, and ignore functions that take a single # string rather than a format argument. sc_prohibit_diagnostic_without_format: - @{ grep -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ - grep -A2 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ + @{ $(GREP) -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ + $(GREP) -A2 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ | $(SED) -rn -e ':l; /[,"]$$/ {N;b l;}' \ -e '/(xenapiSessionErrorHandler|vah_(error|warning))/d' \ -e '/\<$(func_re) *\([^"]*"([^%"]|"\n[^"]*")*"[,)]/p' \ - | grep -vE 'VIR_ERROR' && \ + | $(GREP) -vE 'VIR_ERROR' && \ { echo '$(ME): found diagnostic without %' 1>&2; \ exit 1; } || : @@ -687,16 +687,16 @@ sc_prohibit_useless_translation: # When splitting a diagnostic across lines, ensure that there is a space # or \n on one side of the split. sc_require_whitespace_in_translation: - @grep -n -A1 '"$$' $$($(VC_LIST_EXCEPT)) \ + @$(GREP) -n -A1 '"$$' $$($(VC_LIST_EXCEPT)) \ | $(SED) -ne ':l; /"$$/ {N;b l;}; s/"\n[^"]*"/""/g; s/\\n/ /g' \ - -e '/_(.*[^\ ]""[^\ ]/p' | grep . && \ + -e '/_(.*[^\ ]""[^\ ]/p' | $(GREP) . && \ { echo '$(ME): missing whitespace at line split' 1>&2; \ exit 1; } || : # Enforce recommended preprocessor indentation style. sc_preprocessor_indentation: @if cppi --version >/dev/null 2>&1; then \ - $(VC_LIST_EXCEPT) | grep -E '\.[ch](\.in)?$$' | xargs cppi -a -c \ + $(VC_LIST_EXCEPT) | $(GREP) -E '\.[ch](\.in)?$$' | xargs cppi -a -c \ || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \ exit 1; }; \ else \ @@ -707,13 +707,13 @@ sc_preprocessor_indentation: # (comment-only) C file that mirrors the same layout as the spec file. sc_spec_indentation: @if cppi --version >/dev/null 2>&1; then \ - for f in $$($(VC_LIST_EXCEPT) | grep '\.spec\.in$$'); do \ + for f in $$($(VC_LIST_EXCEPT) | $(GREP) '\.spec\.in$$'); do \ $(SED) -e 's|#|// #|; s|%ifn*\(arch\)* |#if a // |' \ -e 's/%\(else\|endif\|define\)/#\1/' \ -e 's/^\( *\)\1\1\1#/#\1/' \ -e 's|^\( *[^#/ ]\)|// \1|; s|^\( */[^/]\)|// \1|' $$f \ | cppi -a -c 2>&1 | $(SED) "s|standard input|$$f|"; \ - done | { if grep . >&2; then false; else :; fi; } \ + done | { if $(GREP) . >&2; then false; else :; fi; } \ || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \ exit 1; }; \ else \ @@ -803,11 +803,11 @@ sc_prohibit_cross_inclusion: # When converting an enum to a string, make sure that we track any new # elements added to the enum by using a _LAST marker. sc_require_enum_last_marker: - @grep -A1 -nE '^[^#]*VIR_ENUM_IMPL *\(' $$($(VC_LIST_EXCEPT)) \ + @$(GREP) -A1 -nE '^[^#]*VIR_ENUM_IMPL *\(' $$($(VC_LIST_EXCEPT)) \ | $(SED) -ne '/VIR_ENUM_IMPL[^,]*,$$/N' \ -e '/VIR_ENUM_IMPL[^,]*,[^,]*[^_,][^L,][^A,][^S,][^T,],/p' \ -e '/VIR_ENUM_IMPL[^,]*,[^,]\{0,4\},/p' \ - | grep . && \ + | $(GREP) . && \ { echo '$(ME): enum impl needs to use _LAST marker' 1>&2; \ exit 1; } || : @@ -883,7 +883,7 @@ sc_prohibit_wrong_filename_in_comment: if (fail == 1) { \ exit 1; \ } \ - }' $$($(VC_LIST_EXCEPT) | grep '\.[ch]$$') || fail=1; \ + }' $$($(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$') || fail=1; \ if test $$fail -eq 1; then \ { echo '$(ME): The file name in comments must match the' \ 'actual file name' 1>&2; exit 1; } \ @@ -918,7 +918,7 @@ sc_require_if_else_matching_braces: $(_sc_search_regexp) sc_curly_braces_style: - @files=$$($(VC_LIST_EXCEPT) | grep '\.[ch]$$'); \ + @files=$$($(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$'); \ if $(GREP) -nHP \ '^\s*(?!([a-zA-Z_]*for_?each[a-zA-Z_]*) ?\()([_a-zA-Z0-9]+( [_a-zA-Z0-9]+)* ?\()?(\*?[_a-zA-Z0-9]+(,? \*?[_a-zA-Z0-9\[\]]+)+|void)\) ?\{' \ $$files; then \ @@ -931,7 +931,7 @@ sc_curly_braces_style: fi sc_prohibit_windows_special_chars_in_filename: - @files=$$($(VC_LIST_EXCEPT) | grep '[:*?"<>|]'); \ + @files=$$($(VC_LIST_EXCEPT) | $(GREP) '[:*?"<>|]'); \ test -n "$$files" && { echo '$(ME): Windows special chars' \ 'in filename not allowed:' 1>&2; echo $$files 1>&2; exit 1; } || : @@ -996,8 +996,8 @@ sc_prohibit_sysconf_pagesize: $(_sc_search_regexp) sc_prohibit_virSecurity: - @grep -Pn 'virSecurityManager(?!Ptr)' $$($(VC_LIST_EXCEPT) | grep 'src/qemu/' | \ - grep -v 'src/qemu/qemu_security') && \ + @$(GREP) -Pn 'virSecurityManager(?!Ptr)' $$($(VC_LIST_EXCEPT) | $(GREP) 'src/qemu/' | \ + $(GREP) -v 'src/qemu/qemu_security') && \ { echo '$(ME): prefer qemuSecurity wrappers' 1>&2; exit 1; } || : sc_prohibit_pthread_create: @@ -1128,24 +1128,24 @@ endif # Don't include duplicate header in the source (either *.c or *.h) prohibit-duplicate-header: - $(AM_V_GEN)files=$$($(VC_LIST_EXCEPT) | grep '\.[chx]$$'); \ + $(AM_V_GEN)files=$$($(VC_LIST_EXCEPT) | $(GREP) '\.[chx]$$'); \ $(PERL) -W $(top_srcdir)/build-aux/prohibit-duplicate-header.pl $$files spacing-check: - $(AM_V_GEN)files=`$(VC_LIST) | grep '\.c$$'`; \ + $(AM_V_GEN)files=`$(VC_LIST) | $(GREP) '\.c$$'`; \ $(PERL) $(top_srcdir)/build-aux/check-spacing.pl $$files || \ { echo '$(ME): incorrect formatting' 1>&2; exit 1; } mock-noinline: - $(AM_V_GEN)files=`$(VC_LIST) | grep '\.[ch]$$'`; \ + $(AM_V_GEN)files=`$(VC_LIST) | $(GREP) '\.[ch]$$'`; \ $(PERL) $(top_srcdir)/build-aux/mock-noinline.pl $$files header-ifdef: - $(AM_V_GEN)files=`$(VC_LIST) | grep '\.[h]$$'`; \ + $(AM_V_GEN)files=`$(VC_LIST) | $(GREP) '\.[h]$$'`; \ $(PERL) $(top_srcdir)/build-aux/header-ifdef.pl $$files test-wrap-argv: - $(AM_V_GEN)files=`$(VC_LIST) | grep -E '\.(ldargs|args)'`; \ + $(AM_V_GEN)files=`$(VC_LIST) | $(GREP) -E '\.(ldargs|args)'`; \ $(PERL) $(top_srcdir)/tests/test-wrap-argv.pl --check $$files group-qemu-caps: -- 2.20.1
On Thu, Jan 03, 2019 at 01:41:58PM -0600, Eric Blake wrote:
We already used $(GREP) in some places, but might as well use it everywhere during syntax check, in line with similar recent gnulib changes. --- cfg.mk | 54 +++++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 27 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Similar to the gnulib changes we just incorporated into maint.mk, it's time to use '$(VC_LIST) | xargs program' instead of 'program $$($(VC_LIST))', in order to bypass the problem of hitting argv limits due to our large set of files. Drop several uses of $$files as a temporary variable when we can instead directly use xargs. While at it, fix a typo in the prohibit_windows_special_chars error message. Signed-off-by: Eric Blake <eblake@redhat.com> --- cfg.mk | 75 +++++++++++++++++++++++++++++----------------------------- 1 file changed, 38 insertions(+), 37 deletions(-) diff --git a/cfg.mk b/cfg.mk index 9956d20034..e2702d50c9 100644 --- a/cfg.mk +++ b/cfg.mk @@ -639,8 +639,10 @@ sc_libvirt_unmarked_diagnostics: exclude='_\(' \ halt='found unmarked diagnostic(s)' \ $(_sc_search_regexp) - @{ $(GREP) -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ - $(GREP) -A1 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ + @{ $(VC_LIST_EXCEPT) | xargs \ + $(GREP) -nE '\<$(func_re) *\(.*;$$' /dev/null; \ + $(VC_LIST_EXCEPT) | xargs \ + $(GREP) -A1 -nE '\<$(func_re) *\(.*,$$' /dev/null; } \ | $(SED) 's/_("\([^\"]\|\\.\)\+"//;s/[ ]"%s"//' \ | $(GREP) '[ ]"' && \ { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \ @@ -654,8 +656,8 @@ sc_libvirt_unmarked_diagnostics: # there are functions to which this one applies but that do not get marked # diagnostics. sc_prohibit_newline_at_end_of_diagnostic: - @$(GREP) -A2 -nE \ - '\<$(func_re) *\(' $$($(VC_LIST_EXCEPT)) \ + @$(VC_LIST_EXCEPT) | xargs $(GREP) -A2 -nE \ + '\<$(func_re) *\(' /dev/null \ | $(GREP) '\\n"' \ && { echo '$(ME): newline at end of message(s)' 1>&2; \ exit 1; } || : @@ -664,8 +666,10 @@ sc_prohibit_newline_at_end_of_diagnostic: # allow VIR_ERROR to do this, and ignore functions that take a single # string rather than a format argument. sc_prohibit_diagnostic_without_format: - @{ $(GREP) -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ - $(GREP) -A2 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ + @{ $(VC_LIST_EXCEPT) | xargs \ + $(GREP) -nE '\<$(func_re) *\(.*;$$' /dev/null; \ + $(VC_LIST_EXCEPT) | xargs \ + $(GREP) -A2 -nE '\<$(func_re) *\(.*,$$' /dev/null; } \ | $(SED) -rn -e ':l; /[,"]$$/ {N;b l;}' \ -e '/(xenapiSessionErrorHandler|vah_(error|warning))/d' \ -e '/\<$(func_re) *\([^"]*"([^%"]|"\n[^"]*")*"[,)]/p' \ @@ -687,7 +691,7 @@ sc_prohibit_useless_translation: # When splitting a diagnostic across lines, ensure that there is a space # or \n on one side of the split. sc_require_whitespace_in_translation: - @$(GREP) -n -A1 '"$$' $$($(VC_LIST_EXCEPT)) \ + @$(VC_LIST_EXCEPT) | xargs $(GREP) -n -A1 '"$$' /dev/null \ | $(SED) -ne ':l; /"$$/ {N;b l;}; s/"\n[^"]*"/""/g; s/\\n/ /g' \ -e '/_(.*[^\ ]""[^\ ]/p' | $(GREP) . && \ { echo '$(ME): missing whitespace at line split' 1>&2; \ @@ -803,7 +807,8 @@ sc_prohibit_cross_inclusion: # When converting an enum to a string, make sure that we track any new # elements added to the enum by using a _LAST marker. sc_require_enum_last_marker: - @$(GREP) -A1 -nE '^[^#]*VIR_ENUM_IMPL *\(' $$($(VC_LIST_EXCEPT)) \ + @$(VC_LIST_EXCEPT) | xargs \ + $(GREP) -A1 -nE '^[^#]*VIR_ENUM_IMPL *\(' /dev/null \ | $(SED) -ne '/VIR_ENUM_IMPL[^,]*,$$/N' \ -e '/VIR_ENUM_IMPL[^,]*,[^,]*[^_,][^L,][^A,][^S,][^T,],/p' \ -e '/VIR_ENUM_IMPL[^,]*,[^,]\{0,4\},/p' \ @@ -866,8 +871,7 @@ sc_prohibit_atoi: $(_sc_search_regexp) sc_prohibit_wrong_filename_in_comment: - @fail=0; \ - awk 'BEGIN { \ + @$(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$' | xargs awk 'BEGIN { \ fail=0; \ } FNR < 3 { \ n=match($$0, /[[:space:]][^[:space:]]*[.][ch][[:space:]:]/); \ @@ -883,11 +887,8 @@ sc_prohibit_wrong_filename_in_comment: if (fail == 1) { \ exit 1; \ } \ - }' $$($(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$') || fail=1; \ - if test $$fail -eq 1; then \ - { echo '$(ME): The file name in comments must match the' \ - 'actual file name' 1>&2; exit 1; } \ - fi; + }' || { echo '$(ME): The file name in comments must match the' \ + 'actual file name' 1>&2; exit 1; } sc_prohibit_virConnectOpen_in_virsh: @prohibit='\bvirConnectOpen[a-zA-Z]* *\(' \ @@ -918,22 +919,21 @@ sc_require_if_else_matching_braces: $(_sc_search_regexp) sc_curly_braces_style: - @files=$$($(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$'); \ - if $(GREP) -nHP \ + @if $(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$' | xargs $(GREP) -nHP \ '^\s*(?!([a-zA-Z_]*for_?each[a-zA-Z_]*) ?\()([_a-zA-Z0-9]+( [_a-zA-Z0-9]+)* ?\()?(\*?[_a-zA-Z0-9]+(,? \*?[_a-zA-Z0-9\[\]]+)+|void)\) ?\{' \ - $$files; then \ + /dev/null; then \ echo '$(ME): Non-K&R style used for curly braces around' \ 'function body' 1>&2; exit 1; \ fi; \ - if $(GREP) -A1 -En ' ((if|for|while|switch) \(|(else|do)\b)[^{]*$$'\ - $$files | $(GREP) '^[^ ]*- *{'; then \ + if $(VC_LIST_EXCEPT) | $(GREP) '\.[ch]$$' | xargs \ + $(GREP) -A1 -En ' ((if|for|while|switch) \(|(else|do)\b)[^{]*$$' \ + /dev/null | $(GREP) '^[^ ]*- *{'; then \ echo '$(ME): Use hanging braces for compound statements' 1>&2; exit 1; \ fi sc_prohibit_windows_special_chars_in_filename: - @files=$$($(VC_LIST_EXCEPT) | $(GREP) '[:*?"<>|]'); \ - test -n "$$files" && { echo '$(ME): Windows special chars' \ - 'in filename not allowed:' 1>&2; echo $$files 1>&2; exit 1; } || : + @$(VC_LIST_EXCEPT) | $(GREP) '[:*?"<>|]' && \ + { echo '$(ME): Windows special chars in filename not allowed' 1>&2; echo exit 1; } || : sc_prohibit_mixed_case_abbreviations: @prohibit='Pci|Usb|Scsi' \ @@ -949,11 +949,11 @@ sc_require_locale_h: $(_sc_search_regexp) sc_prohibit_empty_first_line: - @awk 'BEGIN { fail=0; } \ + @$(VC_LIST_EXCEPT) | xargs awk 'BEGIN { fail=0; } \ FNR == 1 { if ($$0 == "") { print FILENAME ":1:"; fail=1; } } \ END { if (fail == 1) { \ print "$(ME): Prohibited empty first line" > "/dev/stderr"; \ - } exit fail; }' $$($(VC_LIST_EXCEPT)); + } exit fail; }' sc_prohibit_paren_brace: @prohibit='\)\{$$' \ @@ -996,8 +996,9 @@ sc_prohibit_sysconf_pagesize: $(_sc_search_regexp) sc_prohibit_virSecurity: - @$(GREP) -Pn 'virSecurityManager(?!Ptr)' $$($(VC_LIST_EXCEPT) | $(GREP) 'src/qemu/' | \ - $(GREP) -v 'src/qemu/qemu_security') && \ + @$(VC_LIST_EXCEPT) | $(GREP) 'src/qemu/' | \ + $(GREP) -v 'src/qemu/qemu_security' | \ + xargs $(GREP) -Pn 'virSecurityManager(?!Ptr)' /dev/null && \ { echo '$(ME): prefer qemuSecurity wrappers' 1>&2; exit 1; } || : sc_prohibit_pthread_create: @@ -1128,25 +1129,25 @@ endif # Don't include duplicate header in the source (either *.c or *.h) prohibit-duplicate-header: - $(AM_V_GEN)files=$$($(VC_LIST_EXCEPT) | $(GREP) '\.[chx]$$'); \ - $(PERL) -W $(top_srcdir)/build-aux/prohibit-duplicate-header.pl $$files + $(AM_V_GEN)$(VC_LIST_EXCEPT) | $(GREP) '\.[chx]$$' | xargs \ + $(PERL) -W $(top_srcdir)/build-aux/prohibit-duplicate-header.pl spacing-check: - $(AM_V_GEN)files=`$(VC_LIST) | $(GREP) '\.c$$'`; \ - $(PERL) $(top_srcdir)/build-aux/check-spacing.pl $$files || \ + $(AM_V_GEN)$(VC_LIST) | $(GREP) '\.c$$' | xargs \ + $(PERL) $(top_srcdir)/build-aux/check-spacing.pl || \ { echo '$(ME): incorrect formatting' 1>&2; exit 1; } mock-noinline: - $(AM_V_GEN)files=`$(VC_LIST) | $(GREP) '\.[ch]$$'`; \ - $(PERL) $(top_srcdir)/build-aux/mock-noinline.pl $$files + $(AM_V_GEN)$(VC_LIST) | $(GREP) '\.[ch]$$' | xargs \ + $(PERL) $(top_srcdir)/build-aux/mock-noinline.pl header-ifdef: - $(AM_V_GEN)files=`$(VC_LIST) | $(GREP) '\.[h]$$'`; \ - $(PERL) $(top_srcdir)/build-aux/header-ifdef.pl $$files + $(AM_V_GEN)$(VC_LIST) | $(GREP) '\.[h]$$' | xargs \ + $(PERL) $(top_srcdir)/build-aux/header-ifdef.pl test-wrap-argv: - $(AM_V_GEN)files=`$(VC_LIST) | $(GREP) -E '\.(ldargs|args)'`; \ - $(PERL) $(top_srcdir)/tests/test-wrap-argv.pl --check $$files + $(AM_V_GEN)$(VC_LIST) | $(GREP) -E '\.(ldargs|args)' | xargs \ + $(PERL) $(top_srcdir)/tests/test-wrap-argv.pl --check group-qemu-caps: $(AM_V_GEN)$(PERL) $(top_srcdir)/tests/group-qemu-caps.pl --check $(top_srcdir)/ -- 2.20.1
On Thu, Jan 03, 2019 at 01:41:59PM -0600, Eric Blake wrote:
Similar to the gnulib changes we just incorporated into maint.mk, it's time to use '$(VC_LIST) | xargs program' instead of 'program $$($(VC_LIST))', in order to bypass the problem of hitting argv limits due to our large set of files.
Drop several uses of $$files as a temporary variable when we can instead directly use xargs. While at it, fix a typo in the prohibit_windows_special_chars error message.
Signed-off-by: Eric Blake <eblake@redhat.com> --- cfg.mk | 75 +++++++++++++++++++++++++++++----------------------------- 1 file changed, 38 insertions(+), 37 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 9956d20034..e2702d50c9 100644 --- a/cfg.mk +++ b/cfg.mk @@ -639,8 +639,10 @@ sc_libvirt_unmarked_diagnostics: exclude='_\(' \ halt='found unmarked diagnostic(s)' \ $(_sc_search_regexp) - @{ $(GREP) -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ - $(GREP) -A1 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ + @{ $(VC_LIST_EXCEPT) | xargs \ + $(GREP) -nE '\<$(func_re) *\(.*;$$' /dev/null; \ + $(VC_LIST_EXCEPT) | xargs \ + $(GREP) -A1 -nE '\<$(func_re) *\(.*,$$' /dev/null; } \
Not sure why the /dev/null is needed. If the syntax check rule were to operate on an empty list of files, we can just delete it.
| $(SED) 's/_("\([^\"]\|\\.\)\+"//;s/[ ]"%s"//' \ | $(GREP) '[ ]"' && \ { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \
With the /dev/null changes removed or justified: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On 1/7/19 5:00 AM, Ján Tomko wrote:
On Thu, Jan 03, 2019 at 01:41:59PM -0600, Eric Blake wrote:
Similar to the gnulib changes we just incorporated into maint.mk, it's time to use '$(VC_LIST) | xargs program' instead of 'program $$($(VC_LIST))', in order to bypass the problem of hitting argv limits due to our large set of files.
Drop several uses of $$files as a temporary variable when we can instead directly use xargs. While at it, fix a typo in the prohibit_windows_special_chars error message.
- @{ $(GREP) -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ - $(GREP) -A1 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ + @{ $(VC_LIST_EXCEPT) | xargs \ + $(GREP) -nE '\<$(func_re) *\(.*;$$' /dev/null; \ + $(VC_LIST_EXCEPT) | xargs \ + $(GREP) -A1 -nE '\<$(func_re) *\(.*,$$' /dev/null; } \
Not sure why the /dev/null is needed.
Because of grep semantics. grep pattern file1 grep pattern file1 file2 have different output, and the syntax checkers depend on the grep output that is produced when there are multiple files on the command line. The canonical rewrite of 'grep pattern $(list of files)" into xargs uses "list of files | xargs grep pattern /dev/null", because you presume that 'list of files' produced a non-empty list, but worry that xargs might do a worst-case split into 'grep pattern all but one; grep pattern last', where the different invocation of the last file name alone produces unusual output that messes up the subsequent pipeline. Putting /dev/null in the command line argument to grep ensures that we instead get a split into 'grep pattern /dev/null all but one; grep pattern /dev/null last' where all invocations are with at least two file names. The case of 0 files is different - for that, you would normally use 'xargs -r' to ensure that grep is never called with 0 arguments. But since grep'ing /dev/null will never produce output, we don't need to worry about xargs -r, even if we could end up with an empty list. And since the old code wasn't worrying about an empty list, I didn't see a reason to worry about an empty list in the new code either.
If the syntax check rule were to operate on an empty list of files, we can just delete it.
| $(SED) 's/_("\([^\"]\|\\.\)\+"//;s/[ ]"%s"//' \ | $(GREP) '[ ]"' && \ { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \
With the /dev/null changes removed or justified:
I hope that was the justification you were looking for (and it matches the gnulib rewrite that uses /dev/null in every conversion to xargs grep where grep's output differs based on number of file names present - note that 'grep -L' does not have different output, so it doesn't need the /dev/null trick, but we didn't have uses of grep -L in cfg.mk).
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Jano
-- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Mon, Jan 07, 2019 at 09:11:03AM -0600, Eric Blake wrote: [...]
| $(SED) 's/_("\([^\"]\|\\.\)\+"//;s/[ ]"%s"//' \ | $(GREP) '[ ]"' && \ { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \
With the /dev/null changes removed or justified:
I hope that was the justification you were looking for (and it matches
Yes. Jano
the gnulib rewrite that uses /dev/null in every conversion to xargs grep where grep's output differs based on number of file names present - note that 'grep -L' does not have different output, so it doesn't need the /dev/null trick, but we didn't have uses of grep -L in cfg.mk).
On 1/7/19 9:39 AM, Ján Tomko wrote:
On Mon, Jan 07, 2019 at 09:11:03AM -0600, Eric Blake wrote:
[...]
| $(SED) 's/_("\([^\"]\|\\.\)\+"//;s/[ ]"%s"//' \ | $(GREP) '[ ]"' && \ { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \
With the /dev/null changes removed or justified:
I hope that was the justification you were looking for (and it matches
Yes.
Ok, I improved the commit message and pushed the series. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Thu, Jan 03, 2019 at 01:41:56PM -0600, Eric Blake wrote:
Since v1: fix the gnulib bug I accidentally introduced, then copy the gist of Roman's gnulib changes to also apply to our cfg.mk syntax checks.
Eric Blake (3): maint: update gnulib for syntax-check on BSD maint: prefer $(GREP) in cfg.mk maint: split long lines for BSD syntax-check
.gnulib | 2 +- cfg.mk | 95 +++++++++++++++++++++++++++++---------------------------- 2 files changed, 49 insertions(+), 48 deletions(-)
-- 2.20.1
Hi Eric, Thank you for making the changes. I'm sorry I was on vacation and wasn't able to reply earlier. I've run 'make syntax-check' on libvirt's HEAD. It succeeds on macOS. Perhaps we can add syntax-check to .travis.yml? Best regards, Roman
On Thu, 2019-01-10 at 10:34 +0300, Roman Bolshakov wrote:
I've run 'make syntax-check' on libvirt's HEAD. It succeeds on macOS. Perhaps we can add syntax-check to .travis.yml?
syntax-check passes on macOS, but it doesn't quite succeed: [...] unmarked_diagnostics vulnerable_makefile_CVE-2009-4029 grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression usage: grep [-abcDEFGHhIiJLlmnOoqRSsUVvwxZ] [-A num] [-B num] [-C[num]] [-e pattern] [-f file] [--binary-files=value] [--color=when] [--context[=num]] [--directories=action] [--label] [--line-buffered] [--null] [pattern] [file ...] grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression maint.mk: skipping test sc_preprocessor_indentation: cppi not installed grep: -: No such file or directory maint.mk: skipping sc_prohibit_always-defined_macros: you lack GNU grep grep: repetition-operator operand invalid /usr/bin/sed: illegal option -- r usage: sed script [-Ealn] [-i extension] [file ...] sed [-Ealn] [-i extension] [-e script] ... [-f script_file] ... [file ...] grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression usage: grep [-abcDEFGHhIiJLlmnOoqRSsUVvwxZ] [-A num] [-B num] [-C[num]] [-e pattern] [-f file] [--binary-files=value] [--color=when] [--context[=num]] [--directories=action] [--label] [--line-buffered] [--null] [pattern] [file ...] maint.mk: skipping test sc_spec_indentation: cppi not installed 149.00 GFDL_version 150.00 TAB_in_indentation [...] (From https://travis-ci.org/andreabolognani/libvirt/jobs/477733819) If you had time to look into those issues, that would be grand: we are pretty close to being able to run syntax-check on macOS and FreeBSD, and any help towards that goal is very much appreciated :) -- Andrea Bolognani / Red Hat / Virtualization
On Thu, Jan 10, 2019 at 11:49:35AM +0100, Andrea Bolognani wrote:
On Thu, 2019-01-10 at 10:34 +0300, Roman Bolshakov wrote:
I've run 'make syntax-check' on libvirt's HEAD. It succeeds on macOS. Perhaps we can add syntax-check to .travis.yml?
syntax-check passes on macOS, but it doesn't quite succeed:
[...] unmarked_diagnostics vulnerable_makefile_CVE-2009-4029 grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression usage: grep [-abcDEFGHhIiJLlmnOoqRSsUVvwxZ] [-A num] [-B num] [-C[num]] [-e pattern] [-f file] [--binary-files=value] [--color=when] [--context[=num]] [--directories=action] [--label] [--line-buffered] [--null] [pattern] [file ...] grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression maint.mk: skipping test sc_preprocessor_indentation: cppi not installed grep: -: No such file or directory maint.mk: skipping sc_prohibit_always-defined_macros: you lack GNU grep grep: repetition-operator operand invalid /usr/bin/sed: illegal option -- r usage: sed script [-Ealn] [-i extension] [file ...] sed [-Ealn] [-i extension] [-e script] ... [-f script_file] ... [file ...] grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression grep: empty (sub)expression usage: grep [-abcDEFGHhIiJLlmnOoqRSsUVvwxZ] [-A num] [-B num] [-C[num]] [-e pattern] [-f file] [--binary-files=value] [--color=when] [--context[=num]] [--directories=action] [--label] [--line-buffered] [--null] [pattern] [file ...] maint.mk: skipping test sc_spec_indentation: cppi not installed 149.00 GFDL_version 150.00 TAB_in_indentation [...]
(From https://travis-ci.org/andreabolognani/libvirt/jobs/477733819)
If you had time to look into those issues, that would be grand: we are pretty close to being able to run syntax-check on macOS and FreeBSD, and any help towards that goal is very much appreciated :)
Perhaps we should install grep, gnu-sed and cppi from homebrew in .travis.yml to get it working. I have all of them installed on my laptop and syntax-check passes without the errors for me. grep provides GNU grep and all the "empty (sub)expression" statements should be gone if it's installed. sed on macOS doesn't support "-r flag" but supports "-E". Novertheless, prohibit_diagnostic_without_format relies on regular expressions that are not supported in macOS: prohibit_diagnostic_without_format sed: 1: "/\<(|VIR_ERROR|lxcError ...": RE error: empty (sub)expression We need gnu-sed for the rule. Thanks, Roman
On Thu, 2019-01-10 at 16:20 +0300, Roman Bolshakov wrote:
Perhaps we should install grep, gnu-sed and cppi from homebrew in .travis.yml to get it working. I have all of them installed on my laptop and syntax-check passes without the errors for me.
grep provides GNU grep and all the "empty (sub)expression" statements should be gone if it's installed.
sed on macOS doesn't support "-r flag" but supports "-E". Novertheless, prohibit_diagnostic_without_format relies on regular expressions that are not supported in macOS:
prohibit_diagnostic_without_format sed: 1: "/\<(|VIR_ERROR|lxcError ...": RE error: empty (sub)expression
We need gnu-sed for the rule.
Sure we can install cppi, but I think we should try to make our tests portable rather than throwing in the towel and installing the GNU tools, especially since making sure we pick those up instead of the BSD equivalents, which at least on FreeBSD come first in $PATH, would require adding even more autotools trickery... -- Andrea Bolognani / Red Hat / Virtualization
participants (4)
-
Andrea Bolognani -
Eric Blake -
Ján Tomko -
Roman Bolshakov