Re: interrupt causes parse error in configure script

[Ralf Wildenhues]

Hello again,

first, sorry for being a bit blunt earlier.

* Jim Meyering wrote on Mon, Aug 18, 2008 at 05:17:22PM CEST:
> 
> Yes, I am familiar with that idiom (I've been writing
> autoconf macros that use it for over 12 years).
> However, no code like that is affected, since it is not using
> "test AS_GET_VAR([some_var]) = yes".

Yes, you are right that gnulib macros don't currently use it.
I was too broad here; at least it /could/ be used that way
(without us warning about it).

> Notice that all changed uses in autoconf are something like
> this, where the variable in question is always set:
> 
>     AC_CACHE_CHECK([for $1], [ac_Header],
>                    [AS_VAR_SET([ac_Header], [$literal_or_set])])
>     AS_IF([AS_VAR_YES([ac_Header])], [$2], [$3])[]dnl

Yes.

> > Your patch papers over all those bits.
> 
> As far as I can see, that patch does not paper over anything.
> Can you give an example?

No.  The examples are always clear in that one can see from a handful
lines above that variables are always set.  However, the patch also
doesn't make the actual problem very clear.


* Eric Blake wrote on Mon, Aug 18, 2008 at 05:12:43PM CEST:
> 
> There's a difference between:
> 
> gl_cv_XXX=...
> if test $gl_cv_XXX = ...; then
> 
> and what Jim is attempting to fix:
> 
> if test `...` = ...; then

Indeed, I did not understand that difference earlier.

> It appears that what Jim is seeing is that the shell (which one?)
> catches the Ctrl-C, aborts the subshell, but then proceeds with
> execution of the builtin test before quitting the script.  In other
> words, the Ctrl-C is not properly affecting the parent shell.

ACK.  I've tried some shells with your example:

> I think we can factor it into a simpler testcase.  I just tried this:
> 
> $ sh -c 'if test `sleep 5; echo hi` = hi ; then echo yes; fi'

and found only pdksh and OpenBSD sh (which is a pdksh descendant) to
have this issue.

> So it looks like my copy of ksh (pdksh 5.2.14) suffers from the bug.

> > Beside, the AS_VAR_YES interface is ugly, but that's orthogonal.
> 
> And based on the usage pattern, maybe an interface like this would be nicer:
> 
> AS_VAR_IFELSE(var, [value = yes], if-true, if-false)

Very good idea.  Proposed patch below, for this and the documentation
issue.

* Paul Eggert wrote on Mon, Aug 18, 2008 at 07:24:53PM CEST:
> > $ ksh -c 'if test `sleep 5; echo hi` = hi ; then echo yes; fi'
> > ^Cksh: test: hi: unexpected operator/operand
> 
> I've also observed this problem.  More generally, I've seen ^C not
> interrupt 'configure' scripts properly, in ways other than what Jim is
> fixing here, on various platforms.  I haven't bothered to try to track
> down all the shell bugs involved, but it's nice that Jim is squashing
> one of them....

If you come across such instances again, please report system and shell
here, thanks.


What do you think about this patch (putting Eric and me in ChangeLog as
co authors)?

FWIW I'm still looking into a couple of test failures (one of "AS_IF and
AS_CASE" which I don't think this patch caused).

I suppose the AS_VAR* macros deserve documentation eventually.

Thanks,
Ralf

>From 19518f39b188614a42fcdb7863b679610f17baba Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 18 Aug 2008 22:17:44 +0200
Subject: [PATCH] AS_VAR_IF: new function, to handle an unusual failure

Prompted by this (interrupting a pdksh configure run):

    $ ./configure
    ...
    checking for alloca... (cached) yes
    checking for arpa/inet.h... (cached) yes
    ^C./configure: line 6354: test: =: unary operator expected
    ,make: *** [config.status] Error 1
    [Exit 130 (INT)]

* lib/m4sugar/m4sh.m4 (AS_VAR_IF): New function.
* lib/autoconf/functions.m4 (AC_CHECK_FUNC): Use it in place of
"test AS_VAR_GET([...]) = yes"
* lib/autoconf/general.m4 (AC_CHECK_FILE, AC_CHECK_DECL): Likewise.
* lib/autoconf/headers.m4 (_AC_CHECK_HEADER_MONGREL): Likewise.
(_AC_CHECK_HEADER_NEW, _AC_CHECK_HEADER_OLD): Likewise.
(_AC_CHECK_HEADER_DIRENT): Likewise.
* lib/autoconf/libs.m4 (AC_CHECK_LIB): Likewise.
* lib/autoconf/types.m4 (_AC_CHECK_TYPE_NEW, AC_CHECK_MEMBER): Likewise.
* doc/autoconf.texi (Shell Substitutions): Document the pdksh issue.

