build: Skip secp256k1 ctime tests when tests are not being built

hebasto commented at 7:19 pm on September 10, 2024: member

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.

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.

Type	Reviewers
ACK	maflcko, fanquake

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: member

review 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]

File	commit c66c68345efb0bb3d5613ebac703cde779fa0f01(master)	commit 9b507cac88e4afea9c83887bf9064176cfe01779(master and this pull)
SHA256SUMS.part	`76bfdb62198e59e0...`	`d9a01ab8a75fd173...`
*-aarch64-linux-gnu-debug.tar.gz	`875078a1fc2681a3...`	`6c4716c085205834...`
*-aarch64-linux-gnu.tar.gz	`e5eb6ea13886f601...`	`2a660693078220c9...`
*-arm-linux-gnueabihf-debug.tar.gz	`94e6cf774748aa38...`	`3711ec1c76cfd370...`
*-arm-linux-gnueabihf.tar.gz	`f43246607e0859e8...`	`0adf4333289a45d1...`
*-arm64-apple-darwin-unsigned.tar.gz	`6216b3c8aa5996a3...`	`b63b70a345000cb4...`
*-arm64-apple-darwin-unsigned.zip	`56a1960759a99a71...`	`b97afe1c51615b6f...`
*-arm64-apple-darwin.tar.gz	`41f58765d9fbeed6...`	`66020cb222759374...`
*-powerpc64-linux-gnu-debug.tar.gz	`9ef833a1d4a3477d...`	`7a9569bdc668c8c2...`
*-powerpc64-linux-gnu.tar.gz	`b1c07de85e3695c9...`	`eeaad0b5505f005a...`
*-riscv64-linux-gnu-debug.tar.gz	`28e9d371628c0cb0...`	`db2f5f3b22316344...`
*-riscv64-linux-gnu.tar.gz	`adae8a878fdf3928...`	`d43f95c7c56808ee...`
*-x86_64-apple-darwin-unsigned.tar.gz	`284b48579772fb04...`	`cd84ebbb2e7f1330...`
*-x86_64-apple-darwin-unsigned.zip	`92df47df3f090d6e...`	`dd79eb314dc305f9...`
*-x86_64-apple-darwin.tar.gz	`7661e7b455ecc68e...`	`25a3546d16837b57...`
*-x86_64-linux-gnu-debug.tar.gz	`85c1a5d4db575d13...`	`6e8f4337e420d991...`
*-x86_64-linux-gnu.tar.gz	`55d31cdd6812b956...`	`efe75331fb7a5584...`
*.tar.gz	`c8dc485389534d37...`	`d29953f6ac9bfb4c...`
guix_build.log	`7224be0660b315d7...`	`e0577da45a9eb70a...`
guix_build.log.diff		`b3a6a48160778f94...`

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: member

I think I’m missing something here. Isn’t it a bug that us already setting SECP256K1_BUILD_CTIME_TESTS=OFF is sometimes overridden later on? Otherwise, this change is unclear, because the ctime tests (along with the other secp256k1 tests) are already supposedly controlled by BUILD_TESTS, but now the ctime tests are being wrapped in another BUILD_TESTS conditional? (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=OFF is sometimes overridden later on? Otherwise, this change is unclear, because the ctime tests (along with the other secp256k1 tests) are already supposedly controlled by BUILD_TESTS, but now the ctime tests are being wrapped in another BUILD_TESTS conditional? (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 to ON?

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=OFF when 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 to ON?

We cannot force it to ON because 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=ON is 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, 2024

build: Skip secp256k1 ctime tests when tests are not being built

Co-authored-by: fanquake <fanquake@gmail.com>

23eedc5d1e

hebasto force-pushed on Sep 12, 2024

maflcko commented at 2:19 pm on September 12, 2024: member

re-review ACK 23eedc5d1e24b3d31da7902ece5182b9b24672d8

only change is a comment

fanquake approved

fanquake commented at 2:29 pm on September 12, 2024: member

ACK 23eedc5d1e24b3d31da7902ece5182b9b24672d8

fanquake merged this on Sep 12, 2024

fanquake closed this on Sep 12, 2024

hebasto deleted the branch on Sep 12, 2024

build: Skip secp256k1 ctime tests when tests are not being built #30865

Code Coverage

Reviews

Guix builds (on x86_64) [untrusted test-only build, possibly unsafe, not for production use]