mainAutoconf - Support: sr #110886, autoupdate produces broken...

 
 

sr #110886: autoupdate produces broken configure.ac

Submitter:  Maxim Cournoyer <apteryx>
Submitted:  Thu 15 Jun 2023 01:41:53 AM UTC
Votes: 4
 
Priority:  * 1 - Blocked Severity:  3 - Normal
Status:  Postponed Privacy:  Public
Assigned to:  None Open/Closed:  Open
Operating System:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 08 Dec 2023 06:31:16 PM UTC, comment #7: 

To reiterate, all the issues described in this bug report are caused by limitations in autoupdate that I don't think they can be fixed with the present architecture.  Absent a volunteer to develop something completely new, this is not likely ever to change.

Zack Weinberg <zackw>
Group administrator
Thu 15 Jun 2023 05:54:55 PM UTC, comment #6: 

You should also look carefully at the existing set of autoupdate tests (tests/tools.at, "autoupdate" section), especially the ones marked as expected to fail.

Zack Weinberg <zackw>
Group administrator
Thu 15 Jun 2023 05:51:43 PM UTC, comment #5: 

Yeah, these are all aspects of another really basic limitation: the only thing autoupdate can fix is obsolete macros.  It can't fix quotation errors, it can't fix badly placed `dnl`, and it can't fix misuse of `AC_REQUIRE`.  And all these things may cause it to produce ill-formed output.

If you're seriously thinking of working on this, I think you should look at writing your own dedicated parser for machine-assisted refactoring of Autoconf scripts.  It's not going to be easy.  I'd probably start with the tree-sitter grammar for Bash (there currently isn't a tree-sitter grammar for either M4 or POSIX shell without Bash extensions).

Zack Weinberg <zackw>
Group administrator
Thu 15 Jun 2023 04:39:01 PM UTC, comment #4: 

Another one, which falls into the nested category: a quote was stripped, leaving the result unbalanced:


@@ -3618,17 +3578,14 @@ if test "$wx_cv_size_t_is_uint" = "yes"; then
 else
     AC_CACHE_CHECK([if size_t is unsigned long], [
         wx_cv_size_t_is_ulong, [
-        AC_TRY_COMPILE([#include <stddef.h>],
-            [
+        AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <stddef.h>]], [[
                 return 0; }

                 struct Foo { void foo(size_t); void foo(unsigned long); };

                 int bar() {
-            ],
-            wx_cv_size_t_is_ulong=no,
-            wx_cv_size_t_is_ulong=yes
-        )
+            ]])],[wx_cv_size_t_is_ulong=no],[wx_cv_size_t_is_ulong=yes
+        ])
     )]

     if test "$wx_cv_size_t_is_ulong" = "yes"; then


Maxim Cournoyer <apteryx>
Thu 15 Jun 2023 04:01:16 PM UTC, comment #3: 

The use of AC_REQUIRE at the top level was also ill-advised in:


@@ -2635,7 +2616,7 @@ if test "$wxUSE_EXPAT" != "no"; then
         dnl Expat requires C99 compiler, so define wxCFLAGS_C99 variable which
         dnl is used when compiling it in our makefiles.
         save_CC="$CC"
-        AC_PROG_CC_C99
+        AC_REQUIRE(AC_PROG_CC)
         CC="$save_CC"

         dnl Note that we need just the flags, not the full CC value including


It breaks the generated configure script.

Maxim Cournoyer <apteryx>
Thu 15 Jun 2023 02:52:12 PM UTC, comment #2: 

Hi Zack,

Thanks for the prompt feedback.

Another issue I've found is that the mishandling of 'dnl' comments; they are often added into the argument itself, breaking for example this AC_TRY_RUN at configure time:


@@ -2200,9 +2188,8 @@ if test "$wxUSE_ZLIB" != "no" ; then
         dnl has anything more ancient (1.1.3 was released in July 1998)
         dnl anyhow
         AC_CACHE_CHECK([for zlib.h >= 1.1.4], ac_cv_header_zlib_h,
-            [AC_TRY_RUN(
-                dnl zlib.h defines ZLIB_VERSION="x.y.z"
-                [
+            [AC_RUN_IFELSE([AC_LANG_SOURCE([[dnl zlib.h defines ZLIB_VERSION="x.y.z"
+
                     #include <zlib.h>
                     #include <stdio.h>
                     #include <stdlib.h>
@@ -2218,12 +2205,7 @@ if test "$wxUSE_ZLIB" != "no" ; then
                                  ZLIB_VERSION[4] >= '4')) ? "yes" : "no");
                         return 0;
                     }
-                ],
-                ac_cv_header_zlib_h=`cat conftestval`,
-                ac_cv_header_zlib_h=no,
-                dnl cross-compiling: don't have an answer, try later
-                unset ac_cv_header_zlib_h
-            )]
+                ]])],[ac_cv_header_zlib_h=`cat conftestval`],[ac_cv_header_zlib_h=no],[dnl cross-compiling: don't have an answer])]
         )
         dnl If the test above did not come up with a value (e.g. cross
         dnl compiling) then this should give a definitive answer


