Generated nlohmann_jsonConfig.cmake does not set JSON_INCLUDE_DIR

[matthewOConnell]

Looks like CMake refactoring in f434942 clipped setting JSON_INCLUDE_DIR. This breaks find_package(nlohmann_json) and finding the correct header location.

[dan-42]

Hi @matthewOConnell can you explain more by what you mean does not set JSON_INCLUDE_DIR?
if you really need this information you can access it by

get_target_property(JSON_INCLUDE_DIRS nlohmann_json INTERFACE_INCLUDE_DIRECTORIES)

all needed information is exposed to the target nlohmann_json
there is no need manually calling include_directories(${JSON_INCLUDE_DIRS}) or
target_include_directories(main ${JSON_INCLUDE_DIRS})

by calling target_link_libraries(main nlohmann_json) all needed include paths are set

## note the PATHS is only needed because for testing I did not install it in my system!
find_package(nlohmann_json CONFIG REQUIRED PATHS ../install/lib/cmake) 

add_executable(main main.cpp)
target_link_libraries(main nlohmann_json)

But yes, the refactored CMake does set the include so that the user needs to call #include <nlohmann/json.hpp> with respect to be consistent with the namespace nlohmann::json

I hope that helps.

And If it is needed, we can add a CMake-option to use either
#include <nlohmann/json.hpp>
or
#include <json.hpp>

cheers Daniel

[matthewOConnell]

Sounds like we're just getting down to preference, and probably my lack of understanding on "modern" CMake patterns. This is the first I've run into a find_package() configuration that didn't set include and lib variables. And I admit I'm not sure how target_link_libraries(main nlohmann_json) is superior to target_include_directories(main ${JSON_INCLUDE_DIRS}) especially given how typical the target_include_directories(main ${JSON_INCLUDE_DIRS}) pattern is. (And especially for a header only library - it seems a misnomer to say "link libraries"). Maybe what you've shown is more stylistic to newer cmake and I just need retraining.

I actually agree with changing #include <json.hpp> to #include <nlohmann/json.hpp> the only reason I put it back to #include <json.hpp> was to keep supporting installs that already rely on the originally implemented include location. I would be easily convinced that not polluting the include path is worth a breaking change.

In general build systems are a giant pain, as shown by the two of us discussing intricacies of CMake style for a header-only tool. But when in doubt I lean towards not violating the law of least astonishment.

[dan-42]

No, thats not down to preferences, there are a lot of good reaons to use target_link_libraries() nowdays and there are also a couple good reasons why the target_include_directories has some draw backs.

Why not to use target_include_directories()

• variable ${JSON_INCLUDE_DIRS} is like a C-Style global, which pollutes the namespace. Exspecially when you look at that name. If a user wants to try/benchmark/use differenet C++ Json-libs, how does the user select between the nlohmann/json ann jsoncpp and etc....

• How are library specific compiler flags handled? does the user need to read the whole documentation and figure -std=c++11 is missing

Why use target_link_libraries()

• is a standard way to bind any library to your target, either shared, static or header only
• also triggers/adds linker-flags, compiler-flags and defines needed by the librarie. (not needed/used by nlohmann_json)
• clear and understandable, no misspelling of target_include_directories(man ${JSON_INCLUDE_DIRS}) or target_include_directories(man ${Json_INCLUDE_DIRS}) or was it target_include_directories(man ${NLOHAMNN_JSON_INCLUDE_DIRS})
• can pull in other needed dependancies, like a boost-header-only libs (not used by nlohmann_json)

The changes for using #include <nlohmann/json.hpp> and using target_link_libraries is IMHO ok as this will be in Version V3.0.0 which breaks API as well.
This also embraces the commen and modern way of using CMake

So yes some users might be surprised at first, but so where they when they read first time auto in c++ code.

[nlohmann]

I have no opinion on this as I do not use Cmake for this and cannot judge which way is better.

[nlohmann]

Any opinions on this?

[theodelrieu]

I agree that using target_link_libraries is clearer and less error prone. I would go with that option personally.

[nlohmann]

I tend to close this issue and #697 and leave the code as is unless I oversaw something.