Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: Rework tests target for Coverage configuartion and multi-config generators #1592

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Aug 18, 2024

The "experimental" status of the CMake-based build system can be reconsidered once it is adopted in the Bitcoin Core project. Fixing all known bugs is essential before this reconsideration.

Currently, I'm aware of a single issue: when using a multi-config generator, such as "Ninja Multi-Config", the build system still builds the tests binary for the "Coverage" configuration, which is meaningless:

$ cmake -B build -G "Ninja Multi-Config"
$ cmake --build build --config Coverage
[18/19] Building C object src/CMakeFiles/tests.dir/Coverage/tests.c.o
/home/hebasto/git/secp256k1/secp256k1/src/tests.c:18:13: note: ‘#pragma message: Defining VERIFY for tests being built for coverage analysis support is meaningless.’
   18 |     #pragma message("Defining VERIFY for tests being built for coverage analysis support is meaningless.")
      |             ^~~~~~~
[19/19] Linking C executable src/Coverage/tests

This PR fixes the issue.

It is an alternative to #1251 and #1291.

The last commit aaditionally addresses that comment.

@real-or-random
Copy link
Contributor

@hebasto Can you summarize what this PR does and why is it preferable to #1291? The approach in #1291 seemed natural to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants