search

bilke / cmake-modules

Additional CMake functionality. Most of the modules are from Ryan Pavlik (https://github.com/rpavlik/cmake-modules)
Boost Software License 1.0
542 stars 215 forks source link

More flexible way to specify excludes when using 'setup_target_for_coverage_lcov' #54

Open AlessandroGiusa opened 3 years ago

AlessandroGiusa commented 3 years ago

Hello,

thank you for providing great cmake modules!

I had the problem when using setup_target_for_coverage_lcov such that the excludes where always based on the base path. I was getting coverage results from v1 c++ libs as well. I modified the function a little, that it could just forward the excludes as is to the lcov tool, since the tool already is capable to handle pattern based excludes.

set(LCOV_EXCLUDES "") foreach(EXCLUDE ${Coverage_EXCLUDE} ${COVERAGE_EXCLUDES} ${COVERAGE_LCOV_EXCLUDES}) list(APPEND LCOV_EXCLUDES "${EXCLUDE}") endforeach() list(REMOVE_DUPLICATES LCOV_EXCLUDES)

I just removed the part where the path gets calculated.

For me that way the problem is solved. Maybe you have a better idea how to do it. I wanted to just mention the issue.

Thank you and best regards. Alessandro

ferdnyc commented 3 years ago

@AlessandroGiusa How are you specifying your excludes? If you use absolute paths, they shouldn't be modified by the function.

TBH I always use LCOV_ARGS "--no-external", and only use EXCLUDE for paths within the project. Those I specify either relative to the source dir, or as ${CMAKE_CURRENT_BINARY_DIR}/... absolute paths). So, I haven't experimented a lot with excludes that aren't relative to the source dir.

I originally added the code to convert relative paths to absolute in #39, because at the time only absolute paths worked -- any relative paths you specified would simply be ignored, which was a huge problem for a lot of users and a major source of confusion / bug reports.

But that was before I discovered that adding VERBATIM to the add_custom_command calls cleared a lot of the argument-passing issues up, and enabled correct processing of lcov patterns. (See https://github.com/bilke/cmake-modules/issues/28#issuecomment-586446277.)

With VERBATIM added in, I have a feeling it's probably the case that absolute-path conversion is no longer necessary to make lcov excludes work, and we can just trust lcov to process its arguments as-is. I'll test that out, and file a PR to drop the get_filename_component() calls if they're no longer necessary. I have another change I want to submit, anyway.

ferdnyc commented 3 years ago

@AlessandroGiusa Hmmm.

My only issue with this is that, if the BASE_DIR path isn't prepended to the exclude args, paths still can't be excluded by relative path from the BASE_DIR... or anywhere else, it seems.

IOW, if I have a subdirectory data in my source dir that contained some source files that shouldn't be part of the coverage report, and I want to exclude those files, lcov won't accept "data/*" or ./data/* as the exclude pattern — it has to be "${CMAKE_SOURCE_DIR}/data/*", or the files won't be excluded.

Sure, I could pass */data/* and that will work, but that's not the same thing at all. (It's certainly conceivable that there could be a different path containing .../data/... that shouldn't be excluded.)

Lcov's "patterns-only" exclusion processing just feels kind of limiting, and could lead to erroneous matches if we expect everyone to start using "*/.../*" patterns in order to specify directories without absolute paths. So, I'm not sure what route to go here.