Signed-off-by: Ralf Wildenhues <address@hidden>
---
 doc/autoconf.texi         |   12 ++++++++++++
 lib/autoconf/functions.m4 |    2 +-
 lib/autoconf/general.m4   |    4 ++--
 lib/autoconf/headers.m4   |    8 ++++----
 lib/autoconf/libs.m4      |    2 +-
 lib/autoconf/types.m4     |    4 ++--
 lib/m4sugar/m4sh.m4       |   11 +++++++++++
 7 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/doc/autoconf.texi b/doc/autoconf.texi
index 61cccca..d56d28b 100644
--- a/doc/autoconf.texi
+++ b/doc/autoconf.texi
@@ -13454,6 +13454,18 @@ $ @kbd{echo "`printf 'foo\r\n'`"" bar" | cmp - broken}
 - broken differ: char 4, line 1
 @end example
 
+Upon interrupt, pdksh and OpenBSD sh may abort a command substitution,
+replace it with a null string, and evaluate the enclosing command before
+interrupting the script.  This can lead to spurious errors:
+
address@hidden
+$ @kbd{pdksh -c 'if test `sleep 5; echo hi` = hi ; then echo yes; fi'}
+$ @kbd{^C}
+pdksh: test: hi: unexpected operator/operand
address@hidden example
+
address@hidden
address@hidden can be used to avoid this.
 
 @item $(@var{commands})
 @cindex $(@var{commands})
diff --git a/lib/autoconf/functions.m4 b/lib/autoconf/functions.m4
index 78c7678..c325f30 100644
--- a/lib/autoconf/functions.m4
+++ b/lib/autoconf/functions.m4
@@ -70,7 +70,7 @@ AC_CACHE_CHECK([for $1], [ac_var],
 [AC_LINK_IFELSE([AC_LANG_FUNC_LINK_TRY([$1])],
                [AS_VAR_SET([ac_var], [yes])],
                [AS_VAR_SET([ac_var], [no])])])
-AS_IF([test AS_VAR_GET([ac_var]) = yes], [$2], [$3])dnl
+AS_VAR_IF([ac_var], [yes], [$2], [$3])dnl
 AS_VAR_POPDEF([ac_var])dnl
 ])# AC_CHECK_FUNC
 
diff --git a/lib/autoconf/general.m4 b/lib/autoconf/general.m4
index 8af0dc4..265d78b 100644
--- a/lib/autoconf/general.m4
+++ b/lib/autoconf/general.m4
@@ -2614,7 +2614,7 @@ if test -r "$1"; then
 else
   AS_VAR_SET([ac_File], [no])
 fi])
-AS_IF([test AS_VAR_GET([ac_File]) = yes], [$2], [$3])[]dnl
+AS_VAR_IF([ac_File], [yes], [$2], [$3])[]dnl
 AS_VAR_POPDEF([ac_File])dnl
 ])# AC_CHECK_FILE
 
@@ -2651,7 +2651,7 @@ AC_CACHE_CHECK([whether $1 is declared], [ac_Symbol],
 ])],
                   [AS_VAR_SET([ac_Symbol], [yes])],
                   [AS_VAR_SET([ac_Symbol], [no])])])
-AS_IF([test AS_VAR_GET([ac_Symbol]) = yes], [$2], [$3])[]dnl
+AS_VAR_IF([ac_Symbol], [yes], [$2], [$3])[]dnl
 AS_VAR_POPDEF([ac_Symbol])dnl
 ])# AC_CHECK_DECL
 
diff --git a/lib/autoconf/headers.m4 b/lib/autoconf/headers.m4
index 476df37..16b2737 100644
--- a/lib/autoconf/headers.m4
+++ b/lib/autoconf/headers.m4
@@ -143,7 +143,7 @@ esac
 AC_CACHE_CHECK([for $1], [ac_Header],
               [AS_VAR_SET([ac_Header], [$ac_header_preproc])])
 ])dnl ! set ac_HEADER
-AS_IF([test AS_VAR_GET([ac_Header]) = yes], [$2], [$3])[]dnl
+AS_VAR_IF([ac_Header], [yes], [$2], [$3])[]dnl
 AS_VAR_POPDEF([ac_Header])dnl
 ])# _AC_CHECK_HEADER_MONGREL
 
@@ -160,7 +160,7 @@ AC_CACHE_CHECK([for $1], [ac_Header],
 @%:@include <$1>])],
                                  [AS_VAR_SET([ac_Header], [yes])],
                                  [AS_VAR_SET([ac_Header], [no])])])
-AS_IF([test AS_VAR_GET([ac_Header]) = yes], [$2], [$3])[]dnl
+AS_VAR_IF([ac_Header], [yes], [$2], [$3])[]dnl
 AS_VAR_POPDEF([ac_Header])dnl
 ])# _AC_CHECK_HEADER_NEW
 
