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

Use Clang's native code coverage workflow #233

Closed
wants to merge 128 commits into from

Conversation

vasild
Copy link

@vasild vasild commented Jun 18, 2024

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.

hebasto and others added 30 commits June 4, 2024 11:52
Also add a sanity check for non-encapsulated (directory-wide) build
properties.
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
If any of {C,CXX,CPP,LD}FLAGS is specified it should be assigned to
a non-type-specific variable.
To configure CMake for cross-compiling, use
`--toolchain depends/${HOST}/toolchain.cmake` command-line option.
Add vcpkg-specific comment.
Switch to `check_include_file_cxx` when checking headers.
This speeds up configuration.

Only one Boost.Test header is being checked now.
e104fa9 fixup! cmake: Build `bitcoind` executable (Hennadii Stepanov)
b38d2bc fixup! cmake: Build `bitcoind` executable (Hennadii Stepanov)

Pull request description:

  The first commit was requested offline.

  The second commit speeds up configuration.

Top commit has no ACKs.

Tree-SHA512: c520fbe44c2601bc7052c411445f54a89a53352e71f67c1464bc96c413782f461449e95170cc519a9613942d78ee517de8d629c7c7e5f8b20226b7b37df0eff4
…tests

7e94254 qa: Do not assume running `feature_asmap.py` from source directory (Hennadii Stepanov)
1ec9926 qa: Consider `cache` and `config.ini` relative to invocation directory (Hennadii Stepanov)
ca1f7b8 fixup! cmake: Add test config and runners (Hennadii Stepanov)

Pull request description:

  Resolves hebasto#146.

  > It should be possible to run all functional tests directly.

  Done.

  > Furthermore, these should be symlinked as making changes to the test in the source directory should result in the executed test also changing without needing a rebuild.

  Done.

  > For example, trying to run `test/functional/wallet_basic.py` from the build dir is not possible.

  Works as expected now.

Top commit has no ACKs.

Tree-SHA512: 624fbfe5a5892c60c044e05694c53cd6727e412846a4f6950eb90eb4769de3fc671ca235dca65fa4b9478c7eea3864bcb419b23c239a2eb980f6f098d357d197
939434d cmake, doc: Update `build-windows-mingw.md` (Hennadii Stepanov)

Pull request description:

ACKs for top commit:
  m3dwards:
    ACK hebasto@939434d

Tree-SHA512: 449d079d4ec957dc629db441b34796e73746ba299cc1c6c3ec01a36f2f0f0b416f74d80f9d761f002d958d6c4b1c2289e1123638de1a4bea7291d2fae6d06ed1
`WITH_CCACHE` is an opportunistic option now and its default is `ON`.
4cd15f1 cmake: Remove unused `TristateOption` module (Hennadii Stepanov)
686a731 cmake [KILL 3-STATE]: Switch `WITH_CCACHE` to boolean (Hennadii Stepanov)

Pull request description:

  This PR makes `WITH_CCACHE` an opportunistic boolean option with the default value `ON`.

  Also the build system learned to tell ccache in the [masquerade](https://ccache.dev/manual/4.9.1.html#_run_modes) mode, which addresses this hebasto#93 (comment).

  The `TristateOption` module has been deleted as planned.

ACKs for top commit:
  m3dwards:
    ACK hebasto@4cd15f1

Tree-SHA512: 6df4d0b9ca06a2362596cfa972d5914ef95d3def15ba51a30745933a8a9bab212d02a7fc22ca9f8a564874aca853c3a9203b421f5e2dd7a1121f7cce7aaa968d
@vasild
Copy link
Author

vasild commented Jun 18, 2024

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

@hebasto
Copy link
Owner

hebasto commented Jun 18, 2024

What is the suggested workflow when using the "Ninja Multi-Config" generator?

@fanquake
Copy link

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.

@vasild
Copy link
Author

vasild commented Jun 19, 2024

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.

Use an option for enabling coverage instead of a build target

Enabling code coverage boils down to adding extra compiler options,
like the WERROR option. This can be done for any build target and
regardless of the build target, thus switch from using a build target
to a -DCOVERAGE=ON|OFF option.

Also use the native Clang options when the compiler is Clang, as per
https://clang.llvm.org/docs/SourceBasedCodeCoverage.html#compiling-with-coverage-enabled
Use Clang's native code coverage compilation, gathering and result
reporting if the compiler is Clang.
@vasild
Copy link
Author

vasild commented Jun 19, 2024

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.

@vasild vasild marked this pull request as draft June 19, 2024 14:21
@hebasto hebasto force-pushed the cmake-staging branch 2 times, most recently from 02afae4 to 19d4d92 Compare July 31, 2024 13:10
@hebasto hebasto force-pushed the cmake-staging branch 3 times, most recently from 46ff477 to 954193e Compare August 10, 2024 11:01
@hebasto
Copy link
Owner

hebasto commented Aug 28, 2024

Since bitcoin#30454 has been merged, further development in this repository no longer makes sense. Please consider moving this PR to the main repository.

@vasild
Copy link
Author

vasild commented Aug 28, 2024

Closing this because, for sure now the point here is not to merge it into hebasto/cmake-staging.

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.

4 participants