changed include statements without prefix 'include/'

whb07 commented at 5:56 pm on April 25, 2021: contributor

Referencing #924 , this PR splits the two issues brought on to a smaller to digest change. What this does is removes the prefix “include/” when referencing the local library header files.

e.g: from:

0#include "include/secp256k1.h"

to:

0#include "secp256k1.h"

Rationale besides styling and consistency across other files in the repo, it makes it easier for outside builds to properly locate the headers.

A live example seen here when attempting to build this library within bitcoin repo:

0[ 14%] Building CXX object leveldb/CMakeFiles/leveldb.dir/util/bloom.cc.o
1/tmp/bitcoin/src/secp256k1/src/secp256k1.c:7:10: fatal error: include/secp256k1.h: No such file or directory
2    7 | #include "include/secp256k1.h"
3      |          ^~~~~~~~~~~~~~~~~~~~~
4compilation terminated.
5make[2]: *** [secp256k1/CMakeFiles/Secp256k1.dir/build.make:76: secp256k1/CMakeFiles/Secp256k1.dir/src/secp256k1.c.o] Error 1
6make[1]: *** [CMakeFiles/Makefile2:537: secp256k1/CMakeFiles/Secp256k1.dir/all] Error 2
7make[1]: *** Waiting for unfinished jobs....

gmaxwell commented at 7:20 pm on April 28, 2021: contributor

Alternatively, the internal usage could use correct relative paths, changing include/ to ../include/. This is a smaller diff, and doesn’t require any include settings in the build system to build the library:

E.g.

0$ gcc -Wall -Wextra -Wno-unused-function -O2  -c ./src/secp256k1.c -DSECP256K1_BUILD -D ECMULT_GEN_PREC_BITS=4 -D ECMULT_WINDOW_SIZE=15 && ls secp256k1.o
1secp256k1.o

I believe classically "" includes didn’t perform any search, so paths had to be explicit… though all modern compilers do search with it.

As far as that Cmake build goes– it looks broken. the top level directory is (currently) supposed to be in the includes path. I went to go find the cmake input to see if it was making more serious errors (e.g. failing to include SECP256K1_BUILD) but I couldn’t find it.

whb07 commented at 7:38 pm on April 28, 2021: contributor

Alternatively, the internal usage could use correct relative paths, changing include/ to ../include/. This is a smaller diff, and doesn’t require any include settings in the build system to build the library:

The deeper nested includes would end up needing more ../../../ as a prefix (don’t quote me on the preciseness of the example).

i agree there’s multiple ways to skin this cat, so let’s get something done?

i can make the changes be a relative path if you will it so.

real-or-random commented at 10:27 am on April 29, 2021: contributor

Alternatively, the internal usage could use correct relative paths, changing include/ to ../include/. This is a smaller diff, and doesn’t require any include settings in the build system to build the library:

E.g.
0$ gcc -Wall -Wextra -Wno-unused-function -O2  -c ./src/secp256k1.c -DSECP256K1_BUILD -D ECMULT_GEN_PREC_BITS=4 -D ECMULT_WINDOW_SIZE=15 && ls secp256k1.o
1secp256k1.o
I believe classically "" includes didn’t perform any search, so paths had to be explicit… though all modern compilers do search with it.

I noticed this include problem when looking into #923, and I agree we should aim for a solution that does not need -I when building without autotools. So I’d prefer to add .. to the #include. Unless this results in some other problem but I don’t see any.

whb07 commented at 2:08 pm on April 29, 2021: contributor

I noticed this include problem when looking into #923, and I agree we should aim for a solution that does not need -I when building without autotools. So I’d prefer to add .. to the #include. Unless this results in some other problem but I don’t see any.

I have just made the changes to be a relative path as mentioned. The changes to the Makefile.am file have been reverted to reflect the state of the master branch.

The changes do fix the cmake build i was working on so thats a plus.

real-or-random commented at 2:31 pm on April 29, 2021: contributor

The changes look fine. Let’s see if the CI build works. If yes, can you squash this into a single commit?

real-or-random cross-referenced this on Apr 29, 2021 from issue Persistent worker failed to start the task: Parallels isolation failed by real-or-random

whb07 commented at 3:26 pm on April 29, 2021: contributor

