Skip to content

Conversation

@kevkevinpal
Copy link
Contributor

Motivation

I wanted to try and run the benchmarking scripts and I noticed the recovery benchmark in bench.c. I wanted to run but I was using cmake and the error message telling me to use ./configure -enable-module-recovery wasn't sufficient.

I figure rather than forcing users to look into the CMakeLists.txt file or anywhere else we should add this to the output

Solution

I appended to the message to include the -DSECP256K1_ENABLE_MODULE_...=ON in the message.

@real-or-random real-or-random added the meta/development processes, conventions, developer documentation, etc. label Dec 5, 2025
@real-or-random
Copy link
Contributor

Thanks for spotting this. Maybe a simpler fix (which will be easier to maintain in the future) is to remove this line (or simply refer to the README).

@kevkevinpal kevkevinpal force-pushed the compileErrorMessageWithCMake branch from d85a791 to f179329 Compare December 5, 2025 14:24
@kevkevinpal kevkevinpal force-pushed the compileErrorMessageWithCMake branch from f179329 to 3b5b03f Compare December 5, 2025 14:25
@kevkevinpal
Copy link
Contributor Author

Thanks for spotting this. Maybe a simpler fix (which will be easier to maintain in the future) is to remove this line (or simply refer to the README).

Sounds good I pushed 3b5b03f to replace the message with this instead:

"See README.md for configuration instructions.\n\n"

let me know if this looks good, I can reword or simply delete the print if that is wanted

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 3b5b03f

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3b5b03f, I have reviewed the code and it looks OK.

Another PR might also improve this comment:

* has been configured with --enable-external-default-callbacks. Then the

@real-or-random real-or-random merged commit aa2a39c into bitcoin-core:master Dec 15, 2025
122 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

meta/development processes, conventions, developer documentation, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants