CMake refactor

[sam-lunt]

Splitting off the CMake changes from the earlier pull request (#1505)

[codecov-io]

Codecov Report

Merging #1510 into master will increase coverage by 17.7%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1510      +/-   ##
==========================================
+ Coverage    5.92%   23.62%   +17.7%     
==========================================
  Files         162       49     -113     
  Lines        9037     2061    -6976     
==========================================
- Hits          535      487      -48     
+ Misses       8502     1574    -6928

Flag	Coverage Δ
#unittests	23.62% <ø> (+17.7%)	⬆️

Impacted Files	Coverage Δ
include/events/signal.hpp	0% <0%> (ø)	⬆️
include/events/signal_receiver.hpp	0% <0%> (ø)	⬆️
include/components/config.hpp	0% <0%> (ø)	⬆️
include/x11/connection.hpp	0% <0%> (ø)	⬆️
include/x11/extensions/xkb.hpp	0% <0%> (ø)	⬆️
include/events/types.hpp
src/modules/xbacklight.cpp
include/modules/temperature.hpp
include/modules/date.hpp
src/adapters/net.cpp
... and 115 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8e4a59...e98fe51. Read the comment docs.

[patrick96]

Thank you!

I feel like cmake is quite bloated in some places, this is a good step forward.

I just noticed that codecov now reports very high coverage because libpoly isn't compiled with any of the coverage flags.

How would you tackle this?

I thought about either adding a new build type Coverage that extends debug flags with those flags. Or adding those flags to libpoly in tests/CMakeLists.txt, if that's possible, regardless of build type.

The second one feels a bit weird to me because your executable starts producing coverage files when -DBUILD_TESTS=ON is used even if that was not really intended. The Coverage build type would also create those files, but there it's more intentional.

But maybe we just won't get around compiling libpoly separately for our tests.

[sam-lunt]

Good catch on the coverage. I agree, I think create a separate build type for coverage is the best idea.

I don't think there's any way to avoid compiling specially for coverage, since all the source files you want data for need to be compiled with the --coverage flag. However, I don't think that's too big an issue: you compile one way for debugging and another way to distribute, so compiling a third way for generating coverage data seems fair.

To enable coverage, I'm thinking I'd add this in the top level CMakeLists (or one of the files included from cmake, which would you recommend?):

set(CMAKE_CXX_FLAGS_COVERAGE "${CMAKE_CXX_FLAGS_DEBUG} ${CMAKE_CXX_FLAGS_COVERAGE} --coverage")

Then CMake could be invoked as

cmake /path/to/polybar -DCMAKE_BUILD_TYPE=Coverage

From there, it would be built as normal.

Couple of notes on the flag choices:

• Using CMAKE_CXX_FLAGS_DEBUG as the first argument allows the defaults from Debug builds to be used, as you had said
• Using CMAKE_CXX_FLAGS_COVERAGE as the second argument allows passing extra arguments for Coverage builds in the command line. Putting it after the debug flags allows overriding those flags (when flags conflict, the later one wins)
• --coverage is an option not a lot of people know about (I just learned it recently), but it's been in GCC for at least 10 years (that's based on the git blame of a GCC source file, so I might be off). It passes -fprofile-arcs -ftest-coverage when compiling and -lgcov when linking

[sam-lunt]

So looking through the files, it seems like cmake/01-core.cmake is the best place to add the coverage configuration (there's already one in there for Sanitize, actually).

I also had a couple of thoughts on potential changes, but they might alter the default build behavior, so I didn't want to jump right in, considering I don't have much experience with the project's current setup.

In `cmake/01-core.cmake':

• On line 30, the default flags add -O2, but normally in CMake individual build configurations (Release, Debug, etc) are responsible for setting the optimization level. I think that's the case now, in fact.
• If that's changed, we could also remove the explicit -O0 for debug builds (line 49)

In the top level CMakeLists:

• This one would likely change the build behavior for a lot of people, but I'd propose removing the EXCLUDE_FROM_ALL on line 50. It's already guarded by the BUILD_TESTS variable: I'd assume that if someone already defined BUILD_TESTS, then they probably want to build all the tests. Not a big deal, I just keep forgetting to specify the all_unit_tests target when building ; )

[sam-lunt]

Oh, also, I wasn't able to figure out how to change the .codecov.yml so that it uses the build type "Coverage". Do you know how to do that? (The documentation on the site wasn't clear)

[patrick96]

You don't have to change it in .codecov.yml, the coverage is generated in travis. So you can change it in .travis.yml, specifically, here.

[patrick96]

On line 30, the default flags add -O2, but normally in CMake individual build configurations (Release, Debug, etc) are responsible for setting the optimization level. I think that's the case now, in fact.

I agree, you can remove that. This shouldn't change the default behaviour since RelWithDebInfo is the default build type and its default optimization flag is -O2.

If that's changed, we could also remove the explicit -O0 for debug builds (line 49)

We could use -Og

This one would likely change the build behavior for a lot of people, but I'd propose removing the EXCLUDE_FROM_ALL on line 50. It's already guarded by the BUILD_TESTS variable: I'd assume that if someone already defined BUILD_TESTS, then they probably want to build all the tests. Not a big deal, I just keep forgetting to specify the all_unit_tests target when building ; )

The reasoning behind this was that as a dev one might configure with BUILD_TESTS by default so that we can run tests if we want to but we don't additionally have to build the tests when we just want to compile polybar. But since with this PR the overhead of compiling the tests is not that big, we can remove it too.

[sam-lunt]

I updated travis to use the coverage build. I also enabled building the tests as part of the "all" build.

we don't additionally have to build the tests when we just want to compile polybar.

You can also use make polybar or cmake --build . --target polybar to just build the executable component.

I removed -O2 from the default flags. I didn't add in -Og for Debug builds though, since I've found that's not always perfect. If someone is debugging something and sees a line get skipped that they didn't expect, they might not realize it's because Og is enabled. On the other hand, if someone does want to turn on Og, they can do so from the command line (this works since we removed O2 from the default flags and O0 from the debug flags)

cmake /path/to/polybar -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS="-Og"
cmake /path/to/polybar -DCMAKE_BUILD_TYPE=Debug -DCMAKE_CXX_FLAGS_DEBUG="-Og"

[sam-lunt]

@patrick96 I fixed the merge conflict (there was a unit test added for components/parser)

[patrick96]

Awesome. Everything seems to be in order. Thanks!

[patrick96]

You can now add the non-cmake commits from #1505 into a new PR :)

[sam-lunt]

Great, thanks! Just created a pull request : )