The changes look fine. Let’s see if the CI build works. If yes, can you squash this into a single commit?

Yep, I’ll squash once the build passes.

whb07 force-pushed on Apr 29, 2021

in Makefile.am:165 in 9d5ac8c9b4 outdated

161@@ -162,3 +162,4 @@ endif
162 if ENABLE_MODULE_SCHNORRSIG
163 include src/modules/schnorrsig/Makefile.am.include
164 endif
165+

gmaxwell commented at 7:59 pm on April 29, 2021:

This seems to add a blank line for no obvious reason. :)

whb07 commented at 8:16 pm on April 29, 2021:

Eagle eyes! Isn’t the standard to have a “\n” as the last line?

123 A source file that is not empty shall end in a new-line character, which shall not be immediately preceded by a backslash character before any such splicing takes place.

quick-reference

i got an editor and GitHub warning so I caved and added/left it post paste from master 🤷‍♂️.

let me know what you want to do

gmaxwell commented at 1:33 am on April 30, 2021:

The file already ended with a newline, this is adding an additional blank line.

whb07 commented at 1:58 am on April 30, 2021:

You’re right! Gross, GitHub is showing that there isn’t one! Fixing it right now

gmaxwell commented at 1:39 am on April 30, 2021: contributor

It should probably also change tests.c:

0#include "../contrib/lax_der_parsing.c"
1#include "../contrib/lax_der_privatekey_parsing.c"

Though this isn’t enough to compile tests.c with no -I lines to the compiler. The issue is that the modules in contrib correctly use <secp256k1.h> as its ’end user code’ – but then that doesn’t work as expected here. Still, I think moving the issue back to there is more correct, and at least fewer -I are needed.

whb07 force-pushed on Apr 30, 2021

whb07 commented at 2:17 am on April 30, 2021: contributor

It should probably also change tests.c:
0#include "../contrib/lax_der_parsing.c"
1#include "../contrib/lax_der_privatekey_parsing.c"
Though this isn’t enough to compile tests.c with no -I lines to the compiler. The issue is that the modules in contrib correctly use <secp256k1.h> as its ’end user code’ – but then that doesn’t work as expected here. Still, I think moving the issue back to there is more correct, and at least fewer -I are needed.

This is another good catch. Though the action isn’t clear to me. You’re saying we should change this to be a relative path as well ../contrib?

gmaxwell commented at 2:36 am on April 30, 2021: contributor

Yes, it should be changed to ../contrib.

real-or-random commented at 9:41 am on April 30, 2021: contributor

We could do something like this in the contrib files:

0/* #include secp256k1.h only when it hasn't been included yet.
1   This enables this file to be #included directly in other project
2   files (such as tests.c) without the need to set an explicit -I flag,
3   which would be necessary to locate secp256k1.h.  */
4#ifndef SECP256K1_H
5#include <secp256k1.h>
6#endif

whb07 force-pushed on Apr 30, 2021

whb07 commented at 1:59 pm on April 30, 2021: contributor

We could do something like this in the contrib files:

0/* #include secp256k1.h only when it hasn't been included yet.
1   This enables this file to be #included directly in other project
2   files (such as tests.c) without the need to set an explicit -I flag,
3   which would be necessary to locate secp256k1.h.  */
4#ifndef SECP256K1_H
5#include <secp256k1.h>
6#endif

Care to expand? I was able to properly build outside of the repo the executable tests.c and run it just fine post changes (including the “../contrib” ones).

real-or-random commented at 3:29 pm on April 30, 2021: contributor

We could do something like this in the contrib files:
0/* #include secp256k1.h only when it hasn't been included yet.
1   This enables this file to be #included directly in other project
2   files (such as tests.c) without the need to set an explicit -I flag,
3   which would be necessary to locate secp256k1.h.  */
4#ifndef SECP256K1_H
5#include <secp256k1.h>
6#endif
Care to expand? I was able to properly build outside of the repo the executable tests.c and run it just fine post changes (including the “../contrib” ones).

Maybe you have secp256k1.h installed in your system (in /usr/include)?

I think what should happen is that when trying to compile without -I, e.g.,

0gcc  -c ./src/tests.c -DSECP256K1_BUILD -D ECMULT_GEN_PREC_BITS=4 -D ECMULT_WINDOW_SIZE=15

