build: fix OpenSSL EC detection on macOS #735

1. 
   fanquake commented at 9:41 am on April 9, 2020: member

   Fixes #734.

   The first commit fixes OpenSSL EC detection on macOS, by making CRYPTO_CPPFLAGS available during configure. Similar to how GMP_CPPFLAGS are used.

   The second commit, which I’d like feedback on, fixes a compilation error:

   0  CC       src/bench_verify.o
   1src/bench_verify.c:15:10: fatal error: 'openssl/bn.h' file not found
   2#include <openssl/bn.h>
   3         ^~~~~~~~~~~~~~
   41 error generated.
   

   as CRYPTO_CPPFLAGS are then needed in bench_verify, and are not included otherwise.

   Not sure if this is the way you’d like to fix this, as its a bit of a hack for macOS.

2. build: fix OpenSSL EC detection on macOS 84b5fc5bc3
3. build: add SECP_TEST_INCLUDES to bench_verify CPPFLAGS

   This is needed so that bench_verify gets CRYPTO_CPPFLAGS, which would
   otherwise not be included, at least on macOS.

   3b7d26b23c
4. 
   real-or-random commented at 11:09 am on April 9, 2020: contributor

   Thanks! I’m fine with the code changes but I can’t test on macOS. The second commit seems correct and I don’t see why it should be a hack.
5. real-or-random cross-referenced this on Apr 9, 2020 from issue Remove OpenSSL benchmarks? by real-or-random
6. 
   gwillen commented at 9:16 pm on April 12, 2020: none

   I have a mac (10.14.6) and am willing to test this – can someone give me a fairly precise description of a negative test and a positive test for me to try?
7. 
   fanquake commented at 0:17 am on April 13, 2020: member

   @gwillen

   Master 4f27e344c69c33b4f3f448baa0196b9892287081:

   0./configure --enable-openssl-tests
   1...
   2checking for libcrypto... yes
   3checking for main in -lcrypto... yes
   4checking for EC functions in libcrypto... no
   5configure: error: OpenSSL tests requested but OpenSSL with EC support is not available
   

   This PR @ https://github.com/bitcoin-core/secp256k1/pull/735/commits/84b5fc5bc34cfb6e9df4f06b3b6fc98c3373dd0e. Configure works, compiling doesn’t:

   0./configure --enable-openssl-tests
   1# make files
   2  CC       src/bench_verify.o
   3src/bench_verify.c:15:10: fatal error: 'openssl/bn.h' file not found
   4#include <openssl/bn.h>
   5         ^~~~~~~~~~~~~~
   61 error generated.
   

   This PR @ https://github.com/bitcoin-core/secp256k1/pull/735/commits/3b7d26b23c9811deee244ede7bb344bb43544153. Everything works as expected.

8. 
   gwillen commented at 0:25 am on April 13, 2020: none

   Is this all meant to be done with brew’s openssl linked? On my system, brew says:

   openssl@1.1 is keg-only, which means it was not symlinked into /usr/local, because openssl/libressl is provided by macOS so don't link an incompatible version.

   And I get the configure error regardless of which version I check out.

9. 
   fanquake commented at 0:40 am on April 13, 2020: member

   Is this all meant to be done with brew’s openssl linked?

   No

   I get the configure error regardless of which version I check out.

   Did you run autogen.sh before running configure after checking out the changes?

10. 
    gwillen commented at 0:49 am on April 13, 2020: none

    Aha, nope I did not. That causes the behavior to be exactly as predicted! Configure works at 84b5fc5, build works at 3b7d26b. Let me know if you want me to check anything else.
11. 
    real-or-random commented at 9:03 am on April 13, 2020: contributor

    ACK 3b7d26b23c9811deee244ede7bb344bb43544153 the diff looks good to me

    I think it’s okay to merge this before now even though #738 is there. If we think that we can quickly merge #738, we can also do this instead.

12. jonasnick approved
13. 
    jonasnick commented at 7:50 pm on April 13, 2020: contributor

    ACK 3b7d26b23c9811deee244ede7bb344bb43544153
14. jonasnick merged this on Apr 13, 2020
15. jonasnick closed this on Apr 13, 2020

    ---

16. sipa cross-referenced this on Jun 9, 2020 from issue Update libsecp256k1 subtree by sipa
17. fanquake referenced this in commit 8c97780db8 on Jun 13, 2020
18. sidhujag referenced this in commit 8a3a072968 on Jun 13, 2020
19. ComputerCraftr referenced this in commit b98f1c6e6c on Jun 16, 2020
20. fanquake deleted the branch on Jul 22, 2020
21. jasonbcox referenced this in commit 40f898ad13 on Sep 27, 2020
22. deadalnix referenced this in commit eaabf25149 on Sep 28, 2020
23. UdjinM6 referenced this in commit 9d36ba6570 on Aug 10, 2021
24. 5tefan referenced this in commit 8ded2caa74 on Aug 12, 2021
25. gades referenced this in commit d855cc511d on May 8, 2022

Contributors
fanquake real-or-random gwillen jonasnick