pboettch / json-schema-validator

JSON schema validator for JSON for Modern C++
Other
464 stars 133 forks source link

do not fetch nlohmann_json if it already added in main project #287

Closed andrejlevkovitch closed 7 months ago

andrejlevkovitch commented 9 months ago

If somebody uses json-schema-validator and nlohman/json as cmake subprojects, then there are problem with building nlohman_json twice. Instead fetch nlohman_json only if such TARGET doesn't exist

LecrisUT commented 5 months ago

@andrejlevkovitch thank for the PR, can you show me your project structure that needed this design? I wonder if the issue is that the find_package is mixed with FetchContent, or are you using through add_subdirectory directly?

pboettch commented 5 months ago

I think it is add_subdirectory directly, like I was doing (and still doing so).

andrejlevkovitch commented 5 months ago

I think it is add_subdirectory directly, like I was doing (and still doing so).

Yeah, exactly

LecrisUT commented 5 months ago

Hmm, haven't considered that. It could make an issue if the find_package/FetchContent contain a version restriction, or custom options are prepended. Is there any reason to have chosen add_subdirectory instead of FetchContent?

pboettch commented 5 months ago

FetchContent was new in version 3.11.

People were using CMake before that. That is the reason.

LecrisUT commented 5 months ago

Let me rephrase that, is there any reason for keeping the add_subdirectory instead of switching to FetchContent workflow, particularly when the downstream projects enforce the cmake_minimum_required >= 3.11, and these projects are using FetchContent internally? The main obstacle right now that comes to mind is FetchContent(FIND_PACKAGE_ARGS) which is introduced in 3.24, but there are workarounds introduced here.

pboettch commented 5 months ago

People (aka. users) won't switch their methods voluntarily.

I still have never used FetchContent in a project, but I what see is that it fetches stuff at configure or build time - each time when in a new build dir.

This is a problem, because sometimes you can't access github during build- but only at checkout time.

I'm not sure how we can convince people to switch to use fetch-content.

Start with me:

I don't really like that it opaquely fetches nlohmann-json during config (or build), it takes 10-20 seconds without any messages - drives me crazy. Especially that when I have a working, manually selected version of nlohmann::json right next to this repo which previously I just pointed to using nlohmann_json_DIR.

Unless there is a good convenient answer to this problem, I can understand users wanting the old behavior back.

LecrisUT commented 5 months ago

Good points. I want to get some of these feedbacks to know what to write about a guide to use FetchContent and other modern CMake methods.

Let me explain some points here:

But the FetchContent should not even be triggered if:

Are the amount of variables problematic? Maybe some presets can be exported with these variants set. But then should also give some instructions on how to create a CMakeUserPresets.json?

pboettch commented 5 months ago

One additionally need: how to handle the fact, that I have a locally modified, non-committed nlohmann-json used by the validator. How to handle this case with FetchContent?

I did so when adding functionality related to json-paths in nlohmann-json.

LecrisUT commented 5 months ago

One additionally need: how to handle the fact, that I have a locally modified, non-committed nlohmann-json used by the validator. How to handle this case with FetchContent?

This one is:

  • My preferred approach when developing is to have a git repo of the specific project and point FETCHCONTENT_SOURCE_DIR_<uppercaseName> to that path

The path there does not have to be a git repository, and it always picks up the latest changes you made. Modern IDEs (CLion in my case) even let you edit the projects individually: Here's an example of a CMakeUserPresets.json that can help you.

Screenshot_20231128_180431

I did so when adding functionality related to json-paths in nlohmann-json.

Shouldn't we add a minimum nlohmann-json requirement than?