then gcc, when it encouters #include <secp256k1.h>, it looks up secp256k1.h in the system paths and would error out if the file can’t be found.

See https://gcc.gnu.org/onlinedocs/cpp/Search-Path.html

gmaxwell commented at 11:59 pm on April 30, 2021: contributor

@real-or-random Seems fine to me, ever so slightly kludgy but argument free compiles are nice and the test harness is kind of inherently weird because its a test.

in src/modules/ecdh/main_impl.h:11 in 188637cf1a outdated

 6@@ -7,7 +7,7 @@
 7 #ifndef SECP256K1_MODULE_ECDH_MAIN_H
 8 #define SECP256K1_MODULE_ECDH_MAIN_H
 9 
10-#include "include/secp256k1_ecdh.h"
11+#include "../include/secp256k1_ecdh.h"
12 #include "ecmult_const_impl.h"

gmaxwell commented at 5:05 pm on May 1, 2021:

../../ecmult_const_impl.h

in src/modules/schnorrsig/main_impl.h:12 in 188637cf1a outdated

 6@@ -7,8 +7,8 @@
 7 #ifndef SECP256K1_MODULE_SCHNORRSIG_MAIN_H
 8 #define SECP256K1_MODULE_SCHNORRSIG_MAIN_H
 9 
10-#include "include/secp256k1.h"
11-#include "include/secp256k1_schnorrsig.h"
12+#include "../include/secp256k1.h"
13+#include "../include/secp256k1_schnorrsig.h"
14 #include "hash.h"

gmaxwell commented at 5:05 pm on May 1, 2021:

../../hash.h

in src/modules/ecdh/main_impl.h:10 in 188637cf1a outdated

 6@@ -7,7 +7,7 @@
 7 #ifndef SECP256K1_MODULE_ECDH_MAIN_H
 8 #define SECP256K1_MODULE_ECDH_MAIN_H
 9 
10-#include "include/secp256k1_ecdh.h"
11+#include "../include/secp256k1_ecdh.h"

gmaxwell commented at 5:20 pm on May 1, 2021:

../../../include/

in src/modules/extrakeys/main_impl.h:11 in 188637cf1a outdated

 6@@ -7,8 +7,8 @@
 7 #ifndef SECP256K1_MODULE_EXTRAKEYS_MAIN_H
 8 #define SECP256K1_MODULE_EXTRAKEYS_MAIN_H
 9 
10-#include "include/secp256k1.h"
11-#include "include/secp256k1_extrakeys.h"
12+#include "../include/secp256k1.h"
13+#include "../include/secp256k1_extrakeys.h"

gmaxwell commented at 5:20 pm on May 1, 2021:

../../../include/ for both

in src/modules/extrakeys/tests_exhaustive_impl.h:11 in 188637cf1a outdated

 7@@ -8,7 +8,7 @@
 8 #define SECP256K1_MODULE_EXTRAKEYS_TESTS_EXHAUSTIVE_H
 9 
10 #include "src/modules/extrakeys/main_impl.h"
11-#include "include/secp256k1_extrakeys.h"
12+#include "../include/secp256k1_extrakeys.h"

gmaxwell commented at 5:21 pm on May 1, 2021:

../../../include/

in src/modules/recovery/main_impl.h:10 in 188637cf1a outdated

 6@@ -7,7 +7,7 @@
 7 #ifndef SECP256K1_MODULE_RECOVERY_MAIN_H
 8 #define SECP256K1_MODULE_RECOVERY_MAIN_H
 9 
10-#include "include/secp256k1_recovery.h"
11+#include "../include/secp256k1_recovery.h"

gmaxwell commented at 5:21 pm on May 1, 2021:

../../../include/

in src/modules/recovery/tests_exhaustive_impl.h:11 in 188637cf1a outdated

 7@@ -8,7 +8,7 @@
 8 #define SECP256K1_MODULE_RECOVERY_EXHAUSTIVE_TESTS_H
 9 
10 #include "src/modules/recovery/main_impl.h"
11-#include "include/secp256k1_recovery.h"
12+#include "../include/secp256k1_recovery.h"

gmaxwell commented at 5:21 pm on May 1, 2021:

../../../include/

