Merge lp:~allanlesage/libcolumbus/enable-coverage-option into lp:libcolumbus

[Allan LeSage]

Demonstrate use of our standard coverage-report-generating CMake files for Jussi's review, specifically using the coverage as an option (instead of build type).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:467
http://jenkins.qa.ubuntu.com/job/libcolumbus-ci/37/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libcolumbus-utopic-amd64-ci/1
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libcolumbus-utopic-armhf-ci/1
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libcolumbus-utopic-i386-ci/1

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/libcolumbus-ci/37/rebuild

review: Approve (continuous-integration)

[Jussi Pakkanen (jpakkane)]

Looking good. A few things to note:

This sets --coverage to CMAKE_CXX_FLAGS but line 108 has SET(CMAKE_CXX_FLAGS_COVERAGE ${CMAKE_CXX_FLAGS_DEBUG} PARENT_SCOPE). The latter is probably leftovers from the old way?

Currently the flags are set at the top level CMakeLists.txt. It would be better if this was moved into the coverage module itself. Thus the only thing you would need to do to add coverage to a new target is this:

if(enable_coverage)
 include(cmake/EnableCoverageReport.cmake)
 ENABLE_COVERAGE_REPORT(TARGETS ${COL_LIB_BASENAME} FILTER /usr/include ${CMAKE_SOURCE_DIR}/tests/* {CMAKE_BINARY_DIR}/*)
endif()

If we want this to be generally usable then the coverage macro should only add coverage flags to those languages that are enabled. Something along the lines of:

get_property(GLOBAL langlist LANGUAGES)
list(FIND langlist C index)
if(NOT ${index] EQUAL -1)
  set flags for plain c
endif()
... then the same for C++ etc

review: Needs Fixing

[lp:~allanlesage/libcolumbus/enable-coverage-option]

468. By Allan LeSage on 2014-05-19

Remove duplicate setting of linker and compilation flags for coverage.

[Allan LeSage (allanlesage)]

Removed those extraneous settings of link and compilation flags for coverage. Jussi I see the wisdom of your second point re: adding flags only for affected languages--what do you think of l. 113-14, where we're just adding the flags to COMPILE_FLAGS and LINK_FLAGS, presumably this adequately serves the need?

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:468
http://jenkins.qa.ubuntu.com/job/libcolumbus-ci/38/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libcolumbus-utopic-amd64-ci/2
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libcolumbus-utopic-armhf-ci/2
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libcolumbus-utopic-i386-ci/2

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/libcolumbus-ci/38/rebuild

review: Approve (continuous-integration)

[Jussi Pakkanen (jpakkane)]

Adding compiler flags via the target property is good.

There is the issue, though, that the code is lgpl3, which might mean we can't put it in our projects without a CLA. In addition it prevents submitting this upstream as CMake is BSD licensed.

review: Needs Information

[Allan LeSage (allanlesage)]

In Malta we've packaged a cmake-extras set of modules which includes the coverage file, this should satisfy our licensing needs, will update when that has landed.

[lp:~allanlesage/libcolumbus/enable-coverage-option]

469. By Allan LeSage on 2014-06-04

Switch to installed EnableCoverageReport macro from cmake-extras.

[Allan LeSage (allanlesage)]

Now that cmake-extras has landed in utopic universe, this MP becomes much simpler :) --I suppose the last thing is to decide whether or not to debian/control include cmake-extras or to ignore the error of not being able to import EnableCoverageReport?

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:469
http://jenkins.qa.ubuntu.com/job/libcolumbus-ci/39/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libcolumbus-utopic-amd64-ci/3
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libcolumbus-utopic-armhf-ci/3
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libcolumbus-utopic-i386-ci/3

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/libcolumbus-ci/39/rebuild

review: Approve (continuous-integration)

[Jussi Pakkanen (jpakkane)]

AFAIK we can't build-dep on cmake-extras as it is not in main (yet). We have two build types we need to support:

- regular package builds without coverage
- coverage builds that don't produce packages (or do they? I don't really know)

Not having cmake-extras in build-deps is good for the first case as it minimizes dependencies. But that would mean that the environment setup of coverage builds needs an extra hook to install it before building. Maybe it already has this? It does need gcov + genhtml installed at least and they are not in build-deps.

As far as the CMake side is concerned, I recommend doing this in all our packages:

option(enable_coverage "Build with coverage flags." OFF)
if(enable_coverage)
 include(coverage file)
 ENABLE_COVERAGE_REPORT(TARGETS your targets here)
endif()

Thus by default it is off and does not require cmake-extras, but if you enable it manually, it is loaded and will produce an error if missing.

[Allan LeSage (allanlesage)]

Ok I think we'll leave as-is, then :) --thanks Jussi for your reviews.

[Allan LeSage (allanlesage)]

Bump!

[Jussi Pakkanen (jpakkane)]

If we want to merge this then debian/control needs to add cmake-extras to the build deps. But more importantly cmake-extras needs to get into main, as libcolumbus may not build-depend on packages in universe.

review: Needs Fixing

[Jussi Pakkanen (jpakkane)]

Or is this handled by the coverage build environment already?

[Allan LeSage (allanlesage)]

I'll add a note to the readme.txt, need to verify with an utopic build--but yes the -Denable_coverage=ON option *should* work. I recall discussing this in Malta however briefly: I'd be happy to pursue getting cmake-extras into main but I think the optional nature of this option makes it optional, no? Lastly, do we want to call this 'coverage'? (As opposed to 'enable_coverage'?)

[Jussi Pakkanen (jpakkane)]

This will work if cmake-extras is installed in the system doing the build. In pbuilder and the like this won't be the case because only the packages listed in build dependencies are installed. Someone needs to special case that package in (as it can't be in build deps for the abovementioned reasons).

I don't have an opinion on the name, really. Either one is fine.

[lp:~allanlesage/libcolumbus/enable-coverage-option]

470. By Allan LeSage on 2014-07-23

Explain coverage in readme.txt.

[Allan LeSage (allanlesage)]

Sorry for the delay in replying.

Jussi you mention the special case of installing cmake-extras for testing builds--indeed this is handled by the Jenkins builders presently (and we'll have to carry it forward to the ci-airline system).

I've updated the readme, please review for style :) .

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:470
http://jenkins.qa.ubuntu.com/job/libcolumbus-ci/40/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libcolumbus-utopic-amd64-ci/4
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libcolumbus-utopic-armhf-ci/4
    SUCCESS: http://jenkins.qa.ubuntu.com/job/libcolumbus-utopic-i386-ci/4

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/libcolumbus-ci/40/rebuild

review: Approve (continuous-integration)

[Jussi Pakkanen (jpakkane)]

Looks good to me.

review: Approve

Unmerged revisions

470. By Allan LeSage on 2014-07-23

Explain coverage in readme.txt.

469. By Allan LeSage on 2014-06-04

Switch to installed EnableCoverageReport macro from cmake-extras.

468. By Allan LeSage on 2014-05-19

Remove duplicate setting of linker and compilation flags for coverage.

467. By Allan LeSage on 2014-05-08

Oops remove existing version of coverage.cmake.

466. By Allan LeSage on 2014-05-08

Demonstrate coverage option with standard tooling for Jussi's review.

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2014-03-11 14:02:22 +0000
+++ CMakeLists.txt	2014-07-23 01:12:43 +0000
@@ -11,6 +11,7 @@
 option(debug_messages "Print debug messages.")
 option(full_unicode "Enable full Unicode support (takes lots of memory).")
 option(use_python2 "Build Python bindings against Python 2 (UNSUPPORTED)." OFF)
+option(enable_coverage "Add coverage targets.")
 
 if(use_python2)
   message(WARNING "Python 2 bindings are NOT SUPPORTED! If they break, you get to keep both pieces.")
@@ -19,7 +20,6 @@
 include(FindPkgConfig)
 include(cmake/pch.cmake)
 include(cmake/python.cmake)
-include(cmake/coverage.cmake)
 
 set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wall -pedantic -Wextra")
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -pedantic -Wextra")
@@ -123,6 +123,11 @@
   add_subdirectory(python)
 endif()
 
+if(enable_coverage)
+  include(EnableCoverageReport)
+  ENABLE_COVERAGE_REPORT(TARGETS ${COL_LIB_BASENAME} FILTER /usr/include ${CMAKE_SOURCE_DIR}/tests/* ${CMAKE_BINARY_DIR}/*)
+endif()
+
 if(${enable_tests})
   enable_testing()
   add_subdirectory(test)

=== removed file 'cmake/coverage.cmake'
--- cmake/coverage.cmake	2013-02-15 15:25:59 +0000
+++ cmake/coverage.cmake	1970-01-01 00:00:00 +0000
@@ -1,34 +0,0 @@
-if (CMAKE_BUILD_TYPE MATCHES coverage)
-  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} --coverage")
-  set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} --coverage")
-  set(CMAKE_MODULE_LINKER_FLAGS "${CMAKE_MODULE_LINKER_FLAGS} --coverage")
-  set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} --coverage")
-
-  find_program(GCOVR_EXECUTABLE gcovr HINTS ${GCOVR_ROOT} "${GCOVR_ROOT}/bin")
-  if (NOT GCOVR_EXECUTABLE)
-    message(STATUS "Gcovr binary was not found, can not generate XML coverage info.")
-  else ()
-    message(STATUS "Gcovr found, can generate XML coverage info.")
-    add_custom_target (coverage-xml
-      WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
-      COMMAND "${GCOVR_EXECUTABLE}" --exclude="test.*" -x -r "${CMAKE_SOURCE_DIR}" 
-      --object-directory=${CMAKE_BINARY_DIR} -o coverage.xml)
-  endif()
-
-  find_program(LCOV_EXECUTABLE lcov HINTS ${LCOV_ROOT} "${GCOVR_ROOT}/bin")
-  find_program(GENHTML_EXECUTABLE genhtml HINTS ${GENHTML_ROOT})
-  if (NOT LCOV_EXECUTABLE)
-    message(STATUS "Lcov binary was not found, can not generate HTML coverage info.")
-  else ()
-    if(NOT GENHTML_EXECUTABLE)
-      message(STATUS "Genthml binary not found, can not generate HTML coverage info.")
-    else()
-      message(STATUS "Lcov and genhtml found, can generate HTML coverage info.")
-      add_custom_target (coverage-html
-        WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
-        COMMAND "${LCOV_EXECUTABLE}" --directory ${CMAKE_BINARY_DIR} --capture --output-file coverage.info --no-checksum
-        COMMAND "${GENHTML_EXECUTABLE}" --prefix ${CMAKE_BINARY_DIR} --output-directory coveragereport --title "Code Coverage" --legend --show-details coverage.info
-        )
-    endif()
-  endif()
-endif()

=== modified file 'readme.txt'
--- readme.txt	2012-05-23 07:57:25 +0000
+++ readme.txt	2014-07-23 01:12:43 +0000
@@ -1,6 +1,6 @@
 Columbus - a library for fast error-tolerant searching
 
-(C) 2012 Canonical ltd
+(C) 2012, 2014 Canonical ltd
 
 
 Compiling
@@ -25,3 +25,14 @@
 There are additional scalability tests that need to be enabled separately,
 since they take quite a lot of time to run. They are enabled in the same way
 as regular tests.
+
+To view test coverage results, first apt-get cmake-extras (under ubuntu), 
+then make, enabling the coverage option, run the tests, and generate the 
+report:
+
+cd build
+cmake -Denable_coverage=ON ..
+make
+make test
+make coverage
+firefox coveragereport/index.html