This simplifies building without a build system.
This is in line with #925; the paths fixed here were either forgotten there or only introduced later. This commit also makes the Makefile stricter so that further “wrong” #include paths will lead to build errors even in autotools builds.
This belongs to #929.
341@@ -342,15 +342,15 @@ void test_exhaustive_sign(const secp256k1_context *ctx, const secp256k1_ge *grou
342 }
343
344 #ifdef ENABLE_MODULE_RECOVERY
345-#include "src/modules/recovery/tests_exhaustive_impl.h"
346+#include "modules/recovery/tests_exhaustive_impl.h"
Should this diff be added as well
0--- a/src/modules/recovery/tests_exhaustive_impl.h
1+++ b/src/modules/recovery/tests_exhaustive_impl.h
2@@ -7,7 +7,7 @@
3 #ifndef SECP256K1_MODULE_RECOVERY_EXHAUSTIVE_TESTS_H
4 #define SECP256K1_MODULE_RECOVERY_EXHAUSTIVE_TESTS_H
5
6-#include "src/modules/recovery/main_impl.h"
7+#include "main_impl.h"
8 #include "../../../include/secp256k1_recovery.h"
9
10 void test_exhaustive_recovery_sign(const secp256k1_context *ctx, const secp256k1_ge *group) {
?
#include <...> style exclusively, even for the project’s own files, because they’re never relative w.r.t. the source file path. Is that something which would be useful for us too?
In order to simplify building without build system (#929), we tried to get rid of as many mandatory compiler args as possible. #925 tried to get rid of -I but missed a few cases. This is fixed by this PR.
Changing everything to <> would be a switch of paradigms. I think in the end there’s no definitive answer (see https://stackoverflow.com/a/72065059 for a nice discussion of pros and cons) but I think simple manual builds is a useful goal for us (e.g. for embedded users), so I think we should stick to our ""-only convention unless we have a good reason to change to <>.
Should this diff be added as well?
Done, thanks.
but I think simple manual builds is a useful goal for us (e.g. for embedded users), so I think we should stick to our
""-only convention unless we have a good reason to change to<>.
Good point; minimizing complexity for people building without build system support is a good reason to stick with “”.
but I think simple manual builds is a useful goal for us (e.g. for embedded users), so I think we should stick to our
""-only convention unless we have a good reason to change to<>.Good point; minimizing complexity for people building without build system support is a good reason to stick with “”.
Wanna ACK then?
Removing of -I$(top_srcdir)/include here made me believe that include_directories(${PROJECT_SOURCE_DIR}/include) can be dropped in #1113 as well.
But that is not the case. Two headers cannot be found (when combining this PR and #1113): https://github.com/bitcoin-core/secp256k1/blob/43756da819a235d813e7ecd53eae6df073b53247/src/modules/ecdh/bench_impl.h#L10 and https://github.com/bitcoin-core/secp256k1/blob/43756da819a235d813e7ecd53eae6df073b53247/src/modules/recovery/bench_impl.h#L10
Wondering why those headers are found using Autotools?
FWIW, all other #includes in the modules directory use correct paths:
0$ git grep include/secp256k1 -- src/modules/*.h
1src/modules/ecdh/bench_impl.h:#include "../include/secp256k1_ecdh.h"
2src/modules/ecdh/main_impl.h:#include "../../../include/secp256k1_ecdh.h"
3src/modules/extrakeys/main_impl.h:#include "../../../include/secp256k1.h"
4src/modules/extrakeys/main_impl.h:#include "../../../include/secp256k1_extrakeys.h"
5src/modules/extrakeys/tests_exhaustive_impl.h:#include "../../../include/secp256k1_extrakeys.h"
6src/modules/extrakeys/tests_impl.h:#include "../../../include/secp256k1_extrakeys.h"
7src/modules/recovery/bench_impl.h:#include "../include/secp256k1_recovery.h"
8src/modules/recovery/main_impl.h:#include "../../../include/secp256k1_recovery.h"
9src/modules/recovery/tests_exhaustive_impl.h:#include "../../../include/secp256k1_recovery.h"
10src/modules/schnorrsig/bench_impl.h:#include "../../../include/secp256k1_schnorrsig.h"
11src/modules/schnorrsig/main_impl.h:#include "../../../include/secp256k1.h"
12src/modules/schnorrsig/main_impl.h:#include "../../../include/secp256k1_schnorrsig.h"
13src/modules/schnorrsig/tests_exhaustive_impl.h:#include "../../../include/secp256k1_schnorrsig.h"
14src/modules/schnorrsig/tests_impl.h:#include "../../../include/secp256k1_schnorrsig.h"
-I. -I./src in our case, see https://stackoverflow.com/questions/5641480/automake-overwriting-default-includes… The suggestion of overriding it doesn’t work for me. I’ll see if there’s another way of getting rid of that.
That’s an “interesting” observation. Apparently autotools always add
-I. -I./srcin our case, see https://stackoverflow.com/questions/5641480/automake-overwriting-default-includes… The suggestion of overriding it doesn’t work for me. I’ll see if there’s another way of getting rid of that.
Another reason to rid of the config header.
This simplifies building without a build system.
This is in line with #925; the paths fixed here were either forgotten
there or only introduced later. This commit also makes the Makefile
stricter so that further "wrong" #include paths will lead to build
errors even in autotools builds.
This belongs to #929.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
@hebasto Okay I added these additional fixes, I hope that was everything now.
The issue is that we can’t really enforce that policy using autotools. One can add nostdinc to the Automake options, and this indeed removes the -I. -I./src but this breaks out-of-tree ( yes, this is supported by autotools, called “VPATH” builds) and more impoatantly make distcheck, which relies on VPATH builds… The reasons those breaks build is the config header.
Another reason to rid of the config header.
Yep. If we do this, then we could enforce the policy… We could also just add a “manual” build to CI. This is already on the task list in #929. But this can be done in a separate PR.
Funnily, the build worked for me locally when setting stdinc but without fixing secp256k1/src/modules/(ecdh|recovery)/bench_impl.h. This confused me a lot… Guess why? My system installation libsecp256k1: :worried: (-H is really useful flag…)
0. src/modules/ecdh/bench_impl.h
1.. /usr/include/secp256k1_ecdh.h
2... /usr/include/secp256k1.h
3. src/modules/recovery/bench_impl.h
4.. /usr/include/secp256k1_recovery.h
5. src/modules/schnorrsig/bench_impl.h
6.. src/modules/schnorrsig/../../../include/secp256k1_schnorrsig.h
7... src/modules/schnorrsig/../../../include/secp256k1.h
8... src/modules/schnorrsig/../../../include/secp256k1_extrakeys.h
0. src/modules/ecdh/bench_impl.h
1.. src/modules/ecdh/../../../include/secp256k1_ecdh.h
2... src/modules/ecdh/../../../include/secp256k1.h
3. src/modules/recovery/bench_impl.h
4.. src/modules/recovery/../../../include/secp256k1_recovery.h
5... src/modules/recovery/../../../include/secp256k1.h
6. src/modules/schnorrsig/bench_impl.h
7.. src/modules/schnorrsig/../../../include/secp256k1_schnorrsig.h
8... src/modules/schnorrsig/../../../include/secp256k1.h
9... src/modules/schnorrsig/../../../include/secp256k1_extrakeys.h