GNU bug report logs - #8893
[PATCH 1/2] tests: make test runner a script, not a shell function (was: Automake patches for custom test drivers' support break coreutils testsuite)

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 8893 in the body.
You can then email your comments to 8893 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefano Lattarini <stefano.lattarini <at> gmail.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: Ralf Wildenhues <Ralf.Wildenhues <at> gmx.de>, bug-coreutils <at> gnu.org,
	automake-patches <at> gnu.org
Subject: [PATCH 1/2] tests: make test runner a script,
	not a shell function (was: Automake patches for custom test drivers'
	support break coreutils testsuite)
Date: Sat, 18 Jun 2011 19:41:11 +0200

[Message part 1 (text/plain, inline)]

On Saturday 18 June 2011, Jim Meyering wrote:
> Stefano Lattarini wrote:
> >> ...
> >> >
> >> > In order to work with the upcoming new Automake testsuite harness, coreutils
> >> > have two possibilities:
> >> >  1. move the `shell_or_perl_' subroutine's functionality into a real acript,
> >> >     and define the LOG_COMPILER to point to it; or
> >> >  2. add a `.pl' extension to the perl test scripts, and define PL_LOG_COMPILER
> >> >     appropriately (might be a little tricky, considering the hops that the
> >> >     `shell_or_perl_' subroutine goes through in order to get the flags and
> >> >     imports right).
> >>
> >> 1) sounds preferable.
> >>
> > But it has a serious drawback: the redirection `9>&2' placed at the end
> > of TESTS_ENVIRONMENT will be rendered useless by the final exec done
> > in the new `shell_or_perl' script (at least for with shells using the
> > `cloexec' flag on fds > 2); this will bring back the problems fixed by
> > commit `v8.12-82-g6b68745' :-(
> >
Well, no, that's not true; the "problematic" shells only set the 'close-on-exec'
flag on the file descriptors they themselves create, not on the ones they inherit
from their parents:

  $ ksh -c 'exec 9>&1; sh -c "echo foo >&9"'
  sh: 9: Bad file descriptor
  $ ksh -c 'sh -c "echo foo >&9"' 9>&1
  foo
  $ (exec 9>&1; ksh -c 'sh -c "echo foo >&9"')
  foo

I should have tested better before reporting an imaginary problem.

> > So I now think we should go with solution (2).
> 
> Ok.
> 
I've gone with the less invasive solution (1) instead.  See the attached
patch.  I will soon post a follow up that tries to compensate for the
extra forks I've introduced here by removing a couple of grep calls from
the `shell-or-perl' script (this is for Cygwin, I know that the overhead
on Unix is utterly irrelevant).

Regards,
  Stefano

[0001-tests-make-test-runner-a-script-not-a-shell-function.patch (text/x-patch, inline)]

From e4dd05df8ea1f0fc99649627f895a76ec776279a Mon Sep 17 00:00:00 2001
Message-Id: <e4dd05df8ea1f0fc99649627f895a76ec776279a.1308418725.git.stefano.lattarini <at> gmail.com>
From: Stefano Lattarini <stefano.lattarini <at> gmail.com>
Date: Sat, 18 Jun 2011 10:26:15 +0200
Subject: [PATCH 1/2] tests: make test runner a script, not a shell function

This changes implements a more correct and idiomatic use of the
features of the Automake-provided 'parallel-tests' harness.
Moreover, this change is required in order for the testsuite to
continue to work with the new testsuite harness that is planned
to be introduced in Automake 1.12 (which, as of the writing date,
is still under development and in alpha state).

* tests/shell-or-perl: New auxiliary script.
* tests/Makefile.am (EXTRA_DIST): Distribute it.
* tests/check.mk (TESTS_ENVIRONMENT): Remove definition of the
`shell_or_perl_' shell function, whose code has been moved in
the new script above (with few improvements and extensions). Do
not use it to run the test scripts.
(LOG_COMPILER): New, properly invoking `shell-or-perl'.
---
 tests/Makefile.am   |    1 +
 tests/check.mk      |   27 +++++-------
 tests/shell-or-perl |  111 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 123 insertions(+), 16 deletions(-)
 create mode 100644 tests/shell-or-perl

diff --git a/tests/Makefile.am b/tests/Makefile.am
index a324dc1..f8fbd38 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -20,6 +20,7 @@ EXTRA_DIST =		\
   other-fs-tmpdir	\
   require-perl		\
   sample-test		\
+  shell-or-perl		\
   $(pr_data)
 
 root_tests =					\
diff --git a/tests/check.mk b/tests/check.mk
index 9db96af..d45c288 100644
--- a/tests/check.mk
+++ b/tests/check.mk
@@ -48,6 +48,16 @@ check-am: .built-programs
             && MAKEFLAGS= $(MAKE) -s built_programs.list)		\
           > $@-t && mv $@-t $@
 
