search

ArthurSonzogni / nlohmann_json_cmake_fetchcontent

A lightweight Release-tracking repository for nlohmann/json. Suitable for CMake fetchcontent. Automatically upgraded every weeks.
MIT License
79 stars 25 forks source link

Make the imported target installable #7

Closed NicolasIRAGNE closed 2 years ago

NicolasIRAGNE commented 2 years ago

I am by no means a CMake expert so this is most likely incorrect but can provide a proof of concept of this change.

When I was trying to integrate this repository into my project, I bumped into a problem. I always try to provide two options for dependencies: try to find an existing package or download the dependency (so my code is roughly something like:

Find_Package(Foo)
if (NOT Foo-FOUND)
  Fetchcontent_Declare(...)
 endif()

and keep the rest of the CMake code the same.

This repository is very useful but is incompatible with this approach if the target that is linking against json is exported later.

Doing something like:

target_link_libraries(Foo nlohmann_json::nlohmann_json)
...
install(EXPORT Foo ...)

throws an error.

CMake Error in CMakeLists.txt:
  export called with target "Foo" which requires target "nlohmann_json"
  that is not in any export set.

By using the generator expressions for the include directories, we do not rely on the path where the content was downloaded and instead where it is installed. This allows a proper installation.

Adding the package (copied from the original repo) then allows a relocatable installation and provides a cmake file to help integrating it. I am not sure if this is the correct way to do it, but I did not yet succeed with making it work without this.

NicolasIRAGNE commented 2 years ago

Alright I think I have to admit: I have no idea what I'm doing. I'll set this as a draft until I can figure out how any of this works.

I am doing a lot of testing (trial and error and pulling my hair), I will update this if I can make something work.

NicolasIRAGNE commented 2 years ago

Alright, I think I got something to work. Transitive dependencies are not something I was used to. I will try to make a simple proof-of-concept repo to demonstrate when I have the time. Sorry for the overall confusion.

nlohmann commented 2 years ago

I am not a CMake expert either, and I personally always use FetchContent with the main repository nlohmann/json.

NicolasIRAGNE commented 2 years ago

Alright, after a bit of tinkering I made this repository: https://github.com/NicolasIRAGNE/CMakePackageWithTransitiveDependencies

It shows the use case I currently have with nlohmann_json.

Theses changes allow this configuration: image

and simply having:

find_package(LibB REQUIRED)

add_executable(MainExe src/main.cpp)
target_link_libraries(MainExe LibB::LibB)

Because installing LibB also installs nlohmann_json and knows how to find it. As for the JSON_Install variable, it could be implemented. However, that would mean that linking to LibB will require specifying the path to nlohmann_json like you would usually do (CMAKE_PREFIX_PATH or nlohmann_json_DIR, I think.)

EDIT: Actually, no. Setting JSON_Install to False would throw the following error:

[cmake] CMake Error: install(EXPORT "LibBTargets" ...) includes target "LibB" which requires target "nlohmann_json" that is not in any export set.

I am not sure what the proper methodology would be here. Maybe it could be handled on LibB side?

Anyhow, I think these changes are relevant still. I do think it is far from being perfect, but they can make integration smoother with other projects. Feel free to tell me what you think.

melroy89 commented 2 years ago

This PR is actually interesting for me as well. I want to install nlohmann json as well. Currently using it as an external dependency in one of my CMake projects for IPFS: https://github.com/vasild/cpp-ipfs-http-client/blob/master/CMakeLists.txt#L60

Currently causing issues when the IPFS Client is installed rather than static compiled:

/usr/local/include/ipfs/client.h:26:10: fatal error: nlohmann/json.hpp: No such file or directory
   26 | #include <nlohmann/json.hpp>
      |          ^~~~~~~~~~~~~~~~~~~

EDIT: Ow, well. I think I already solved it!?

I just do the following in my CMakeLists.txt file:

install(FILES ${json_SOURCE_DIR}/include/nlohmann/json.hpp DESTINATION include/nlohmann)
ArthurSonzogni commented 2 years ago

Since both of you needs it, I will merge this PR. Thank you!

I tried it locally and reviewed it. I checked it make sense and was indeed copy-pasting from parts from the nlohmann's repository. I made some very minor tweaks.

melroy89 commented 2 years ago

Just to confirm if I'm understand it correctly.

Can I now remove this from my CMakeLists.txt file:

install(FILES ${json_SOURCE_DIR}/include/nlohmann/json.hpp DESTINATION include/nlohmann)

And will nlohmann json (header files) automatically installed if I run: make install? (of course assuming FetchContent_Declare(), etc etc. is done)

ArthurSonzogni commented 2 years ago

Yes the headers will be installed by default. It will behave the same way as the original repository. The goal is to be interchangeable. The original repository may want to add some option to help folks to decide what to do.

Please note that CMakeFetchContent have the option "EXCLUDE_FROM_ALL", allowing you to decide independently from the dependency.

So in your case, you probably want to switch from:

FetchContent_GetProperties(json)
if(NOT json_POPULATED)
  FetchContent_Populate(json)
  add_subdirectory(${json_SOURCE_DIR} ${json_BINARY_DIR} EXCLUDE_FROM_ALL)
endif()

to 

FetchContent_GetProperties(json)
if(NOT json_POPULATED)
  FetchContent_Populate(json)
  add_subdirectory(${json_SOURCE_DIR} ${json_BINARY_DIR})
endif()
nlohmann commented 2 years ago

Is any of this relevant for the upstream nlohmann/json library?

melroy89 commented 2 years ago

Is any of this relevant for the upstream nlohmann/json library?

Possibly this whole project can be relevant for the upstream project.

nlohmann commented 2 years ago

Is any of this relevant for the upstream nlohmann/json library?

Possibly this whole project can be relevant for the upstream project.

How?

ArthurSonzogni commented 2 years ago

Is any of this relevant for the upstream nlohmann/json library?

I don't think much. I just CCed you for transparency, since this repository is referenced from the upstream one, in case you had some interesting comments. This is cloned ~5k/day, so it is important to do the right things, and minimize humans interventions.

nlohmann commented 2 years ago

I just asked because you made changes to the CMake files, and I so far expected you used the same ones as in nlohmann/json.

ArthurSonzogni commented 2 years ago

Okay. If there is a way to just reuse the original CMakeFiles please let me know. I think at the beginning, it was not possible, because it was doing additional work: tests, merging headers, ...

Reading more the new CMakeLists, it seems it might be possible to make the script to directly copy the CMakeLists.txt.
I see you just made changes to better support fetch content and detect whether the repository is being used via FetchContent or not.

I will take a look to update the update script to directly take the origin one. I will let you know when it's done.

melroy89 commented 2 years ago

Is any of this relevant for the upstream nlohmann/json library?

Possibly this whole project can be relevant for the upstream project.

How?

Well. Support cmake fetchcontent feature directly in your project.

ArthurSonzogni commented 2 years ago

Well. Support cmake fetchcontent feature directly in your project.

The upstream project does support cmake fetchcontent directly. However, the upstream repository size is This repository is almost nothing in comparison. This preserves resources and avoid waiting too much.

nlohmann commented 2 years ago

If you use a shallow clone

FetchContent_Declare(json
  GIT_REPOSITORY https://github.com/nlohmann/json
  GIT_TAG v3.10.5
  GIT_SHALLOW TRUE
)

the download size is much smaller (6.82 MB vs. 285.41 MB) - numbers directly from the Git binary.

ArthurSonzogni commented 2 years ago

I just tried:

FetchContent_Declare(json
  GIT_REPOSITORY https://github.com/nlohmann/json
  GIT_TAG v3.10.4
  GIT_PROGRESS TRUE
  GIT_SHALLOW TRUE
)

and I get:

Clonage dans 'json-src'...
remote: Enumerating objects: 7203, done.        
remote: Counting objects: 100% (7203/7203), done.        
remote: Compressing objects: 100% (4158/4158), done.        
remote: Total 7203 (delta 3660), reused 5153 (delta 2035), pack-reused 0        
Réception d'objets: 100% (7203/7203), 149.69 Mio | 1.79 Mio/s, fait.
Résolution des deltas: 100% (3660/3660), fait.

As a comparison:

FetchContent_Declare(json
  GIT_REPOSITORY https://github.com/ArthurSonzogni/nlohmann_json_cmake_fetchcontent
  GIT_TAG v3.10.4
  GIT_PROGRESS TRUE
  GIT_SHALLOW TRUE
)
[ 25%] Performing download step (git clone) for 'json-populate'
Clonage dans 'json-src'...
remote: Enumerating objects: 288, done.        
remote: Counting objects: 100% (288/288), done.        
remote: Compressing objects: 100% (180/180), done.        
remote: Total 288 (delta 131), reused 205 (delta 52), pack-reused 0        
Réception d'objets: 100% (288/288), 483.98 Kio | 1.63 Mio/s, fait.

I wait 300x longer with the upstream repository. (150MB vs 483KB). I don't get the 6.82 MB you have. I am not sure why we get different results.

nlohmann commented 2 years ago

You are right. As of https://gitlab.kitware.com/cmake/cmake/-/issues/17770, GIT_SHALLOW TRUE does not call

git clone --depth 1

as expected, but it calls

git clone --depth 1 --no-single-branch

which has the effect you observed.

I only called Git manually and did not measure directly inside CMake.

melroy89 commented 2 years ago

If you use a shallow clone

FetchContent_Declare(json
  GIT_REPOSITORY https://github.com/nlohmann/json
  GIT_TAG v3.10.5
  GIT_SHALLOW TRUE
)

the download size is much smaller (6.82 MB vs. 285.41 MB) - numbers directly from the Git binary.

If this will also fix the install. I will prefer this git shallow method over this seperate project (no offense/hard feelings).

nlohmann commented 2 years ago

Is the fix for the install relevant to the upstream library?

NicolasIRAGNE commented 2 years ago

Using

FetchContent_Declare(json
  GIT_REPOSITORY <URL>
  GIT_TAG v3.10.5
  GIT_SHALLOW TRUE
)
FetchContent_MakeAvailable(json)

gives me the following results:

So with the current state of CMake, I think this repo is still relevant.

About the "fix": If I remember correctly, the commits I made were just mostly using code from the upstream library's CMakeLists.txt. There is one thing I am not sure about, however. If you have a bit of time, I have updated the POC i made previously, with a use case similar to the one I have with one of my projects: https://github.com/NicolasIRAGNE/CMakePackageWithTransitiveDependencies

For this use case, using the upstream library is not ideal because:

I understand the purpose of the JSON_Install variable but wouldn't it be reasonable to ignore it if the library is not the root project ?

NicolasIRAGNE commented 2 years ago

Actually... does this option ever make sense ? Since installing is the user's responsibility, I'm not sure it should be gated behind this option.

Again, I am not a CMake expert but wouldn't removing it make more sense ? If you're just building the library, just don't use cmake --install .

If you're building a projet that depends on json and want to install it:

I think this is a reasonable behavior. Thoughts?

melroy89 commented 2 years ago
  • as discussed previously, size is a lot bigger

I think again that @nlohmann pointed out that your cmake version is too old. Causing this repo size difference.

Regarding the order remarks I leave that up to Niels again.

NicolasIRAGNE commented 2 years ago

Hmm I don't think this is what was being said ? The issue has not been resolved as far as I understand and appears to be the expected Fetchcontent behavior.

(I am using CMake 3.22, and the project I tested this on had CMake 3.20 set as minimum required)

melroy89 commented 2 years ago

Ah my bad you're right. It seems still an open issue.

ArthurSonzogni commented 2 years ago

I proposed a PR making the repository to directly take the original CMakeLists.txt files and a few others from the upstream repository: https://github.com/ArthurSonzogni/nlohmann_json_cmake_fetchcontent/pull/8

This wasn't possible in the early days, due to how the upstream CMakeLists.txt was made. Today it is possible. I will let it some time the PR umerged if someone has comments. After that I will save the current branch, land it before v3.10.5 and let the bot commit the new v3.10.5.

nlohmann commented 2 years ago

FYI, this is now possible:

FetchContent_Declare(json URL https://github.com/nlohmann/json/releases/download/v3.10.5/json-3.10.5.tar.xz)
FetchContent_MakeAvailable(json)

The download is 101 KB in size. Archives like this are available for all versions since 3.1.0.

I am currently in the state of testing whether everything works and will update the documentation once I am sure.

NicolasIRAGNE commented 2 years ago

Awesome news. I will try it out asap. Also, please disregard my previous comments regarding JSON_Install; I had a misunderstanding of how cmake installs worked. I forgot calling cmake --install installed all targets even imported ones by default and not just the ones that the main project depends on. Whoops.

melroy89 commented 2 years ago

FYI, this is now possible:

FetchContent_Declare(json URL https://github.com/nlohmann/json/releases/download/v3.10.5/json-3.10.5.tar.xz)
FetchContent_MakeAvailable(json)

The download is 101 KB in size. Archives like this are available for all versions since 3.1.0.

I am currently in the state of testing whether everything works and will update the documentation once I am sure.

really awesome.

nlohmann commented 2 years ago

FYI: I changed the URL - it's https://github.com/nlohmann/json/releases/download/v3.10.5/json.tar.xz now (no version number in the filename as it is already in the URL). It's now part of the official documentation, see https://github.com/nlohmann/json/commit/c11f98228dac03fdddc89e5991861fc306874ef9.