bitcoin
bitcoin copied to clipboard
hebasto
Use Clang's native code coverage workflow
Use Clang's native code coverage compilation, gathering and result reporting if the compiler is Clang, as described in https://clang.llvm.org/docs/SourceBasedCodeCoverage.html.
Also use an option for enabling coverage instead of a build target because they are orthogonal - can have each build target with or without coverage. Mimic -DWERROR.
To test this in a quicker way I temporarily applied this patch to run just a few tests instead of all:
--- i/cmake/script/Coverage.cmake
+++ w/cmake/script/Coverage.cmake
@@ -10,12 +10,15 @@ if(EXTENDED_FUNCTIONAL_TESTS)
endif()
if(DEFINED JOBS)
list(APPEND CMAKE_CTEST_COMMAND -j ${JOBS})
list(APPEND functional_test_runner -j ${JOBS})
endif()
+list(APPEND functional_test_runner p2p_ping)
+list(APPEND CMAKE_CTEST_COMMAND --tests-regex addrman_tests)
+
# Run the tests.
execute_process(
COMMAND ${CMAKE_CTEST_COMMAND}
WORKING_DIRECTORY ${CMAKE_CURRENT_LIST_DIR}
COMMAND_ERROR_IS_FATAL ANY
General Concept NACK. Anytime we are trying to branch on compiler inside the build system, something is generally going wrong. It's unclear why we need to continue to hard code more cases here, rather than using presets, or, just letting devs set flags as required. I think the only reason the coverage stuff was ported was for parity, but if everyone is using something else, then it'd make more sense to have that (alone) in the build system, if anything, rather than adding more ways to generate coverage.
What is the suggested workflow when using the "Ninja Multi-Config" generator?
It should be the same as the suggested workflow to run the tests when using "Ninja Multi-Config" (without coverage enabled). How do we suggest to the users/devs to run the tests in that case?
Anytime we are trying to branch on compiler inside the build system, something is generally going wrong.
The branching on compiler in CMakeLists.txt can be substituted with try_append_cxx_flags("-fprofile-instr-generate;-fcoverage-mapping") otherwise try_append_cxx_flags("-Og;--coverage") otherwise error: don't know how to enable coverage. Does that sound better? Branching on compiler in cmake/script/Coverage* is already in cmake-stage without this PR, do you consider that "wrong" as well?
It's unclear why we need to continue to hard code more cases here
Because it can be done better. Clang native reports are better.
rather than using presets, or, just letting devs set flags as required.
Are you talking about just the configure step? Presets and/or APPEND_CXX_FLAGS can be used to do the configure step. But the current code (cmake -P build/Coverage.cmake in cmake-staging without this PR) also runs the tests and generates the reports in a gcc-specific way. This PR accommodates the Clang way into that, I am open to suggestions how to do that better.
acb8df9a5a...194758b08e: remove the branching on compiler from CMakeLists.txt
This is incomplete wrt collecting coverage from the fuzz and dealing with ninja multi-config. Will convert it to draft.
Since https://github.com/bitcoin/bitcoin/pull/30454 has been merged, further development in this repository no longer makes sense. Please consider moving this PR to the main repository.
Closing this because, for sure now the point here is not to merge it into hebasto/cmake-staging.