Do not embed nlohmann::json directly, use find_package / FetchContent instead #254
Conversation
7a05bf3 to
978d65d
Compare
| @@ -1,4 +1,4 @@ | |||
| cmake_minimum_required(VERSION 3.12) | |||
| cmake_minimum_required(VERSION 3.14) | |||
There was a problem hiding this comment.
Because of FetchContent_MakeAvailable
CMakeLists.txt
Outdated
|
|
||
| FetchContent_Declare( | ||
| json | ||
| GIT_REPOSITORY https://github.com/ArthurSonzogni/nlohmann_json_cmake_fetchcontent.git |
There was a problem hiding this comment.
As much as I understand the benefits, but rather see the real repository.
This "first to record, wins" approach is what allows hierarchical projects to have parent projects override content details of child projects.
| GIT_REPOSITORY https://github.com/ArthurSonzogni/nlohmann_json_cmake_fetchcontent.git | |
| GIT_REPOSITORY https://github.com/nlohmann/json.git |
There was a problem hiding this comment.
Are you sure about this ? Main repository is really huge and it takes time to download all of it.
There was a problem hiding this comment.
It looks more official using the real one, that's just me.
You only pay the price once when you clone + you can install system wide and avoid it (I need to check the logs first) + downstream projects can override it
Don't change it, I think it's good to merge test the performance across the CI workflows can wait for me
There was a problem hiding this comment.
ok, thanks for the review, finally CI is OK ;)
There was a problem hiding this comment.
I know this has already been merged, but I think a good alternative would be to pull the official release tar.
It's official, but unlike the git pull its only a couple hundret K, not a couple hundret megs.
E.g.
FetchContent_Declare(nlohmann_json
URL https://github.com/nlohmann/json/releases/download/v3.11.2/json.tar.xz
DOWNLOAD_EXTRACT_TIMESTAMP OFF
)
FetchContent_MakeAvailable(nlohmann_json)|
thanks for this amazing PR ❤️ |
975229f to
7b19091
Compare
7b19091 to
453e2d1
Compare
|
Please do not force push 🙏 GitHub forces us to restart the review which is not fun! |
|
It's working as intended https://github.com/Thalhammer/jwt-cpp/actions/runs/3170117371/jobs/5166681733#step:11:1452 Install is picked no fetch content 🚀 |
Headers are no longer vendored
|
I'll hit merge assuming I didn't break it lol |
Draft PR dealing with this issue
closes #250