Which results in the config.log to:


conftest.c:43:1: error: unknown type name 'dnl'
   43 | dnl zlib.h defines ZLIB_VERSION="x.y.z"
      | ^~~
conftest.c:43:9: error: expected '=', ',', ';', 'asm' or '__attribute__' before '.' token
   43 | dnl zlib.h defines ZLIB_VERSION="x.y.z"
      |         ^


I understand redesigning to fix the root of these problems may take a long time to fix (or never happen).  I may take a peek myself if I find the time.

Cheers,

Maxim

Maxim Cournoyer <apteryx>
Thu 15 Jun 2023 02:30:54 PM UTC, comment #1: 

Thank you for the nice clear bug report.

Unfortunately, it looks like you have tripped over one of the known limitations of autoupdate. Nested replacements just don't work, and it's unlikely that they can be made to work in the present design (this involves trying to set up m4 to expand some but not all macros, which is inherently fragile and limited).

Also unfortunately, none of the autoconf core developers have the time that would be required to come up with a design that can handle nested replacements.

Therefore, unless you or someone else develops a fix, you should not expect this to be fixed in the foreseeable future.

Zack Weinberg <zackw>
Group administrator
Thu 15 Jun 2023 01:41:53 AM UTC, original submission:  

Hi,

I've been trying to use autoupdate on the wxWidgets project [0]

[0]  https://github.com/wxWidgets/wxWidgets

It's a good test, being a 8k lines file apparently last updated for Autoconf 2.59.

After fixing a couple of quoting problems with the help of Alfred (ams), I was able to have it run once happily.  The problem is that the output it produces then trips it when running again; in other words, it produces a broken configure.ac file.

It seems the problem has to do with the _au_m4_changequote([,]) it generates, that are placed before nested AC_TRY_COMPILE or AC_TRY_LINK usages.

Attaches are the Git patches of my debugging session and recorded observations / edits.  They can be applied to wxWidgets at commit 1093bb492bc90531162490ba1711f9dba09f59e0.

Here's the first hunk of the recorded 'autoupdate broken substitutions', for example:

modified   configure.in
@@ -1407,12 +1407,10 @@ dnl This case is for OS X vs. everything else
 case "${host}" in
   --darwin* )
     AC_MSG_CHECKING([if CoreFoundation/CFBase.h is usable])
-    AC_TRY_COMPILE([#include <CoreFoundation/CFBase.h>
-],[],
-        [AC_MSG_RESULT([yes])],
-        [AC_MSG_RESULT([no])
+    AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <CoreFoundation/CFBase.h>
+]], [[]])],[AC_MSG_RESULT([yes])],[AC_MSG_RESULT([no])
         AC_MSG_CHECKING([if _CF_USE_FRAMEWORK_INCLUDES_ is required])
-        AC_TRY_COMPILE([#define _CF_USE_FRAMEWORK_INCLUDES_
+        au_m4_changequote([,])AC_TRY_COMPILE([#define __CF_USE_FRAMEWORK_INCLUDES_
 #include <CoreFoundation/CFBase.h>
         ],[],
         [AC_MSG_RESULT([yes])
@@ -1421,8 +1419,8 @@ case "${host}" in
         CPPFLAGS="-D__CF_USE_FRAMEWORK_INCLUDES__ $CPPFLAGS"],
         [AC_MSG_FAILURE([no.  CoreFoundation not available.])]
         )
-        ]
-    )
+       
+    ])
   ;;
 esac

You can replay the patches on the mentioned wxWidgets commit above and see for yourself.  My testing was done using Autoconf 2.71 from GNU Guix at commit 3fc26c6dc4b6bbf558a17b5e3ef8092ea0eeeacd.

I hope that my report is actionable!  Thank you for maintaining this important (and probably a bit hairy at places) tool.

Maxim Cournoyer <apteryx>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by egallager (Voted in favor of this item)
  • -email is unavailable- added by zackw (Posted a comment)
  • -email is unavailable- added by apteryx (Submitted the item)
  •  

    There are 4 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-12-08 zackw Priority2 - Eventually 1 - Blocked
    2023-12-08 zackw StatusNone Postponed
    2023-12-08 zackw Priority5 - Unprioritized 2 - Eventually
    2023-07-14 egallager Carbon-Copy- Added egallager
    2023-06-15 apteryx Attached File- Added 0005-fixups-for-autoupdate-bug.patch, #54840
        Attached File- Added 0006-new-rounds-of-changes-on-re-run.patch, #54841
    2023-06-15 apteryx Attached File- Added 0001-build-Properly-quote-else-clause-of-AC_TRY_COMPILE.patch, #54836
        Attached File- Added 0002-configure.in-Fix-more-quoting-problems.patch, #54837
        Attached File- Added 0003-autoupdate-valid-substitutions.patch, #54838
        Attached File- Added 0004-autoupdate-broken-substitutions.patch, #54839

    Back to the top

    Powered by Savane 3.13-c54d.
    Corresponding source code