build: Use correct variable name

hebasto commented at 3:34 pm on September 2, 2024: member

This was overlooked after https://github.com/bitcoin-core/secp256k1/pull/1546.

Also see:

hebasto added the label Build system on Sep 2, 2024

hebasto marked this as a draft on Sep 2, 2024

hebasto force-pushed on Sep 2, 2024

hebasto commented at 8:43 pm on September 2, 2024: member

Please review https://github.com/bitcoin-core/secp256k1/pull/1600 first.

DrahtBot commented at 8:43 pm on September 2, 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	fanquake

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

fanquake commented at 8:45 pm on September 2, 2024: member

I guess this is also now a follow up for: https://github.com/hebasto/bitcoin/pull/192#discussion_r1660826044.

hebasto commented at 8:46 pm on September 2, 2024: member

I guess this is also now a follow up for: hebasto#192 (comment).

Indeed.

DrahtBot commented at 10:50 pm on September 2, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29578740495

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Sep 2, 2024

hebasto force-pushed on Sep 7, 2024

hebasto commented at 5:24 pm on September 7, 2024: member

Rebased on top of the #30845.

DrahtBot removed the label CI failed on Sep 8, 2024

fanquake referenced this in commit df86a4f333 on Sep 9, 2024

fanquake commented at 2:45 pm on September 9, 2024: member

ACK - post rebase.

build: Use correct variables when passing `-fsanitize` to libsecp256k1

This was overlooked after https://github.com/bitcoin-core/secp256k1/pull/1546

2d68c3b1c2

hebasto force-pushed on Sep 9, 2024

hebasto marked this as ready for review on Sep 9, 2024

hebasto commented at 2:48 pm on September 9, 2024: member

Rebased on top of the merged #30845.

fanquake approved

fanquake commented at 10:45 am on September 10, 2024: member

ACK 2d68c3b1c2e4f8fb881efc3569506d426ee5155d

fanquake merged this on Sep 10, 2024

fanquake closed this on Sep 10, 2024

hebasto deleted the branch on Sep 10, 2024

maflcko commented at 1:48 pm on September 10, 2024: member

This uncovers a bug in the build system: 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.

Steps to reproduce:

Ensure valgrind devel package is installed
Call rm -rf ./bld-cmake/ && cmake -B ./bld-cmake -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DBUILD_FOR_FUZZING=ON -DSANITIZERS=fuzzer and observe that the output says " ctime_tests ……………………. ON"
Call cmake --build ./bld-cmake/ and observe the build failure:

 0# cmake --build ./bld-cmake/ 
 1[  1%] Building C object src/secp256k1/src/CMakeFiles/secp256k1_precomputed.dir/precomputed_ecmult.c.o
 2[  1%] Building C object src/secp256k1/src/CMakeFiles/secp256k1_precomputed.dir/precomputed_ecmult_gen.c.o
 3[  1%] Built target secp256k1_precomputed
 4[  1%] Building C object src/secp256k1/src/CMakeFiles/secp256k1.dir/secp256k1.c.o
 5[  1%] Linking C static library libsecp256k1.a
 6[  1%] Built target secp256k1
 7[  1%] Building C object src/secp256k1/src/CMakeFiles/ctime_tests.dir/ctime_tests.c.o
 8[  2%] Linking C executable ctime_tests
 9/usr/bin/ld: CMakeFiles/ctime_tests.dir/ctime_tests.c.o: in function `main':
10/b-c/src/secp256k1/src/ctime_tests.c:39: multiple definition of `main';  : in function `main':
11(.text+0x16): undefined reference to `LLVMFuzzerTestOneInput'
12clang: error: linker command failed with exit code 1 (use -v to see invocation)
13gmake[2]: *** [src/secp256k1/src/CMakeFiles/ctime_tests.dir/build.make:98: src/secp256k1/src/ctime_tests] Error 1
14gmake[1]: *** [CMakeFiles/Makefile2:1741: src/secp256k1/src/CMakeFiles/ctime_tests.dir/all] Error 2
15gmake: *** [Makefile:136: all] Error 2

hebasto commented at 3:42 pm on September 10, 2024: member

This uncovers a bug in the build system: 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.

Thanks! I’ll submit a fix shortly.

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

This uncovers a bug in the build system: 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.

Fixed in #30865.

build: Use correct variable name #30791

Code Coverage

Reviews