[libvirt] [PATCH] maint: use $(SED) instead of sed for syntax-check
Some syntax-check rules use GNU sed specific regexps, so allow to override which sed would be used to fix 'syntax-check' for non GNU-userland systems. --- cfg.mk | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/cfg.mk b/cfg.mk index 207dfeb..bd984bd 100644 --- a/cfg.mk +++ b/cfg.mk @@ -614,7 +614,7 @@ sc_libvirt_unmarked_diagnostics: $(_sc_search_regexp) @{ grep -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ grep -A1 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ - | sed 's/_("\([^\"]\|\\.\)\+"//;s/[ ]"%s"//' \ + | $(SED) 's/_("\([^\"]\|\\.\)\+"//;s/[ ]"%s"//' \ | grep '[ ]"' && \ { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \ exit 1; } || : @@ -639,7 +639,7 @@ sc_prohibit_newline_at_end_of_diagnostic: sc_prohibit_diagnostic_without_format: @{ grep -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ grep -A2 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ - | sed -rn -e ':l; /[,"]$$/ {N;b l;}' \ + | $(SED) -rn -e ':l; /[,"]$$/ {N;b l;}' \ -e '/(xenapiSessionErrorHandler|vah_(error|warning))/d' \ -e '/\<$(func_re) *\([^"]*"([^%"]|"\n[^"]*")*"[,)]/p' \ | grep -vE 'VIR_ERROR' && \ @@ -661,7 +661,7 @@ sc_prohibit_useless_translation: # or \n on one side of the split. sc_require_whitespace_in_translation: @grep -n -A1 '"$$' $$($(VC_LIST_EXCEPT)) \ - | sed -ne ':l; /"$$/ {N;b l;}; s/"\n[^"]*"/""/g; s/\\n/ /g' \ + | $(SED) -ne ':l; /"$$/ {N;b l;}; s/"\n[^"]*"/""/g; s/\\n/ /g' \ -e '/_(.*[^\ ]""[^\ ]/p' | grep . && \ { echo '$(ME): missing whitespace at line split' 1>&2; \ exit 1; } || : @@ -681,11 +681,11 @@ sc_preprocessor_indentation: sc_spec_indentation: @if cppi --version >/dev/null 2>&1; then \ for f in $$($(VC_LIST_EXCEPT) | grep '\.spec\.in$$'); do \ - sed -e 's|#|// #|; s|%ifn*\(arch\)* |#if a // |' \ + $(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|"; \ + | cppi -a -c 2>&1 | $(SED) "s|standard input|$$f|"; \ done | { if grep . >&2; then false; else :; fi; } \ || { echo '$(ME): incorrect preprocessor indentation' 1>&2; \ exit 1; }; \ @@ -777,7 +777,7 @@ sc_prohibit_cross_inclusion: # elements added to the enum by using a _LAST marker. sc_require_enum_last_marker: @grep -A1 -nE '^[^#]*VIR_ENUM_IMPL *\(' $$($(VC_LIST_EXCEPT)) \ - | sed -ne '/VIR_ENUM_IMPL[^,]*,$$/N' \ + | $(SED) -ne '/VIR_ENUM_IMPL[^,]*,$$/N' \ -e '/VIR_ENUM_IMPL[^,]*,[^,]*[^_,][^L,][^A,][^S,][^T,],/p' \ -e '/VIR_ENUM_IMPL[^,]*,[^,]\{0,4\},/p' \ | grep . && \ @@ -878,7 +878,7 @@ ifeq (0,$(MAKELEVEL)) # b653eda3ac4864de205419d9f41eec267cb89eeb # # Keep this logic in sync with autogen.sh. - _submodule_hash = sed 's/^[ +-]//;s/ .*//' + _submodule_hash = $(SED) 's/^[ +-]//;s/ .*//' _update_required := $(shell \ cd '$(srcdir)'; \ test -d .git || { echo 0; exit; }; \ -- 1.8.4.3
On 01/27/2014 09:37 AM, Roman Bogorodskiy wrote:
Some syntax-check rules use GNU sed specific regexps, so allow to override which sed would be used to fix 'syntax-check' for non GNU-userland systems. --- cfg.mk | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
[Adding bug-gnulib] Hmm - this is an interesting situation. I imagine we will probably hit cases where maint.mk also starts assuming GNU sed, so it becomes a trade of whether it is easier to rewrite the expressions to portable sed that works out of the box on BSD, or to rewrite the rules to use $(SED) uniformly. And what happens if we use $(SED) but the configure.ac did not request AC_PROG_SED in configure.ac? (In gnulib, m4/po.m4 requires AC_PROG_SED, which means any package using gettext is safe, but we'd have to hoist the requirement of AC_PROG_SED into gnulib's gl_INIT macro if we wanted to work even for packages that don't use gettext).
diff --git a/cfg.mk b/cfg.mk index 207dfeb..bd984bd 100644 --- a/cfg.mk +++ b/cfg.mk @@ -614,7 +614,7 @@ sc_libvirt_unmarked_diagnostics: $(_sc_search_regexp) @{ grep -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ grep -A1 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ - | sed 's/_("\([^\"]\|\\.\)\+"//;s/[ ]"%s"//' \ + | $(SED) 's/_("\([^\"]\|\\.\)\+"//;s/[ ]"%s"//' \
Is the non-portable sed usage merely the fact that I used \+ here? If so, \{1,\} is a more portable way to write the 1-or-more modifier.
| grep '[ ]"' && \ { echo '$(ME): found unmarked diagnostic(s)' 1>&2; \ exit 1; } || : @@ -639,7 +639,7 @@ sc_prohibit_newline_at_end_of_diagnostic: sc_prohibit_diagnostic_without_format: @{ grep -nE '\<$(func_re) *\(.*;$$' $$($(VC_LIST_EXCEPT)); \ grep -A2 -nE '\<$(func_re) *\(.*,$$' $$($(VC_LIST_EXCEPT)); } \ - | sed -rn -e ':l; /[,"]$$/ {N;b l;}' \ + | $(SED) -rn -e ':l; /[,"]$$/ {N;b l;}' \ -e '/(xenapiSessionErrorHandler|vah_(error|warning))/d' \ -e '/\<$(func_re) *\([^"]*"([^%"]|"\n[^"]*")*"[,)]/p' \
POSIX has decided to add support for 'sed -E' in a future version (GNU sed supports that as an alias for 'sed -r', the alias has been around for a long time, but was only recently documented as a result of the POSIX decision): http://austingroupbugs.net/view.php?id=528 Would using 'sed -En -e ...' fix it for you? Or should we just rewrite this in basic rather than extended regex: -e /\(xenapiSessionErrorHandler\|vah_\(error\|warning\)\)/d' -e /\<$func_re) *([^"*"\([^%"]\|"\n[^"\*"\)*"[,)]/p'
@@ -661,7 +661,7 @@ sc_prohibit_useless_translation: # or \n on one side of the split. sc_require_whitespace_in_translation: @grep -n -A1 '"$$' $$($(VC_LIST_EXCEPT)) \ - | sed -ne ':l; /"$$/ {N;b l;}; s/"\n[^"]*"/""/g; s/\\n/ /g' \ + | $(SED) -ne ':l; /"$$/ {N;b l;}; s/"\n[^"]*"/""/g; s/\\n/ /g' \ -e '/_(.*[^\ ]""[^\ ]/p' | grep . && \
Hmm, I'm not seeing the non-portable aspect here. Wonder what I'm missing...
sc_spec_indentation: @if cppi --version >/dev/null 2>&1; then \ for f in $$($(VC_LIST_EXCEPT) | grep '\.spec\.in$$'); do \ - sed -e 's|#|// #|; s|%ifn*\(arch\)* |#if a // |' \ + $(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 \
Again, not seeing the non-portable use here, what am I missing?
- | cppi -a -c 2>&1 | sed "s|standard input|$$f|"; \ + | cppi -a -c 2>&1 | $(SED) "s|standard input|$$f|"; \
And I doubt this use needs $(SED), as that looks pretty bog-standard.
sc_require_enum_last_marker: @grep -A1 -nE '^[^#]*VIR_ENUM_IMPL *\(' $$($(VC_LIST_EXCEPT)) \ - | sed -ne '/VIR_ENUM_IMPL[^,]*,$$/N' \ + | $(SED) -ne '/VIR_ENUM_IMPL[^,]*,$$/N' \ -e '/VIR_ENUM_IMPL[^,]*,[^,]*[^_,][^L,][^A,][^S,][^T,],/p' \ -e '/VIR_ENUM_IMPL[^,]*,[^,]\{0,4\},/p' \
Again, not seeing a non-portable use here.
@@ -878,7 +878,7 @@ ifeq (0,$(MAKELEVEL)) # b653eda3ac4864de205419d9f41eec267cb89eeb # # Keep this logic in sync with autogen.sh. - _submodule_hash = sed 's/^[ +-]//;s/ .*//' + _submodule_hash = $(SED) 's/^[ +-]//;s/ .*//'
And this one's portable as well. Or is the idea to replace ALL use of s/sed/$(SED)/ even where it makes no difference? If so, we should probably also do it in maint.mk. I'd like some feedback from the gnulib community if favoring $(SED) is the right thing to do, or if I should just rewrite libvirt's rules to stick to portable sed constructs. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/27/2014 01:03 PM, Eric Blake wrote:
I'd like some feedback from the gnulib community if favoring $(SED) is the right thing to do, or if I should just rewrite libvirt's rules to stick to portable sed constructs.
My kneejerk reaction is that we'd probably have fewer problems using '$(SED)' everywhere in a makefile, rather than using '$(SED)' some places and 'sed' in others, because it'd be one less 'sed' implementation to worry about.
Paul Eggert wrote:
On 01/27/2014 01:03 PM, Eric Blake wrote:
I'd like some feedback from the gnulib community if favoring $(SED) is the right thing to do, or if I should just rewrite libvirt's rules to stick to portable sed constructs.
My kneejerk reaction is that we'd probably have fewer problems using '$(SED)' everywhere in a makefile, rather than using '$(SED)' some places and 'sed' in others, because it'd be one less 'sed' implementation to worry about.
Yes, that was the idea. That's why I replaced all the usage of 'sed' even though not all of them are non-portable, as Eric pointed. For sure, using portable regexps is better way, but I don't know how much of effort that would be. Roman Bogorodskiy
Roman Bogorodskiy wrote:
Some syntax-check rules use GNU sed specific regexps, so allow to override which sed would be used to fix 'syntax-check' for non GNU-userland systems. --- cfg.mk | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
Any updates on that? Roman Bogorodskiy
participants (3)
-
Eric Blake -
Paul Eggert -
Roman Bogorodskiy