Clean CMake to not be compiler dependant, and allow users not to use compiler specific flags

[Lectem]

Hi,

It would be nice to clean the CMakeLists to follow modern cmake guidelines (there's a bunch of articles here https://github.com/onqtam/awesome-cmake, I also have a few tutorials here).

Setting CMAKE_CXX_FLAGS is not recommended as you are basicly forcing the user to use exactly the flags you put in it. For example, this is giving me issues because on my CI, coverage flags result in not being able to link the executable.

/home/vsts/work/1/s/pistache/lib/libpistached.a(http.cc.o): In function `_sub_I_00100_0':
/home/vsts/work/1/s/pistache-a54a4fab00252a9bcc42a38b62abf102f7de7cf6/build-Debug/../src/common/http.cc:1009: undefined reference to `__gcov_init'
/home/vsts/work/1/s/pistache/lib/libpistached.a(http.cc.o): In function `_sub_D_00100_1':
/home/vsts/work/1/s/pistache-a54a4fab00252a9bcc42a38b62abf102f7de7cf6/build-Debug/../src/common/http.cc:1009: undefined reference to `__gcov_exit'
/home/vsts/work/1/s/pistache/lib/libpistached.a(http.cc.o):(.data.rel+0x20): undefined reference to `__gcov_merge_add'
/home/vsts/work/1/s/pistache/lib/libpistached.a(http_defs.cc.o): In function `_sub_I_00100_0':
/home/vsts/work/1/s/pistache-a54a4fab00252a9bcc42a38b62abf102f7de7cf6/build-Debug/../src/common/http_defs.cc:177: undefined reference to `__gcov_init'
/home/vsts/work/1/s/pistache/lib/libpistached.a(http_defs.cc.o): In function `_sub_D_00100_1':
/home/vsts/work/1/s/pistache-a54a4fab00252a9bcc42a38b62abf102f7de7cf6/build-Debug/../src/common/http_defs.cc:177: undefined reference to `__gcov_exit'

[dennisjenkins75]

I am not a cmake expert (I still prefer hand-crafted, artisanal, makefiles). Feel free to submit a PR (or multiple PRs, each fixing one thing), and I am likely to merge them.

[kiplingw]

@Lectem, can you please let @Tachi107 and I know if his recently merged PR addresses your concerns?

[Lectem]

Hi,

sadly using CMAKE_CXX_FLAGS or add_compile_options (though the latter is better) doesn't change the main issue: the user can not control wether or not to use the flags.

For example https://github.com/pistacheio/pistache/blob/master/CMakeLists.txt#L22 won't work if the compiler doesn't support -ftest-coverage .

The LTO check is better now though, since you can manually change CMAKE_INTERPROCEDURAL_OPTIMIZATION

[Tachi107]

sadly using CMAKE_CXX_FLAGS or add_compile_options (though the latter is better) doesn't change the main issue: the user can not control wether or not to use the flags.

Yes that's true, but what if those flags are "needed" by Pistache (obviously flags like -Wall are not needed to compile the library, I'm talking about useful flags for debugging, CI, etc)? Maybe it is better to enable those flags only when defining Pistache-specific CMake options, for example

if(PISTACHE_DEV)
    add_compile_options(-Wall -ftest-coverage ...)
endif()

so that users wouldn't need to use any compiler-specific flags, only Pistache contributors.

This would require to change some things outside of CMakeLists.txt though, like telling the CI to define PISTACHE_DEV when running cmake, and I wanted my PR to cause as little trouble as possible, so I didn't do that.

I'm not an expert in Travis and stuff, @kiplingw @dennisjenkins75 do you think this is doable?

[dennisjenkins75]

Sadly, I have scant knowledge of cmake. I still create hand-crafted old-school makefiles for my personal projects. I defer to Kip and others that know more about cmake than I. I will merge any cmake related PR that Kip or Octal give the LGTM for.

[dennisjenkins75]

However, the "PISTACHE_DEV" suggestion above sounds reasonable to me.

[kiplingw]

@Tachi107, something else to keep in mind is that if the new build flags also require changes in flags used by a user who is linking against the library, those need to trickle down to the pkg-config manifest, otherwise it will break.

[Tachi107]

@kiplingw I was talking about only flags needed to work with Pistache, not the essential ones needed for the library to work. In my opinion every change should have no impact to end users interested only in using Pistache, and, as you say, nothing should break.

[kiplingw]

Sorry, do you mean flags necessary to build the library, or those necessary to link against the binary?

[Tachi107]

I mean these flags: CMakeLists.txt#L16, CMakeLists.txt#L22. Required libraries are already compiler independent (like src/CMakeLists.txt#L44), and the only place where they are specified using compiler dependent flags is when defining pkg-config data (like in CMakeLists.txt#L137), and I think that that's fine.