Enhance cmake script

[dota17]

User @AMDmi3 put forward a requirement in issue #1170 , that is to use cmake to compile both static and shared libraries. After some investigation, I found that this is a very practical usage scenario. At the same time, I also know a common software usage scenario: In a large software system, the JSON parsing library(like JsonCpp) is the basic library of the system. Client may only need the xxx.o files of these basic libraries, and then link them into a shared library(yyy.so file) to use. Therefore, I made this patch.

During this time, I noticed the PR #827, I think the names of static libraries and dynamic libraries should be different. When users call them externally, they should be very clear about what type of libraries they are using, and a different names can guarantee this.
So, basically, the part of this patch is a rollback to that patch.

Cc: @cdunn2001 @hjmjohnson

[coveralls]

Coverage remained the same at 93.801% when pulling 7b8ec81 on dota17:enhance-cmake-script into c8453d3 on open-source-parsers:master.

[gizmocuz]

@dota17 , we are using this great library as a submodule in our project (https://github.com/domoticz/domoticz)
Since this change it seems only the shared libary is build, we would like to have the static library

[  1%] Building CXX object extern/jsoncpp/src/lib_json/CMakeFiles/jsoncpp_lib.dir/json_reader.cpp.o
[  1%] Building CXX object extern/jsoncpp/src/lib_json/CMakeFiles/jsoncpp_lib.dir/json_value.cpp.o
[  1%] Building CXX object extern/jsoncpp/src/lib_json/CMakeFiles/jsoncpp_lib.dir/json_writer.cpp.o
[  1%] Linking CXX shared library ../../../../lib/libjsoncpp.so
[  1%] Built target jsoncpp_lib

https://travis-ci.org/github/domoticz/domoticz/builds/707882330

I tried adding the following lines at the top of our CMakeList.txt file, but it did not make a difference:

option(BUILD_SHARED_LIBS "Build jsoncpp_lib as a shared library." OFF)
option(BUILD_STATIC_LIBS "Build jsoncpp_lib as a static library." ON)

Could you tell us what to add ?

Maybe the option names could use a slight name change like BUILD_JSONCPP_STATIC_LIBS ?

Thanks in advance!

[dota17]

@gizmocuz Thank you for your timely feedback! It seems that you only need the static library, that ok, but we don't need to change the option name, cause I find real problem, I will fix it today.

[gizmocuz]

@dota17 , thanks for your fast reply and fix ! Appreciated very much !!!

[gizmocuz]

@dota17 , unfortunately this change only builds the shared library not the static library

I also tried adding the following lines at the top of our CMakeList.txt file, but no luck....

option(BUILD_SHARED_LIBS "Build jsoncpp_lib as a shared library." OFF)
option(BUILD_STATIC_LIBS "Build jsoncpp_lib as a static library." ON)

There is a small discussion about it here:
domoticz/domoticz#4264

We are building from source following this tutorial:
https://www.domoticz.com/wiki/Build_Domoticz_from_source

[dota17]

This is strange, in my local compile, when I turned off BUILD_SHARED_LIBS, then it only have libjsoncpp.a.

I think you should forced to set the option BUILD_SHARED_LIBS off in the top of your CMakeList.txt file by using set, rather than add this line option(BUILD_SHARED_LIBS "Build jsoncpp_lib as a shared library." OFF) in your CMake file, cause I'm adraid it can't effect the inner CMakeList.txt setting.

[SpaceIm]

This PR will break projects relying on CMake imported target, which was jsoncpp_lib. Now we have two different imported targets: jsoncpp_lib and jsoncpp_static.
It's also worth noting that package managers like conan or vcpkg prefer build scripts building either static or shared, not both (they can handle this case, but it puts more pressure on maintainers), and specifically having the same imported target name for static and shared (avoid delegating shared/static logic in downstream dependencies, since it's usually final app decision to control shared/static of each dependency of the dependency graph).

[dota17]

This PR don't aim to let users use both of them at the same time, but provide these packages, as available choices.
conan or vcpkg can still pick their default kind of package without any change.

[SpaceIm]

Correct, but what about breaking change regarding CMake imported target?

[cdunn2001]

@SpaceIm , if you have an important fix, please submit it.

@dota17, of course we appreciate the help, but in general I avoid touching the cmake crap at all. I have found it nearly impossible to maintain with all the versions of cmake's history and all the crazy usage patterns. Cmake maintenance was far more time-consuming than the sum of all other issues. I concentrate on Meson, and I simply accept Cmake patches from users if they seem to know what they're doing.

I think the names of static libraries and dynamic libraries should be different. When users call them externally, they should be very clear about what type of libraries they are using, and a different names can guarantee this.

I disagree with this. In UNIX, it is normal to have something like /usr/lib/ with both static and shared libraries of the same name. The static library will not include versions, so that's a difference. A good way to select shared versions is to use pkg-config and *.pc files. (Meson actually generates those, in fact.) I haven't looked at your change closely because I honestly don't care about Cmake at all, not even slightly. But if the breaking change was to rename the library, I think that's a mistake. As for "imported targets", I have no idea.

[dota17]

Yes, it's normal to have the same name for static/shared library packages, it is easy to fix, I'll fix it later :-)

[SpaceIm]

@SpaceIm , if you have an important fix, please submit it.

The modification would be to revert this one, see this issue: #1276

[cdunn2001]

Please submit a PR with a revert, or work with the users on #1271 which has been merged.

We honestly have no opinion on cmake changes. But I can tell you that supporting cmake is a never-ending problem. Most issues relate to cmake. It's unbelievable.

If you submit a fix and the tests pass, we'll merge it, even if it's a revert.