-
Notifications
You must be signed in to change notification settings - Fork 129
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.
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
base: 29.x-knots
Are you sure you want to change the base?
Conversation
fa8fc63 to
7aa60c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend moving lines 5-17 to after the find_program() calls (after line 40) to ensure variables exist before use.
build the Coverage profile, run the tests and generate the lcov html
reports with:
cmake -S . -B ${PWD}/cmake-build-debug -DCMAKE_BUILD_TYPE=Coverage
cmake --build ${PWD}/cmake-build-debug
LCOV_OPTS="--ignore-errors inconsistent,gcov,source,format --base-directory ${PWD}/src --rc geninfo_auto_base=1" GENHTML_OPTS="--ignore-errors unsupported,inconsistent,count,category --filter missing" cmake -P ${PWD}/cmake-build-debug/Coverage.cmake
e4b821d to
842bf4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously genhtml incorrectly received LCOV_OPTS
Now that they are properly separated, you may want to document the behavior change to confirm no one was relying on passing genhtml options via LCOV_OPTS
Also may want to rename HTML_OPTS to GENHTML_OPTS throughout for consistency and clarity.
hi @kwsantiago yes I was thinking about that yesterday in the point you make about confirming no one was relying on passing LCOV_OPTS - haven't tried it yet but could solve it by falling back to LCOV_OPTS by default and then only overriding with HTML_OPTS if it's passed and that way no ones existing setups would be affected |
…option (not envvar), update doc ref
842bf4f to
b71b968
Compare
doc/developer-notes.md
Outdated
| ``` | ||
|
|
||
| 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"`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tested this?
From my understanding, --exclude boost is a lcov option not a genhtml option, so this example will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is an option for both lcov and genhtml - so I've used it as a reference, certainly tested it to filter out the boost entries in the coverage report, however, because there is currently no genhtml distribution in homebrew you need to install a custom binary which may or may not support the same options. In my case the version I'm using doesn't support the same options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be better to provide a reference in the docs for a compatible genhtml implementation that would officially "work" on macOs/windows (if it exists) that would render this OPTS overriding change irrelevant, but I don't know what that is or would look like currently
Summary
Introduce
HTML_OPTSforgenhtmlwith a sensible default: inheritLCOV_OPTSwhenHTML_OPTSis not provided. This removes duplication for shared settings while keeping a clear escape hatch forgenhtml-specific control.Background (previous behavior)
genhtml) always receivedLCOV_OPTS.--rcflags likebranch_coverage=1) but made it awkward to pass options that apply only togenhtml.What’s changing
HTML_OPTS, forgenhtml.HTML_OPTSis omitted, passLCOV_OPTStogenhtml(maintains the historical behavior and keeps shared config in sync).HTML_OPTSis provided, use it instead ofLCOV_OPTSforgenhtml.HTML_OPTSis explicitly set to an empty string, pass no extra options togenhtmland do not inheritLCOV_OPTS.Why this change
lcovandgenhtmlare separate tools with partially overlapping options.--rc), but some aregenhtml-only (e.g.,--title).genhtmlwhen needed.User-facing behavior
genhtmlreceivesLCOV_OPTSautomatically.genhtmlreceivesHTML_OPTSinstead ofLCOV_OPTS.genhtmlreceives no options; nothing is inherited.Examples
Implementation notes
cmake/script/CoverageInclude.cmake.in:HTML_OPTSwas defined (even if empty) before splitting.GENHTML_OPTStoHTML_OPTSwhen defined; otherwise fall back toLCOV_OPTS.GENHTML_COMMANDwithGENHTML_OPTS.Key snippet:
Backward compatibility
HTML_OPTSis omitted,genhtmlstill receivesLCOV_OPTS.HTML_OPTScontinue to behave as before.genhtmloptions via-DHTML_OPTS="".Documentation
doc/developer-notes.mdto describe the default inheritance, the explicit override, and the empty override, with examples using agenhtml-only flag (--title).Impact and risk
genhtml-specific customization and intentional minimalism.