@@ -175,7 +175,7 @@ AC_CACHE_CHECK([for $1], [ac_Header],
               [AC_PREPROC_IFELSE([AC_LANG_SOURCE(address@hidden:@include 
<$1>])],
                                         [AS_VAR_SET([ac_Header], [yes])],
                                         [AS_VAR_SET([ac_Header], [no])])])
-AS_IF([test AS_VAR_GET([ac_Header]) = yes], [$2], [$3])[]dnl
+AS_VAR_IF([ac_Header], [yes], [$2], [$3])[]dnl
 AS_VAR_POPDEF([ac_Header])dnl
 ])# _AC_CHECK_HEADER_OLD
 
@@ -403,7 +403,7 @@ AC_CACHE_CHECK([for $1 that defines DIR], [ac_Header],
 return 0;])],
                   [AS_VAR_SET([ac_Header], [yes])],
                   [AS_VAR_SET([ac_Header], [no])])])
-AS_IF([test AS_VAR_GET([ac_Header]) = yes], [$2], [$3])[]dnl
+AS_VAR_IF([ac_Header], [yes], [$2], [$3])[]dnl
 AS_VAR_POPDEF([ac_Header])dnl
 ])# _AC_CHECK_HEADER_DIRENT
 
diff --git a/lib/autoconf/libs.m4 b/lib/autoconf/libs.m4
index 429918c..a1b8cfe 100644
--- a/lib/autoconf/libs.m4
+++ b/lib/autoconf/libs.m4
@@ -130,7 +130,7 @@ AC_LINK_IFELSE([AC_LANG_CALL([], [$2])],
               [AS_VAR_SET([ac_Lib], [yes])],
               [AS_VAR_SET([ac_Lib], [no])])
 LIBS=$ac_check_lib_save_LIBS])
-AS_IF([test AS_VAR_GET([ac_Lib]) = yes],
+AS_VAR_IF([ac_Lib], [yes],
       [m4_default([$3], [AC_DEFINE_UNQUOTED(AS_TR_CPP(HAVE_LIB$1))
   LIBS="-l$1 $LIBS"
 ])],
diff --git a/lib/autoconf/types.m4 b/lib/autoconf/types.m4
index 50a489c..0ab85a5 100644
--- a/lib/autoconf/types.m4
+++ b/lib/autoconf/types.m4
@@ -160,7 +160,7 @@ AC_COMPILE_IFELSE(
          return 0;])],
      [],
      [AS_VAR_SET([ac_Type], [yes])])])])
-AS_IF([test AS_VAR_GET([ac_Type]) = yes], [$2], [$3])[]dnl
+AS_VAR_IF([ac_Type], [yes], [$2], [$3])[]dnl
 AS_VAR_POPDEF([ac_Type])dnl
 ])# _AC_CHECK_TYPE_NEW
 
@@ -834,7 +834,7 @@ if (sizeof ac_aggr.m4_bpatsubst([$1], [^[^.]*\.]))
 return 0;])],
                [AS_VAR_SET([ac_Member], [yes])],
                [AS_VAR_SET([ac_Member], [no])])])])
-AS_IF([test AS_VAR_GET([ac_Member]) = yes], [$2], [$3])dnl
+AS_VAR_IF([ac_Member], [yes], [$2], [$3])dnl
 AS_VAR_POPDEF([ac_Member])dnl
 ])# AC_CHECK_MEMBER
 
diff --git a/lib/m4sugar/m4sh.m4 b/lib/m4sugar/m4sh.m4
index 1517630..c3af80a 100644
--- a/lib/m4sugar/m4sh.m4
+++ b/lib/m4sugar/m4sh.m4
@@ -1576,6 +1576,17 @@ m4_define([AS_VAR_SET_IF],
 [AS_IF([AS_VAR_TEST_SET([$1])], [$2], [$3])])
 
 
+# AS_VAR_IF(VARIABLE, [VALUE = yes], IF-TRUE, IF-FALSE)
+# -----------------------------------------------------
+# Implement a shell `if test $VARIABLE = VALUE; then-else'.
+# Polymorphic, and avoids pdksh expansion error upon interrupt.
+m4_define([AS_VAR_IF],
+[AS_LITERAL_IF([$1],
+  [AS_IF([test "x$$1" = x""m4_default([$2], [yes])], [$3], [$4])],
+  [as_val=AS_VAR_GET([$1])
+   AS_IF([test "x$as_val" = x""m4_default([$2], [yes])], [$3], [$4])])])
+
+
 # AS_VAR_PUSHDEF and AS_VAR_POPDEF
 # --------------------------------
 #
-- 
1.5.5.40.g4cdda