Skip to content

split lcov and genhtml cmake opts for the Coverage profile #224

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.

Sign up for GitHub

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

Draft
wants to merge 9 commits into
base: 29.x-knots
Choose a base branch
from
+20 −1
Draft

split lcov and genhtml cmake opts for the Coverage profile #224

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7aa60c6
split rcov and genhtml cmake opts for the Coverage profile
tevio Oct 16, 2025
39f21c9
remove unnessecary duplicated set commands
tevio Oct 17, 2025
c115fc1
document the option
tevio Oct 19, 2025
e2d638f
remove duplicated lines and inline genhtml seperate_arguments
tevio Oct 19, 2025
b71b968
remove ENV overrides, shorten opt name, only allow setting via cmake …
tevio Oct 19, 2025
2c2d23e
make HTML_OPTS an override to LCOV_OPTS, for genhtml
tevio Oct 21, 2025
e3e917f
adapt commentary and make notes more concise
tevio Oct 24, 2025
97a898f
Default genhtml opts to LCOV_OPTS; respect explicitly empty HTML_OPTS
tevio Oct 24, 2025
b581b44
simplify logic for genhtml opt passing options, remove unnecessary guard
tevio Nov 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions cmake/script/CoverageInclude.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,19 @@ separate_arguments(LCOV_OPTS)
set(LCOV_COMMAND ${LCOV_EXECUTABLE} --gcov-tool ${CMAKE_CURRENT_LIST_DIR}/cov_tool_wrapper.sh ${LCOV_OPTS})

find_program(GENHTML_EXECUTABLE genhtml REQUIRED)
# If HTML_OPTS is not provided, implicitly pass LCOV_OPTS to genhtml.

# HTML_OPTS is optionally passed via -D flag.
# If HTML_OPTS is not provided at all, implicitly pass LCOV_OPTS to genhtml.
# If HTML_OPTS is provided but empty (e.g. -DHTML_OPTS=""), use an empty list
# and do NOT inherit LCOV_OPTS.
set(_HTML_OPTS_WAS_DEFINED FALSE)
Copy link

@kwsantiago kwsantiago Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why track _HTML_OPTS_WAS_DEFINED separately instead of checking DEFINED HTML_OPTS directly in line 37?

Copy link
Author

@tevio tevio Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwsantiago I've tried removing that and it doesn't cause an obvious issue but because separate_arguments(HTML_OPTS) (called earlier) creates/modifies the HTML_OPTS variable, if you checked if(DEFINED HTML_OPTS) after that, it would always be true, losing the ability to distinguish “user didn’t provide HTML_OPTS” from “user provided it (possibly empty)”. The _HTML_OPTS_WAS_DEFINED flag captured whether it was defined before separate_arguments() ran.

Copy link
Author

@tevio tevio Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is probably the cleanest way, setting the default to LCOV_OPTS, but I need to test:

 # HTML_OPTS is optionally passed via -D flag.
-# If HTML_OPTS is not provided at all, implicitly pass LCOV_OPTS to genhtml.
-# If HTML_OPTS is provided but empty (e.g. -DHTML_OPTS=""), use an empty list
-# and do NOT inherit LCOV_OPTS.
-set(_HTML_OPTS_WAS_DEFINED FALSE)
+# Default: inherit LCOV_OPTS. If HTML_OPTS is provided (even if empty), use it instead.
+set(GENHTML_OPTS ${LCOV_OPTS})
 if(DEFINED HTML_OPTS)
-  set(_HTML_OPTS_WAS_DEFINED TRUE)
-endif()
-separate_arguments(HTML_OPTS)
-if(_HTML_OPTS_WAS_DEFINED)
+  separate_arguments(HTML_OPTS)
   set(GENHTML_OPTS ${HTML_OPTS})
-else()
-  set(GENHTML_OPTS ${LCOV_OPTS})
 endif()
 set(GENHTML_COMMAND ${GENHTML_EXECUTABLE} --show-details ${GENHTML_OPTS})

Copy link
Author

@tevio tevio Nov 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwsantiago with regards to this change b581b44 I've tested this locally and for brevity just summarised the output here: tl;dr: each mode renders a different result on my machine and the only one that succeeds is when passing the specific HTML_OPTS (not empty)

The three modes:

1) pass specific HTML_OPTS

rm -rf build
cmake -S . -B build -DCMAKE_BUILD_TYPE=Coverage
cmake --build build

TMPDIR=/tmp LANG=en_US.UTF-8 LC_ALL=en_US.UTF-8 cmake -DLCOV_OPTS="--ignore-errors inconsistent,gcov,source,format --base-directory $(pwd -P)/src --rc geninfo_auto_base=1" -DHTML_OPTS="--ignore-errors unsupported,inconsistent,count,category --filter missing" -P build/Coverage.cmake

should complete and generate coverage report:

Overall coverage rate:
source files: 762
lines.......: 87.5% (101483 of 115966 lines)
functions...: 78.2% (33941 of 43392 functions)
Message summary:
200 warning messages:
category: 100
inconsistent: 100
1361 ignore messages:
category: 351
count: 2
inconsistent: 1007
unsupported: 1
➜ bitcoin-knots git:(rcov-and-genhtml)

2) pass empty HTML_OPTS

TMPDIR=/tmp LANG=en_US.UTF-8 LC_ALL=en_US.UTF-8 cmake -DLCOV_OPTS="--ignore-errors inconsistent,gcov,source,format --base-directory $(pwd -P)/src --rc geninfo_auto_base=1" -DHTML_OPTS="" -P build/Coverage.cmake

was previously untested but now fails with a corrupt trace error:

Reading tracefile test_bitcoin_coverage.info.
genhtml: ERROR: (corrupt) unable to read trace file 'test_bitcoin_coverage.info': genhtml: ERROR: (inconsistent) "./bitcoin-knots/src/wallet/coincontrol.h":27: function '_ZN6wallet16PreselectedInputC1Ev' is hit but no contained lines are hit.
To skip consistency checks, see the 'check_data_consistency' section in man lcovrc(5).
(use "genhtml --ignore-errors inconsistent ..." to bypass this error)
(use "genhtml --ignore-errors corrupt ..." to bypass this error)
CMake Error at build/Coverage.cmake:46 (execute_process):
execute_process failed command indexes:
1: "Child return code: 1"

2) Do not pass HTML_OPTS (fall through to LCOV_OPTS)

TMPDIR=/tmp LANG=en_US.UTF-8 LC_ALL=en_US.UTF-8 cmake -DLCOV_OPTS="--ignore-errors inconsistent,gcov,source,format --base-directory $(pwd -P)/src --rc geninfo_auto_base=1" -P build/Coverage.cmake

should fail as it did, with unknown options:

lcov: WARNING: (count) max_message_count=100 reached for 'inconsistent' messages: no more will be reported.
To increase or decrease this limit use '--rc max_message_count=value'.
(use "lcov --ignore-errors count,count ..." to suppress this warning)
Combining tracefiles.
.. found 2 files to aggregate.
Combining tracefiles.
Merging baseline_filtered.info..1 remaining
Merging test_bitcoin_filtered.info..0 remaining
Writing data to test_bitcoin_coverage.info
Summary coverage rate:
source files: 762
lines.......: 69.4% (80524 of 115966 lines)
functions...: 70.1% (30439 of 43392 functions)
Message summary:
102 warning messages:
count: 1
inconsistent: 100
unsupported: 1
1544 ignore messages:
inconsistent: 1544
genhtml: WARNING: Unknown option: base-directory
Use genhtml --help to get usage information
CMake Error at build/Coverage.cmake:46 (execute_process):
execute_process failed command indexes: 1: "Child return code: 1"

if(DEFINED HTML_OPTS)
set(_HTML_OPTS_WAS_DEFINED TRUE)
endif()
separate_arguments(HTML_OPTS)
Copy link

@kwsantiago kwsantiago Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should separate_arguments(HTML_OPTS) be called after checking if it's defined to avoid modifying an undefined variable?

set(GENHTML_OPTS ${HTML_OPTS})
if(NOT GENHTML_OPTS)
if(_HTML_OPTS_WAS_DEFINED)
set(GENHTML_OPTS ${HTML_OPTS})
else()
set(GENHTML_OPTS ${LCOV_OPTS})
endif()
set(GENHTML_COMMAND ${GENHTML_EXECUTABLE} --show-details ${GENHTML_OPTS})
Expand Down
10 changes: 7 additions & 3 deletions doc/developer-notes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,11 +508,15 @@ enabled by setting `LCOV_OPTS="--rc branch_coverage=1"`:
cmake -DLCOV_OPTS="--rc branch_coverage=1" -P build/Coverage.cmake
```

HTML_OPTS can be specified to provide an options override to genhtml from the default LCOV_OPTS, the program that generates the
html report from lcov: `HTML_OPTS="--exclude boost"`.
HTML_OPTS can override the genhtml options (which default to LCOV_OPTS). If HTML_OPTS is omitted, LCOV_OPTS are implicitly passed to genhtml. If HTML_OPTS is provided but empty (e.g. -DHTML_OPTS=""), no options are passed to genhtml and LCOV_OPTS are not inherited.
Copy link

@kwsantiago kwsantiago Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May want to add an example showing combined use of both LCOV_OPTS and HTML_OPTS to clarify precedence.

Copy link

@kwsantiago kwsantiago Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if LCOV_OPTS is also not set? Does it default to empty?


Examples:
```
cmake -DHTML_OPTS="--exclude boost" -P build/Coverage.cmake
# Override genhtml options explicitly
cmake -DHTML_OPTS="--title 'Knots Coverage'" -P build/Coverage.cmake

# Provide an empty override: pass nothing to genhtml (do not inherit LCOV_OPTS)
cmake -DHTML_OPTS="" -P build/Coverage.cmake
```

To enable test parallelism:
Expand Down