Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CMake fetchcontent documentation and tests #2074

Merged
merged 2 commits into from
May 1, 2020

Conversation

ArthurSonzogni
Copy link
Contributor

@ArthurSonzogni ArthurSonzogni commented Apr 29, 2020

Github issue:
#2073

nlohmann::json documents 2 way of depending on it using CMake

  1. Copy-paste the project/source into your own project.
  2. Install nlohman::json and then use find_package.

(1) pollutes your git repository, (2) requires everyone to install the
dependencies themselves.

Since 2018, CMake provide some kind of 'package manager' features using
FetchContent
It gives the following:

include(FetchContent)

FetchContent_Declare(json
  GIT_REPOSITORY https://github.com/nlohmann/json
  GIT_TAG v3.7.3)

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

Then declares the dependency in the target using it:

target_link_library(my_project PRIVATE nlohmann_json::nlohmann_json

This patch updates the documentation and provides tests.

@ArthurSonzogni
Copy link
Contributor Author

ArthurSonzogni commented Apr 29, 2020

Hi Niels,
Could you please take a look?

I was very interested seeing what kind of CI you had. Nice! Let's see how it goes.

Please do not accept this pull request yet. I just realized by cloning this repository that its download size is huge: 170MiB. CMake FetchContent use shallow clone, so this will be only 113MiB.

I think this might be a problem for some users. What do you think about proposing the users to use some alternatives repository URL(s). There are some nice fork tracking only the required files likes:
https://github.com/astoeckel/json
Nevertheless, I will have to request them adding a CMakeList.txt to their repository for this to work as a drop-in replacement.

@nlohmann
Copy link
Owner

Thanks! I am already working on splitting tests and code (see https://github.com/nlohmann/json/tree/tests_external), but this will take some more time. Until then, I think we should live with a shallow checkout. It's not perfect, but it works and this is about documenting possibilities for integration - not forcing anyone to actually use this way.

@ArthurSonzogni
Copy link
Contributor Author

The branch "tests_external" looks promising indeed!


git clone --depth 1  --branch tests_external https://github.com/nlohmann/json

=> 4.61 MiB (download size)


git clone --depth 1  --branch masterl https://github.com/nlohmann/json

=> 113 MiB (download size)


git clone  --branch masterl https://github.com/nlohmann/json

=> 170 MiB (download size)


Github issue:
nlohmann#2073

nlohmann::json documents 2 way of depending on it using CMake
1) Copy-paste the project/source into your own project.
2) Install nlohman::json and then use find_package.

(1) pollutes your git repository, (2) requires everyone to install the
dependencies themselves.

Since 2018, CMake provide some kind of 'package manager' features using
[FetchContent](https://cmake.org/cmake/help/v3.17/module/FetchContent.html)
It gives the following:
~~~cmake
include(FetchContent)

FetchContent_Declare(json
  GIT_REPOSITORY https://github.com/nlohmann/json
  GIT_TAG v3.7.3)

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

Then declares the dependency in the target using it:
~~~cmake
target_link_library(my_project PRIVATE nlohmann_json::nlohmann_json
~~~

This patch updates the documentation and provides tests.
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-Authored-By: Niels Lohmann <niels.lohmann@gmail.com>
Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@nlohmann nlohmann linked an issue Apr 30, 2020 that may be closed by this pull request
@nlohmann nlohmann self-assigned this May 1, 2020
@nlohmann nlohmann added this to the Release 3.8.0 milestone May 1, 2020
@nlohmann nlohmann merged commit b27d8a3 into nlohmann:develop May 1, 2020
@nlohmann
Copy link
Owner

nlohmann commented May 1, 2020

Thanks! I may remove the link to your repo once we have slimmed this repository a bit, see #96.

@nlohmann
Copy link
Owner

nlohmann commented May 1, 2020


🔖 Release item

This issue/PR will be part of the next release of the library. This template helps preparing the release notes.

Type

  • ✨ New Feature
  • 🐛 Bug Fix
  • ⚡️ Improvement
  • 🔨 Further Change
  • 🔥 Deprecated function

Description


@ArthurSonzogni
Copy link
Contributor Author

Thanks! I may remove the link to your repo once we have slimmed this repository a bit, see #96.

Yes sure. This make sense!
Thanks for the review!

ArthurSonzogni added a commit to ArthurSonzogni/json that referenced this pull request May 1, 2020
About pull request:
nlohmann#2074

An error has been introduced by accepting the suggestions:
4be4a03

One was about removing ~~~, but it was meant to be replaced by ``` in
reality. This caused the README.md to be slightly broken.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CMake FetchContent > Updating the documentation?
2 participants