cmake: Add option to generate precomputed files#1791
Conversation
cbc6679 to
5373c6a
Compare
|
are we able to do this via or is this meant to be only done for CMake? |
Yes. Run |
5373c6a to
f9eeb11
Compare
|
Thank you for the review! Your feedback has been addressed. |
f9eeb11 to
fdfda7b
Compare
47eb709 ecmult: Use size_t for array indices in _odd_multiplies_table (Tim Ruffing) bb1d199 ecmult: Use size_t for array indices into tables (Tim Ruffing) Pull request description: I don't think the current code is incorrect, but using `size_t` improves readability because the type makes it clear that we're dealing with array indices. Also, making the result of the `ECMULT_TABLE_SIZE` macro (hopefully) a `size_t` fixes a compiler warning on MSVC, see #1791. ACKs for top commit: hebasto: re-ACK 47eb709. jonasnick: ACK 47eb709 theStack: ACK 47eb709 Tree-SHA512: e484fd610d50e972021c0184a683993364290eb58e09b65f9521b4507ec8d0639b402c67002005630b389bc863a7aa05b75f7224524dbcbafbfa5f9a4812b4a5
fdfda7b to
8b58c55
Compare
|
Concept ACK Tested quickly that succeeds on the PR branch as expected, while on master fails ( The generated files |
That's correct. We have to assume the source tree might be strictly read-only. |
There was a problem hiding this comment.
Light ACK 8b58c55
I'm not too familiar with the ins and outs of CMake, but the changes look good and I tested the new build setting both with higher ecmult window sizes (as shown above) and with different ecmult_gen CONFIGS table values (taken from theStack@3421153), and everything seems to work as expected.
// EDIT: I guess this is worth a release note
8b58c55 to
8460c59
Compare
|
The most recent feedback has been addressed. |
The truth is that it also outputs I think either approach is fine. I guess even mixing them is fine. Maybe we should just change the error message for now? "If this is an autotools build, try deleting precomputed_ecmult.c before the build. If this is a CMake build, pass -DSECP256K1_BUILD_PRECOMPUTED=ON to cmake." Further thoughts: I'm wondering if I like the "build option" approach as in this PR more. It's easier for those users who need it, and the code is simpler. And even for contributors working on changes to the generation binaries, they can just set -DSECP256K1_BUILD_PRECOMPUTED=ON in their cache. If others agree, this raises the question of whether we want to get rid of that stuff in autotools. It never worked 100%. And it seems that all users who want this feature use CMake. (@kevkevinpal Why did you ask about autotools? Do you need this feature in autotools?) |
real-or-random
left a comment
There was a problem hiding this comment.
Consider rebasing this. The branch is way behind (for our standards ;)).
There was a problem hiding this comment.
Pull request overview
This PR adds a new CMake build option, SECP256K1_BUILD_PRECOMPUTED, to optionally regenerate the precomputed_ecmult*.c sources in the build tree for packagers that require recreating these generated files.
Changes:
- Introduces
SECP256K1_BUILD_PRECOMPUTED(default OFF) and wires it into thesrc/CMake logic to switch between in-tree vs build-tree precomputed sources. - Adds a new
cmake/Precompute.cmakehelper that builds/runsprecompute_*generators to producesrc/precomputed_*.cin the build directory. - Enables the option in the
dev-modeCMake preset and in the Windows CMake CI job to exercise the path.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/CMakeLists.txt |
Switches secp256k1_precomputed object sources between shipped and regenerated precomputed .c files based on the new option. |
CMakePresets.json |
Enables precompute regeneration in the dev-mode preset. |
CMakeLists.txt |
Adds the new SECP256K1_BUILD_PRECOMPUTED option definition. |
cmake/Precompute.cmake |
Adds a precompute() helper to build generator executables and register custom commands for generated .c outputs. |
.github/workflows/ci.yml |
Turns on SECP256K1_BUILD_PRECOMPUTED for Windows CMake CI configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(SECP256K1_BUILD_PRECOMPUTED) | ||
| include(Precompute) | ||
| precompute(ecmult) | ||
| precompute(ecmult_gen) |
There was a problem hiding this comment.
Yes. The current CMake implementation doesn't support cross-compiling. And neither does Autotools.
FWIW, I have a branch that introduces this support, but I'm not sure whether the extra complexity is justified.
There was a problem hiding this comment.
FWIW, I have a branch that introduces this support, but I'm not sure whether the extra complexity is justified.
Ah please don't. :D Years ago, when we always ran the precomputation during the build (no precomputed files in repo), we had support for this in Autotools. But handling the different compilers (one for host and one for target) was a nightmare for users who wanted to cross-compile and for us to maintain. It's one of the reasons why added the precomputed files to the repo.
I think if we really want to support this during cross-compilation, we'd rather rewrite the precomputation helpers in python or sage.
There was a problem hiding this comment.
Ah please don't. :D Years ago, when we always ran the precomputation during the build (no precomputed files in repo), we had support for this in Autotools.
TIL. Not arguing, but CMake has built-in ways to achieve this.
There was a problem hiding this comment.
CMake has built-in ways to achieve this.
autoconf has this here: https://www.gnu.org/software/autoconf-archive/ax_prog_cc_for_build.html (not exactly built-in but that's the standard way on autoconf)
I totally believe that the CMake way of doing this is much cleaner, but I think part of the problem is on a conceptual level. Now you get effectively two build configs with all their complexity. You have separate CFLAGS, CPPFLAGS, LDFLAGS, which raises all kinds of questions, e.g., which warning options do you want to set on the compiler for the build machine? Maybe the compiler is entirely different, e.g., you have a new clang for the build machine but an old gcc for the target, etc... And some of this complexity is necessarily pushed down to the user.
Just assuming the existence of Python or similar is probably simpler. Back then we had already considered using Python instead, but approach to add the precomputed files to the repo was just less work, so we went with this instead.
There was a problem hiding this comment.
But do you think some error message like suggested by the LLM makes sense for now?
There was a problem hiding this comment.
But do you think some error message like suggested by the LLM makes sense for now?
Done.
8460c59 to
80edad3
Compare
Rebased. The recent feedback has been addressed.
Done. |
Trying to remember why but I think was just trying to understand why it wasn't available. If anything I agree with you and also prefer CMake. |
We have the same message in precomputed_ecmult_gen.c. Sorry, I assumed that's implied. I should have mentioned this explicitly. |
|
For autotools builds on CI, we have Lines 138 to 147 in 0f4a7e6 |
Of course, I've regenerated |
Oops, we're talking past each other. Sorry, I meant to write So you've changed |
80edad3 to
37152af
Compare
|
Reworked per recent feedback. Added The PR description has been updated.
My bad. Fixed.
Done. |
| # Enforce fail-fast behavior for PowerShell. | ||
| shell: pwsh -Command "$PSNativeCommandUseErrorActionPreference = $true; $ErrorActionPreference = 'Stop'; & '{0}'" |
There was a problem hiding this comment.
Shouldn't this be implied according to the docs?
There was a problem hiding this comment.
Shouldn't this be implied according to the docs?
The docs state: "Fail-fast behavior when possible." Effectively, it works for cmdlets only. Non-zero exit codes from native commands are ignored unless $PSNativeCommandUseErrorActionPreference is also set.
There was a problem hiding this comment.
Sigh, why can't we just have nice things...
37152af to
9c08b45
Compare
Add a new `SECP256K1_BUILD_PRECOMPUTED` option (`OFF` by default) that generates `precomputed_ecmult.c` and `precomputed_ecmult_gen.c` in the build tree. Those files are then compiled instead of the ones checked into the source tree. When configuring with CMake 3.31 or newer, the built-in `codegen` target can be used to generate those files.
9c08b45 to
00b73a4
Compare
|
Rebased on top of the recent CI changes. |
Using the CMake-based build system enables smoother integration with parent projects by providing both CMake package configuration files and the traditional
libsecp256k1.pc.However, packagers on some systems, such as Gentoo and Guix, insist on recreating the precomputed files, which is not currently supported by the CMake-based build system.
This PR addresses the issue by introducing a new
SECP256K1_BUILD_PRECOMPUTEDoption (OFFby default) that generatesprecomputed_ecmult.candprecomputed_ecmult_gen.cin the build tree. Those files are then compiled instead of the ones checked into the source tree.Here is an example:
When configuring with CMake 3.31 or newer, the built-in
codegentarget can be used to generate those files: