Cmake config improvements #1271

sergeyrachev · 2021-02-27T23:26:39Z

added CMake configuration template to improve find_package(JsonCpp) usage

CMake helper check_required_components() set necessary variable to let caller project detect result of package search.

cdunn2001 · 2021-03-06T07:32:10Z

Does anyone else have an opinion on this? I lean towards merging user-submitted cmake changes, as long as CI builds pass.

BillyDonahue

I don't understand it, but I have only minimal cmake experience.

BillyDonahue · 2021-03-06T07:36:19Z

CMakeLists.txt

@@ -177,11 +177,13 @@ if(JSONCPP_WITH_CMAKE_PACKAGE)
    include(CMakePackageConfigHelpers)
    install(EXPORT jsoncpp
        DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/jsoncpp
-        FILE        jsoncppConfig.cmake)
+        FILE        jsoncpp-targets.cmake)
+    configure_package_config_file(jsoncppConfig.cmake.in ${CMAKE_CURRENT_BINARY_DIR}/jsoncppConfig.cmake INSTALL_DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/jsoncpp)


Can we keep the lines narrower?
The rest of the file doesn't spread out quite as much as these new lines.

Made lines narrower.

BillyDonahue · 2021-03-06T07:47:30Z

jsoncppConfig.cmake.in

+include ( "${CMAKE_CURRENT_LIST_DIR}/jsoncpp-targets.cmake" )
+
+if(TARGET jsoncpp_static)
+    add_library(JsonCpp::JsonCpp ALIAS jsoncpp_static)


I wish I understood why JsonCpp appears twice in this name.
https://cmake.org/cmake/help/latest/command/add_library.html?highlight=add_library#id6

We're making JsonCpp::JsonCpp an alias for whatever the TARGET is. Why?

CMake 3.0 target namespaces introduce a limited naming scope for the targets to avoid name clashes when targets named the same from different project or libraries. CMake recommends to put import targets of the library into appropriate namespace and I would like to use NAMESPACE feature of install(EXPORT) command to implement that. But it would break an existing target importing rules as JsonCpp already exports jsoncpp_static and jsoncpp_lib targets.
Adding NAMESPACE directly to the install command would put them into namespace. An user would require to use JsonCpp::jsoncpp_static or JsonCpp::jsoncpp_lib to repair broken importing code.
To keep backward compatibility and improve a future user experience I conditionally add new ALIAS target that looks like namespaced target and may be used to import actual jsoncpp library that available.
I assume the library is used usually as a source code to compile into the another project or a system level dependency in the form of static or shared library. Object files are not redistributable.
Although, static and shared library can be available at same time in the one build environment, it is rare and very specific case, as I know. So I would like to export an available library as an alias to use JsonCpp::JsonCpp generic namespaced library target and let CMake does all magic to set necessary compiler and linker flags. In such case projects that use JsonCpp::JsonCpp will able to use static or shared library depends on availability in build environment.

JonasVautherin · 2021-04-02T19:10:52Z

@sergeyrachev: I was just trying it in my project, and I get the following error:

CMake Error at build/third_party/install/lib/cmake/jsoncpp/jsoncppConfig.cmake:33 (add_library):
  add_library cannot create ALIAS target "JsonCpp::JsonCpp" because target
  "jsoncpp_static" is imported but not globally visible.

Is that something I would be doing wrong on my side? The offending line is at:

if(TARGET jsoncpp_static)
    add_library(JsonCpp::JsonCpp ALIAS jsoncpp_static)

The way I use it is by calling find_package(jsoncpp REQUIRED) and then use JsonCpp::JsonCpp in target_link_libraries.

sergeyrachev · 2021-04-02T20:35:36Z

CMake <3.11 didn't allow to make ALIAS to any IMPORTED targets; Since v3.11 the restriction level has been lowered
to allow GLOBAL IMPORTED targets be aliased, since v3.18 any IMPORTED target may be aliased and
its GLOBAL property is inherited to ALIAS target.

https://cmake.org/cmake/help/v3.10/command/add_library.html#alias-libraries
https://cmake.org/cmake/help/v3.11/command/add_library.html#alias-libraries
https://cmake.org/cmake/help/v3.18/command/add_library.html#alias-libraries

c-ares library has similar issue and applied the same workaround.
c-ares/c-ares#104 fixed with c-ares/c-ares#108

…file generated from template to use automatic package resolving and resolution logic CMake provides helpers to generate config file. Generated config file has usefull macro check_required_components() to set necessary variables like PackageName_FOUND if requirements has been satisfied. An absence of dedicated config file confuses user project as necessary variables are not set.

When the static libary is available use it as exported alias, otherwise use shared library. Cmake takes care about import library when Windows platform DLL is used

… non-GLOBAL IMPORTED target

JonasVautherin

Works for me now! Thanks 🚀

EDIT: if that can help the maintainers, here is where I tested it. The CI there is building jsoncpp and linking it to the project on different platforms such as Linux (Ubuntu, Fedora, Alpine), Windows, macOS, iOS, Android, and then using jsoncpp in some integration tests 👍.

SpaceIm · 2021-04-14T11:42:19Z

jsoncppConfig.cmake.in

+if(TARGET jsoncpp_static)
+    add_library(JsonCpp::JsonCpp INTERFACE IMPORTED )
+    set_target_properties(JsonCpp::JsonCpp PROPERTIES INTERFACE_LINK_LIBRARIES "jsoncpp_static")
+elseif(TARGET jsoncpp_lib)
+    add_library(JsonCpp::JsonCpp INTERFACE IMPORTED )
+    set_target_properties(JsonCpp::JsonCpp PROPERTIES INTERFACE_LINK_LIBRARIES "jsoncpp_lib")
+endif()


I would put this logic in a file jsoncpp-namespaced-targets.cmake exported with install(EXPORT ...).
Moreover, for consistency with add_subdirectory(), JsonCpp::JsonCpp ALIAS target should be added in CMakeLists.

I would put this logic in a file jsoncpp-namespaced-targets.cmake exported with install(EXPORT ...).

You mean such that lines 6-7 above read like this?

include ( "${CMAKE_CURRENT_LIST_DIR}/jsoncpp-targets.cmake" ) include ( "${CMAKE_CURRENT_LIST_DIR}/jsoncpp-namespaced-targets.cmake" )

That's it 😃

Right. And about the ALIAS target, where would you do that?

In /src/lib_json/CMakeLists.txt close to add_library() calls. Unfortunately, it will probably requires some ugly branching because currently jsoncpp can build both shared & static, or shared only, or static only.

I would be glad to add ALIAS to CMakeLists to use the namespaced target in build tree also.
What is the usecase of having both static and shared build at same time? CMake allows to specify the library type during generation with -DBUILD_SHARED_LIBS=Off/On although it relies on CMakeLists.txt to not specify types explicitly.

@spacelm: Could we imagine fixing the include part of your comment in this PR, and leaving the ALIAS part for another PR? So that there could be a discussion about @sergeyrachev's point with which I agree: why not relying on BUILD_SHARED_LIBS instead? That makes it simpler IMO, and if you want both, you can run the build twice 🤔.

No problem for me to add ALIAS target in another PR, it would be an improvement.

Then, you can introduce a new breaking change in jsoncpp by removing again the ability to build both static & shared (AFAIK, it was removed for 1.9.0, then introduced again in 1.9.4...). Since I'm against build of both static & shared, I would be happy, but I'm not the maintainer.

sergeyrachev · 2021-04-14T19:15:40Z

@spacelm, a current solution is a workaround to not touch an original implementation that provides jsoncpp_static and/or jsoncpp_lib targets. Target name changing may break a backward compatibility for projects that uses JsonCpp in build-tree.

I am not aware how to create a custom cmake file with install(EXPORT ) call as you suggested. I made a separate file to include it into jsoncppConfig.cmake and I install it explicitly.

As I know install(EXPORTS) call creates a jsoncpp-targets.cmake and jsoncpp-targets-.cmake based on install(TARGETS ) with EXPORT directive. NAMESPACE directive for install(EXPORTS) prepends namespace to existing targets only.
To have JsonCpp::JsonCpp we need either to have a target named JsonCpp and export it with a namespace using install(EXPORT... NAMESPACE), or make an ALIAS target manually as I made.

Future improvements may be:

Make single library output with no explicit type to let user choose static or shared library to get.
Name output library as JsonCpp to make unificatiion with imported target and general CMake practices.
Export JsonCpp target with Namespace and define ALIAS target in build-tree to make unification between build-tree and imported library.

But any of them change or break a public contract 'how to use JsonCpp library'. Such changes may(should?) impact the version number.

JonasVautherin

Tested the latest changes again here 👍

JonasVautherin · 2021-04-17T23:31:12Z

@SpaceIm: Is it fine for you with the latest changes? If yes, would you mind approving, so that @cdunn2001 or @BillyDonahue can maybe merge it 🙂

JonasVautherin · 2021-04-27T22:10:09Z

Kindly pinging on that regard, I think it would be a very nice addition to jsoncpp, e.g. for package managers 😊.

