build: Fix #include "..." paths to get rid of further -I arguments #1116

real-or-random · 2022-06-30T15:26:10Z

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.

hebasto

Concept ACK.

src/tests_exhaustive.c

hebasto · 2022-06-30T16:52:45Z

Should this diff be added as well

--- a/src/modules/recovery/tests_exhaustive_impl.h
+++ b/src/modules/recovery/tests_exhaustive_impl.h
@@ -7,7 +7,7 @@
 #ifndef SECP256K1_MODULE_RECOVERY_EXHAUSTIVE_TESTS_H
 #define SECP256K1_MODULE_RECOVERY_EXHAUSTIVE_TESTS_H
 
-#include "src/modules/recovery/main_impl.h"
+#include "main_impl.h"
 #include "../../../include/secp256k1_recovery.h"
 
 void test_exhaustive_recovery_sign(const secp256k1_context *ctx, const secp256k1_ge *group) {

?

sipa · 2022-06-30T17:59:10Z

@real-or-random Bitcoin Core uses #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?

real-or-random · 2022-06-30T18:25:46Z

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 <>.

real-or-random · 2022-06-30T18:30:12Z

Should this diff be added as well?

Done, thanks.

sipa · 2022-06-30T18:31:47Z

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 "".

real-or-random · 2022-06-30T20:35:37Z

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?

sipa · 2022-06-30T21:10:54Z

ACK ac39773

hebasto · 2022-07-01T09:29:15Z

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):

secp256k1/src/modules/ecdh/bench_impl.h

Line 10 in 43756da

#include "../include/secp256k1_ecdh.h"

and

secp256k1/src/modules/recovery/bench_impl.h

Line 10 in 43756da

#include "../include/secp256k1_recovery.h"

Wondering why those headers are found using Autotools?

FWIW, all other #includes in the modules directory use correct paths:

$ git grep include/secp256k1 -- src/modules/*.h
src/modules/ecdh/bench_impl.h:#include "../include/secp256k1_ecdh.h"
src/modules/ecdh/main_impl.h:#include "../../../include/secp256k1_ecdh.h"
src/modules/extrakeys/main_impl.h:#include "../../../include/secp256k1.h"
src/modules/extrakeys/main_impl.h:#include "../../../include/secp256k1_extrakeys.h"
src/modules/extrakeys/tests_exhaustive_impl.h:#include "../../../include/secp256k1_extrakeys.h"
src/modules/extrakeys/tests_impl.h:#include "../../../include/secp256k1_extrakeys.h"
src/modules/recovery/bench_impl.h:#include "../include/secp256k1_recovery.h"
src/modules/recovery/main_impl.h:#include "../../../include/secp256k1_recovery.h"
src/modules/recovery/tests_exhaustive_impl.h:#include "../../../include/secp256k1_recovery.h"
src/modules/schnorrsig/bench_impl.h:#include "../../../include/secp256k1_schnorrsig.h"
src/modules/schnorrsig/main_impl.h:#include "../../../include/secp256k1.h"
src/modules/schnorrsig/main_impl.h:#include "../../../include/secp256k1_schnorrsig.h"
src/modules/schnorrsig/tests_exhaustive_impl.h:#include "../../../include/secp256k1_schnorrsig.h"
src/modules/schnorrsig/tests_impl.h:#include "../../../include/secp256k1_schnorrsig.h"

real-or-random · 2022-07-01T10:30:39Z

@hebasto That's an "interesting" observation. Apparently autotools always add -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.

hebasto · 2022-07-01T10:39:34Z

@real-or-random

As the goal of this PR is to "Fix #include "..." paths", could those two paths be fixed as well?

hebasto · 2022-07-01T10:41:39Z

That's an "interesting" observation. Apparently autotools always add -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.

Another reason to rid of the config header.

This simplifies building without a build system. This is in line with bitcoin-core#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 bitcoin-core#929. Co-authored-by: Hennadii Stepanov <[email protected]>

real-or-random · 2022-07-01T13:13:25Z

@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: 😟 (-H is really useful flag...)

before fix: gcc -DENABLE_MODULE_ECDH -DENABLE_MODULE_RECOVERY -H -c src/bench.c (snippet)

. src/modules/ecdh/bench_impl.h
.. /usr/include/secp256k1_ecdh.h
... /usr/include/secp256k1.h
. src/modules/recovery/bench_impl.h
.. /usr/include/secp256k1_recovery.h
. src/modules/schnorrsig/bench_impl.h
.. src/modules/schnorrsig/../../../include/secp256k1_schnorrsig.h
... src/modules/schnorrsig/../../../include/secp256k1.h
... src/modules/schnorrsig/../../../include/secp256k1_extrakeys.h

after fix: gcc -DENABLE_MODULE_ECDH -DENABLE_MODULE_RECOVERY -H -c src/bench.c (snippet)

. src/modules/ecdh/bench_impl.h
.. src/modules/ecdh/../../../include/secp256k1_ecdh.h
... src/modules/ecdh/../../../include/secp256k1.h
. src/modules/recovery/bench_impl.h
.. src/modules/recovery/../../../include/secp256k1_recovery.h
... src/modules/recovery/../../../include/secp256k1.h
. src/modules/schnorrsig/bench_impl.h
.. src/modules/schnorrsig/../../../include/secp256k1_schnorrsig.h
... src/modules/schnorrsig/../../../include/secp256k1.h
... src/modules/schnorrsig/../../../include/secp256k1_extrakeys.h

hebasto

ACK 40a3473

real-or-random mentioned this pull request Jun 30, 2022

build: Add CMake-based build system #1113

Merged

hebasto reviewed Jun 30, 2022

View reviewed changes

src/tests_exhaustive.c Show resolved Hide resolved

real-or-random force-pushed the 202206-include-fixes branch from d63719f to ac39773 Compare June 30, 2022 18:29

real-or-random force-pushed the 202206-include-fixes branch from ac39773 to 40a3473 Compare July 1, 2022 13:04

hebasto approved these changes Jul 1, 2022

View reviewed changes

real-or-random merged commit af65d30 into bitcoin-core:master Jul 1, 2022

jonasnick mentioned this pull request Jul 17, 2023

Upstream PRs 1056, 1104, 1105, 1084, 1114, 1115, 1116, 1120, 1122, 1121, 1128, 1131, 1144, 1150, 1146 BlockstreamResearch/secp256k1-zkp#238

Merged

Uh oh!

build: Fix #include "..." paths to get rid of further -I arguments #1116

build: Fix #include "..." paths to get rid of further -I arguments #1116

Uh oh!

Conversation

real-or-random commented Jun 30, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Jun 30, 2022

Uh oh!

sipa commented Jun 30, 2022

Uh oh!

real-or-random commented Jun 30, 2022

Uh oh!

real-or-random commented Jun 30, 2022

Uh oh!

sipa commented Jun 30, 2022

Uh oh!

real-or-random commented Jun 30, 2022

Uh oh!

sipa commented Jun 30, 2022

Uh oh!

hebasto commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

real-or-random commented Jul 1, 2022

Uh oh!

hebasto commented Jul 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jul 1, 2022

Uh oh!

real-or-random commented Jul 1, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hebasto commented Jul 1, 2022 •

edited

Loading

hebasto commented Jul 1, 2022 •

edited

Loading