changed include statements without prefix 'include/' #925

whb07 · 2021-04-25T17:56:37Z

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:

#include "include/secp256k1.h"

to:

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

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

gmaxwell · 2021-04-28T19:20:39Z

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.

$ 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
secp256k1.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 · 2021-04-28T19:38:29Z

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 · 2021-04-29T10:27:35Z

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.
$ 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
secp256k1.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 · 2021-04-29T14:08:47Z

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 · 2021-04-29T14:31:14Z

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

whb07 · 2021-04-29T15:26:13Z

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.

Makefile.am

gmaxwell · 2021-04-30T01:39:51Z

It should probably also change tests.c:

#include "../contrib/lax_der_parsing.c"
#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 · 2021-04-30T02:17:48Z

It should probably also change tests.c:
#include "../contrib/lax_der_parsing.c"
#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 · 2021-04-30T02:36:03Z

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

real-or-random · 2021-04-30T09:41:53Z

We could do something like this in the contrib files:

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

whb07 · 2021-04-30T13:59:08Z

We could do something like this in the contrib files:

/* #include secp256k1.h only when it hasn't been included yet.
   This enables this file to be #included directly in other project
   files (such as tests.c) without the need to set an explicit -I flag,
   which would be necessary to locate secp256k1.h.  */
#ifndef SECP256K1_H
#include <secp256k1.h>
#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 · 2021-04-30T15:29:17Z

We could do something like this in the contrib files:
/* #include secp256k1.h only when it hasn't been included yet.
   This enables this file to be #included directly in other project
   files (such as tests.c) without the need to set an explicit -I flag,
   which would be necessary to locate secp256k1.h.  */
#ifndef SECP256K1_H
#include <secp256k1.h>
#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.,

gcc  -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 · 2021-04-30T23:59:47Z

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

gmaxwell · 2021-05-01T17:05:06Z

src/modules/ecdh/main_impl.h

@@ -7,7 +7,7 @@
 #ifndef SECP256K1_MODULE_ECDH_MAIN_H
 #define SECP256K1_MODULE_ECDH_MAIN_H

-#include "include/secp256k1_ecdh.h"
+#include "../include/secp256k1_ecdh.h"
 #include "ecmult_const_impl.h"


../../ecmult_const_impl.h

gmaxwell · 2021-05-01T17:05:40Z

src/modules/schnorrsig/main_impl.h

-#include "include/secp256k1.h"
-#include "include/secp256k1_schnorrsig.h"
+#include "../include/secp256k1.h"
+#include "../include/secp256k1_schnorrsig.h"
 #include "hash.h"


../../hash.h

gmaxwell · 2021-05-01T17:20:35Z

src/modules/ecdh/main_impl.h

@@ -7,7 +7,7 @@
 #ifndef SECP256K1_MODULE_ECDH_MAIN_H
 #define SECP256K1_MODULE_ECDH_MAIN_H

-#include "include/secp256k1_ecdh.h"
+#include "../include/secp256k1_ecdh.h"


../../../include/

src/modules/extrakeys/main_impl.h

src/modules/extrakeys/tests_exhaustive_impl.h

src/modules/recovery/main_impl.h

src/modules/recovery/tests_exhaustive_impl.h

src/modules/schnorrsig/main_impl.h

src/modules/schnorrsig/tests_exhaustive_impl.h

src/modules/schnorrsig/tests_impl.h

gmaxwell · 2021-05-01T17:24:56Z

$ 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 · 2021-05-02T09:46:59Z

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

gmaxwell · 2021-05-04T18:17:56Z

Great!

This just needs code like

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

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

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

will work.

real-or-random · 2021-05-04T19:14:29Z

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 · 2021-05-05T01:44:58Z

#ifndef SECP256K1_H
#include <secp256k1.h>
#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 · 2021-05-05T02:55:37Z

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

…e/" dir added relative paths to header files imported from src directory added include guards for contrib/ files when referring to secp256k1.h

whb07 · 2021-05-05T13:31:31Z

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:

$ 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

&&

$ 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 · 2021-05-05T16:22:46Z

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

real-or-random · 2021-05-05T16:25:36Z

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
cirruslabs/cirrus-cli#373 (comment) .) Let me restart that ask, but let's not get that in our way.

real-or-random

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

gmaxwell · 2021-05-05T18:08:57Z

ACK 3c90bdd

sipa · 2021-05-05T18:33:17Z

Posthumous utACK 3c90bdd

They were added in bitcoin-core#925 and deserve a comment.

22a9ea1 contrib: Explain explicit header guards (Tim Ruffing) Pull request description: They were added in #925 and deserve a comment. ACKs for top commit: gmaxwell: ACK 22a9ea1 sipa: ACK 22a9ea1 Tree-SHA512: 832e28d71857d52912dae7e6c0e08a3183bb788996bb2470616c6fbbac6ba601cc74bb51a4c908aec7df9ae4f4cbf2cbb1b451cefde1b5a7359dc93299840278

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.

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 <32963518+hebasto@users.noreply.github.com>

… arguments 40a3473 build: Fix #include "..." paths to get rid of further -I arguments (Tim Ruffing) Pull request description: 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. ACKs for top commit: hebasto: ACK 40a3473 Tree-SHA512: 6f4d825ea3cf86b13f294e2ec19fafc29660fa99450e6b579157d7a6e9bdb3404d761edf89c1135fa89b984d6431a527beeb97031dc90f2fae9761528f4d06d1

They were added in bitcoin-core#925 and deserve a comment.

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 <32963518+hebasto@users.noreply.github.com>

real-or-random mentioned this pull request Apr 29, 2021

Persistent worker failed to start the task: Parallels isolation failed cirruslabs/cirrus-cli#373

Closed

whb07 force-pushed the lib_header_refactor branch from 486c2a8 to 9d5ac8c Compare April 29, 2021 17:05

gmaxwell reviewed Apr 29, 2021

View reviewed changes

Makefile.am Outdated Show resolved Hide resolved

whb07 force-pushed the lib_header_refactor branch from 9d5ac8c to c12d719 Compare April 30, 2021 02:10

whb07 force-pushed the lib_header_refactor branch from c12d719 to 188637c Compare April 30, 2021 13:43