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
547 stars 214 forks source link

CodeCoverage: Use VERBATIM for target commands #44

Closed ferdnyc closed 4 years ago

ferdnyc commented 4 years ago

@lieser raised a remaining issue in #28 with the coverage excludes, specifically that special characters (wildcards) aren't making it to the coverage commands because they're not properly quoted/escaped.

This PR adds VERBATIM to theadd_custom_target() call for all three coverage modes, ensuring that CMake will protect all of the arguments on the COMMAND lines used to execute the tools.

Fixes: #28

disengaged commented 4 years ago

+1

I have also had problems with characters not being escaped. I fixed it by adding the character " (as \") when appending excludes to the list internally, but this appears to work for my case and seems like a better solution.

lieser commented 4 years ago

Thanks a lot for the quick fix. Works as expected now with lcov.

Unfortunately the change causes problems for gcovr. Previously the following worked:

setup_target_for_coverage_gcovr_html( NAME coverage EXECUTABLE ctest EXCLUDE "deps/" ".*/tests/" )

With the change the non-direct test subdirectory is no longer excluded. Looking with VERBOSE=1 at the call to gcovr, the extra escaping of * seems to cause problems now that the arguments are quoted:

/usr/bin/gcovr --html --html-details -r /builds/libs/cryptod -e /builds/libs/cryptod/deps -e "/builds/libs/cryptod/.\\*/tests" --object-directory=/builds/libs/cryptod -o coverage/index.html

Removing the string(REPLACE "*" "\\*" EXCLUDE_REPLACED ${EXCLUDE}) line (and changing in the following line EXCLUDE_REPLACED to EXCLUDE) fixes this.

Did not test it, but I assume setup_target_for_coverage_gcovr_xml has the same problem.

ferdnyc commented 4 years ago

Thanks a lot for the quick fix. Works as expected now with lcov.

Unfortunately the change causes problems for gcovr.

Ah, yeah, I didn't have a working gcovr install to test with, I hadn't realized it was doing manual escaping. I'll pull that out, since it makes way more sense to let CMake do it. Thanks!

ferdnyc commented 4 years ago

Done.

Hmm. That FOREACH loop can probably be replaced with a generator expression.

(It rewites the excludes list before passing it as a series of command line args to gcovr:)

At least, a generator expression would work in new enough CMake. I'll have to check how long they've been supported in add_custom_target() COMMAND lines. (Then again, there really isn't any problem with the current looped implementation either.)

ferdnyc commented 4 years ago

At least, a generator expression would work in new enough CMake. I'll have to check how long they've been supported in add_custom_target() COMMAND lines.

Heh. Seems generator expressions have always been supported (at least in CMake 3+), buuut it wasn't until CMake 3.8 that they added a flag needed to properly support this exact use case:

COMMAND_EXPAND_LISTS Lists in COMMAND arguments will be expanded, including those created with generator expressions, allowing COMMAND arguments such as ${CC} "-I$<JOIN:$<TARGET_PROPERTY:foo,INCLUDE_DIRECTORIES>,;-I>" foo.cc to be properly expanded.

bilke commented 4 years ago

@ferdnyc If you like you can go ahead with the generator expression implementation. 3.8 is fine as a requirement, it is already 3 years old...

ferdnyc commented 4 years ago

@ferdnyc If you like you can go ahead with the generator expression implementation. 3.8 is fine as a requirement, it is already 3 years old...

Nah, I don't see any real advantage TBH, the syntax is pretty impenetrable for no real benefit. And selfishly, for my own purposes I need the module to support CMake 3.2+. :wink: