Autoconf improvements #862
Conversation
real-or-random
changed the title
real-or-random
force-pushed
the
202012-valgrind-brew-include
branch
from
5cac98b
to
517c44c
Compare
|
Installing Valgrind on MacOS Big Sur to test this seems problematic https://stackoverflow.com/questions/65009780/macos-valgrind-alternative |
|
@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. |
|
Required a reinstall of the MacOS CommandLineTools to install that LouisBrunner version of Valgrind. But this PR seems to work fine. Before this PR |
Yep, this was necessary also here previously, see the commented lines in |
|
@DvalCaruso Stop spamming this repository with meaningless operations. You will get banned if you continue. |
|
Is it possible to split the 2nd commit into ones that move stuff around, add comments, and provide the no-crosscompile special casing? |
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).
No behavioral changes.
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 (bitcoin-core#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.
real-or-random
force-pushed
the
202012-valgrind-brew-include
branch
from
517c44c
to
3c15130
Compare
|
@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. |
|
utACK 3c15130 |
|
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 |
Summary: ``` 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 (#621 (comment)) 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... ``` Backport of [[bitcoin-core/secp256k1#862 | secp256k1#862]] Test Plan: ./autogen.sh ./configure make I didn't test valgrind from brew on OSX, I have no old enough OSX that support valgrind. Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D9382
Summary: ``` 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 (#621 (comment)) 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... ``` Backport of [[bitcoin-core/secp256k1#862 | secp256k1#862]] Test Plan: ./autogen.sh ./configure make I didn't test valgrind from brew on OSX, I have no old enough OSX that support valgrind. Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D9382
1ecb94e build: Make `SECP_VALGRIND_CHECK` preserve `CPPFLAGS` (Hennadii Stepanov) Pull request description: It was overlooked in #862 and #1027. ACKs for top commit: real-or-random: utACK 1ecb94e Tree-SHA512: 263fc600ce9743e4aad767150f706bf7d4325dabb9c363ed57f08fe38faea94d7d1999804947cffeacbe698bb6d959ee6de3f6e50400050a390ecc0db957e426
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 (#621 (comment))
When working on the autoconf stuff, I noticed two things that I just want to write down here:
AC_PROG_CC_C89. It's not needed anymore becauseAC_PROG_CCcares 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...