Issue with CMake FetchContent with vendored picojson and nlohmann json files

[sjanel]

What would you like to see added?

Configurable usage of picojson and nlohmann json

Additional Context

Hello,

Would it be possible to make usage of the two json external libraries optional by a CMake flag / config ?

I am using jwt-cpp as a FetchContent dependency in my project and also using myself nlohmann/json (a newer version) as a direct dependency. Hence I run into issues when I am including nlohmann/json.hpp, as it violates the ODR with two files in parts of the project.

If I have the time, I will work on a PR to solve this dependency issues.
Thanks

[Thalhammer]

There is JWT_DISABLE_PICOJSON to disable picojson support and JWT_EXTERNAL_PICOJSON to make it use a picojson install already available.

However from what I can tell the cmakelists shouldn't install the nlohmann json header at all, its only in the repo for the unit tests. Can you post a snippet of how you import it using fetch content ?

EDIT: I did a quick make & make install on the master branch and indeed the nlohmann header is not installed.

[prince-chrismc]

I suspect because the headers are cloned with using FetchContent and a git URL plus the target points to include so both versions are seen by different areas of the code 🤔

I would love to see a PR improving this use case!

[Thalhammer]

plus the target points to include

I might be wrong, but AFAIK FetchContent should set up the project as if you build and install the project and then adds the install path to the path where find_package looks for packages. TLDR: It shouldn't matter if you do fetchcontent or use a systemwide install.

[prince-chrismc]

https://cmake.org/cmake/help/latest/module/FetchContent.html#overview

FetchContent module makes content available immediately, allowing the configure step to use the content in commands like add_subdirectory(), include() or file() operations

You should get the whole source tree... it's a great sign we have two different understanding.

let's wait for OP to help us 💟

[sjanel]

I include it like this:

include(FetchContent)

FetchContent_Declare(
  jwt-cpp
  GIT_REPOSITORY https://github.com/Thalhammer/jwt-cpp
  GIT_TAG        v0.6.0
)

set(JWT_BUILD_EXAMPLES OFF CACHE BOOL "" FORCE)

FetchContent_MakeAvailable(jwt-cpp)

Code is here if you need more information.
And this PR does not build because I am updating nlohmann/json to 3.11 and it seems that it causes issues with the nlohmann/json of this repo which is v3.9

[Robostyle]

I ran into the same issue today by including the lib with FetchContent. The git clone is used directly according to the CMake documentation.

The CMakeLists.txt of this lib sets the include path to the internal include folder which contains a copy (v3.9) of nlohmann-json (and also picojson). If you have also an external nlohmann-json (I use v3.11.2, or picojson). The CMake generator generates two -I flags for the compiler.

e.g., gcc -I <my_nlohmann> -I <jwt-cpp-nlohmann>

Since jwt-cpp also contains a copy it is a bit of luck which is placed first, but it break things in case the jwt-cpp one is first. As @sjanel commented, it will violate the ODR.

[sjanel]

This commit seems to fix the issue, removing hardcoded nlohmann/json with a FetchContent. CMake seems to be smart enough to avoid include two times a same file, even with different versions. WDYT ?

@Robostyle can you check on your project if it fixes the issue as well?

[Robostyle]

That is quick @sjanel, I think it will fix my issue but I'm not sure this is how I would fix it, but that is me.

I can't comment on the pull request, but I wonder if we can't just move the nlohmann and picojson lib to an external folder and just add these to build-time only target_includes. Although, if you include it with FetchContent, I'm not sure if that is not also considered as build time.

Anyway, the fix seems ok, but it only solves one part, the other is that this also should be done for picojson. But that is a bit harder doing it your way since it also has some conditional-based decisions.

Also, there were some lengthy discussions on the CMake bug tracker about this. It seems that FetchContent is not building and thus not installed and this is a limitation of FetchContent. Otherwise, this was not even a problem. But still, a fix like yours would be nice.

I'm reconsidering using Hunter for my project and then this is not an issue since it will build/install.

[Thalhammer]

This commit seems to fix the issue

I don't think it fixes the issue, it merely shifts it, cause now you run into issues if someone already has another json install (thats not from fetchcontent or with a different name). I think the proper way to fix this would be to separate the include folder into two. One for the jwt part and one for optional dependencies. That way we can just set an option that disables the addition of the second folder.

Another idea worth considering would be to drop support for "batteries included" and not ship any json lib at all. It is this way because I primarily used picojson for everything back when this was still a private project and thus it was convenient to have it ready, but thats no longer the case and most people seem to not use picojson anyway. We could preserve the old behaviour by having it fetch the newest version of picojson (similar to your pr) unless you set JWT_DISABLE_PICOJSON, but I wonder if thats even worth the effort.

@prince-chrismc Seems like you where right, fetchcontent does not build/install ahead of time, it only sets the binary dir for dependend projects so they dont conflict.Guess I'm just too used to hunter's behaviour.

[sjanel]

Normally, FetchContent_MakeAvailable does not add any source code if the package is installed and seen by find_package, so it does not bring duplicated code. More information here.

Also, when present several times, it's the top level FetchContent_MakeAvailable who wins and deploys first the dependency, in most cases this is what you want.

I also like the solution to just remove completely the provided json, IMO it's cleaner and the user of the library has more freedom about which json to use.

[prince-chrismc]

My full time job is pitching Conan lol it's better i learn about this stuff so I can answer "why should I use Conan instead of...."

I will look at the code above when I get back to my hotel 😄

[Thalhammer]

"why should I use Conan instead of...."

What do you mean "it being pitched by you" ain't enough ? 😄

[prince-chrismc]

I also like the solution to just remove completely the provided json,

I agree with this... thats how the installer works... I have not used fetchcontent in ages so I will need to try you changes to refresh my memory.
👉 Feel free to open a PR, I am curious how the CI will react 🙈

Maybe it's old school but I still prefer find_package since it's more widely know with better integrations from package managers... idk how this will impact vcpkg or if it will require extra code 🤔

[Thalhammer]

Maybe it's old school but I still prefer find_package since it's more widely know

They are not really comparable though. find_package simply searches form a package somewhere. It wont go and get the dependency for you. FetchContent well fetches content and allows downloading the dependency directly inside cmake. Usually you end up using find_package to check if the dev lib is installed system wide and if its not use fetchcontent to pull it (assuming its a hard dependency).

[prince-chrismc]

They just fixed that in 3.24 😆 you can now have a "dependency" provider which and "fetch content" and override the find package behavior.

So if we can find_package the downstream project could either cmake --install nlohmann_json or FetchContent(nlohmann_json ... )