Skip to content

Conversation

@laanwj
Copy link
Member

@laanwj laanwj commented Feb 2, 2022

According to autoconf 2.70 documentation, the AC_PROG_CC_C89 is replaced by AC_PROG_CC, which defines the same variable ac_cv_prog_cc_c89 under the same conditions.

Avoids the following message:

configure.ac:23: warning: The macro `AC_PROG_CC_C89' is obsolete.

I'm not sure when the behavior was introduced, but it goes back to at least 2.60.

@real-or-random
Copy link
Contributor

real-or-random commented Feb 2, 2022

Last time I overhauled the autoconf stuff, I assumed that removing AC_PROG_CC_C89 would raise our minimum required version to 2.70: f2d9aea

I think we would need to have a closer look and check if 2.70 only deprecated AC_PROG_CC_C89, or if it changed AC_PROG_CC to check for versions and therefore deprecated AC_PROG_CC_C89. I think the latter was my understanding back then. If that's true, we could also do a version check if we want to get rid of the warning.

edit: Or is the only thing AC_PROG_CC_C89 does is exclude pre-ANSI compilers? Then we may as well just drop it...

@real-or-random
Copy link
Contributor

real-or-random commented Feb 2, 2022

Hm so this is from the commit that made removed the macro: autotools-mirror/autoconf@27eb3ae#diff-0c067bec9d061f1ebfce4c4d145fece50221a1d23e9adb62a4b6b2b11c8de1fcL7340-L7342

This macro is rarely needed.  It should be used only if your application
requires C89 and will not work in later C versions.  Typical applications
should use @code{AC_PROG_CC} instead.

(The commit is from 2012 but AFAIU still this was only included in 2.70 as there was no release for a few years. (The tag listing on the commit weird there. "v2.69e v2.69d v2.69c v2.69b" are beta versions and I think "v2.62a" is just a mistake...)

So now I understand: We currently call this to force C89 mode. It will add -ansi. I think one reason why we do this is to make sure our code is pure C89. But we would of course consider it a bug if we need C89 in the sense that C99 or C11 would break our code. But it may still be conservative to always add -ansi to make sure what we're testing is closer to what users will run. Unfortunately, autoconf does not seem to support this use case (or made it obsolete at least)

Having said this, I'm not entirely sure what we should do:

  • comply with autoconf's thinking and drop AC_PROG_CC89 and thereby -ansi (and add separate -ansi compilation test to CI)
  • drop AC_PROG_CC89 and add -ansi manually
  • leave it as it is and accept the warning (until autoconf decides the remove the macro in another 20 years)

@laanwj
Copy link
Member Author

laanwj commented Feb 2, 2022

think we would need to have a closer look and check if 2.70 only deprecated AC_PROG_CC_C89, or if it changed AC_PROG_CC to check for versions and therefore deprecated AC_PROG_CC_C89.

To be clear, I checked the 2.60 (!) documentation and the behavior of AC_PROG_CC automatically calling AC_PROG_CC_C89 was already there. I don't know about adding -ansi, but version compatibility is no reason to not do this.

https://www.gnu.org/software/autoconf/manual/autoconf-2.60/html_node/C-Compiler.html#AC_005fPROG_005fCC

— Macro: AC_PROG_CC_C89

This macro is called automatically by AC_PROG_CC.

@laanwj
Copy link
Member Author

laanwj commented Feb 2, 2022

I checked the make V=1 output with and without this change. At least with autoconf 2.70 (I did not check with any other version), it's exactly the same.

There's no -ansi in either case but -std=c89 is passed, which should be sufficient.

libtool: compile:  gcc -DHAVE_CONFIG_H -I. -I./src -I./include -I./src -O2 -std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef -Wno-overlength-strings -Wall -Wno-unused-function -Wextra -Wcast-align -Wcast-align=strict -fvisibility=hidden -g -O2 -MT src/libsecp256k1_la-secp256k1.lo -MD -MP -MF src/.deps/libsecp256k1_la-secp256k1.Tpo -c src/secp256k1.c  -fPIC -DPIC -o src/.libs/libsecp256k1_la-secp256k1.o

It's already being added manually:

SECP_TRY_APPEND_CFLAGS([-std=c89 -pedantic -Wno-long-long -Wnested-externs -Wshadow -Wstrict-prototypes -Wundef], $1)

@real-or-random
Copy link
Contributor

To be clear, I checked the 2.60 (!) documentation and the behavior of AC_PROG_CC automatically calling AC_PROG_CC_C89 was already there. I don't know about adding -ansi, but version compatibility is no reason to not do this.

Indeed, you're right. The commit I mentioned was just making it obsolete...

It's already being added manually:

You're right again.

Ok, I'm convinced.

AM_PROG_CC_C_O
AC_PROG_CC_C89
AC_PROG_CC
if test x"$ac_cv_prog_cc_c89" = x"no"; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Fun that ac_cv_prog_cc_c89 is still present in configure, so it's correct to keep that check)

According to [autoconf 2.70](https://www.gnu.org/software/autoconf/manual/autoconf-2.70/html_node/Obsolete-Macros.html)
documentation, the `AC_PROG_CC_C89' is replaced by `AC_PROG_CC`, which
defines the same variable `ac_cv_prog_cc_c89`.

Avoids the following message:
```
configure.ac:23: warning: The macro `AC_PROG_CC_C89' is obsolete.
```

Also, remove deprecated `AM_PROG_CC_C_O`.
@laanwj laanwj force-pushed the 2022-02-autoconf-deprecated branch from 31fbb79 to e0db3f8 Compare February 3, 2022 08:01
@real-or-random
Copy link
Contributor

utACK e0db3f8

Copy link
Contributor

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e0db3f8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants