Re: AC_INIT regression

[Ralf Wildenhues]

[ 
<http://thread.gmane.org/gmane.comp.sysutils.autoconf.patches/7202/focus=7217> ]

* Paolo Bonzini wrote on Sat, Jul 10, 2010 at 09:24:31PM CEST:
> On 07/10/2010 06:28 PM, Eric Blake wrote:
> >On 07/10/2010 08:49 AM, Eric Blake wrote:
> >>It would still be nice to address Bruno's report about AC_INIT and valid
> >>version strings first.
> >
> >Here's where I got before running out of time to actually test it with
> >AC_INIT:
> 
> Is it even clear that it is a bug in the first place?

Well, in the sense that Eric's patch series before 2.66 was not intended
to stricten semantics, it is a regression.  In the sense of saving the
innocent user of common setups where PACKAGE and VERSION _are_ reused in
places outside Autoconf (e.g., Automake) or even just outside of here-
douments, the unintended semantic change certainly was an improvement.

I have a patch below that turns Eric's diff into a patch for PACKAGE and
VERSION.  It really makes me cringe though: we ourselves break the
promise not to use PACKAGE and VERSION (nor BUG-REPORT btw.) outside of
here-douments:
  ./configure --help=short
  ./configure --help=recursive

and running
  ./config.status

extends config.log with a version of PACKAGE_STRING that was set to a
double-quoted string after going through a quoted here-document.  So at
least double quotes in these strings are a deal-breaker here.

Question is: Eric, do you really want to have different standards for
the first three arguments for AC_INIT, and do you want to limit all
future Autoconf versions to allowing to use PACKAGE and VERSION only in
here-documents?

More fundamentally, I wonder whether this semantic change:

  ** The macro AS_LITERAL_IF is slightly more conservative; text
     containing shell quotes are no longer treated as literals.

shouldn't better have been implemented under a new macro name.  Surely,
heuristics are what they are, but the user will come to depend on them,
which is why they cause friction when altered.

> It should be reported in the release notes for 2.67 of course.

Of course.

So, concluded, if Eric tells me to then I will apply the patch below and
can go for 2.67, but I'm not too happy with it, for reasons stated
above.

Thanks,
Ralf

PS: to add to ugliness, Bruno's use case could easily be worked around
by `autoreconf -Wno-syntax', except of course older aclocal didn't
accept -W* flags so autoreconf doesn't pass it to them.  Yes, autoreconf
should check and then pass, another bug to fix ...


2010-07-11  Eric Blake  <address@hidden>
        and Ralf Wildenhues  <address@hidden>

        Relax AC_INIT requirements for PACKAGE and VERSION strings again.
        * lib/m4sugar/m4sh.m4 (AS_LITERAL_HEREDOC_IF): New macro.
        (_AS_LITERAL_HEREDOC_IF, _AS_LITERAL_HEREDOC_IF_YES)
        (_AS_LITERAL_HEREDOC_IF_NO): New helper macros.
        * lib/autoconf/general.m4 (_AC_INIT_PACKAGE): Use
        AS_LITERAL_HEREDOC_IF for PACKAGE and VERSION strings.
        * tests/base.at (AC_INIT with unusual version strings): New test.
        * tests/m4sh.at (AS@&address@hidden): Extend test.
        * NEWS: Update.

diff --git a/NEWS b/NEWS
index 0d72608..97c9739 100644
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,12 @@ GNU Autoconf NEWS - User visible changes.
 ** AC_CHECK_SIZEOF of a pointer type works again.  Regression introduced in
    2.66.
 
+** AC_INIT again allows spaces and other characters that are literal in both
+   quoted and unquoted here-documents again in its PACKAGE and VERSION
+   arguments.  This is relevant for packages not using these strings nor
+   products like PACKAGE_STRING in other contexts, e.g., for Automake.
+   Regression introduces in 2.66.
+
 ** New macros AC_FC_LINE_LENGTH to accept long Fortran source code lines
    and AC_FC_MODULE_EXTENSION to compute the Fortran module extension.
 
diff --git a/lib/autoconf/general.m4 b/lib/autoconf/general.m4
index fc4905b..598b87b 100644
--- a/lib/autoconf/general.m4
+++ b/lib/autoconf/general.m4
@@ -229,8 +229,10 @@ AU_ALIAS([AC_HELP_STRING], [AS_HELP_STRING])
 # _AC_INIT_PACKAGE(PACKAGE-NAME, VERSION, BUG-REPORT, [TARNAME], [URL])
 # ---------------------------------------------------------------------
 m4_define([_AC_INIT_PACKAGE],
-[AS_LITERAL_IF([$1], [], [m4_warn([syntax], [AC_INIT: not a literal: $1])])
-AS_LITERAL_IF([$2], [],  [m4_warn([syntax], [AC_INIT: not a literal: $2])])
+[AS_LITERAL_HEREDOC_IF([$1], [],
+                      [m4_warn([syntax], [AC_INIT: not a literal: $1])])
+AS_LITERAL_HEREDOC_IF([$2], [],
+                     [m4_warn([syntax], [AC_INIT: not a literal: $2])])
 AS_LITERAL_IF([$3], [],  [m4_warn([syntax], [AC_INIT: not a literal: $3])])
 m4_ifndef([AC_PACKAGE_NAME],
          [m4_define([AC_PACKAGE_NAME],     [$1])])
diff --git a/lib/m4sugar/m4sh.m4 b/lib/m4sugar/m4sh.m4
index 6fbc9e1..3cc868c 100644
--- a/lib/m4sugar/m4sh.m4
+++ b/lib/m4sugar/m4sh.m4
@@ -1592,6 +1592,23 @@ m4_define([_AS_LITERAL_IF_NO], [$2])
 m4_define([AS_LITERAL_WORD_IF],
 [_AS_LITERAL_IF(m4_expand([$1]))([$4], [$3], [$2])])
 
+# AS_LITERAL_HEREDOC_IF(EXPRESSION, IF-LITERAL, IF-NOT-LITERAL)
+# -------------------------------------------------------------
+# Like AS_LITERAL_IF, except that a string is considered literal
+# if it results in the same output in both quoted and unquoted
+# here-documents.
+m4_define([AS_LITERAL_HEREDOC_IF],
+[_$0(m4_expand([$1]))([$2], [$3])])
+
+m4_define([_AS_LITERAL_HEREDOC_IF],
+[m4_if(m4_index([$1], address@hidden|@]), [-1],
+  [m4_if(m4_index(m4_translit([[$1]], [\`], [$]), [$]), [-1],
+    [$0_YES], [$0_NO])],
+  [$0_NO])])
+
+m4_define([_AS_LITERAL_HEREDOC_IF_YES], [$1])
+m4_define([_AS_LITERAL_HEREDOC_IF_NO], [$2])
+
 
 # AS_TMPDIR(PREFIX, [DIRECTORY = $TMPDIR [= /tmp]])
 # -------------------------------------------------
diff --git a/tests/base.at b/tests/base.at
index 8f6e30f..ac4ab27 100644
--- a/tests/base.at
+++ b/tests/base.at
@@ -219,6 +219,27 @@ AT_CHECK([grep "^PACKAGE_TARNAME='fu'\$" configure], [], 
[ignore])
 AT_CLEANUP
 
 
+## ------------------------------------- ##
+## AC_INIT with unusual version strings. ##
+## ------------------------------------- ##
+
+AT_SETUP([AC_INIT with unusual version strings])
+
+AT_DATA([configure.ac],
+[[AC_INIT([GNU String++ with spaces (foo)],
+         [2.48++ (2010-07-03)], [http://clisp.cons.org/], [clisp])
+AC_OUTPUT
+]])
+
+AT_CHECK_AUTOCONF([-Werror])
+AT_CHECK_CONFIGURE([-q])
+AT_CHECK_CONFIGURE([--help])
+AT_CHECK_CONFIGURE([--version])
+AT_CHECK([./config.status --version], [], [ignore])
+
+AT_CLEANUP
+
+
 ## -------------- ##
 ## AC_COPYRIGHT.  ##
 ## -------------- ##
diff --git a/tests/m4sh.at b/tests/m4sh.at
index 2f79332..52ab4ee 100644
--- a/tests/m4sh.at
+++ b/tests/m4sh.at
@@ -1119,7 +1119,7 @@ AT_CLEANUP
 ## --------------- ##
 
 AT_SETUP([AS@&address@hidden)
-AT_KEYWORDS([m4sh AS@&address@hidden)
+AT_KEYWORDS([m4sh AS@&address@hidden AS@&address@hidden)
 
 AT_DATA_M4SH([script.as], [[dnl
 AS_INIT
@@ -1141,9 +1141,17 @@ m4_define([mac], [l``it])
 echo AS_LITERAL_IF([mac], [ERR], [ok]) 13
 echo AS_LITERAL_IF([mac], [ERR1], [ok], [ERR2]) 14
 echo AS_LITERAL_IF([   a ][
-b], [ok], [ok]) 15
+b], [ok], [ERR]) 15
 echo AS_LITERAL_WORD_IF([      a ][
 b], [ERR], [ok]) 16
+echo AS_LITERAL_HEREDOC_IF([   a ][
+b], [ok], [ERR]) 17
+echo AS_LITERAL_IF([(a)], [ERR], [ok]) 18
+echo AS_LITERAL_WORD_IF([(a)], [ERR], [ok]) 19
+echo AS_LITERAL_HEREDOC_IF([(a)], [ok], [ERR]) 20
+echo AS_LITERAL_IF(address@hidden|@a], [ERR], [ok]) 21
+echo AS_LITERAL_WORD_IF(address@hidden|@a], [ERR], [ok]) 22
+echo AS_LITERAL_HEREDOC_IF(address@hidden|@a], [ERR], [ok]) 23
 ]])
 
 AT_CHECK_M4SH
@@ -1164,6 +1172,13 @@ ok 13
 ok 14
 ok 15
 ok 16
+ok 17
+ok 18
+ok 19
+ok 20
+ok 21
+ok 22
+ok 23
 ]])
 
 AT_CLEANUP