Add section on configuration flags to README

[ZenulAbidin]

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.

[real-or-random]

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: https://github.com/bitcoin-core/secp256k1/pull/1113#issuecomment-1171307979.

[ZenulAbidin]

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

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.

[apoelstra]

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.

[ZenulAbidin]

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?

SECP_DEFINE_FLAGS="$SECP_DEFINE_FLAGS -DUSE_ASM_X86_64=1"
dnl 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?

[real-or-random]

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 https://github.com/bitcoin-core/secp256k1/pull/1113#issuecomment-1171307979:

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.ac with the docstring suffice, such as this?

SECP_DEFINE_FLAGS="$SECP_DEFINE_FLAGS -DUSE_ASM_X86_64=1"
dnl Define this symbol to enable x86_64 assembly optimizations

But 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 https://github.com/bitcoin-core/secp256k1/pull/1113#issuecomment-1171307979: 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.

[ZenulAbidin]

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.

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:

Configuration options

-DENABLE_TESTS

Autotools
--enable-tests

Compile tests. (Default: yes)

-DENABLE_EXAMPLES

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.

[ZenulAbidin]

@real-or-random Config options have been added to the README.

[ZenulAbidin]

Also, do you think that the ./configure docs should be trimmed, considering they are a duplicate of the README?

[hebasto]

Concept ACK.

[hebasto]

It is possible now to cleanup the .gitignore file:

--- a/.gitignore
+++ b/.gitignore
@@ -44,8 +44,6 @@ coverage.*.html
 *.gcno
 *.gcov
 
-src/libsecp256k1-config.h
-src/libsecp256k1-config.h.in
 build-aux/ar-lib
 build-aux/config.guess
 build-aux/config.sub
@@ -60,5 +58,4 @@ build-aux/m4/ltversion.m4
 build-aux/missing
 build-aux/compile
 build-aux/test-driver
-src/stamp-h1
 libsecp256k1.pc

[sipa]

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.

[hebasto]

... drop the documentation changes. I'm happy to work on documenting the flags in a follow-up.

+1

[ZenulAbidin]

Agreed.

Perhaps the flags could be documented separately in a separate Markdown file in the sparsely-populated docs folder.

[sipa]

I've included the first commit from here in #1169, as it simplifies things a bit there, but I'm happy for this PR to settle first.

[real-or-random]

@ZenulAbidin Can you drop the commit that changes the README for now, and (optional, if you want) make the changes to .gitignore proposed here? https://github.com/bitcoin-core/secp256k1/pull/1142#issuecomment-1295099597

Then we could get this merged and work on the doc stuff in a separate issue/PR.

[hebasto]

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)

See #1178.

[ZenulAbidin]

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

[hebasto]

@ZenulAbidin

I'm going to leave this open for now in case anyone has more feedback about the flags & docs.

As #1178 has been recently merged, maybe update this PR description and title?

[hebasto]

The changes to the documentation have been incorporated in https://github.com/bitcoin-core/secp256k1/pull/1372.