This change eases the use of alternate build systems by moving the variables in src/libsecp256k1-config.h to compiler macros for each invocation, preventing duplication of these variables for each build system.
This does not include documentation for each configure option, and would need to be added in a separate commit.
Hi, thanks for the PR.
The reason why CI doesn’t like this is that the files precomputed_ecmult[_gen].c are generated via precompute_ecmult[_gen].c. You’ll also need to change the generating code in the latter to not generate the #include lines. Actually this suffices and then make clean-precomp && make precomp should generate the files.
For the change itself, I think I’m in favor of this because it’s in line with what I said here: #1113 (comment).
Hi, thanks for the PR.
The reason why CI doesn’t like this is that the files
precomputed_ecmult[_gen].care generated viaprecompute_ecmult[_gen].c. You’ll also need to change the generating code in the latter to not generate the#includelines. Actually this suffices and thenmake clean-precomp && make precompshould generate the files.For the change itself, I think I’m in favor of this because it’s in line with what I said here: #1113 (comment).
Alright, I updated the precompute_ files, and also added the SECP_ prefix to DEFINE_FLAGS.
I am wondering if it’s a good idea to put the configure option descriptions in the README or in a separate docs file.
I am wondering if it’s a good idea to put the configure option descriptions in the README or in a separate docs file.
My inclination is that they should be beside the options themselves, maybe with some sentinel like BUILD-DEFINE: that the README would tell people to grep for. If the docs are in a separate file I’m sure they’ll come out of sync.
My inclination is that they should be beside the options themselves, maybe with some sentinel like
BUILD-DEFINE:that the README would tell people to grep for. If the docs are in a separate file I’m sure they’ll come out of sync.
Will single-line comments in configure.ac with the docstring suffice, such as this?
0SECP_DEFINE_FLAGS="$SECP_DEFINE_FLAGS -DUSE_ASM_X86_64=1"
1dnl Define this symbol to enable x86_64 assembly optimizations
But then this raises an issue with duplication. Assuming CMake functionality will be merged, and DRY principles apply, we should make the Autoconf file the source of truth for these docstrings?
I think there’s no straight answer where to put the docs if we want to support multiple build systems, and that decision shouldn’t block this PR. Note that this PR doesn’t really remove docs, the doc strings are still in the configure.ac macros and are presented to the user in a nice way in ./configure --help.
Entering that discussion about where to put the docs:
Quoting myself from #1113 (comment):
In the end, we need a least one place (and as few as possible) places where the options are listed and documented, and some duplication will be unavoidable if we want to support more than one of building even if it’s just “autotools” and “manual”.
Will single-line comments in
configure.acwith the docstring suffice, such as this?0SECP_DEFINE_FLAGS="$SECP_DEFINE_FLAGS -DUSE_ASM_X86_64=1" 1dnl Define this symbol to enable x86_64 assembly optimizationsBut then this raises an issue with duplication.
Indeed, I think this will make the problem worse. And once we have autotools and cmake, it’s not clear where the canonical source is, and also the command line arguments will look different…
I still lean towards what I suggested in #1113 (comment): A table in the README, or maybe in a file referenced from the README, that has a row for every option (see mockup there). If people are motivated, it would be nice to see a worked out PR for this suggestion.
If we have this table, we could either remove the strings from ./configure --help entirely and leave them in very brief form like “enable ECDH module”. I think either possibility is ok. We’ll anyway have to live with some coupling and duplication: Every time we add/change/remove an option, we need to perform that change at least i) in the actual source, ii) in autotools, iii) and in cmake.
66@@ -67,7 +67,7 @@ noinst_HEADERS += examples/random.h
67 PRECOMPUTED_LIB = libsecp256k1_precomputed.la
68 noinst_LTLIBRARIES = $(PRECOMPUTED_LIB)
69 libsecp256k1_precomputed_la_SOURCES = src/precomputed_ecmult.c src/precomputed_ecmult_gen.c
70-libsecp256k1_precomputed_la_CPPFLAGS = $(SECP_INCLUDES)
71+libsecp256k1_precomputed_la_CPPFLAGS = $(SECP_INCLUDES) $(SECP_DEFINE_FLAGS)
SECP_CONFIG_DEFINES would be a slightly more descriptive name if you’re willing to make that change.
I still lean towards what I suggested in #1113 (comment): A table in the README, or maybe in a file referenced from the README, that has a row for every option (see mockup there). If people are motivated, it would be nice to see a worked out PR for this suggestion.
If we have this table, we could either remove the strings from
./configure --helpentirely and leave them in very brief form like “enable ECDH module”. I think either possibility is ok. We’ll anyway have to live with some coupling and duplication: Every time we add/change/remove an option, we need to perform that change at least i) in the actual source, ii) in autotools, iii) and in cmake.
I suggest reserving the table for the autoconf/CMake option specifiers, because we can just make the short option description as a heading above it, and the long description as a paragraph below it, so 1) the long description has more reading space, and 2) it embeds in the autogenerated Table of Contents for Markdown files. However, I advise keeping at least a simple option description in the ./configure --help output, like the way it is for --enable-test, --enable-benchmark and many others.
So basically, instead of this:
| Autotools | CMake | Manual | Description |
|---|---|---|---|
| –enable-tests | -DENABLE_TESTS | -DENABLE_TESTS | compile tests [default=yes] |
| –enable-examples | -DENABLE_EXAMPLES | -DENABLE_EXAMPLES | compile the examples [default=no] |
We can have something like this:
| Autotools |
|---|
| –enable-tests |
Compile tests. (Default: yes)
| Autotools |
|---|
| –enable-examples |
Compile the examples. (Default: yes)
Of course, I did not list CMake entries because that has not been merged yet, but for now, it will be an autotools list, and the manual options will be in the markdown titles.
This change eases the use of alternate build systems by moving
the variables in src/libsecp256k1-config.h to compiler macros
for each invocation, preventing duplication of these variables
for each build system.
./configure docs should be trimmed, considering they are a duplicate of the README?
It is possible now to cleanup the .gitignore file:
0--- a/.gitignore
1+++ b/.gitignore
2@@ -44,8 +44,6 @@ coverage.*.html
3 *.gcno
4 *.gcov
5
6-src/libsecp256k1-config.h
7-src/libsecp256k1-config.h.in
8 build-aux/ar-lib
9 build-aux/config.guess
10 build-aux/config.sub
11@@ -60,5 +58,4 @@ build-aux/m4/ltversion.m4
12 build-aux/missing
13 build-aux/compile
14 build-aux/test-driver
15-src/stamp-h1
16 libsecp256k1.pc
Concept ACK, and ACK the first commit.
I’m not really a fan of the current documentation changes, with a dozen tiny tables and subsections added to the readme, which isn’t very readable. I don’t think that should hold up the changes here, though.
We can merge it as-is, or drop the documentation changes. I’m happy to work on documenting the flags in a follow-up.
… drop the documentation changes. I’m happy to work on documenting the flags in a follow-up.
+1
Agreed.
Perhaps the flags could be documented separately in a separate Markdown file in the sparsely-populated docs folder.
@ZenulAbidin Can you drop the commit that changes the README for now, and (optional, if you want) make the changes to .gitignore proposed here? #1142 (comment)
Then we could get this merged and work on the doc stuff in a separate issue/PR.
Can you drop the commit that changes the README for now, and (optional, if you want) make the changes to
.gitignoreproposed here? #1142 (comment)
See #1178.
@hebasto Thanks for that. I was busy with other stuff last week so couldn’t get to this PR in a timely manner.
I’m going to leave this open for now in case anyone has more feedback about the flags & docs.