[libvirt] [PATCH] maint: use $(SED) instead of sed for syntax-check

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 604 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140127/6f0b134b/attachment-0001.sig>