Re: [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

signature.asc
Description: OpenPGP digital signature