in src/modules/schnorrsig/main_impl.h:10 in 188637cf1a outdated

 6@@ -7,8 +7,8 @@
 7 #ifndef SECP256K1_MODULE_SCHNORRSIG_MAIN_H
 8 #define SECP256K1_MODULE_SCHNORRSIG_MAIN_H
 9 
10-#include "include/secp256k1.h"
11-#include "include/secp256k1_schnorrsig.h"
12+#include "../include/secp256k1.h"

gmaxwell commented at 5:21 pm on May 1, 2021:

../../../include/

in src/modules/schnorrsig/tests_exhaustive_impl.h:10 in 188637cf1a outdated

 6@@ -7,7 +7,7 @@
 7 #ifndef SECP256K1_MODULE_SCHNORRSIG_TESTS_EXHAUSTIVE_H
 8 #define SECP256K1_MODULE_SCHNORRSIG_TESTS_EXHAUSTIVE_H
 9 
10-#include "include/secp256k1_schnorrsig.h"
11+#include "../include/secp256k1_schnorrsig.h"

gmaxwell commented at 5:22 pm on May 1, 2021:

../../../include/

in src/modules/schnorrsig/tests_impl.h:10 in 188637cf1a outdated

 6@@ -7,7 +7,7 @@
 7 #ifndef SECP256K1_MODULE_SCHNORRSIG_TESTS_H
 8 #define SECP256K1_MODULE_SCHNORRSIG_TESTS_H
 9 
10-#include "secp256k1_schnorrsig.h"
11+#include "../include/secp256k1_schnorrsig.h"

gmaxwell commented at 5:22 pm on May 1, 2021:

../../../include/

gmaxwell commented at 5:24 pm on May 1, 2021: contributor

0$ gcc -o ./tests ./src/tests.c -DECMULT_GEN_PREC_BITS=4 -DECMULT_WINDOW_SIZE=15 -DENABLE_MODULE_SCHNORRSIG -DENABLE_MODULE_EXTRAKEYS -DENABLE_MODULE_RECOVERY -DENABLE_MODULE_ECDH

should work without errors. After applying my above review comments and the include work around mentioned by real-or-random (it’s needed in four places in contrib, everywhere <secp256k1.h> is used) it does for me.

real-or-random commented at 9:46 am on May 2, 2021: contributor

This needs rebase now that #928 has been merged.

@real-or-random Seems fine to me, ever so slightly kludgy but argument free compiles are nice and the test harness is kind of inherently weird because its a test.

That was also my thinking. Tt’s not exactly elegant but it does the job in the end…

real-or-random cross-referenced this on May 2, 2021 from issue Tracking issue for manual (non-autotools) builds by real-or-random

whb07 force-pushed on May 4, 2021

gmaxwell commented at 6:17 pm on May 4, 2021: contributor

Great!

This just needs code like

0/* #include secp256k1.h only when it hasn't been included yet.
1   This enables this file to be #included directly in other project
2   files (such as tests.c) without the need to set an explicit -I flag,
3   which would be necessary to locate secp256k1.h.  */
4#ifndef SECP256K1_H
5#include <secp256k1.h>
6#endif

around the uses of #include <secp256k1.h> in contrib, and then

0gcc -o ./tests ./src/tests.c -DECMULT_GEN_PREC_BITS=4 -DECMULT_WINDOW_SIZE=15 -DENABLE_MODULE_SCHNORRSIG -DENABLE_MODULE_EXTRAKEYS -DENABLE_MODULE_RECOVERY -DENABLE_MODULE_ECDH

will work.

real-or-random commented at 7:14 pm on May 4, 2021: contributor