+## `$f' is set by the Automake-generated test harness to the path of the
+## current test script stripped of VPATH components, and is used by the
+## shell-or-perl script to determine the name of the temporary files to be
+## used.  Note that $f is a shell variable, not a make macro, so the use of
+## `$$f' below is correct, and not a typo.
+LOG_COMPILER = \
+  $(SHELL) $(srcdir)/shell-or-perl \
+  --test-name "$$f" --srcdir '$(srcdir)' \
+  --shell '$(SHELL)' --perl '$(PERL)' --
+
 # Note that the first lines are statements.  They ensure that environment
 # variables that can perturb tests are unset or set to expected values.
 # The rest are envvar settings that propagate build-related Makefile
@@ -58,21 +68,6 @@ TESTS_ENVIRONMENT =				\
   test -d "$$tmp__" && test -w "$$tmp__" || tmp__=.;	\
   . $(srcdir)/envvar-check;			\
   TMPDIR=$$tmp__; export TMPDIR;		\
-  shell_or_perl_() {				\
-    if grep '^\#!/usr/bin/perl' "$$1" > /dev/null; then			\
-      if $(PERL) -e 'use warnings' > /dev/null 2>&1; then		\
-	grep '^\#!/usr/bin/perl -T' "$$1" > /dev/null && T_=T || T_=;	\
-        $(PERL) -w$$T_ -I$(srcdir) -MCoreutils -MCuSkip			\
-	      -M"CuTmpdir qw($$f)" -- "$$1";	\
-      else					\
-	echo 1>&2 "$$tst: configure did not find a usable version of Perl," \
-	  "so skipping this test";		\
-	(exit 77);				\
-      fi;					\
-    else					\
-      $(SHELL) "$$1";				\
-    fi;						\
-  };						\
   export					\
   VERSION='$(VERSION)'				\
   LOCALE_FR='$(LOCALE_FR)'			\
@@ -99,6 +94,6 @@ TESTS_ENVIRONMENT =				\
   REPLACE_GETCWD=$(REPLACE_GETCWD)		\
   ; test -d /usr/xpg4/bin && PATH='/usr/xpg4/bin$(PATH_SEPARATOR)'"$$PATH"; \
   PATH='$(abs_top_builddir)/src$(PATH_SEPARATOR)'"$$PATH" \
-  ; shell_or_perl_ 9>&2
+  ; 9>&2
 
 VERBOSE = yes
