[libvirt] [PATCH] tests: test-lib.sh portability and clean-up
No big deal, but I saw recent additions of "test ... -a ..." (not portable) so fixed the rest, too. Now, searching for violations shows none: git grep '\<test .* -a ' Whether it's possible to rely on test -a in test scripts is debatable: perhaps you've ensured that the SHELL you use when running tests is POSIX compliant or better (I do that in coreutils), but at least in configure.ac, we should toe the line wrt portability (because *it* has less choice), so those are in a separate commit. Since this is a global change, it deserves a syntax-check rule. That's the 3/3 patch, below. 1/3 fixes test-lib.sh 2/3 fixes configure.ac
From ca7db6cb8000cc283fcee7899140d2fc892b0296 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 24 Mar 2010 09:05:27 +0100 Subject: [PATCH 1/3] tests: shell script portability and clean-up
* tests/test-lib.sh: "echo -n" is not portable. Use printf instead. Remove unnecessary uses of "eval-in-subshell" (subshell is sufficient). Remove uses of tests' -a operator; it is not portable. Instead, use "test cond && test cond2". * tests/schematestutils.sh: Replace use of test's -a. --- tests/schematestutils.sh | 2 +- tests/test-lib.sh | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/schematestutils.sh b/tests/schematestutils.sh index 301b9eb..f172857 100644 --- a/tests/schematestutils.sh +++ b/tests/schematestutils.sh @@ -21,7 +21,7 @@ do ret=$? test_result $n $(basename $(dirname $xml))"/"$(basename $xml) $ret - if test "$verbose" = "1" -a $ret != 0 ; then + if test "$verbose" = "1" && test $ret != 0 ; then echo -e "$cmd\n$result" fi if test "$ret" != 0 ; then diff --git a/tests/test-lib.sh b/tests/test-lib.sh index 57fd438..28b830e 100644 --- a/tests/test-lib.sh +++ b/tests/test-lib.sh @@ -19,7 +19,7 @@ test_intro() name=$1 if test "$verbose" = "0" ; then echo "TEST: $name" - echo -n " " + printf " " fi } @@ -29,15 +29,15 @@ test_result() name=$2 status=$3 if test "$verbose" = "0" ; then - mod=`eval "expr \( $counter - 1 \) % 40"` - if test "$counter" != 1 -a "$mod" = 0 ; then - printf " %-3d\n" `eval "expr $counter - 1"` - echo -n " " + mod=`expr \( $counter + 40 - 1 \) % 40` + if test "$counter" != 1 && test "$mod" = 0 ; then + printf " %-3d\n" `expr $counter - 1` + printf " " fi if test "$status" = "0" ; then - echo -n "." + printf "." else - echo -n "!" + printf "!" fi else if test "$status" = "0" ; then @@ -54,11 +54,11 @@ test_final() status=$2 if test "$verbose" = "0" ; then - mod=`eval "expr \( $counter + 1 \) % 40"` - if test "$mod" != "0" -a "$mod" != "1" ; then + mod=`expr \( $counter + 1 \) % 40` + if test "$mod" != "0" && test "$mod" != "1" ; then for i in `seq $mod 40` do - echo -n " " + printf " " done fi if test "$status" = "0" ; then -- 1.7.0.3.435.g097f4
From 7998714d60b997357bfea15d6f2d0f729fc8fb29 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 24 Mar 2010 09:10:13 +0100 Subject: [PATCH 2/3] build: don't use "test cond1 -a cond2" in configure: it's not portable
* configure.ac: Use "test cond1 && test cond2" instead. --- configure.ac | 26 +++++++++++++------------- 1 files changed, 13 insertions(+), 13 deletions(-) diff --git a/configure.ac b/configure.ac index bcf1d5a..2e6d2e4 100644 --- a/configure.ac +++ b/configure.ac @@ -197,10 +197,10 @@ dnl if --prefix is /usr, don't use /usr/var for localstatedir dnl or /usr/etc for sysconfdir dnl as this makes a lot of things break in testing situations -if test "$prefix" = "/usr" -a "$localstatedir" = '${prefix}/var' ; then +if test "$prefix" = "/usr" && test "$localstatedir" = '${prefix}/var' ; then localstatedir='/var' fi -if test "$prefix" = "/usr" -a "$sysconfdir" = '${prefix}/etc' ; then +if test "$prefix" = "/usr" && test "$sysconfdir" = '${prefix}/etc' ; then sysconfdir='/etc' fi @@ -240,7 +240,7 @@ AC_ARG_WITH([libvirtd], dnl dnl specific tests to setup DV devel environments with debug etc ... dnl -if [[ "${LOGNAME}" = "veillard" -a "`pwd`" = "/u/veillard/libvirt" ]] ; then +if [[ "${LOGNAME}" = "veillard" && test "`pwd`" = "/u/veillard/libvirt" ]] ; then STATIC_BINARIES="-static" else STATIC_BINARIES= @@ -351,7 +351,7 @@ LIBXENSERVER_LIBS="" LIBXENSERVER_CFLAGS="" dnl search for the XenServer library if test "$with_xenapi" != "no" ; then - if test "$with_xenapi" != "yes" -a "$with_xenapi" != "check" ; then + if test "$with_xenapi" != "yes" && test "$with_xenapi" != "check" ; then LIBXENSERVER_CFLAGS="-I$with_xenapi/include" LIBXENSERVER_LIBS="-L$with_xenapi" fi @@ -390,7 +390,7 @@ XEN_LIBS="" XEN_CFLAGS="" dnl search for the Xen store library if test "$with_xen" != "no" ; then - if test "$with_xen" != "yes" -a "$with_xen" != "check" ; then + if test "$with_xen" != "yes" && test "$with_xen" != "check" ; then XEN_CFLAGS="-I$with_xen/include" XEN_LIBS="-L$with_xen/lib64 -L$with_xen/lib" fi @@ -571,7 +571,7 @@ AC_ARG_WITH([libxml], AC_HELP_STRING([--with-libxml=@<:@PFX@:>@], [libxml2 locat if test "x$with_libxml" = "xno" ; then AC_MSG_CHECKING(for libxml2 libraries >= $LIBXML_REQUIRED) AC_MSG_ERROR([libxml2 >= $LIBXML_REQUIRED is required for libvirt]) -elif test "x$with_libxml" = "x" -a "x$PKG_CONFIG" != "x" ; then +elif test "x$with_libxml" = "x" && test "x$PKG_CONFIG" != "x" ; then PKG_CHECK_MODULES(LIBXML, libxml-2.0 >= $LIBXML_REQUIRED, [LIBXML_FOUND=yes], [LIBXML_FOUND=no]) fi if test "$LIBXML_FOUND" = "no" ; then @@ -661,7 +661,7 @@ AC_ARG_WITH([sasl], SASL_CFLAGS= SASL_LIBS= if test "x$with_sasl" != "xno"; then - if test "x$with_sasl" != "xyes" -a "x$with_sasl" != "xcheck"; then + if test "x$with_sasl" != "xyes" && test "x$with_sasl" != "xcheck"; then SASL_CFLAGS="-I$with_sasl" SASL_LIBS="-L$with_sasl" fi @@ -716,7 +716,7 @@ AC_ARG_WITH([yajl], YAJL_CFLAGS= YAJL_LIBS= if test "x$with_yajl" != "xno"; then - if test "x$with_yajl" != "xyes" -a "x$with_yajl" != "xcheck"; then + if test "x$with_yajl" != "xyes" && test "x$with_yajl" != "xcheck"; then YAJL_CFLAGS="-I$with_yajl/include" YAJL_LIBS="-L$with_yajl/lib" fi @@ -1004,7 +1004,7 @@ AC_ARG_WITH([numactl], NUMACTL_CFLAGS= NUMACTL_LIBS= -if test "$with_qemu" = "yes" -a "$with_numactl" != "no"; then +if test "$with_qemu" = "yes" && test "$with_numactl" != "no"; then old_cflags="$CFLAGS" old_libs="$LIBS" if test "$with_numactl" = "check"; then @@ -1062,7 +1062,7 @@ dnl dnl libssh checks dnl -if test "$with_libssh2" != "yes" -a "$with_libssh2" != "no"; then +if test "$with_libssh2" != "yes" && test "$with_libssh2" != "no"; then libssh2_path="$with_libssh2" elif test "$with_libssh2" = "yes"; then libssh2_path="/usr/local/lib/" @@ -1143,7 +1143,7 @@ dnl introduced in 0.4.0 release which need as minimum dnl CAPNG_CFLAGS= CAPNG_LIBS= -if test "$with_qemu" = "yes" -a "$with_capng" != "no"; then +if test "$with_qemu" = "yes" && test "$with_capng" != "no"; then old_cflags="$CFLAGS" old_libs="$LIBS" if test "$with_capng" = "check"; then @@ -1453,7 +1453,7 @@ if test "$with_storage_disk" = "yes" -o "$with_storage_disk" = "check"; then PARTED_FOUND=yes fi - if test "$with_storage_disk" != "no" -a "x$PKG_CONFIG" != "x" ; then + if test "$with_storage_disk" != "no" && test "x$PKG_CONFIG" != "x" ; then PKG_CHECK_MODULES(LIBPARTED, libparted >= $PARTED_REQUIRED, [], [PARTED_FOUND=no]) fi if test "$PARTED_FOUND" = "no"; then @@ -1635,7 +1635,7 @@ else fi AC_MSG_RESULT($RUNNING_XEND) -AM_CONDITIONAL([ENABLE_XEN_TESTS], [test "$RUNNING_XEN" != "no" -a "$RUNNING_XEND" != "no"]) +AM_CONDITIONAL([ENABLE_XEN_TESTS], [test "$RUNNING_XEN" != "no" && test "$RUNNING_XEND" != "no"]) AC_ARG_ENABLE([test-coverage], AC_HELP_STRING([--enable-test-coverage], [turn on code coverage instrumentation @<:@default=no@:>@]), -- 1.7.0.3.435.g097f4
From 95c8ddd2eca90e3024a6f74af84517c1e0115a60 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 24 Mar 2010 09:32:43 +0100 Subject: [PATCH 3/3] maint: add syntax-check rule to prohibit use of test's -a operator
* cfg.mk (sc_prohibit_test_minus_a): New rule. --- cfg.mk | 6 ++++++ 1 files changed, 6 insertions(+), 0 deletions(-) diff --git a/cfg.mk b/cfg.mk index 2d0d278..4302338 100644 --- a/cfg.mk +++ b/cfg.mk @@ -269,6 +269,12 @@ sc_preprocessor_indentation: echo '$(ME): skipping test $@: cppi not installed' 1>&2; \ fi +# Using test's -a operator is not portable. +sc_prohibit_test_minus_a: + @re='\<test .+ -[a] ' \ + msg='use "test C1 && test C2, not "test C1 -''a C2"' \ + $(_prohibit_regexp) + sc_copyright_format: @$(VC_LIST_EXCEPT) | xargs grep -ni 'copyright .*Red 'Hat \ | grep -v Inc \ -- 1.7.0.3.435.g097f4
On 03/24/2010 02:40 AM, Jim Meyering wrote:
No big deal, but I saw recent additions of "test ... -a ..." (not portable) so fixed the rest, too.
Now, searching for violations shows none:
git grep '\<test .* -a '
Better would be git grep '\<test .* -[ao] '
Whether it's possible to rely on test -a in test scripts is debatable: perhaps you've ensured that the SHELL you use when running tests is POSIX compliant or better (I do that in coreutils),
But test -a is optional - POSIX says it is only required by XSI extensions, and some shells (think posh) go so far as to explicitly reject all optional POSIX constructs. (Not that I know anyone crazy enough to choose posh instead of dash as their /bin/sh, but still...)
Since this is a global change, it deserves a syntax-check rule.
All right - that means it should be easy to submit a followup series to kill test -o. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
* configure.ac: Use "test cond1 || test cond2" instead. * m4/compiler-flags.m4 (gl_COMPILER_FLAGS): Likewise. * tests/test-lib.sh (verbose): Likewise. --- I guess this serves as an implicit ACK to the fact that you accidentally pushed your patches along the same lines. Is it worth reformatting any of the lines that now exceed 80 columns? configure.ac | 32 ++++++++++++++++---------------- m4/compiler-flags.m4 | 5 +++-- tests/test-lib.sh | 2 +- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/configure.ac b/configure.ac index 2e6d2e4..52b16ee 100644 --- a/configure.ac +++ b/configure.ac @@ -276,7 +276,7 @@ AC_MSG_CHECKING([for init script flavor]) AC_ARG_WITH([init-script], [AC_HELP_STRING([--with-init-script=@<:@redhat|auto|none@:>@], [Style of init script to install @<:@default=auto@:>@])]) -if test "x$with_init_script" = "x" -o "x$with_init_script" = "xauto"; then +if test "x$with_init_script" = "x" || test "x$with_init_script" = "xauto"; then if test -f /etc/redhat-release ; then with_init_script=redhat else @@ -482,7 +482,7 @@ AC_CHECK_HEADERS([linux/kvm.h]) dnl dnl check for sufficient headers for LXC dnl -if test "$with_lxc" = "yes" -o "$with_lxc" = "check"; then +if test "$with_lxc" = "yes" || test "$with_lxc" = "check"; then AC_CHECK_HEADER([sched.h], dnl Header is there, check for unshare() [ @@ -524,7 +524,7 @@ AM_CONDITIONAL([WITH_LXC], [test "$with_lxc" = "yes"]) dnl dnl check for kernel headers required by src/bridge.c dnl -if test "$with_qemu" = "yes" -o "$with_lxc" = "yes" ; then +if test "$with_qemu" = "yes" || test "$with_lxc" = "yes" ; then AC_CHECK_HEADERS([linux/param.h linux/sockios.h linux/if_bridge.h linux/if_tun.h],, AC_MSG_ERROR([You must install kernel-headers in order to compile libvirt])) fi @@ -538,7 +538,7 @@ dnl XMLRPC_CFLAGS= XMLRPC_LIBS= -if test "x$with_one" = "xyes" -o "x$with_one" = "xcheck"; then +if test "x$with_one" = "xyes" || test "x$with_one" = "xcheck"; then PKG_CHECK_MODULES(XMLRPC, xmlrpc_client >= $XMLRPC_REQUIRED, [with_one=yes], [ if test "x$with_one" = "xcheck" ; then @@ -768,7 +768,7 @@ AC_ARG_WITH([polkit], with_polkit0=no with_polkit1=no -if test "x$with_polkit" = "xyes" -o "x$with_polkit" = "xcheck"; then +if test "x$with_polkit" = "xyes" || test "x$with_polkit" = "xcheck"; then dnl Check for new polkit first - just a binary AC_PATH_PROG([PKCHECK_PATH],[pkcheck], [], [/usr/sbin:$PATH]) if test "x$PKCHECK_PATH" != "x" ; then @@ -826,7 +826,7 @@ AC_ARG_WITH([avahi], AVAHI_CFLAGS= AVAHI_LIBS= -if test "x$with_avahi" = "xyes" -o "x$with_avahi" = "xcheck"; then +if test "x$with_avahi" = "xyes" || test "x$with_avahi" = "xcheck"; then PKG_CHECK_MODULES(AVAHI, avahi-client >= $AVAHI_REQUIRED, [with_avahi=yes], [ if test "x$with_avahi" = "xcheck" ; then @@ -1036,7 +1036,7 @@ dnl dnl Checks for the UML driver dnl -if test "$with_uml" = "yes" -o "$with_uml" = "check"; then +if test "$with_uml" = "yes" || test "$with_uml" = "check"; then AC_CHECK_HEADER([sys/inotify.h], [ with_uml=yes ], [ @@ -1239,7 +1239,7 @@ AC_ARG_WITH([netcf], NETCF_CFLAGS= NETCF_LIBS= -if test "$with_netcf" = "yes" -o "$with_netcf" = "check"; then +if test "$with_netcf" = "yes" || test "$with_netcf" = "check"; then PKG_CHECK_MODULES(NETCF, netcf >= $NETCF_REQUIRED, [with_netcf=yes], [ if test "$with_netcf" = "check" ; then @@ -1298,7 +1298,7 @@ fi AM_CONDITIONAL([WITH_STORAGE_DIR], [test "$with_storage_dir" = "yes"]) -if test "$with_storage_fs" = "yes" -o "$with_storage_fs" = "check"; then +if test "$with_storage_fs" = "yes" || test "$with_storage_fs" = "check"; then AC_PATH_PROG([MOUNT], [mount], [], [$PATH:/sbin:/usr/sbin]) AC_PATH_PROG([UMOUNT], [umount], [], [$PATH:/sbin:/usr/sbin]) if test "$with_storage_fs" = "yes" ; then @@ -1326,7 +1326,7 @@ if test "$with_storage_fs" = "yes"; then [Location or name of the showmount program]) fi -if test "$with_storage_lvm" = "yes" -o "$with_storage_lvm" = "check"; then +if test "$with_storage_lvm" = "yes" || test "$with_storage_lvm" = "check"; then AC_PATH_PROG([PVCREATE], [pvcreate], [], [$PATH:/sbin:/usr/sbin]) AC_PATH_PROG([VGCREATE], [vgcreate], [], [$PATH:/sbin:/usr/sbin]) AC_PATH_PROG([LVCREATE], [lvcreate], [], [$PATH:/sbin:/usr/sbin]) @@ -1386,7 +1386,7 @@ AM_CONDITIONAL([WITH_STORAGE_LVM], [test "$with_storage_lvm" = "yes"]) -if test "$with_storage_iscsi" = "yes" -o "$with_storage_iscsi" = "check"; then +if test "$with_storage_iscsi" = "yes" || test "$with_storage_iscsi" = "check"; then AC_PATH_PROG([ISCSIADM], [iscsiadm], [], [$PATH:/sbin:/usr/sbin]) if test "$with_storage_iscsi" = "yes" ; then if test -z "$ISCSIADM" ; then AC_MSG_ERROR([We need iscsiadm for iSCSI storage driver]) ; fi @@ -1444,7 +1444,7 @@ AC_SUBST([DEVMAPPER_LIBS]) LIBPARTED_CFLAGS= LIBPARTED_LIBS= -if test "$with_storage_disk" = "yes" -o "$with_storage_disk" = "check"; then +if test "$with_storage_disk" = "yes" || test "$with_storage_disk" = "check"; then AC_PATH_PROG([PARTED], [parted], [], [$PATH:/sbin:/usr/sbin]) if test -z "$PARTED" ; then with_storage_disk=no @@ -1495,7 +1495,7 @@ dnl LIBCURL_CFLAGS="" LIBCURL_LIBS="" -if test "$with_esx" = "yes" -o "$with_esx" = "check" -o "$with_xenapi" = "yes" -o "$with_xenapi" = "check"; then +if test "$with_esx" = "yes" || test "$with_esx" = "check" || test "$with_xenapi" = "yes" || test "$with_xenapi" = "check"; then PKG_CHECK_MODULES(LIBCURL, libcurl >= $LIBCURL_REQUIRED, [ if test "$with_esx" = "check"; then with_esx=yes @@ -1817,7 +1817,7 @@ AC_ARG_WITH([hal], if test "$with_libvirtd" = "no" ; then with_hal=no fi -if test "x$with_hal" = "xyes" -o "x$with_hal" = "xcheck"; then +if test "x$with_hal" = "xyes" || test "x$with_hal" = "xcheck"; then PKG_CHECK_MODULES(HAL, hal >= $HAL_REQUIRED, [with_hal=yes], [ if test "x$with_hal" = "xcheck" ; then @@ -1860,7 +1860,7 @@ AC_ARG_WITH([udev], if test "$with_libvirtd" = "no" ; then with_udev=no fi -if test "x$with_udev" = "xyes" -o "x$with_udev" = "xcheck"; then +if test "x$with_udev" = "xyes" || test "x$with_udev" = "xcheck"; then PKG_CHECK_MODULES(UDEV, libudev >= $UDEV_REQUIRED, [], [ if test "x$with_udev" = "xcheck" ; then @@ -1894,7 +1894,7 @@ AC_SUBST([PCIACCESS_CFLAGS]) AC_SUBST([PCIACCESS_LIBS]) with_nodedev=no; -if test "$with_hal" = "yes" -o "$with_udev" = "yes"; +if test "$with_hal" = "yes" || test "$with_udev" = "yes"; then with_nodedev=yes AC_DEFINE_UNQUOTED([WITH_NODE_DEVICES], 1, [with node device driver]) diff --git a/m4/compiler-flags.m4 b/m4/compiler-flags.m4 index 72f9fe1..628bd1f 100644 --- a/m4/compiler-flags.m4 +++ b/m4/compiler-flags.m4 @@ -1,6 +1,7 @@ -# serial 3 +# serial 4 # Find valid warning flags for the C Compiler. -*-Autoconf-*- # +# Copyright (C) 2010 Red Hat, Inc. # Copyright (C) 2001, 2002, 2006 Free Software Foundation, Inc. # # This program is free software; you can redistribute it and/or modify @@ -37,7 +38,7 @@ AC_DEFUN([gl_COMPILER_FLAGS], echo 'int x;' >conftest.c $CC $CFLAGS -c conftest.c 2>conftest.err ret=$? - if test $ret != 0 -o -s conftest.err -o $has_option = "no"; then + if test $ret != 0 || test -s conftest.err || test $has_option = "no"; then AC_MSG_RESULT(no) else AC_MSG_RESULT(yes) diff --git a/tests/test-lib.sh b/tests/test-lib.sh index 28b830e..768f96b 100644 --- a/tests/test-lib.sh +++ b/tests/test-lib.sh @@ -199,7 +199,7 @@ this_test_() { echo "./$0" | sed 's,.*/,,'; } this_test=$(this_test_) verbose=0 -if test -n "$VIR_TEST_DEBUG" -o -n "$VIR_TEST_VERBOSE" ; then +if test -n "$VIR_TEST_DEBUG" || test -n "$VIR_TEST_VERBOSE" ; then verbose=1 fi -- 1.6.6.1
Eric Blake wrote:
* configure.ac: Use "test cond1 || test cond2" instead. * m4/compiler-flags.m4 (gl_COMPILER_FLAGS): Likewise. * tests/test-lib.sh (verbose): Likewise. --- Is it worth reformatting any of the lines that now exceed 80 columns?
I debated the same thing and ended up splitting one line. FWIW, I used this to perform the conversion, perl -pi -e 's/\b(test .+) -o (.*)/$1 || test $2/' but had to run it 3 times to convert the line with three -o's. Don't bother revising. I'll push this as-is, but we should eventually split all long lines in configure.ac.
configure.ac | 32 ++++++++++++++++---------------- m4/compiler-flags.m4 | 5 +++-- tests/test-lib.sh | 2 +- 3 files changed, 20 insertions(+), 19 deletions(-)
* cfg.mk (sc_prohibit_test_minus_a): Rename... (sc_prohibit_test_minus_ao): ...and flag '-o', too. --- cfg.mk | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-) diff --git a/cfg.mk b/cfg.mk index 4302338..27cfca4 100644 --- a/cfg.mk +++ b/cfg.mk @@ -269,11 +269,14 @@ sc_preprocessor_indentation: echo '$(ME): skipping test $@: cppi not installed' 1>&2; \ fi -# Using test's -a operator is not portable. -sc_prohibit_test_minus_a: +# Using test's -a and -o operators is not portable. +sc_prohibit_test_minus_ao: @re='\<test .+ -[a] ' \ msg='use "test C1 && test C2, not "test C1 -''a C2"' \ $(_prohibit_regexp) + @re='\<test .+ -[o] ' \ + msg='use "test C1 || test C2, not "test C1 -''o C2"' \ + $(_prohibit_regexp) sc_copyright_format: @$(VC_LIST_EXCEPT) | xargs grep -ni 'copyright .*Red 'Hat \ -- 1.6.6.1
Eric Blake wrote:
* cfg.mk (sc_prohibit_test_minus_a): Rename... (sc_prohibit_test_minus_ao): ...and flag '-o', too. --- cfg.mk | 7 +++++-- 1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 4302338..27cfca4 100644 --- a/cfg.mk +++ b/cfg.mk @@ -269,11 +269,14 @@ sc_preprocessor_indentation: echo '$(ME): skipping test $@: cppi not installed' 1>&2; \ fi
-# Using test's -a operator is not portable. -sc_prohibit_test_minus_a: +# Using test's -a and -o operators is not portable. +sc_prohibit_test_minus_ao: @re='\<test .+ -[a] ' \ msg='use "test C1 && test C2, not "test C1 -''a C2"' \ $(_prohibit_regexp) + @re='\<test .+ -[o] ' \ + msg='use "test C1 || test C2, not "test C1 -''o C2"' \ + $(_prohibit_regexp)
I actually wrote a separate target before I saw you'd done it. Seeing yours, I realize that both tests do belong in the same rule, but would prefer the efficiency of combining the searches, so that we don't make two passes through all sources, when one will do. Then the only trick is to produce a combined diagnostic. Maybe like this (thought the resulting line is too long -- for a diagnostic it's not a big deal). _m1 = use "test C1 && test C2, not "test C1 -''a C2" _m2 = use "test C1 || test C2, not "test C1 -''o C2" sc_prohibit_test_minus_ao: @re='\<test .+ -[ao] ' \ msg='$(_m1); $(_m2)' \ $(_prohibit_regexp)
On 03/25/2010 02:22 AM, Jim Meyering wrote:
-# Using test's -a operator is not portable. -sc_prohibit_test_minus_a: +# Using test's -a and -o operators is not portable. +sc_prohibit_test_minus_ao: @re='\<test .+ -[a] ' \ msg='use "test C1 && test C2, not "test C1 -''a C2"' \ $(_prohibit_regexp) + @re='\<test .+ -[o] ' \ + msg='use "test C1 || test C2, not "test C1 -''o C2"' \ + $(_prohibit_regexp)
I actually wrote a separate target before I saw you'd done it. Seeing yours, I realize that both tests do belong in the same rule, but would prefer the efficiency of combining the searches, so that we don't make two passes through all sources, when one will do. Then the only trick is to produce a combined diagnostic. Maybe like this (thought the resulting line is too long -- for a diagnostic it's not a big deal).
Agreed, since the diagnostic never prints on a successful run :)
_m1 = use "test C1 && test C2, not "test C1 -''a C2" _m2 = use "test C1 || test C2, not "test C1 -''o C2"
sc_prohibit_test_minus_ao: @re='\<test .+ -[ao] ' \ msg='$(_m1); $(_m2)' \ $(_prohibit_regexp)
I like this change. ACK to your tweaks to my patch, and looking forward to the push. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 03/25/2010 02:22 AM, Jim Meyering wrote:
-# Using test's -a operator is not portable. -sc_prohibit_test_minus_a: +# Using test's -a and -o operators is not portable. +sc_prohibit_test_minus_ao: @re='\<test .+ -[a] ' \ msg='use "test C1 && test C2, not "test C1 -''a C2"' \ $(_prohibit_regexp) + @re='\<test .+ -[o] ' \ + msg='use "test C1 || test C2, not "test C1 -''o C2"' \ + $(_prohibit_regexp)
I actually wrote a separate target before I saw you'd done it. Seeing yours, I realize that both tests do belong in the same rule, but would prefer the efficiency of combining the searches, so that we don't make two passes through all sources, when one will do. Then the only trick is to produce a combined diagnostic. Maybe like this (thought the resulting line is too long -- for a diagnostic it's not a big deal).
Agreed, since the diagnostic never prints on a successful run :)
_m1 = use "test C1 && test C2, not "test C1 -''a C2" _m2 = use "test C1 || test C2, not "test C1 -''o C2"
sc_prohibit_test_minus_ao: @re='\<test .+ -[ao] ' \ msg='$(_m1); $(_m2)' \ $(_prohibit_regexp)
I like this change. ACK to your tweaks to my patch, and looking forward to the push.
I've pushed this:
From 271945a148395230066a61b426437b99ca39091b Mon Sep 17 00:00:00 2001 From: Eric Blake <eblake@redhat.com> Date: Fri, 26 Mar 2010 08:41:05 +0100 Subject: [PATCH] maint: update syntax-check rule to also catch test's -o operator
* cfg.mk (sc_prohibit_test_minus_a): Rename... (sc_prohibit_test_minus_ao): ...and flag '-o', too. --- cfg.mk | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-) diff --git a/cfg.mk b/cfg.mk index bf5eae3..7fdb069 100644 --- a/cfg.mk +++ b/cfg.mk @@ -270,10 +270,12 @@ sc_preprocessor_indentation: echo '$(ME): skipping test $@: cppi not installed' 1>&2; \ fi -# Using test's -a operator is not portable. -sc_prohibit_test_minus_a: - @re='\<test .+ -[a] ' \ - msg='use "test C1 && test C2, not "test C1 -''a C2"' \ +_m1 = use "test C1 && test C2", not "test C1 -''a C2" +_m2 = use "test C1 || test C2", not "test C1 -''o C2" +# Using test's -a and -o operators is not portable. +sc_prohibit_test_minus_ao: + @re='\<test .+ -[ao] ' \ + msg='$(_m1); $(_m2)' \ $(_prohibit_regexp) sc_copyright_format: -- 1.7.0.3.448.g82eeb
participants (2)
-
Eric Blake -
Jim Meyering