Fixes #30791 (comment):
Building with a fuzz engine fails, because the ctime tests are auto-detected in cmake, based on whether or not valgrind-devel is installed or not.
build: Skip secp256k1 ctime tests when tests are not being built #30865
pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:240910-ctime-test changing 1 files +5 −0-
hebasto commented at 7:19 pm on September 10, 2024: member
-
hebasto added the label Build system on Sep 10, 2024
-
DrahtBot commented at 7:19 pm on September 10, 2024: contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Code Coverage
For detailed information about the code coverage, see the test coverage report.
Reviews
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
-
maflcko added the label DrahtBot Guix build requested on Sep 10, 2024
-
maflcko commented at 8:00 pm on September 10, 2024: memberreview ACK 8fb81c21cfaaee4ea68e5023c31d58b01f53fa4f
-
Oaksta85 approved
-
DrahtBot commented at 3:08 am on September 11, 2024: contributor
Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]
-
DrahtBot removed the label DrahtBot Guix build requested on Sep 11, 2024
-
DrahtBot added the label CI failed on Sep 11, 2024
-
fanquake commented at 10:17 am on September 11, 2024: memberI think I’m missing something here. Isn’t it a bug that us already setting
SECP256K1_BUILD_CTIME_TESTS=OFFis sometimes overridden later on? Otherwise, this change is unclear, because the ctime tests (along with the other secp256k1 tests) are already supposedly controlled byBUILD_TESTS, but now the ctime tests are being wrapped in anotherBUILD_TESTSconditional? (adding a comment would be good, otherwise I assume anyone reading this code will assume the second conditional is redundant). -
in src/CMakeLists.txt:56 in 8fb81c21cf outdated
48@@ -49,6 +49,9 @@ set(SECP256K1_ENABLE_MODULE_RECOVERY ON CACHE BOOL "" FORCE) 49 set(SECP256K1_BUILD_BENCHMARK OFF CACHE BOOL "" FORCE) 50 set(SECP256K1_BUILD_TESTS ${BUILD_TESTS} CACHE BOOL "" FORCE) 51 set(SECP256K1_BUILD_EXHAUSTIVE_TESTS ${BUILD_TESTS} CACHE BOOL "" FORCE) 52+if(NOT BUILD_TESTS) 53+ set(SECP256K1_BUILD_CTIME_TESTS ${BUILD_TESTS} CACHE BOOL "" FORCE) 54+endif()
hebasto commented at 1:39 pm on September 11, 2024:I think I’m missing something here. Isn’t it a bug that us already setting
SECP256K1_BUILD_CTIME_TESTS=OFFis sometimes overridden later on? Otherwise, this change is unclear, because the ctime tests (along with the other secp256k1 tests) are already supposedly controlled byBUILD_TESTS, but now the ctime tests are being wrapped in anotherBUILD_TESTSconditional? (adding a comment would be good, otherwise I assume anyone reading this code will assume the second conditional is redundant). @fanquake Is the following comment a good explanation:0if(NOT BUILD_TESTS) 1 # Do not force SECP256K1_BUILD_CTIME_TESTS to ON, as its default value depends on SECP256K1_VALGRIND. 2 set(SECP256K1_BUILD_CTIME_TESTS ${BUILD_TESTS} CACHE BOOL "" FORCE) 3endif()?
fanquake commented at 11:29 am on September 12, 2024:I’m not sure, because in the case being fixed here (
-DBUILD_FOR_FUZZING),BUILD_TESTS=OFF, so we aren’t forcing it toON?It also doesn’t explain why our setting (of
OFF) is being overriden later. The same thing doesn’t happen if you pass-DSECP256K1_BUILD_CTIME_TESTS=OFFwhen invoking cmake.
hebasto commented at 11:33 am on September 12, 2024:I’m not sure, because in the case being fixed here (
-DBUILD_FOR_FUZZING),BUILD_TESTS=OFF, so we aren’t forcing it toON?We cannot force it to
ONbecause the build will require the installed Valgrind, which is not always the case.It also doesn’t explain why our setting (of
OFF) is being overriden later.When?
fanquake commented at 12:39 pm on September 12, 2024:When-DBUILD_FOR_FUZZING=ONis passed.
hebasto commented at 12:48 pm on September 12, 2024:Could you please suggest your alternative?
fanquake commented at 1:15 pm on September 12, 2024:Something like:
0# Always skip the ctime tests, if we are building no other tests. 1# Otherwise, they are built if Valgrind is available. See SECP256K1_VALGRIND.?
hebasto commented at 1:34 pm on September 12, 2024:Thank you! Accepted.DrahtBot removed the label CI failed on Sep 11, 202423eedc5d1ebuild: Skip secp256k1 ctime tests when tests are not being built
Co-authored-by: fanquake <fanquake@gmail.com>
hebasto force-pushed on Sep 12, 2024maflcko commented at 2:19 pm on September 12, 2024: memberre-review ACK 23eedc5d1e24b3d31da7902ece5182b9b24672d8
only change is a comment
fanquake approvedfanquake commented at 2:29 pm on September 12, 2024: memberACK 23eedc5d1e24b3d31da7902ece5182b9b24672d8fanquake merged this on Sep 12, 2024fanquake closed this on Sep 12, 2024
hebasto deleted the branch on Sep 12, 2024
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-21 15:12 UTC
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-21 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me
More mirrored repositories can be found on mirror.b10c.me