diff --git a/tests/shell-or-perl b/tests/shell-or-perl
new file mode 100644
index 0000000..ff92009
--- /dev/null
+++ b/tests/shell-or-perl
@@ -0,0 +1,111 @@
+#! /bin/sh
+# Run a test script of the coreutils test scripts, picking up the right
+# interpreter (i.e., perl or the shell) and the right flags for it (e.g.,
+# perl `-T' flag for perl scripts that must rn in tainted mode).
+#
+# Copyright (C) 2011 Free Software Foundation, Inc.
+#
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# ---------------------------------- #
+#  Readonly variables and functions  #
+# ---------------------------------- #
+
+# Help to avoid typo-related bugs.
+set -u
+
+me=shell-or-perl
+
+error_ ()
+{
+  echo "$me: $*" >&2
+  exit 99
+}
+
+print_help_ ()
+{
+  cat <<EOH
+Usage: $me [--help] [--srcdir DIR] [--shell SHELL-CMD] [--perl PERL-CMD]
+       [--test-name NAME-WITHOUT-VPATH] TEST-SCRIPT [ARGS..]
+EOH
+}
+
+# ---------------- #
+#  Option parsing  #
+# ---------------- #
+
+assign_optarg_to_var='
+  test $# -gt 1 || error_ "option '\''$1'\'' requires an argument"
+  eval "$var=\$2"
+  shift'
+
+srcdir=${srcdir-.}
+cu_PERL=${PERL-perl}
+cu_SHELL=/bin/sh # Getting $SHELL from the environment is dangerous.
+test_name=
+while test $# -gt 0; do
+  var=
+  case $1 in
+    --help) print_help_; exit $?;;
+    --shell) var=cu_SHELL;;
+    --perl) var=cu_PERL;;
+    --srcdir) var=srcdir;;
+    --test-name) var=test_name;;
+    --) shift; break;;
+    -*) error_ "unknown option '$1'";;
+    *) break;;
+  esac
+  test -z "$var" || eval "$assign_optarg_to_var"
+  shift
+done
+
+unset assing_optarg_to_var var
+
+case $# in
+  0) error_ "missing argument";;
+  *) test_script=$1; shift;;
+esac
+
+test -z "$test_name" && test_name=$test_script
+
+# --------------------- #
+#  Run the test script  #
+# --------------------- #
+
+if grep '^#!/usr/bin/perl' "$test_script" >/dev/null; then
+  # The test is a perl script.
+  if $cu_PERL -e 'use warnings' > /dev/null 2>&1; then
+    # Perl is available, see if we must run the test with taint
+    # mode on or not.
+    grep '^#!/usr/bin/perl -T' "$test_script" >/dev/null && T_=T || T_=
+    # Now run it.
+    exec $cu_PERL -w$T_ -I"$srcdir" -MCoreutils -MCuSkip \
+                  -M"CuTmpdir qw($test_name)" \
+                  -- "$test_script" ${1+"$@"}
+  else
+    # Perl is not available, skip the test.
+    echo "$test_name: skip: no usable version of Perl found"
+    exit 77
+  fi
+else
+  # Assume the test is a shell script.
+  exec $cu_SHELL "$test_script" ${1+"$@"}
+fi
+
+# ------------- #
+#  Not reached  #
+# ------------- #
+
+error_ "dead code reached"
-- 
1.7.2.3

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefano Lattarini <stefano.lattarini <at> gmail.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: Ralf Wildenhues <Ralf.Wildenhues <at> gmx.de>, bug-coreutils <at> gnu.org,
	automake-patches <at> gnu.org
Subject: Re: [PATCH 1/2] tests: make test runner a script,
	not a shell function (was: Automake patches for custom test drivers'
	support break coreutils testsuite)
Date: Sat, 18 Jun 2011 20:18:10 +0200

[Message part 1 (text/plain, inline)]

On Saturday 18 June 2011, Stefano Lattarini wrote:
> On Saturday 18 June 2011, Jim Meyering wrote:
> > Stefano Lattarini wrote:
> > >> ...
> > >> >
> > >> > In order to work with the upcoming new Automake testsuite harness, coreutils
> > >> > have two possibilities:
> > >> >  1. move the `shell_or_perl_' subroutine's functionality into a real acript,
> > >> >     and define the LOG_COMPILER to point to it; or
> > >> >  2. add a `.pl' extension to the perl test scripts, and define PL_LOG_COMPILER
> > >> >     appropriately (might be a little tricky, considering the hops that the
> > >> >     `shell_or_perl_' subroutine goes through in order to get the flags and
> > >> >     imports right).
> > >>
> > >> 1) sounds preferable.
> > >>
> > > But it has a serious drawback: the redirection `9>&2' placed at the end
> > > of TESTS_ENVIRONMENT will be rendered useless by the final exec done
> > > in the new `shell_or_perl' script (at least for with shells using the
> > > `cloexec' flag on fds > 2); this will bring back the problems fixed by
> > > commit `v8.12-82-g6b68745' :-(
> > >
> Well, no, that's not true; the "problematic" shells only set the 'close-on-exec'
> flag on the file descriptors they themselves create, not on the ones they inherit
> from their parents:
> 
>   $ ksh -c 'exec 9>&1; sh -c "echo foo >&9"'
>   sh: 9: Bad file descriptor
>   $ ksh -c 'sh -c "echo foo >&9"' 9>&1
>   foo
>   $ (exec 9>&1; ksh -c 'sh -c "echo foo >&9"')
>   foo
> 
> I should have tested better before reporting an imaginary problem.
> 
> > > So I now think we should go with solution (2).
> > 
> > Ok.
> > 
> I've gone with the less invasive solution (1) instead.  See the attached
> patch.  I will soon post a follow up that tries to compensate for the
> extra forks I've introduced here by removing a couple of grep calls from
> the `shell-or-perl' script
>
Here it is.

Regards,
  Stefano

[0002-tests-avoid-extra-forks-in-the-testsuite.patch (text/x-patch, inline)]

From bb3cbef9eece619dd694a60e5a738e71e939f9aa Mon Sep 17 00:00:00 2001
Message-Id: <bb3cbef9eece619dd694a60e5a738e71e939f9aa.1308420965.git.stefano.lattarini <at> gmail.com>
In-Reply-To: <e4dd05df8ea1f0fc99649627f895a76ec776279a.1308420965.git.stefano.lattarini <at> gmail.com>
References: <e4dd05df8ea1f0fc99649627f895a76ec776279a.1308420965.git.stefano.lattarini <at> gmail.com>
From: Stefano Lattarini <stefano.lattarini <at> gmail.com>
Date: Sat, 18 Jun 2011 17:57:53 +0200
Subject: [PATCH 2/2] tests: avoid extra forks in the testsuite

* tests/shell-or-perl: Prefer the `read' builtin over `grep' to
look at the shebang line of test scripts.  Since `read' is a
special builtin, it might abort the whole program upon failures,
so add extra sanity checks, verifying that the test script exists
and is readable, before trying to read from it.
---
 tests/shell-or-perl |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/tests/shell-or-perl b/tests/shell-or-perl
index ff92009..8c92f87 100644
--- a/tests/shell-or-perl
+++ b/tests/shell-or-perl
@@ -84,12 +84,19 @@ test -z "$test_name" && test_name=$test_script
 #  Run the test script  #
 # --------------------- #
 
-if grep '^#!/usr/bin/perl' "$test_script" >/dev/null; then
+test -f "$test_script" && test -r "$test_script" \
+  || error_ "test script '$test_script' does not exist, or isn't readable"
+
+read shebang_line < "$test_script" \
+  || error_ "cannot read from the test script '$test_script'"
+
+case $shebang_line in
+'#!/usr/bin/perl'*)
   # The test is a perl script.
   if $cu_PERL -e 'use warnings' > /dev/null 2>&1; then
     # Perl is available, see if we must run the test with taint
     # mode on or not.
-    grep '^#!/usr/bin/perl -T' "$test_script" >/dev/null && T_=T || T_=
+    case $shebang_line in *\ -T*) T_=T;; *) T_=;; esac
     # Now run it.
     exec $cu_PERL -w$T_ -I"$srcdir" -MCoreutils -MCuSkip \
                   -M"CuTmpdir qw($test_name)" \
@@ -99,10 +106,11 @@ if grep '^#!/usr/bin/perl' "$test_script" >/dev/null; then
     echo "$test_name: skip: no usable version of Perl found"
     exit 77
   fi
-else
+  ;;
+*)
   # Assume the test is a shell script.
   exec $cu_SHELL "$test_script" ${1+"$@"}
-fi
+esac
 
 # ------------- #
 #  Not reached  #
-- 
1.7.2.3

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Stefano Lattarini <stefano.lattarini <at> gmail.com>
Cc: Ralf Wildenhues <Ralf.Wildenhues <at> gmx.de>, bug-coreutils <at> gnu.org,
	automake-patches <at> gnu.org
Subject: Re: [PATCH 1/2] tests: make test runner a script, not a shell function
Date: Sun, 19 Jun 2011 21:51:42 +0200

Thank you!
That patch looks fine modulo two typos.
I'm folding in these corrections and have adjusted the
grammar in the commit log (included below).

diff --git a/tests/shell-or-perl b/tests/shell-or-perl
index ff92009..08604eb 100644
--- a/tests/shell-or-perl
+++ b/tests/shell-or-perl
@@ -1,7 +1,7 @@
 #! /bin/sh
 # Run a test script of the coreutils test scripts, picking up the right
 # interpreter (i.e., perl or the shell) and the right flags for it (e.g.,
-# perl `-T' flag for perl scripts that must rn in tainted mode).
+# perl `-T' flag for perl scripts that must run in tainted mode).
 #
 # Copyright (C) 2011 Free Software Foundation, Inc.
 #
@@ -71,7 +71,7 @@ while test $# -gt 0; do
   shift
 done

-unset assing_optarg_to_var var
+unset assign_optarg_to_var var

 case $# in
   0) error_ "missing argument";;