cdunn2001 · 2021-05-05T04:34:21Z

Well, I think we should merge this and see if we get any complaints.

(creating merge-commit late, after accidental "rebase-and-merge")

target added in upstream config file in 1.9.5 (open-source-parsers/jsoncpp#1271)

BillyDonahue reviewed Mar 6, 2021

View reviewed changes

JonasVautherin mentioned this pull request Apr 2, 2021

Initial hunter support #1391 mavlink/MAVSDK#1393

Closed

sergeyrachev added 5 commits April 2, 2021 22:36

- declare namespaced export target to simplify the library usage

6a076e3

When the static libary is available use it as exported alias, otherwise use shared library. Cmake takes care about import library when Windows platform DLL is used

- empty line at end of file

cfb9120

- narrowed lines to be aligned with overall file line width

37ce902

- workaround for CMake < 3.18 ALIAS target limitation to not point to…

6f6613a

… non-GLOBAL IMPORTED target

sergeyrachev force-pushed the cmake-config-improvements branch from 16cf732 to 6f6613a Compare April 2, 2021 20:37

JonasVautherin mentioned this pull request Apr 2, 2021

Fix jsoncpp Werror with NDK r22b mavlink/MAVSDK#1394

Merged

JonasVautherin approved these changes Apr 2, 2021

View reviewed changes

SpaceIm reviewed Apr 14, 2021

View reviewed changes

- isolated namespace targets into separate file

816ef22

JonasVautherin approved these changes Apr 15, 2021

View reviewed changes

cdunn2001 merged commit 993e4e2 into open-source-parsers:master May 5, 2021

cdunn2001 added a commit that referenced this pull request May 5, 2021

Merge branch 'cmake-config-improvements' #1271

bb34617

(creating merge-commit late, after accidental "rebase-and-merge")

bazfp mentioned this pull request May 18, 2021

Make a New Release #1293

Closed

SpaceIm added a commit to SpaceIm/conan-center-index that referenced this pull request Sep 20, 2022

provide JsonCpp::JsonCpp target

255dcae

target added in upstream config file in 1.9.5 (open-source-parsers/jsoncpp#1271)

SpaceIm mentioned this pull request Sep 20, 2022

jsoncpp: also provide JsonCpp::JsonCpp target conan-io/conan-center-index#13027

Merged

4 tasks

conan-center-bot pushed a commit to conan-io/conan-center-index that referenced this pull request Sep 21, 2022

(#13027) jsoncpp: also provide JsonCpp::JsonCpp target

e2c75e7

target added in upstream config file in 1.9.5 (open-source-parsers/jsoncpp#1271)

sergeyrachev deleted the cmake-config-improvements branch July 5, 2024 18:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cmake config improvements #1271

Cmake config improvements #1271

sergeyrachev commented Feb 27, 2021

cdunn2001 commented Mar 6, 2021

BillyDonahue left a comment

BillyDonahue Mar 6, 2021

sergeyrachev Mar 6, 2021

BillyDonahue Mar 6, 2021

sergeyrachev Mar 6, 2021

JonasVautherin commented Apr 2, 2021

sergeyrachev commented Apr 2, 2021

JonasVautherin left a comment •

edited

Loading

SpaceIm Apr 14, 2021

JonasVautherin Apr 14, 2021

SpaceIm Apr 14, 2021

JonasVautherin Apr 14, 2021

SpaceIm Apr 14, 2021 •

edited

Loading

sergeyrachev Apr 14, 2021

JonasVautherin Apr 14, 2021

SpaceIm Apr 14, 2021 •

edited

Loading

sergeyrachev commented Apr 14, 2021

JonasVautherin left a comment

JonasVautherin commented Apr 17, 2021

JonasVautherin commented Apr 27, 2021

cdunn2001 commented May 5, 2021

Cmake config improvements #1271

Cmake config improvements #1271

Conversation

sergeyrachev commented Feb 27, 2021

cdunn2001 commented Mar 6, 2021

BillyDonahue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonasVautherin commented Apr 2, 2021

sergeyrachev commented Apr 2, 2021

JonasVautherin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm Apr 14, 2021 • edited Loading

Choose a reason for hiding this comment

sergeyrachev commented Apr 14, 2021

JonasVautherin left a comment

Choose a reason for hiding this comment

JonasVautherin commented Apr 17, 2021

JonasVautherin commented Apr 27, 2021

cdunn2001 commented May 5, 2021

JonasVautherin left a comment •

edited

Loading

SpaceIm Apr 14, 2021 •

edited

Loading

SpaceIm Apr 14, 2021 •

edited

Loading