Argh, this needs rebase again (and CI failures just appeared because #936 was merged during CI was running…). Sorry that this needs so many ping pongs.

whb07 force-pushed on May 4, 2021

whb07 commented at 1:44 am on May 5, 2021: contributor

0#ifndef SECP256K1_H
1#include <secp256k1.h>
2#endif

This works, I’ll tackle it tomorrow morning assuming this is the way to move forward. Sometimes i don’t understand how the search paths are distinguished for these compilers, seems I don’t read English too good when looking at their docs!

gmaxwell commented at 2:55 am on May 5, 2021: contributor

ACK on the update so far, looking forward to the rest. I’ll review as soon as it’s up. Thanks!

change local lib headers to be relative for those pointing at "include/" dir

added relative paths to header files imported from src directory

added include guards for contrib/ files when referring to secp256k1.h

3c90bdda95

whb07 force-pushed on May 5, 2021

whb07 commented at 1:31 pm on May 5, 2021: contributor

This should be good to go now. For better context:

In contrib/lax_der_parsing.c and contrib/lax_der_privatekey_parsing.c the include statement for secp256k1.h has been removed and the header guard protected include for respective header files imports secp256k1.h instead.

This cleans up the unnecessary wordy include duplication and satisfies the definitions by being included in the header which is required for the source file to run.

As mentioned previously:

0$ gcc -o ./tests ./src/tests.c -DECMULT_GEN_PREC_BITS=4 -DECMULT_WINDOW_SIZE=15 -DENABLE_MODULE_SCHNORRSIG -DENABLE_MODULE_EXTRAKEYS -DENABLE_MODULE_RECOVERY -DENABLE_MODULE_ECDH

&&

0$ clang -o ./tests ./src/tests.c -DECMULT_GEN_PREC_BITS=4 -DECMULT_WINDOW_SIZE=15 -DENABLE_MODULE_SCHNORRSIG -DENABLE_MODULE_EXTRAKEYS -DENABLE_MODULE_RECOVERY -DENABLE_MODULE_ECDH

Run just fine now. I even ran the “include what you use” tooling and it gives good-to-go.

whb07 commented at 4:22 pm on May 5, 2021: contributor

The test suite needs to be run again, seems like theres an underlying issue there.

real-or-random commented at 4:25 pm on May 5, 2021: contributor

The test suite needs to be run again, seems like theres an underlying issue there.

his failure is really spurious. If you look at the logs, all the tests pass, just Cirrus is confused about about result. (I reported it at https://github.com/cirruslabs/cirrus-cli/issues/373#issuecomment-832829630 .) Let me restart that ask, but let’s not get that in our way.

real-or-random approved

real-or-random commented at 4:29 pm on May 5, 2021: contributor

ACK 3c90bdda95aa4e79ff33bfbbbe91872417650ae9 code looks good and even the tests compile fine now without -I args

gmaxwell commented at 6:08 pm on May 5, 2021: contributor

ACK 3c90bdda95aa4e79ff33bfbbbe91872417650ae9

real-or-random merged this on May 5, 2021

real-or-random closed this on May 5, 2021

sipa commented at 6:33 pm on May 5, 2021: contributor

Posthumous utACK 3c90bdda95aa4e79ff33bfbbbe91872417650ae9

real-or-random referenced this in commit 22a9ea154a on May 6, 2021

real-or-random cross-referenced this on May 6, 2021 from issue contrib: Explain explicit header guards by real-or-random

sipa referenced this in commit 1e78c18d5b on May 12, 2021

real-or-random cross-referenced this on May 18, 2021 from issue Refactor includes by whb07

jonasnick cross-referenced this on Jun 14, 2021 from issue Upstream PRs 831, 907, 903, 889, 918, 906, 928, 922, 933, Merge bitcoin-core/secp256k1#936: Fix gen_context/ASM build on ARM, 925, 937, 926, Merge bitcoin-core/secp256k1#940: contrib: Explain explicit header guards, 850, 930, 941, 846, 947, 662, 950 by jonasnick

real-or-random cross-referenced this on Jul 6, 2021 from issue Replace ecmult_context with a generated static array. by roconnor-blockstream

real-or-random referenced this in commit d63719fb10 on Jun 30, 2022

real-or-random cross-referenced this on Jun 30, 2022 from issue build: Fix #include "..." paths to get rid of further -I arguments by real-or-random

real-or-random referenced this in commit ac39773ba9 on Jun 30, 2022

real-or-random referenced this in commit 40a3473a9d on Jul 1, 2022

real-or-random referenced this in commit af65d30cc8 on Jul 1, 2022

real-or-random cross-referenced this on Dec 7, 2022 from issue Fix backport of bitcoin-core/secp256k1#925 (drop 'include/' prefix from -I) by mattiaferrari02

dderjoel referenced this in commit 7d548f23cd on May 23, 2023

dderjoel referenced this in commit 16062f8875 on May 23, 2023

changed include statements without prefix ‘include/’ #925