Add codecov.io support

[rhaschke]

Further improvement of #91:

• Avoid debug build to speed up test runs
• Use lcov to facilitate filtering of relevant source files

I successfully tested this on MoveIt and rviz. From my point of view this is ready for merging.
@tylerjw, @davetcoleman: Please have a look.

[tylerjw]

As I commented over on the moveit PR that uses this, the codecov.io report it generates is from the wrong base directory and does not show a report on any of the source files in the moveit repo.

https://codecov.io/gh/ros-planning/moveit/tree/edf504374309899cc14da451a8f69688d936e1bf

I'm not sure why it isn't working but it is not generating a coverage report for the moveit repository instead it is only header files found in install/include (all packages, including dependencies) and some source files from the geometric_shapes project.

[tylerjw]

Also, I'm opposed to taking the shortcut of only running our tests with optimization. This seems like an oversight that diminishes the quality of CI reporting. If the optimizer optimizes out a section of code with a bug in it and CI passes that seems just as bad as not having CI only run with a debug build as you won't catch bugs introduced because the optimizer step did something unintended. This situation could cause a really frustrating situation where someone is using a debug build of the library to fix a bug in their code and instead has to debug issues introduced because CI never runs in Debug mode.

I think we should debug why ccache getting low hit rates instead. The point of coverage reporting is to make CI give more useful feedback to each PR, it is not just about putting a badge in the readme.

Lastly, I don't like that we are setting build options with environment variables inside the travis script. The problem with this is if someone builds and runs the tests without using this script they will get a different build. If we want to build our tests with specific build options those should be called out in the cmake files and not in this bash script. Specifically enabling warnings and setting warnings as errors should just be set as the default option in the cmake files for all builds so when someone is building while they are developing a feature they fix those issues before they submit their code to wait on CI. Ideally all build configuration for the project is done in the cmake files and the test environment is exactly the same as the environment used by someone developing against the library and developing for the library.

[rhaschke]

Sorry, but I don't agree with most of your writing:

1. The compiler doesn't "optimize out" bugs. If there is a bug, it will be in both, optimized and debugging versions. Usually, the opposite of what you claim is the case: issues observed in release code are not trackable in debugging mode (because of e.g. explicit variable initialization). Hence, it is more important to validate to-be-released code.
2. Setting global build options from commandline or environment is common best practice. I, even argue the other way around usually: A project should not impose specific build options - if not required - because it's the user's choice to configure optimization, warning levels, etc.
   By the way, enabling coverage in your solution also requires passing corresponding arguments to catkin.

I agree with you, though that we should debug the ccache performance drop in debug mode.
I already opened a ticket on Travis, because it looks like a bad interference of ccache and Travis. At least, locally, I get 100% hit rate also in debug mode.

[tylerjw]

I have seen a case where a whole block of code was optimized out because it was deemed to not matter by the optimizer with -O3 that had a runtime bug in it that was only found when running tests with a debug build. I will admit that that is much rarer than code that works under debug but doesn't perform as expected with optimizations.

[tylerjw]

I would argue that --coverage is a special case that only matters when run in CI and not normally to a user so that is ok to be added outside of the build specification. But someone writing a PR for moveit should get the same compiler warnings when building locally as would be produced in CI as that will slow them down not knowing about those warnings as soon as possible. That has nothing to do with this pr though so I'll drop it.

[tylerjw]

Passing configuration to catkin as arguments to cmake is different imho than setting build options via environment variables. I'm not arguing that you shouldn't have different build targets, just that configuration that applies build settings should be specified in the build system and not in a script that runs the build system.

At this point I will have to admit that I don't like catkin either and think that having a wrapper around cmake is silly.

[tylerjw]

I tested this locally and confirmed this is some issue with codecov.io as you reported in the other pr. Hopefully they can fix that soon. I also tested ccache and I can't reproduce the low cache hits locally :/ I think it has something to do with travis and I don't know yet how to debug that.

[rhaschke]

Someone writing a PR for moveit should get the same compiler warnings when building locally as would be produced in CI ... to inform about those warnings as soon as possible.

In principle I agree, but I propose another method to ensure that, namely asking developers to configure their development environment appropriately. @davetcoleman, we should discuss this on the next maintainer meeting.

[tylerjw]

I got yours to work with one small change... https://codecov.io/gh/ros-planning/moveit/pull/1787/tree

[tylerjw]

I tried to create a pull request against this PR by making one against your branch.
That didn't seem to work, so here it is: rhaschke#1

[tylerjw]

If you accept my small change I vote we submit this and debug the ccache issues in a new PR.

[rhaschke]

@tylerjw, thanks a lot for finally solving this issue.