------------------------------
    tests: make test runner a script, not a shell function

    This change implements a more correct and idiomatic use of the
    features of the Automake-provided 'parallel-tests' harness.
    Moreover, this change is required in order for the testsuite to
    continue to work with the new testsuite harness that is planned
    to be introduced in Automake 1.12 (which, as of the writing date,
    is still under development and in alpha state).

    * tests/shell-or-perl: New auxiliary script.
    * tests/Makefile.am (EXTRA_DIST): Distribute it.
    * tests/check.mk (TESTS_ENVIRONMENT): Remove definition of the
    `shell_or_perl_' shell function, whose code has been moved in
    the new script above (with a few improvements and extensions).
    Do not use it to run the test scripts.
    (LOG_COMPILER): New, properly invoking `shell-or-perl'.

Message #14 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Stefano Lattarini <stefano.lattarini <at> gmail.com>
Cc: Ralf Wildenhues <Ralf.Wildenhues <at> gmx.de>, bug-coreutils <at> gnu.org,
	automake-patches <at> gnu.org
Subject: Re: [PATCH 1/2] tests: make test runner a script, not a shell function
Date: Sun, 19 Jun 2011 22:26:26 +0200

Stefano Lattarini wrote:
> Subject: [PATCH 2/2] tests: avoid extra forks in the testsuite
>
> * tests/shell-or-perl: Prefer the `read' builtin over `grep' to
> look at the shebang line of test scripts.  Since `read' is a
> special builtin, it might abort the whole program upon failures,
> so add extra sanity checks, verifying that the test script exists
> and is readable, before trying to read from it.
> ---
>  tests/shell-or-perl |   16 ++++++++++++----
>  1 files changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/tests/shell-or-perl b/tests/shell-or-perl
> index ff92009..8c92f87 100644
> --- a/tests/shell-or-perl
> +++ b/tests/shell-or-perl
> @@ -84,12 +84,19 @@ test -z "$test_name" && test_name=$test_script
>  #  Run the test script  #
>  # --------------------- #
>
> -if grep '^#!/usr/bin/perl' "$test_script" >/dev/null; then
> +test -f "$test_script" && test -r "$test_script" \
> +  || error_ "test script '$test_script' does not exist, or isn't readable"
> +
> +read shebang_line < "$test_script" \
> +  || error_ "cannot read from the test script '$test_script'"
> +
> +case $shebang_line in
> +'#!/usr/bin/perl'*)
>    # The test is a perl script.
>    if $cu_PERL -e 'use warnings' > /dev/null 2>&1; then
>      # Perl is available, see if we must run the test with taint
>      # mode on or not.
> -    grep '^#!/usr/bin/perl -T' "$test_script" >/dev/null && T_=T || T_=
> +    case $shebang_line in *\ -T*) T_=T;; *) T_=;; esac
>      # Now run it.
>      exec $cu_PERL -w$T_ -I"$srcdir" -MCoreutils -MCuSkip \
>                    -M"CuTmpdir qw($test_name)" \
> @@ -99,10 +106,11 @@ if grep '^#!/usr/bin/perl' "$test_script" >/dev/null; then
>      echo "$test_name: skip: no usable version of Perl found"
>      exit 77
>    fi
> -else
> +  ;;
> +*)
>    # Assume the test is a shell script.
>    exec $cu_SHELL "$test_script" ${1+"$@"}
> -fi
> +esac

Thank you.  I pushed that as-is.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.