Autoconf improvements

real-or-random commented at 9:22 pm on December 23, 2020: contributor

See individual commit messages. These are improvements in preparation of the switch to Cirrus CI. (Maybe I’ll just open a PR on top of this one.)

The first commit made the difference between successful build https://cirrus-ci.com/task/6740575057608704 and unsuccessful build https://cirrus-ci.com/task/4909571074424832.

I’ve tested the second commit without cross-compilation and with cross-compilation for android (https://github.com/bitcoin-core/secp256k1/issues/621#issuecomment-495703399)

When working on the autoconf stuff, I noticed two things that I just want to write down here:

At some point we should update build-aux/m4/ax_prog_cc_for_build.m4. This is outdated, and there have been a lot of fixes But the latest version is broken, so now is probably not the time.
The latest autoconf 2.70 deprecates AC_PROG_CC_C89. It’s not needed anymore because AC_PROG_CC cares about testing for version support. This makes autoconf 2.70 output a warning that we should probably just ignore. We don’t want to force users onto 2.70…

real-or-random renamed this:
~~Ask brew for valgrind include path~~
Autoconf improvements
on Jan 2, 2021

real-or-random force-pushed on Jan 2, 2021

michaelfolkson commented at 3:00 pm on January 2, 2021: member

Installing Valgrind on MacOS Big Sur to test this seems problematic https://stackoverflow.com/questions/65009780/macos-valgrind-alternative

real-or-random commented at 3:05 pm on January 2, 2021: contributor

@michaelfolkson Indeed yeah, I’m using https://github.com/LouisBrunner/valgrind-macos on Catalina on my branch. https://github.com/real-or-random/secp256k1/blob/202012-cirrusci/.cirrus.yml#L113

This seems to do the job. I hope it’s stable enough that we won’t have to fix this once per month.

michaelfolkson commented at 3:46 pm on January 2, 2021: member

Required a reinstall of the MacOS CommandLineTools to install that LouisBrunner version of Valgrind. But this PR seems to work fine. Before this PR CPPFLAGS was empty on configuring and after this PR CPPFLAGS contains -I/usr/local/opt/valgrind/include

real-or-random commented at 4:10 pm on January 2, 2021: contributor

Required a reinstall of the MacOS CommandLineTools to install that LouisBrunner version of Valgrind.

Yep, this was necessary also here previously, see the commented lines in .cirrus.yml. But the Cirrus CI images have been upgraded now: https://github.com/cirruslabs/osx-images/issues/30

real-or-random cross-referenced this on Jan 7, 2021 from issue Add support for Cirrus CI by real-or-random

sipa commented at 7:25 pm on January 7, 2021: contributor

@DvalCaruso Stop spamming this repository with meaningless operations. You will get banned if you continue.

sipa commented at 8:34 pm on January 7, 2021: contributor

Is it possible to split the 2nd commit into ones that move stuff around, add comments, and provide the no-crosscompile special casing?

Ask brew for valgrind include path

Valgrind is typically installed using brew on macOS. This commit
makes ./configure detect this case set the appropriate include
directory (in the same way as we already do for openssl and gmp).

252c19dfc6

Restructure and tidy configure.ac

No behavioral changes.

47802a4762

Improve CC_FOR_BUILD detection

This commits simply uses CC as CC_FOR_BUILD and the same for
corresponding flags if we're not cross-compiling. This has a number of
benefits in this common case:
 - It avoids strange cases where very old compilers are used (#768).
 - Flags are consistently set for CC and CC_FOR_BUILD.
 - ./configure is faster.
 - You get compiler x consistently if you set CC=x; we got this wrong
   in CI in the past.

./configure warns if a _FOR_BUILD variable is set but ignored because
we're not cross-compiling.

The change exposed that //-style comments are used in gen_context.c,
which is also fixed by this commit.

This commit also reorganizes code in configure.ac to have a cleaner
separation of sections.

3c15130709

in configure.ac:278 in 517c44c271 outdated

273+    # If we're not cross-compiling, simply use the same compiler for building the static precompation code.
274+    CC_FOR_BUILD="$CC"
275+    CFLAGS_FOR_BUILD="$CFLAGS"
276+    CPPFLAGS_FOR_BUILD="$CPPFLAGS"
277+    LDFLAGS_FOR_BUILD="$LDFLAGS"
278+  else

real-or-random commented at 8:37 pm on January 7, 2021:

@sipa I think this is the part that provides the no-crosscompile casing. But yes, I can split it if you want. I was somewhat lazy because I was hacking on the code at various places and then just had a bunch of changes…

sipa commented at 8:40 pm on January 7, 2021:

@real-or-random Yes, sorry, I realize that that is what this part does. The code looks good, but it’s easier to ascertain it doesn’t introduce other changes if the move-only part and the rest are in separate commits.

real-or-random force-pushed on Jan 8, 2021

real-or-random commented at 3:14 pm on January 8, 2021: contributor

@sipa Ok, I split a commit. It’s now easier to review this commit by commit. The second commit should introduce no behavioral changes.When viewing the (now) third commit, it’s a good idea to hide whitespace changes because otherwise the diff will again be large due to changes in indentation.

sipa commented at 9:02 pm on January 8, 2021: contributor

utACK 3c15130709da26a6d2f25a483aa45e14bf1e4feb

jonasnick commented at 3:26 pm on January 11, 2021: contributor

utACK 3c15130 makes sense (with my very basic understanding of autoconf)

real-or-random commented at 4:08 pm on January 11, 2021: contributor

I think two utACKs are okay for this to be merged but let me rebase #864 on top of this, and then we can at least see this once being run on and tested on Cirrus CI. Then we can for example see if the brew commit still works, which was introduced to make the Cirrus PR work.

real-or-random commented at 2:32 pm on January 12, 2021: contributor

I think two utACKs are okay for this to be merged but let me rebase #864 on top of this, and then we can at least see this once being run on and tested on Cirrus CI. Then we can for example see if the brew commit still works, which was introduced to make the Cirrus PR work.

Ok, this work on CI: https://cirrus-ci.com/build/4517681179131904, merging then

real-or-random merged this on Jan 12, 2021

real-or-random closed this on Jan 12, 2021

real-or-random cross-referenced this on Jan 12, 2021 from issue doc: set CC_FOR_BUILD when building on OpenBSD by fanquake

sipa cross-referenced this on Jan 13, 2021 from issue Safegcd inverses, drop Jacobi symbols, remove libgmp by sipa

real-or-random cross-referenced this on Feb 17, 2021 from issue build: configure script warnings on macOS by hebasto

Fabcien referenced this in commit 2098e555bf on Apr 8, 2021

deadalnix referenced this in commit 17d00b2b8b on Apr 9, 2021

hebasto cross-referenced this on Apr 11, 2023 from issue build: Make `SECP_VALGRIND_CHECK` preserve `CPPFLAGS` by hebasto

real-or-random referenced this in commit 2e035af251 on Apr 20, 2023

Autoconf improvements #862